Re: [PATCH] drm/i915: Fix mismatched INIT power domain disabling during suspend

[Imre Deak <imre.deak@intel.com>]

On to, 2016-10-13 at 15:43 +0300, Ville Syrjälä wrote:
> On Thu, Oct 13, 2016 at 02:34:06PM +0300, Imre Deak wrote:
> > Currently the display INIT power domain disabling/enabling happens in a
> > mismatched way in the suspend/resume_early hooks respectively. This can
> > leave display power wells incorrectly disabled in the resume hook if the
> > suspend sequence is aborted for some reason resulting in the
> > suspend/resume hooks getting called but the suspend_late/resume_early
> > hooks being skipped. In particular this change fixes "Unclaimed read
> > from register 0x1e1204" on BYT/BSW triggered from i915_drm_resume()->
> > intel_pps_unlock_regs_wa() when suspending with /sys/power/pm_test set
> > to devices.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: David Weinehall <david.weinehall@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index e9b3bfc..5adabac 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1430,8 +1430,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> >  
> >  	dev_priv->suspend_count++;
> >  
> > -	intel_display_set_init_power(dev_priv, false);
> > -
> >  	intel_csr_ucode_suspend(dev_priv);
> >  
> >  out:
> > @@ -1449,6 +1447,8 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> >  
> >  	disable_rpm_wakeref_asserts(dev_priv);
> >  
> > +	intel_display_set_init_power(dev_priv, false);
> > +
> 
> Makes sense in most cases.
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> >  	fw_csr = !IS_BROXTON(dev_priv) &&
> >  		suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload;
> 
> However this part is quite confusing. We do the init_power_put
> unconditionally, but the init_power_get is done from the init_hw()
> which is conditional on this supend_to_idle special casing. So it seems
> quite assymmetrical, and I can no longer claim to understand it.

Yep, that's a problem of its own for the suspend-to-idle case, enabling the
INIT domain should be also unconditional and happen from this function.
I suppose this happened to work so far since it only applies to SKL where
nothing depended on the INIT domain, everything takes its own domain ref.
Will look into fixing this.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx