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

[Tvrtko Ursulin]

On 06/06/2019 12:06, Chris Wilson wrote:
> 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.

Brain fart.. Okay.. but pin_count check itself is not sufficient for 
both platforms? Can't we skip pin_count == 0 && ce->state != NULL on 
execlists?

Regards,

Tvrtko