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

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-06-06 10:19:10)
> 
> On 06/06/2019 10:09, Chris Wilson wrote:
> > 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).
> 
> That's not the path which would be covered by !atomic_read(&ce->pin_count) ?

And when pin_count > 0 it would then skip due to !ce->state, leading us
back to the current problem.
-Chris