[Intel-gfx] [PATCH] drm/i915: Skip context_barrier emission for unused contexts

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-06-05 11:40:27)
> 
> On 04/06/2019 16:24, Chris Wilson wrote:
> > The intent was to skip unused HW contexts by checking ce->state.
> > However, this only works for execlists where the ppGTT pointers is
> > stored inside the HW context. For gen7, the ppGTT is alongside the
> > logical state and must be updated on all active engines but, crucially,
> > only on active engines. As we need different checks, and to keep
> > context_barrier_task() agnostic, pass in the predicate.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110836
> > Fixes: 62c8e423450d ("drm/i915: Skip unused contexts for context_barrier_task()")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_context.c   | 15 ++++++++++++++-
> >   .../drm/i915/gem/selftests/i915_gem_context.c | 19 +++++++++++++++----
> >   2 files changed, 29 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 08721ef62e4e..6819b598d226 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -902,6 +902,7 @@ static void cb_retire(struct i915_active *base)
> >   I915_SELFTEST_DECLARE(static intel_engine_mask_t context_barrier_inject_fault);
> >   static int context_barrier_task(struct i915_gem_context *ctx,
> >                               intel_engine_mask_t engines,
> > +                             bool (*skip)(struct intel_context *ce, void *data),
> >                               int (*emit)(struct i915_request *rq, void *data),
> >                               void (*task)(void *data),
> >                               void *data)
> > @@ -931,7 +932,10 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> >                       break;
> >               }
> >   
> > -             if (!(ce->engine->mask & engines) || !ce->state)
> > +             if (!(ce->engine->mask & engines))
> > +                     continue;
> > +
> > +             if (skip && skip(ce, data))
> >                       continue;
> >   
> >               rq = intel_context_create_request(ce);
> > @@ -1058,6 +1062,14 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> >       return 0;
> >   }
> >   
> > +static bool skip_ppgtt_update(struct intel_context *ce, void *data)
> > +{
> > +     if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
> > +             return !ce->state;
> > +     else
> > +             return !atomic_read(&ce->pin_count);
> 
> Would "return !atomic_read(&ce->pin_count) || !ce->state;" work and 
> avoid the somewhat irky HAS_LOGICAL_RING_CONTEXTS check?

No, because we need the barrier on gen7 !rcs which doesn't have
ce->state (but does need to switch mm).
-Chris