On Wed, Mar 29, 2017 at 11:00:52AM +0100, Chris Wilson wrote:
> This reverts commit 6c943de6686f ("drm/i915: Skip execlists_dequeue()
> early if the list is empty").
>
> The validity of using READ_ONCE there depends upon having a mb to
> coordinate the assignment of engine->execlist_first inside
> submit_request() and checking prior to taking the spinlock in
> execlists_dequeue(). We wrote "the update to TASKLET_SCHED incurs a
> memory barrier making this cross-cpu checking safe", but failed to
> notice that this mb was *conditional* on the execlists being ready, i.e.
> there wasn't the request mb when it was most necessary!
s/request/required/
> We could install an unconditional memory barrier to fixup the
> READ_ONCE():
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> b/drivers/gpu/drm/i915/intel_lrc.c
> index 7dd732cb9f57..1ed164b16d44 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -616,6 +616,7 @@ static void execlists_submit_request(struct
> drm_i915_gem_request *request)
>
> if (insert_request(&request->priotree, &engine->execlist_queue))
> {
> engine->execlist_first = &request->priotree.node;
> + smp_wmb();
> if (execlists_elsp_ready(engine))
>
> But we have opted to remove the race as it should be rarely effective,
> and saves us having to explain the necessary memory barriers which we
> quite clearly failed at.
>
Reported-and-tested-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> Fixes: 6c943de6686f ("drm/i915: Skip execlists_dequeue() early if the list is empty")
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
