Re: [PATCH v1 2/2] drm/i915: Deferring uncore early sanitize, sanitize, disable pc8 to resume

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

On Mon, 2014-12-01 at 15:59 +0530, Sagar Arun Kamble wrote:
> Thanks Daniel.
> This particular commit is moving power_domain_init into resume early.
> Does not have details about ordering with uncore early sanitize and
> uncore sanitize.
> 
> Imre,
> Can you please clarify why this ordering with power domain init was
> done?

We call intel_uncore_early_sanitize() and intel_uncore_sanitize() before
any other HW access. They are called from i915_drm_resume_early() since
the hda driver's resume handler can run before i915_drm_resume() is
called. The hda resume handler can in turn request the display power
well resulting in an i915 HW access.

> 
> Thanks,
> Sagar
> 
> On Mon, 2014-12-01 at 10:16 +0100, Daniel Vetter wrote:
> > On Mon, Dec 01, 2014 at 12:28:05PM +0530, sagar.a.kamble@intel.com wrote:
> > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > 
> > > Due to disabling of RC6 in uncore_sanitize in early resume, power is drained
> > > till it RC6 is re-enabled post resume.
> > > With this change RC6 disabling will be done at beginning of resume only.
> > > This helps yield additional power benefits.
> > > 
> > > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> > 
> > A bit of git blame turns up
> > 
> > commit 76c4b250080fff6e4befaa3619942422fd0ea380
> > Author: Imre Deak <imre.deak@intel.com>
> > Date:   Tue Apr 1 19:55:22 2014 +0300
> > 
> >     drm/i915: move power domain init earlier during system resume
> >     
> >     During resume the intel hda audio driver depends on the i915 driver
> >     reinitializing the audio power domain. Since the order of calling the
> >     i915 resume handler wrt. that of the audio driver is not guaranteed,
> >     move the power domain reinitialization step to the resume_early
> >     handler. This is guaranteed to run before the resume handler of any
> >     other driver.
> >     
> >     The power domain initialization in turn requires us to enable the i915
> >     pci device first, so move that part earlier too.
> >     
> >     Accordingly disabling of the i915 pci device should happen after the
> >     audio suspend handler ran. So move the disabling later from the i915
> >     resume handler to the resume_late handler.
> >     
> >     v2:
> >     - move intel_uncore_sanitize/early_sanitize earlier too, so they don't
> >       get reordered wrt. intel_power_domains_init_hw()
> >     
> >     Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76152
> >     Signed-off-by: Imre Deak <imre.deak@intel.com>
> >     Reviewed-by: Takashi Iwai <tiwai@suse.de>
> >     Cc: stable@vger.kernel.org
> >     [danvet: Add cc: stable and loud comments that this is just a hack.]
> >     [danvet: Fix "Should it be static?" sparse warning reported by Wu
> >     Fengguang's kbuilder.]
> >     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > 
> > How does this patch not break stuff?
> > 
> > And a general rule of thumb: If you change anything in the driver load
> > sequence please digg around in the git historly (with git log --pickaxe
> > and git blame) to gather evidence for your changes and make sure you don't
> > break anything.
> > 
> > Thanks, Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 13 +++++++------
> > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 71be3c9..0e08ec0 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -675,6 +675,13 @@ static int i915_drm_resume(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  
> > > +	intel_uncore_early_sanitize(dev, true);
> > > +
> > > +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > > +		hsw_disable_pc8(dev_priv);
> > > +
> > > +	intel_uncore_sanitize(dev);
> > > +
> > >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > >  		mutex_lock(&dev->struct_mutex);
> > >  		i915_gem_restore_gtt_mappings(dev);
> > > @@ -761,12 +768,6 @@ static int i915_drm_resume_early(struct drm_device *dev)
> > >  	if (ret)
> > >  		DRM_ERROR("Resume prepare failed: %d,Continuing resume\n", ret);
> > >  
> > > -	intel_uncore_early_sanitize(dev, true);
> > > -
> > > -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > > -		hsw_disable_pc8(dev_priv);
> > > -
> > > -	intel_uncore_sanitize(dev);
> > >  	intel_power_domains_init_hw(dev_priv);
> > >  
> > >  	return ret;
> > > -- 
> > > 1.8.5
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


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