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@xxxxxxxxxxxxxxxxxx>
> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > >Cc: <stable@xxxxxxxxxxxxxxx> # 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