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

Zanoni, Paulo R
2015-10-28T17:24:35Z

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!

>