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

Chris Wilson
2015-10-27T20:29:11Z

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

/* Track whether the FBC worker has already been queued,
* or asynchronously cancel the worker whilst it waits
* before activation.
*/

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.

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

--
Chris Wilson, Intel Open Source Technology Centre