Re: [PATCH] drm/i915: Only free the unpin_work if cancelled before being run

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 17, 2012 at 10:53:24AM +0100, Chris Wilson wrote:
> On Tue, 17 Apr 2012 10:29:38 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > The unpin worker frees it work struct and so during intel_crtc_disable
> > we should only also free the work struct if cancel_work_sync() reports
> > that it successfully cancelled the work prior to it being executed and
> > thus avoid the double free.
> > 
> > The impact is only for people unloading modules during a fullscreen game
> > or movie playback, so extremely small.
> 
> Futher review (hunting for some sign of workqueue corruption, cf
> https://bugs.freedesktop.org/show_bug.cgi?id=48798) says that if work is
> non-NULL here it will not have been scheduled so cancel_work_sync() will
> always return true.

Well, I've failed to notice this while reviewing the unpin_work life-cycle
...

> Which also means that we have no way of waiting upon the scheduled
> unpin_work. :|

... but have noticed that we lose any reference from intel_crtc to the
unpin work once it's scheduled, and checked that we properly flush the
work queue: See the ordering of irq disable, flush workqueue, then crtc
destroy (in mode_config_cleanup).

We even have a flush_workqueu in i915_dma.c before tearing down gem, but
that won't work too well now that unpin also frobs around with the fbc
state (which is gone by now). But there's still the misleading comment
that this syncs up with unpin work.

Care to clean this up a bit by
- ditching the unnecessary flush_workqueu in i915_dma.c
- move the comment about syncing up with unpin_work to where we actually
  sync up
- and kill the superflous cancel_work?

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48