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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC