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

[Daniel Vetter]

On Wed, Aug 13, 2014 at 12:32:33PM +0100, Chris Wilson wrote:
> 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.

Given that the work creates new work when it can't do it's job I don't
think that flush is sufficient. Iirc that only flush out pending stuff,
not newly queued work.

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

Open-coding console_lock(); fb_set_suspend(); console_unlock(); doesn't
really look onerous to me.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch