[Intel-gfx] [PATCH 23/26] drm/i915: use a single intel_fbc_work struct

[chris at chris-wilson.co.uk]

On Wed, Oct 28, 2015 at 05:24:18PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2015-10-27 às 20:29 +0000, Chris Wilson escreveu:
> > On Tue, Oct 27, 2015 at 02:50:25PM -0200, Paulo Zanoni wrote:
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index a9434d1..fdbe068 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -917,9 +917,11 @@ struct i915_fbc {
> > >  	bool active;
> > >  
> > >  	struct intel_fbc_work {
> > > -		struct delayed_work work;
> > > +		bool scheduled;
> > 
> > Ah, I found this confusingly named. scheduled implied to me that you
> > wanted work_pending(), but you just want to asynchronously cancel the
> > fbc worker. Just bool cancel? Or bool active? Though now I've come
> > full
> > circle and suggest bool scheduled. So
> 
> I agree 'bool scheduled' is not the perfect name. I thought about
> renaming it to 'bool cancel' many times. I'm willing to change in case
> someone requests it, but my understanding is that the paragraph above
> is not asking for a rename.
> 
> > 
> > /* Track whether the FBC worker has already been queued,
> >  * or asynchronously cancel the worker whilst it waits
> >  * before activation.
> >  */
> 
> I can add this, although if someone suggests a better name we may not
> need it :)

Even if we do rename the variable, we might as well keep the comment. I
think it's a reasonably succinct description of why that bool should
exist.

> > 
> > What happens then if we quickly queue, cancel and want to requeue?
> > The
> > schedule_work() fails as the task is already pending, but the
> > scheduled
> > flag gets reset, so it just works. Perfect.
> 
> /me is confused.
> 
> This case should work since everything is done with fbc.lock grabbed.

I was just thinking about the case where the bool scheduled=false but
the work was still queued, and you then wanted to reschedule it. It
works just fine, the original work item remains queued and gets
reactivated correctly

(I was trying to find a flaw in the code and decided that it wasn't a
flaw at all, at which point I was happy!)
 
> 
> > > +		struct work_struct work;
> > >  		struct drm_framebuffer *fb;
> > 
> > Hmm, don't we actually need to take references on the fb we schedule
> > for
> > activation? Since we already account for that the crtc->fb may be
> > changed between queuing the work and executing it, for extra paranoia
> > we
> > should ensure that we have a reference in work->fb. (long standing
> > bug,
> > might as well fix it before we see it in the wild, time for another
> > kms-flip race!)
> 
> I'm not super familiar with this area, so I have to ask: what bad
> things can happen if we don't have a reference on work->fb?

The worst depends on the locking - if we borrow the reference and
operate on work->fb == crtc->fb without the right lock, that can blow
up. Otherwise, as just use work->fb as a pointer, it is possible for the
fb to unref'ed and reallocated without us noticing and for us to then
incorrectly schedule the fbc (unless you can demonstrate that the
work->fb is guarded by the fbc.lock + bool scheduled).

> We're just comparing pointers here, so if work->fb is not referenced by
> the CRTC we won't do anything with it. If work->fb is referenced by the
> CRTC, it will already have a reference, right?
> 
> I'm also not 100% sure if it's even possible to have crtc->fb != work-
> >fb without anything else canceling the work thread, so I just kept the
> old code around for now.

Indeed, I'm not sure if it is possible but (a) we should check that we
do have sufficient locks to prevent crtc->fb disappearing, and (b)
verify that when crtc->fb is changed the work is cancelled. As a bonus,
document that work->fb is just a pointer whose validity is guarded by
bool scheduled (or whatever). Or even better, having just completed (a)
+ (b), you can drop the drm_framebuffer pointer from the work item
entirely and rely on crtc->fb.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre