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@xxxxxxxxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > ---
> > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
