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

[Chris Wilson]

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 at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre