Re: [PATCH 23/26] drm/i915: use a single intel_fbc_work struct

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

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 :)

> 
> 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.


> > +		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?

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.

> 
> I think I've covered the basic issues with changing the type of
> worker
> and it looks fine,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

Thanks!

> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx