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@xxxxxxxxxxxxxxxxxx> > >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > >>> --- > >>> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
