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@xxxxxxxxxxxxxxxxxx>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
