public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early
@ 2016-04-27 18:46 Todd Brandt
  2016-04-27 19:31 ` Ville Syrjälä
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Todd Brandt @ 2016-04-27 18:46 UTC (permalink / raw)
  To: intel-gfx, todd.e.brandt; +Cc: rodrigo.vivi

 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.

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@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early
  2016-04-27 18:46 [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early Todd Brandt
@ 2016-04-27 19:31 ` Ville Syrjälä
  2016-04-27 20:17   ` Todd Brandt
  2016-04-27 20:33 ` Lukas Wunner
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Ville Syrjälä @ 2016-04-27 19:31 UTC (permalink / raw)
  To: Todd Brandt; +Cc: todd.e.brandt, intel-gfx, rodrigo.vivi

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.
> 
> 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/

Hmm. Looking at those makes me confused. Why isn't the pci bus
.resume_noirq hook (pci_pm_resume_noirq()) waking up our pci device?
Instead our wakeup gets delayed until .resume_early for some reason.

> 
> Signed-off-by: Todd Brandt <todd.e.brandt@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early
  2016-04-27 19:31 ` Ville Syrjälä
@ 2016-04-27 20:17   ` Todd Brandt
  2016-04-27 20:49     ` Ville Syrjälä
  0 siblings, 1 reply; 12+ messages in thread
From: Todd Brandt @ 2016-04-27 20:17 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: todd.e.brandt, intel-gfx, rodrigo.vivi

On Wed, 2016-04-27 at 22:31 +0300, Ville Syrjälä wrote:
> 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.
> > 
> > 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/
> 
> Hmm. Looking at those makes me confused. Why isn't the pci bus
> .resume_noirq hook (pci_pm_resume_noirq()) waking up our pci device?
> Instead our wakeup gets delayed until .resume_early for some reason.

Which timeline are you referring to? The "late" ones are the unaltered
versions.

> 
> > 
> > Signed-off-by: Todd Brandt <todd.e.brandt@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early
  2016-04-27 18:46 [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early Todd Brandt
  2016-04-27 19:31 ` Ville Syrjälä
@ 2016-04-27 20:33 ` Lukas Wunner
  2016-04-27 20:49   ` Todd Brandt
  2016-04-27 21:36   ` Imre Deak
  2016-04-28  6:46 ` ✓ Fi.CI.BAT: success for " Patchwork
  2016-04-29  9:38 ` [PATCH] " Ville Syrjälä
  3 siblings, 2 replies; 12+ messages in thread
From: Lukas Wunner @ 2016-04-27 20:33 UTC (permalink / raw)
  To: Todd Brandt; +Cc: intel-gfx, rodrigo.vivi

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.

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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early
  2016-04-27 20:33 ` Lukas Wunner
@ 2016-04-27 20:49   ` Todd Brandt
  2016-04-27 20:51     ` Todd Brandt
  2016-04-27 21:36   ` Imre Deak
  1 sibling, 1 reply; 12+ messages in thread
From: Todd Brandt @ 2016-04-27 20:49 UTC (permalink / raw)
  To: Lukas Wunner; +Cc: intel-gfx, rodrigo.vivi

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.


> 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@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early
  2016-04-27 20:17   ` Todd Brandt
@ 2016-04-27 20:49     ` Ville Syrjälä
  2016-04-27 21:27       ` Todd Brandt
  0 siblings, 1 reply; 12+ messages in thread
From: Ville Syrjälä @ 2016-04-27 20:49 UTC (permalink / raw)
  To: Todd Brandt; +Cc: todd.e.brandt, intel-gfx, rodrigo.vivi

On Wed, Apr 27, 2016 at 01:17:15PM -0700, Todd Brandt wrote:
> On Wed, 2016-04-27 at 22:31 +0300, Ville Syrjälä wrote:
> > 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.
> > > 
> > > 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/
> > 
> > Hmm. Looking at those makes me confused. Why isn't the pci bus
> > .resume_noirq hook (pci_pm_resume_noirq()) waking up our pci device?
> > Instead our wakeup gets delayed until .resume_early for some reason.
> 
> Which timeline are you referring to? The "late" ones are the unaltered
> versions.

Referring to the current code, ie. the "late" timeline.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early
  2016-04-27 20:49   ` Todd Brandt
@ 2016-04-27 20:51     ` Todd Brandt
  0 siblings, 0 replies; 12+ messages in thread
From: Todd Brandt @ 2016-04-27 20:51 UTC (permalink / raw)
  To: Lukas Wunner; +Cc: intel-gfx, rodrigo.vivi

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@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>


> 
> > 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@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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early
  2016-04-27 20:49     ` Ville Syrjälä
@ 2016-04-27 21:27       ` Todd Brandt
  0 siblings, 0 replies; 12+ messages in thread
From: Todd Brandt @ 2016-04-27 21:27 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: todd.e.brandt, intel-gfx, rodrigo.vivi

On Wed, 2016-04-27 at 23:49 +0300, Ville Syrjälä wrote:
> On Wed, Apr 27, 2016 at 01:17:15PM -0700, Todd Brandt wrote:
> > On Wed, 2016-04-27 at 22:31 +0300, Ville Syrjälä wrote:
> > > 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.
> > > > 
> > > > 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/
> > > 
> > > Hmm. Looking at those makes me confused. Why isn't the pci bus
> > > .resume_noirq hook (pci_pm_resume_noirq()) waking up our pci device?
> > > Instead our wakeup gets delayed until .resume_early for some reason.
> > 
> > Which timeline are you referring to? The "late" ones are the unaltered
> > versions.
> 
> Referring to the current code, ie. the "late" timeline.
> 

Well it does wakeup with noirq in the suspend mode (S3) version:
https://01org.github.io/suspendresume/i915/suspend-160422-155735-ivybridge-dev-late/

It's in freeze (S2) that it gets delayed to resume_early, which I
assumed was normal (I'll try it on other systems to compare):
https://01org.github.io/suspendresume/i915/freeze-160422-155931-ivybridge-dev-late/



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early
  2016-04-27 20:33 ` Lukas Wunner
  2016-04-27 20:49   ` Todd Brandt
@ 2016-04-27 21:36   ` Imre Deak
  2016-04-28  8:32     ` Daniel Vetter
  1 sibling, 1 reply; 12+ messages in thread
From: Imre Deak @ 2016-04-27 21:36 UTC (permalink / raw)
  To: Lukas Wunner, Todd Brandt; +Cc: intel-gfx, rodrigo.vivi

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@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 */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* ✓ Fi.CI.BAT: success for i915 suspend/resume_noirq instead of suspend_late/resume_early
  2016-04-27 18:46 [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early Todd Brandt
  2016-04-27 19:31 ` Ville Syrjälä
  2016-04-27 20:33 ` Lukas Wunner
@ 2016-04-28  6:46 ` Patchwork
  2016-04-29  9:38 ` [PATCH] " Ville Syrjälä
  3 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2016-04-28  6:46 UTC (permalink / raw)
  To: Todd Brandt; +Cc: intel-gfx

== Series Details ==

Series: i915 suspend/resume_noirq instead of suspend_late/resume_early
URL   : https://patchwork.freedesktop.org/series/6435/
State : success

== Summary ==

Series 6435v1 i915 suspend/resume_noirq instead of suspend_late/resume_early
http://patchwork.freedesktop.org/api/1.0/series/6435/revisions/1/mbox/


bdw-nuci7        total:201  pass:189  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:201  pass:176  dwarn:0   dfail:0   fail:0   skip:25 
byt-nuc          total:200  pass:159  dwarn:0   dfail:0   fail:0   skip:41 
hsw-brixbox      total:201  pass:175  dwarn:0   dfail:0   fail:0   skip:26 
ilk-hp8440p      total:201  pass:140  dwarn:0   dfail:0   fail:0   skip:61 
ivb-t430s        total:201  pass:170  dwarn:0   dfail:0   fail:0   skip:31 
skl-i7k-2        total:201  pass:174  dwarn:0   dfail:0   fail:0   skip:27 
skl-nuci5        total:201  pass:190  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:201  pass:159  dwarn:0   dfail:0   fail:0   skip:42 

Results at /archive/results/CI_IGT_test/Patchwork_2097/

c5c937220f88dcadac8da2377e141cf998968efc drm-intel-nightly: 2016y-04m-27d-19h-36m-40s UTC integration manifest
aa33d9d i915 suspend/resume_noirq instead of suspend_late/resume_early

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early
  2016-04-27 21:36   ` Imre Deak
@ 2016-04-28  8:32     ` Daniel Vetter
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2016-04-28  8:32 UTC (permalink / raw)
  To: Imre Deak; +Cc: rodrigo.vivi, intel-gfx

On Thu, Apr 28, 2016 at 12:36:29AM +0300, Imre Deak 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.
> 
> 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

device_pm_wait_for_dev sounds like the exact counterpart to
EDEFERRED_PROBE for solving load-time ordering issues. It makes sense, and
I think we should use it. Somehow I missed in that entire discussion that
this exists.

In both cases you can fry/deadlock your kernel if you disable the async
queues, and imo that shouldn't be a concern for us.

Of course cc Rafael, in case something changed in this area.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early
  2016-04-27 18:46 [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early Todd Brandt
                   ` (2 preceding siblings ...)
  2016-04-28  6:46 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2016-04-29  9:38 ` Ville Syrjälä
  3 siblings, 0 replies; 12+ messages in thread
From: Ville Syrjälä @ 2016-04-29  9:38 UTC (permalink / raw)
  To: Todd Brandt; +Cc: todd.e.brandt, intel-gfx, rodrigo.vivi

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.
> 
> 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/

BTW couple of unrelated observations from this data:

- The boot clock power cycle delay thing *seems* to be working. The
  schedule_timeout() wasn't actually shown in the data, but there were
  cases where the late vs. noirq had a huge difference in resume time.
  I can only assume that's because it spent more time actually suspended
  in one of the cases, and that allowd the power cycle delay to
  be shorter. Would be nice to see these sorts of things
  (wait_remaining_ms_from_jiffies(), wait_panel_status()) explicitly
  in the data.

- we really need to make the display ->detect() async at resume time.
  HDMI live status checks cost a ton of time. I think here too I might
  just suggest injecting a hpd for every connector and let the hpd
  machinery deal with it. Or we just toss out an uncoditional uevent
  and let userspace reprobe? But maybe this is still stuck behind MST
  explosions?

- With async modesets coming perhaps, we should make the resume modeset
  async as well

- We need to parallelize ->detect() for all connectors even from normal
  HPD. Again, HDMI live status checks do add up. The main snag here
  might be gmbus since we only have the one. It has a lock, but I'm not
  sure if it's a bit too low level to do this efficiently. Would need to
  measure it I suppose. I thought we had more gmbus instances on BXT,
  but now I can't find them anywhere, so maybe I was just confusing it
  with blc/pps or something.
  
> 
> Signed-off-by: Todd Brandt <todd.e.brandt@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-04-29  9:38 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-27 18:46 [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early Todd Brandt
2016-04-27 19:31 ` Ville Syrjälä
2016-04-27 20:17   ` Todd Brandt
2016-04-27 20:49     ` Ville Syrjälä
2016-04-27 21:27       ` Todd Brandt
2016-04-27 20:33 ` Lukas Wunner
2016-04-27 20:49   ` Todd Brandt
2016-04-27 20:51     ` Todd Brandt
2016-04-27 21:36   ` Imre Deak
2016-04-28  8:32     ` Daniel Vetter
2016-04-28  6:46 ` ✓ Fi.CI.BAT: success for " Patchwork
2016-04-29  9:38 ` [PATCH] " Ville Syrjälä

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox