[Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Chris Wilson]

On Wed, Aug 13, 2014 at 01:26:40PM +0200, Daniel Vetter wrote:
> On Tue, Aug 12, 2014 at 03:15:17PM +0100, Chris Wilson wrote:
> > Rather than take and release the console_lock() around a non-existent
> > DRM_I915_FBDEV, move the lock acquisation into the callee where it will
> > be compiled out by the config option entirely. This includes moving the
> > deferred fb_set_suspend() dance and encapsulating it entirely within
> > intel_fbdev.c.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c    |  5 ++---
> >  drivers/gpu/drm/i915/i915_drv.c    | 26 +-------------------------
> >  drivers/gpu/drm/i915/i915_drv.h    |  8 --------
> >  drivers/gpu/drm/i915/intel_fbdev.c | 33 +++++++++++++++++++++++++++++++++
> >  4 files changed, 36 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 49149406fcd8..d0b3411fc13c 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >  	if (ret)
> >  		goto cleanup_irq;
> >  
> > -	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
> > -
> >  	intel_modeset_gem_init(dev);
> >  
> >  	/* Always safe in the mode setting case. */
> > @@ -1835,6 +1833,8 @@ int i915_driver_unload(struct drm_device *dev)
> >  		return ret;
> >  	}
> >  
> > +	flush_scheduled_work();
> 
> Tbh I prefer the explicit work flushing and keeping it in dev_priv. We can
> shovel it into the I915_FBDEV #ifdef that's already there though.
> 
> > +
> >  	i915_perf_unregister(dev);
> >  	intel_fini_runtime_pm(dev_priv);
> >  
> > @@ -1859,7 +1859,6 @@ int i915_driver_unload(struct drm_device *dev)
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> >  		intel_fbdev_fini(dev);
> >  		intel_modeset_cleanup(dev);
> > -		cancel_work_sync(&dev_priv->console_resume_work);
> >  
> >  		/*
> >  		 * free the memory space allocated for the child device
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 35b119072c11..9adafc7c5819 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  	intel_uncore_forcewake_reset(dev, false);
> >  	intel_opregion_fini(dev);
> >  
> > -	console_lock();
> >  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
> 
> This one here must be synchronous.

Right, but notice that we synchronize the work afterwards. I thought if
we were careful enough to call the waiter that waited for additional
work items to be completed, it would be sufficient.

> > +static void intel_fbdev_set_suspend_worker(struct work_struct *work)
> > +{
> > +	struct set_suspend_work *ss = container_of(work, typeof(*ss), work);
> > +	intel_fbdev_set_suspend(ss->dev, ss->state);
> 
> This still trylocks so ends up busy-looping through launching more work
> items. Probably better also do this synchronously.

Considered that, but I decided that trying to keep the locking
straightforward was better.

I'll look at putting the work item back onto the dev_priv so that we can
cleanly flush it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre