Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 13, 2014 at 02:09:22PM +0100, Chris Wilson wrote:
> On Wed, Aug 13, 2014 at 03:04:02PM +0200, Daniel Vetter wrote:
> > 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@chris-wilson.co.uk>
> > > > > Cc: Daniel Vetter <daniel.vetter@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.
> 
> Ok, I followed the unload->fbdev_fini->unregister_console and concur
> that all you want is the flush_work() in fbdev_fini.

Done&merged, thanks a lot for the patch - looks much better than my #ifdef
hacks ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch