On Wed, Mar 29, 2017 at 10:33:47AM +0100, Tvrtko Ursulin wrote:
>
> On 27/03/2017 21:21, Chris Wilson wrote:
> >Unlocking is dangerous. In this case we combine an early update to the
> >out-of-queue request, because we know that it will be inserted into the
> >correct FIFO priority-ordered slot when it becomes ready in the future.
> >However, given sufficient enthusiasm, it may become ready as we are
> >continuing to reschedule, and so may gazump the FIFO if we have since
> >dropped its spinlock. The result is that it may be executed too early,
> >before its dependencies.
> >
> >v2: Move all work into the second phase over the topological sort. This
> >removes the shortcut on the out-of-rbtree request to ensure that we only
> >adjust its priority after adjusting all of its dependencies.
> >
> >Fixes: 20311bd35060 ("drm/i915/scheduler: Execute requests in order of priorities")
> >Testcase: igt/gem_exec_whisper
> >Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >Cc: <stable@xxxxxxxxxxxxxxx> # v4.10+
> >---
> > drivers/gpu/drm/i915/intel_lrc.c | 44 ++++++++++++++++++----------------------
> > 1 file changed, 20 insertions(+), 24 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index b0c3a029b592..91e38e80a095 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -665,8 +665,8 @@ pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *locked)
> > priotree)->engine;
> > if (engine != locked) {
> > if (locked)
>
> Could replace "if (locked)" with a GEM_BUG_ON(!locked) now.
>
> >- spin_unlock_irq(&locked->timeline->lock);
> >- spin_lock_irq(&engine->timeline->lock);
> >+ spin_unlock(&locked->timeline->lock);
> >+ spin_lock(&engine->timeline->lock);
> > }
> >
> > return engine;
[snip]
> Looks OK to me. Preferably with the tidy in pt_lock_engine:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Pushed with the suggested improvement, thanks.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre