On Wed, Aug 13, 2014 at 03:40:24PM +0200, Daniel Vetter wrote:
> 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 <ch...@chris-wilson.co.uk>
> > > > > > Cc: Daniel Vetter <daniel.vet...@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 ;-)
I disaggree with the conversion of the BUG_ON though, a WARN there is
going to screw up unpredictably (well, a hard hang without any output
is the predictable outcome). I'd like to have asserts for things that
could and should be statically analyzed...
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
