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

Chris Wilson chris at chris-wilson.co.uk
Tue Apr 17 15:30:35 CEST 2012

On Tue, 17 Apr 2012 12:33:46 +0200, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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 at 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
Indeed looks superfluous. If we consider the unlikelihood the cleanup
code being well-tested, I'd prefer that we did do something like
drain_workqueue() as the first step in unload().

> - move the comment about syncing up with unpin_work to where we actually
>   sync up

I'm not finding another point where we explicitly sync with outstanding
unpin work. Probably due to the comment being in the wrong place...

> - and kill the superflous cancel_work?
Which also reminds me that the unpin_work holds onto a few references
that need to be released as well as the kfree. See above about these code
paths being relatively untrod.

Chris Wilson, Intel Open Source Technology Centre

More information about the Intel-gfx mailing list