[Intel-gfx] [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early

[Todd Brandt]

On Wed, 2016-04-27 at 13:49 -0700, Todd Brandt wrote:
> On Wed, 2016-04-27 at 22:33 +0200, Lukas Wunner wrote:
> > Hi,
> > 
> > On Wed, Apr 27, 2016 at 11:46:22AM -0700, Todd Brandt wrote:
> > >  I'd like to propose that we push the i915 suspend_late/resume_early code
> > >  into suspend_noirq/resume_noirq in order to reduce the total suspend time
> > >  by ~15ms. According to the comments, when i915_pm_suspend_late was first
> > >  added to the kernel back in April 2014, it was done so to ensure that it
> > >  was called after the snd_hda_intel driver had finished its suspend.
> > 
> > Ordering issues like this one should be solved with device_pm_wait_for_dev(),
> > not by shuffling code around among the callbacks.
> > 
> > In any case it would be good if you would name the sha1 of the commit
> > that added i915_pm_suspend_late to spare readers the git blame / git log.
> > 
> 
> Here's the actual commit that introduced it:
> 
> git checkout 76c4b250080fff6e4befaa3619942422fd0ea380
> git diff HEAD^ drivers/gpu/drm/i915/i915_drv.c
> 
> That shows the initial addition of the _late code.
> 

Oh, and here's the actual commit log text:

commit 76c4b250080fff6e4befaa3619942422fd0ea380
Author: Imre Deak <imre.deak at 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 at intel.com>
    Reviewed-by: Takashi Iwai <tiwai at suse.de>
    Cc: stable at 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 at ffwll.ch>


> 
> > Thanks,
> > 
> > Lukas
> > 
> > > 
> > > The comments in i915_drv.c are here:
> > > 
> > >     /*
> > >      * We have a suspedn ordering issue with the snd-hda driver also
> > >      * requiring our device to be power up. Due to the lack of a
> > >      * parent/child relationship we currently solve this with an late
> > >      * suspend hook.
> > >      *
> > >      * FIXME: This should be solved with a special hdmi sink device or
> > >      * similar so that power domains can be employed.
> > >      */
> > > 
> > > I believe we could achieve the same ordering by simply pushing it to
> > > suspend/resume_noirq. Thus we can effectively eliminate the suspend_late
> > > and resume_early phases altogether in most simple systems. Does anyone see
> > > a problem with this?
> > > 
> > > analyzesuspend outputs for freeze/suspend/hibernate (WITHOUT PATCH):
> > > https://01org.github.io/suspendresume/i915/freeze-160422-155931-ivybridge-dev-late/
> > > https://01org.github.io/suspendresume/i915/suspend-160422-155735-ivybridge-dev-late/
> > > https://01org.github.io/suspendresume/i915/hibernate-160422-163915-ivybridge-dev-late/
> > > 
> > > analyzesuspend outputs for freeze/suspend/hibernate (WITH PATCH):
> > > https://01org.github.io/suspendresume/i915/freeze-160422-162811-ivybridge-dev-noirq/
> > > https://01org.github.io/suspendresume/i915/suspend-160422-162700-ivybridge-dev-noirq/
> > > https://01org.github.io/suspendresume/i915/hibernate-160422-162952-ivybridge-dev-noirq/
> > > 
> > > Signed-off-by: Todd Brandt <todd.e.brandt at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 30798cb..759d93c 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1628,8 +1628,8 @@ static const struct dev_pm_ops i915_pm_ops = {
> > >  	 * PMSG_RESUME]
> > >  	 */
> > >  	.suspend = i915_pm_suspend,
> > > -	.suspend_late = i915_pm_suspend_late,
> > > -	.resume_early = i915_pm_resume_early,
> > > +	.suspend_noirq = i915_pm_suspend_late,
> > > +	.resume_noirq = i915_pm_resume_early,
> > >  	.resume = i915_pm_resume,
> > >  
> > >  	/*
> > > @@ -1648,12 +1648,12 @@ static const struct dev_pm_ops i915_pm_ops = {
> > >  	 *                            hibernation image [PMSG_RESTORE]
> > >  	 */
> > >  	.freeze = i915_pm_suspend,
> > > -	.freeze_late = i915_pm_suspend_late,
> > > -	.thaw_early = i915_pm_resume_early,
> > > +	.freeze_noirq = i915_pm_suspend_late,
> > > +	.thaw_noirq = i915_pm_resume_early,
> > >  	.thaw = i915_pm_resume,
> > >  	.poweroff = i915_pm_suspend,
> > > -	.poweroff_late = i915_pm_poweroff_late,
> > > -	.restore_early = i915_pm_resume_early,
> > > +	.poweroff_noirq = i915_pm_poweroff_late,
> > > +	.restore_noirq = i915_pm_resume_early,
> > >  	.restore = i915_pm_resume,
> > >  
> > >  	/* S0ix (via runtime suspend) event handlers */
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>