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

[Imre Deak]

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.

We considered using device_pm_wait_for_dev() but decided not to, since
it may dead lock in case of suspend/resume:
https://lists.freedesktop.org/archives/intel-gfx/2014-December/057113.html

> 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.
> 
> 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-ivy
> > bridge-dev-late/
> > https://01org.github.io/suspendresume/i915/suspend-160422-155735-iv
> > ybridge-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-ivy
> > bridge-dev-noirq/
> > https://01org.github.io/suspendresume/i915/suspend-160422-162700-iv
> > ybridge-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 */