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

[Tvrtko Ursulin]

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) ?

Regards,

Tvrtko