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

[Chris Wilson]

On Mon, Mar 27, 2017 at 10:06:45PM +0100, Chris Wilson wrote:
> 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.

Ok, not significant enough to even merit further consideration, just a
fun peak under the lockdep covers.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre