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

[Daniel Vetter]

On Wed, Aug 13, 2014 at 01:58:30PM +0100, Chris Wilson wrote:
> On Wed, Aug 13, 2014 at 02:46:53PM +0200, Daniel Vetter wrote:
> > On Wed, Aug 13, 2014 at 01:09:46PM +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.
> > > 
> > > v2: Use an integral work item so that we can explicitly flush the work
> > > upon suspend/unload.
> > > 
> > > 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    |  3 ---
> > >  drivers/gpu/drm/i915/i915_drv.c    | 28 ++-----------------------
> > >  drivers/gpu/drm/i915/i915_drv.h    |  9 +-------
> > >  drivers/gpu/drm/i915/intel_drv.h   |  4 ++--
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 42 +++++++++++++++++++++++++++++++++++++-
> > >  5 files changed, 46 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 860da759ae34..fbab9370cb5c 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. */
> > > @@ -1857,7 +1855,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);
> > 
> > This one here seems to have been lost - shouldn't we move this to
> > fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
> > while merging if you're ok.
> 
> Hmm, I counted on us calling suspend before intel_fbdev_fini() in
> unload. Is this not the case? Otherwise I think we should be suspending
> the console.

We don't do any of that. Unregistering the fbdev emulation in fbdev_fini
should be enough though to stop anyone from actually using the gpu, and
we're careful to unregister other console drivers like vgacon to make sure
no one else can grow stupid ideas.

So I think the only concern really is that the work item completes before
the text section disappears, and maybe (on multi-gpu systems) that we
don't keep all fbdev devices disabled forever after a racy module unload.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch