[Intel-gfx] [PATCH] Revert "drm/i915: Skip execlists_dequeue() early if the list is empty"

[Chris Wilson]

On Wed, Mar 29, 2017 at 12:18:57PM +0100, Chris Wilson wrote:
> 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 at linux.intel.com>
> > Fixes: 6c943de6686f ("drm/i915: Skip execlists_dequeue() early if the list is empty")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Michał Winiarski <michal.winiarski at intel.com>

Pushed with Tvrtko's r-b from irc.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre