[Intel-gfx] [PATCH] drm/i915: Cancel outstanding modeset workers before suspend

[Chris Wilson]

On Tue, Aug 06, 2013 at 03:01:09PM +0200, Daniel Vetter wrote:
> On Tue, Aug 06, 2013 at 01:24:02PM +0100, Chris Wilson wrote:
> > Upon resume we will do a complete restoration of the mode and so reset
> > all tasks, but during suspend (and unload) we need to make sure that no
> > workers run concurrently with our suspend code. Or worse just after.
> > 
> > The issue was first raised whilst tackling a suspend issue with Takashi
> > Iwai (http://lists.freedesktop.org/archives/intel-gfx/2012-April/016738.html)
> > and then it was independently rediscovered by Chuanshen Lui.
> > 
> > v2: Rebase for the lost year.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Takashi Iwai <tiwai at suse.de>
> > Cc: "Liu, Chuansheng" <chuansheng.liu at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++++-----
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 64b5d55..cb2c59d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2130,6 +2130,7 @@ extern void intel_modeset_init_hw(struct drm_device *dev);
> >  extern void intel_modeset_suspend_hw(struct drm_device *dev);
> >  extern void intel_modeset_init(struct drm_device *dev);
> >  extern void intel_modeset_gem_init(struct drm_device *dev);
> > +extern void intel_modeset_quiesce(struct drm_device *dev);
> >  extern void intel_modeset_cleanup(struct drm_device *dev);
> >  extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
> >  extern void intel_modeset_setup_hw_state(struct drm_device *dev,
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7c7325e..0584480 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9891,6 +9891,7 @@ void intel_modeset_init_hw(struct drm_device *dev)
> >  
> >  void intel_modeset_suspend_hw(struct drm_device *dev)
> >  {
> > +	intel_modeset_quiesce(dev);
> >  	intel_suspend_hw(dev);
> >  }
> >  
> > @@ -10324,9 +10325,19 @@ void intel_modeset_gem_init(struct drm_device *dev)
> >  	intel_modeset_setup_hw_state(dev, false);
> >  }
> >  
> > -void intel_modeset_cleanup(struct drm_device *dev)
> > +void intel_modeset_quiesce(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	cancel_work_sync(&dev_priv->hotplug_work);
> > +	cancel_work_sync(&dev_priv->rps.work);
> > +
> > +	/* catch all required for dev_priv->wq */
> > +	flush_scheduled_work();
> 
> This only flushes the system workqueue, so the changed comment about
> dev_priv->wq is misleading. Can you please change it back?

No, because that would duplicate the comment. It's an entirely new
comment meant to remind us the use of the global wq. But you are
right, the dev_priv->wq mention is completely wrong. We need to flush
miscellaneous work items such as flip and unpin tasks. So
/* catch all required for everything else (e.g. unpin work) */

Since writing this patch, we've gained
  cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
  cancel_delayed_work_sync(&dev_priv->rps.vlv_work); 
which need to be taken in consideration.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre