[4/8] drm/i915: Add a modeset power domain

Submitted by Patrik Jakobsson on Nov. 3, 2015, 12:31 p.m.

Details

Message ID 1446553874-22986-5-git-send-email-patrik.jakobsson@linux.intel.com
State New
Headers show
Series "Skylake DMC/DC-state fixes and redesign" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Patrik Jakobsson Nov. 3, 2015, 12:31 p.m.
We need DC5/DC6 to be disabled around modesets to prevent confusing the
DMC. Also, we've run out of bits in the 32 bit power domain mask so now
it's a 64 bit mask.

Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     | 2 ++
 drivers/gpu/drm/i915/i915_drv.h         | 3 ++-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f7b85fe..ae9a2ed 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2746,6 +2746,8 @@  static const char *power_domain_str(enum intel_display_power_domain domain)
 		return "AUX_D";
 	case POWER_DOMAIN_GMBUS:
 		return "GMBUS";
+	case POWER_DOMAIN_MODESET:
+		return "MODESET";
 	case POWER_DOMAIN_INIT:
 		return "INIT";
 	default:
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c3d3b2a..efb6a00 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -200,6 +200,7 @@  enum intel_display_power_domain {
 	POWER_DOMAIN_AUX_C,
 	POWER_DOMAIN_AUX_D,
 	POWER_DOMAIN_GMBUS,
+	POWER_DOMAIN_MODESET,
 	POWER_DOMAIN_INIT,
 
 	POWER_DOMAIN_NUM,
@@ -1226,7 +1227,7 @@  struct i915_power_well {
 	int count;
 	/* cached hw enabled state */
 	bool hw_enabled;
-	unsigned long domains;
+	unsigned long long domains;
 	unsigned long data;
 	const struct i915_power_well_ops *ops;
 };
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b6ce77f..d0ed38b 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1815,7 +1815,7 @@  int intel_power_domains_init(struct drm_i915_private *dev_priv)
 {
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 
-	BUILD_BUG_ON(POWER_DOMAIN_NUM > 31);
+	BUILD_BUG_ON(POWER_DOMAIN_NUM > 63);
 
 	mutex_init(&power_domains->lock);
 

Comments

On Tue, Nov 03, 2015 at 01:31:10PM +0100, Patrik Jakobsson wrote:
> We need DC5/DC6 to be disabled around modesets to prevent confusing the
> DMC. Also, we've run out of bits in the 32 bit power domain mask so now
> it's a 64 bit mask.

We could get back 4 bits by squashing each 2 and 4 lane DDI power doamin
into just one power domain. I don't think we use the 2 vs. 4 lane
distinciton anywhere. It was basically added for VLV but turns it it
can't be used there, so I think we could just get rid of it.

> 
> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     | 2 ++
>  drivers/gpu/drm/i915/i915_drv.h         | 3 ++-
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
>  3 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index f7b85fe..ae9a2ed 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2746,6 +2746,8 @@ static const char *power_domain_str(enum intel_display_power_domain domain)
>  		return "AUX_D";
>  	case POWER_DOMAIN_GMBUS:
>  		return "GMBUS";
> +	case POWER_DOMAIN_MODESET:
> +		return "MODESET";
>  	case POWER_DOMAIN_INIT:
>  		return "INIT";
>  	default:
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c3d3b2a..efb6a00 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -200,6 +200,7 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_AUX_C,
>  	POWER_DOMAIN_AUX_D,
>  	POWER_DOMAIN_GMBUS,
> +	POWER_DOMAIN_MODESET,
>  	POWER_DOMAIN_INIT,
>  
>  	POWER_DOMAIN_NUM,
> @@ -1226,7 +1227,7 @@ struct i915_power_well {
>  	int count;
>  	/* cached hw enabled state */
>  	bool hw_enabled;
> -	unsigned long domains;
> +	unsigned long long domains;
>  	unsigned long data;
>  	const struct i915_power_well_ops *ops;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b6ce77f..d0ed38b 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1815,7 +1815,7 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>  {
>  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
>  
> -	BUILD_BUG_ON(POWER_DOMAIN_NUM > 31);
> +	BUILD_BUG_ON(POWER_DOMAIN_NUM > 63);
>  
>  	mutex_init(&power_domains->lock);
>  
> -- 
> 2.1.4
On Wed, Nov 4, 2015 at 6:29 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, Nov 03, 2015 at 01:31:10PM +0100, Patrik Jakobsson wrote:
>> We need DC5/DC6 to be disabled around modesets to prevent confusing the
>> DMC. Also, we've run out of bits in the 32 bit power domain mask so now
>> it's a 64 bit mask.
>
> We could get back 4 bits by squashing each 2 and 4 lane DDI power doamin
> into just one power domain. I don't think we use the 2 vs. 4 lane
> distinciton anywhere. It was basically added for VLV but turns it it
> can't be used there, so I think we could just get rid of it.

Good idea. I can't see that we're making a distinction anywhere either.

>
>>
>> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c     | 2 ++
>>  drivers/gpu/drm/i915/i915_drv.h         | 3 ++-
>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
>>  3 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index f7b85fe..ae9a2ed 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2746,6 +2746,8 @@ static const char *power_domain_str(enum intel_display_power_domain domain)
>>               return "AUX_D";
>>       case POWER_DOMAIN_GMBUS:
>>               return "GMBUS";
>> +     case POWER_DOMAIN_MODESET:
>> +             return "MODESET";
>>       case POWER_DOMAIN_INIT:
>>               return "INIT";
>>       default:
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index c3d3b2a..efb6a00 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -200,6 +200,7 @@ enum intel_display_power_domain {
>>       POWER_DOMAIN_AUX_C,
>>       POWER_DOMAIN_AUX_D,
>>       POWER_DOMAIN_GMBUS,
>> +     POWER_DOMAIN_MODESET,
>>       POWER_DOMAIN_INIT,
>>
>>       POWER_DOMAIN_NUM,
>> @@ -1226,7 +1227,7 @@ struct i915_power_well {
>>       int count;
>>       /* cached hw enabled state */
>>       bool hw_enabled;
>> -     unsigned long domains;
>> +     unsigned long long domains;
>>       unsigned long data;
>>       const struct i915_power_well_ops *ops;
>>  };
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index b6ce77f..d0ed38b 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -1815,7 +1815,7 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>>  {
>>       struct i915_power_domains *power_domains = &dev_priv->power_domains;
>>
>> -     BUILD_BUG_ON(POWER_DOMAIN_NUM > 31);
>> +     BUILD_BUG_ON(POWER_DOMAIN_NUM > 63);
>>
>>       mutex_init(&power_domains->lock);
>>
>> --
>> 2.1.4
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi,

On 3 November 2015 at 12:31, Patrik Jakobsson
<patrik.jakobsson@linux.intel.com> wrote:
> We need DC5/DC6 to be disabled around modesets to prevent confusing the
> DMC. Also, we've run out of bits in the 32 bit power domain mask so now
> it's a 64 bit mask.

There are quite a lot of users in intel_display.c (search for
put_domains, display_power_put, put_power_domains) which need updating
for the unsigned long long change.

Cheers,
Daniel
On Thu, Nov 5, 2015 at 4:02 PM, Daniel Stone <daniel@fooishbar.org> wrote:
> Hi,
>
> On 3 November 2015 at 12:31, Patrik Jakobsson
> <patrik.jakobsson@linux.intel.com> wrote:
>> We need DC5/DC6 to be disabled around modesets to prevent confusing the
>> DMC. Also, we've run out of bits in the 32 bit power domain mask so now
>> it's a 64 bit mask.
>
> There are quite a lot of users in intel_display.c (search for
> put_domains, display_power_put, put_power_domains) which need updating
> for the unsigned long long change.

Ah yes, we carry the mask around there as well. Thanks for catching
that. I like the move of POWER_DOMAIN_MODESET into put_domain as well.
I will resend the whole series again rebased on Imre's latest series.
Ok if I incorporate your changes directly?

Thanks
Patrik

>
> Cheers,
> Daniel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi,

On 5 November 2015 at 17:12, Patrik Jakobsson
<patrik.r.jakobsson@gmail.com> wrote:
> On Thu, Nov 5, 2015 at 4:02 PM, Daniel Stone <daniel@fooishbar.org> wrote:
>> On 3 November 2015 at 12:31, Patrik Jakobsson
>> <patrik.jakobsson@linux.intel.com> wrote:
>>> We need DC5/DC6 to be disabled around modesets to prevent confusing the
>>> DMC. Also, we've run out of bits in the 32 bit power domain mask so now
>>> it's a 64 bit mask.
>>
>> There are quite a lot of users in intel_display.c (search for
>> put_domains, display_power_put, put_power_domains) which need updating
>> for the unsigned long long change.
>
> Ah yes, we carry the mask around there as well. Thanks for catching
> that. I like the move of POWER_DOMAIN_MODESET into put_domain as well.
> I will resend the whole series again rebased on Imre's latest series.
> Ok if I incorporate your changes directly?

Of course! Sending it like that wasn't fishing for attribution, just
that as I rebased and shifted around a bunch of other changes,
git-send-email was the most laziness-friendly option. :) Thanks for
pulling it in. If you're an admin on intel-gfx Patchwork, would you
mind marking my patch as superseded or something so it doesn't show
up?

I assume you've co-ordinated with Imre, but he does have a combined
tree on github.com/ideak/linux#dmc-fixes, which may save you some
time.

Cheers,
Daniel
On pe, 2015-11-06 at 13:53 +0000, Daniel Stone wrote:
> Hi,
> 
> On 5 November 2015 at 17:12, Patrik Jakobsson
> <patrik.r.jakobsson@gmail.com> wrote:
> > On Thu, Nov 5, 2015 at 4:02 PM, Daniel Stone <daniel@fooishbar.org>
> > wrote:
> > > On 3 November 2015 at 12:31, Patrik Jakobsson
> > > <patrik.jakobsson@linux.intel.com> wrote:
> > > > We need DC5/DC6 to be disabled around modesets to prevent
> > > > confusing the
> > > > DMC. Also, we've run out of bits in the 32 bit power domain
> > > > mask so now
> > > > it's a 64 bit mask.
> > > 
> > > There are quite a lot of users in intel_display.c (search for
> > > put_domains, display_power_put, put_power_domains) which need
> > > updating
> > > for the unsigned long long change.
> > 
> > Ah yes, we carry the mask around there as well. Thanks for catching
> > that. I like the move of POWER_DOMAIN_MODESET into put_domain as
> > well.
> > I will resend the whole series again rebased on Imre's latest
> > series.
> > Ok if I incorporate your changes directly?
> 
> Of course! Sending it like that wasn't fishing for attribution, just
> that as I rebased and shifted around a bunch of other changes,
> git-send-email was the most laziness-friendly option. :) Thanks for
> pulling it in. If you're an admin on intel-gfx Patchwork, would you
> mind marking my patch as superseded or something so it doesn't show
> up?
> 
> I assume you've co-ordinated with Imre, but he does have a combined
> tree on github.com/ideak/linux#dmc-fixes, which may save you some
> time.

Well, that one has my patches on top while we agreed with Patrik to
have the opposite order in the end, since I think that makes more sense
for bisectability: Patrik's change to move DC6 enabling earlier might
reveal some issues that are supposed to be fixed in my patchset. Anyway
the end result should be the same so hopefully that branch was useful
for testing.

> 
> Cheers,
> Daniel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi,

On 6 November 2015 at 14:17, Imre Deak <imre.deak@intel.com> wrote:
> On pe, 2015-11-06 at 13:53 +0000, Daniel Stone wrote:
>> On 5 November 2015 at 17:12, Patrik Jakobsson
>> <patrik.r.jakobsson@gmail.com> wrote:
>> > Ah yes, we carry the mask around there as well. Thanks for catching
>> > that. I like the move of POWER_DOMAIN_MODESET into put_domain as
>> > well.
>> > I will resend the whole series again rebased on Imre's latest
>> > series.
>> > Ok if I incorporate your changes directly?
>>
>> Of course! Sending it like that wasn't fishing for attribution, just
>> that as I rebased and shifted around a bunch of other changes,
>> git-send-email was the most laziness-friendly option. :) Thanks for
>> pulling it in. If you're an admin on intel-gfx Patchwork, would you
>> mind marking my patch as superseded or something so it doesn't show
>> up?
>>
>> I assume you've co-ordinated with Imre, but he does have a combined
>> tree on github.com/ideak/linux#dmc-fixes, which may save you some
>> time.
>
> Well, that one has my patches on top while we agreed with Patrik to
> have the opposite order in the end, since I think that makes more sense
> for bisectability: Patrik's change to move DC6 enabling earlier might
> reveal some issues that are supposed to be fixed in my patchset. Anyway
> the end result should be the same so hopefully that branch was useful
> for testing.

Yeah, having the combined branch in that order was very useful - thanks a lot!

I have no useful opinion on which order is better, so whatever you
guys decide will be best I'm sure.

Cheers,
Daniel
Hi,

On 6 November 2015 at 13:53, Daniel Stone <daniel@fooishbar.org> wrote:
> On 5 November 2015 at 17:12, Patrik Jakobsson
> <patrik.r.jakobsson@gmail.com> wrote:
>> On Thu, Nov 5, 2015 at 4:02 PM, Daniel Stone <daniel@fooishbar.org> wrote:
>>> On 3 November 2015 at 12:31, Patrik Jakobsson
>>> <patrik.jakobsson@linux.intel.com> wrote:
>>>> We need DC5/DC6 to be disabled around modesets to prevent confusing the
>>>> DMC. Also, we've run out of bits in the 32 bit power domain mask so now
>>>> it's a 64 bit mask.
>>>
>>> There are quite a lot of users in intel_display.c (search for
>>> put_domains, display_power_put, put_power_domains) which need updating
>>> for the unsigned long long change.
>>
>> Ah yes, we carry the mask around there as well. Thanks for catching
>> that. I like the move of POWER_DOMAIN_MODESET into put_domain as well.
>> I will resend the whole series again rebased on Imre's latest series.
>> Ok if I incorporate your changes directly?
>
> Of course! Sending it like that wasn't fishing for attribution, just
> that as I rebased and shifted around a bunch of other changes,
> git-send-email was the most laziness-friendly option. :) Thanks for
> pulling it in. If you're an admin on intel-gfx Patchwork, would you
> mind marking my patch as superseded or something so it doesn't show
> up?

Safe to say though that I'd recommend work->put_power_domains |= (1 <<
POWER_MODESET), rather than |= POWER_DOMAIN_MODESET ...

Cheers,
Daniel
On 3 November 2015 at 22:31, Patrik Jakobsson
<patrik.jakobsson@linux.intel.com> wrote:
> We need DC5/DC6 to be disabled around modesets to prevent confusing the
> DMC. Also, we've run out of bits in the 32 bit power domain mask so now
> it's a 64 bit mask.
>

It was bad enough the original code used unsigned long so was
different on 32/64 bit,

but don't keep going with it. uint64_t already please.

Dave.
On Sat, Nov 07, 2015 at 08:29:49AM +1000, Dave Airlie wrote:
> On 3 November 2015 at 22:31, Patrik Jakobsson
> <patrik.jakobsson@linux.intel.com> wrote:
> > We need DC5/DC6 to be disabled around modesets to prevent confusing the
> > DMC. Also, we've run out of bits in the 32 bit power domain mask so now
> > it's a 64 bit mask.
> >
> 
> It was bad enough the original code used unsigned long so was
> different on 32/64 bit,
> 
> but don't keep going with it. uint64_t already please.

Concurred. Also I don't like typing ;-)
-Daniel
On Wed, Nov 18, 2015 at 10:02 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Nov 07, 2015 at 08:29:49AM +1000, Dave Airlie wrote:
>> On 3 November 2015 at 22:31, Patrik Jakobsson
>> <patrik.jakobsson@linux.intel.com> wrote:
>> > We need DC5/DC6 to be disabled around modesets to prevent confusing the
>> > DMC. Also, we've run out of bits in the 32 bit power domain mask so now
>> > it's a 64 bit mask.
>> >
>>
>> It was bad enough the original code used unsigned long so was
>> different on 32/64 bit,
>>
>> but don't keep going with it. uint64_t already please.
>
> Concurred. Also I don't like typing ;-)

We removed a few unused power domains so didn't need to bump it to 64
bits. Can do that anyway or just change it to an uint32_t in the next
round of PW/DMC cleanups.

-Patrik

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx