Re: [PATCH] drm/i915: use delayed work for resume hotplug v2

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 8 Oct 2014 07:43:34 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue, Oct 07, 2014 at 01:25:23PM -0700, Jesse Barnes wrote:
> > Gets the detect code (which may take awhile) out of the resume path,
> > speeding things up a bit.
> > 
> > v2: use a delayed work queue instead (Daniel)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 10 ++++++++++
> >  drivers/gpu/drm/i915/i915_drv.c |  8 ++++++--
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  3 files changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 85d14e1..633095d 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1303,6 +1303,15 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
> >  	.can_switch = i915_switcheroo_can_switch,
> >  };
> >  
> > +static void intel_resume_hotplug(struct work_struct *work)
> > +{
> > +	struct drm_i915_private *dev_priv =
> > +		container_of(work, struct drm_i915_private,
> > +			     hotplug_resume_work.work);
> > +
> > +	drm_helper_hpd_irq_event(dev_priv->dev);
> > +}
> > +
> >  static int i915_load_modeset_init(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -1364,6 +1373,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >  
> >  	/* Only enable hotplug handling once the fbdev is fully set up. */
> >  	intel_hpd_init(dev_priv);
> > +	INIT_DELAYED_WORK(&dev_priv->hotplug_resume_work, intel_resume_hotplug);
> >  
> >  	/*
> >  	 * Some ports require correctly set-up hpd registers for detection to
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index a05a1d0..83075f9 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -726,8 +726,12 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> >  		 * notifications.
> >  		 * */
> >  		intel_hpd_init(dev_priv);
> > -		/* Config may have changed between suspend and resume */
> > -		drm_helper_hpd_irq_event(dev);
> > +		/* Config may have changed between suspend and resume. Just
> > +		 * try to make sure the rest of driver resume is finished
> > +		 * before we start probing for config changes.
> 
> This is a nice vague statement that scares me. If half of this comment
> is true, using a simple delay is periliously lackadaisical. The commit
> log just talks about speeding up resume, but this comment implies to me
> something more sinister.

Nothing sinister; nothing should blow up if we end up firing this
before resume completes. We mainly want to avoid hitting lock contention
from the mode set above if it ever becomes async.  So a rough estimate
ought to be fine (there's really nothing that should take long after
this point anyway).

Jesse