[Intel-gfx] [PATCH] drm/i915: Keep all engine locks across scheduling

[Chris Wilson]

On Mon, Mar 27, 2017 at 11:11:47AM +0100, Tvrtko Ursulin wrote:
> 
> On 26/03/2017 09:46, 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 dependees.
> >
> >Fixes: 20311bd35060 ("drm/i915/scheduler: Execute requests in order of priorities")
> >Testcase: igt/gem_exec_whisper
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >Cc: <stable at vger.kernel.org> # v4.10+
> >---
> > drivers/gpu/drm/i915/intel_lrc.c | 54 +++++++++++++++++++++++++++-------------
> > 1 file changed, 37 insertions(+), 17 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index dd0e9d587852..3fdabba0a32d 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -658,30 +658,47 @@ static void execlists_submit_request(struct drm_i915_gem_request *request)
> > 	spin_unlock_irqrestore(&engine->timeline->lock, flags);
> > }
> >
> >-static struct intel_engine_cs *
> >-pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *locked)
> >+static inline struct intel_engine_cs *
> >+pt_lock_engine(struct i915_priotree *pt, unsigned long *locked)
> > {
> >-	struct intel_engine_cs *engine;
> >-
> >-	engine = container_of(pt,
> >-			      struct drm_i915_gem_request,
> >-			      priotree)->engine;
> >-	if (engine != locked) {
> >-		if (locked)
> >-			spin_unlock_irq(&locked->timeline->lock);
> >-		spin_lock_irq(&engine->timeline->lock);
> >-	}
> >+	struct intel_engine_cs *engine =
> >+		container_of(pt, struct drm_i915_gem_request, priotree)->engine;
> >+
> >+	/* Locking the engines in a random order will rightfully trigger a
> >+	 * spasm in lockdep. However, we can ignore lockdep (by marking each
> >+	 * as a seperate nesting) so long as we never nest the
> >+	 * engine->timeline->lock elsewhere. Also the number of nesting
> >+	 * subclasses is severely limited (7) which is going to cause an
> >+	 * issue at some point.
> >+	 * BUILD_BUG_ON(I915_NUM_ENGINES >= MAX_LOCKDEP_SUBCLASSES);
> 
> Lets bite the bullet and not hide this BUILD_BUG_ON in a comment. :I

Another option appears to be to disable lockdep for the global engine locks:

diff --git a/drivers/gpu/drm/i915/i915_gem_timeline.c b/drivers/gpu/drm/i915/i91
index b596ca7..f1b7dbe 100644
--- a/drivers/gpu/drm/i915/i915_gem_timeline.c
+++ b/drivers/gpu/drm/i915/i915_gem_timeline.c
@@ -73,12 +73,11 @@ int i915_gem_timeline_init(struct drm_i915_private *i915,
 
 int i915_gem_timeline_init__global(struct drm_i915_private *i915)
 {
-       static struct lock_class_key class;
-
        return __i915_gem_timeline_init(i915,
                                        &i915->gt.global_timeline,
                                        "[execution]",
-                                       &class, "&global_timeline->lock");
+                                       &__lockdep_no_validate__,
+                                       "&global_timeline->lock");
 }

Keeping the shortcut does speed up the rescheduling, but we still have
the icky nesting that requires a fat comment and games with lockdep.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre