[Intel-gfx] [PATCH 3/4] drm/i915/gen9: Fix clearing of the BIOS power well request register

[Imre Deak]

On Thu, Feb 16, 2017 at 11:43:06AM +0200, Ander Conselvan De Oliveira wrote:
> On Wed, 2017-02-15 at 13:59 +0200, Imre Deak wrote:
> > Atm, in the power well sync_hw hook we are clearing all BIOS request
> > bits, not just the one corresponding to the given power well. This could
> > turn off an unrelated power well inadvertently if it didn't have a
> > request bit set in the driver request register.
> > 
> > This didn't cause a problem so far, since we enabled all power wells
> > explicitly before clearing the BIOS request register. A follow-up
> > patchset will add power wells that won't get enabled this way, so fix up
> > the inconsistency.
> > 
> > Cc: Ander Conselvan de Oliveira <conselvan2 at gmail.com>
> > Cc: David Weinehall <david.weinehall at linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index f9aff26..b7b0e0f 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -887,8 +887,13 @@ static bool skl_power_well_enabled(struct drm_i915_private *dev_priv,
> >  static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv,
> >  				struct i915_power_well *power_well)
> >  {
> > +	uint32_t mask = SKL_POWER_WELL_REQ(power_well->id);
> > +	uint32_t bios_req = I915_READ(HSW_PWR_WELL_BIOS);
> > +
> >  	/* Clear any request made by BIOS as driver is taking over */
> > -	I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> > +	if (bios_req & mask) {
> > +		I915_WRITE(HSW_PWR_WELL_BIOS, bios_req & ~mask);
> > +	}
> >  }
> >  
> >  static void skl_power_well_enable(struct drm_i915_private *dev_priv,
> 
> With this change we still end up disabling every power well from the BIOS
> register because of intel_power_domains_sync_hw() iterating over all the power
> wells. 

Yes, that's what is expected. This patch just changes how this clearing
is done, the next one makes sure we preserve the state for any power
well without a reference.

> Maybe that's worth mentioning in the commit message? Dunno.

Ok, will add something.

> Anyway,
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2 at gmail.com>
>