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@xxxxxxxxxxxxxxxxxx>
> -Chris
Thanks!
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
