[Intel-gfx] [PATCH 2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients

[Tvrtko Ursulin]

On 07/05/2020 16:05, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-05-07 15:59:56)
>>
>> On 07/05/2020 09:21, Chris Wilson wrote:
>>> The submit-fence adds a weak dependency to the requests, and for the
>>> purpose of our FQ_CODEL hinting we do not want to treat as a
>>> restriction. This is primarily because clients may treat submit-fences
>>> as a bidirectional bonding between a pair of co-ordinating requests.
>>>
>>> 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_execbuffer.c | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> index 966523a8503f..e8bf0cf02fd7 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> @@ -2565,6 +2565,17 @@ static void retire_requests(struct intel_timeline *tl, struct i915_request *end)
>>>                        break;
>>>    }
>>>    
>>> +static bool new_client(struct i915_request *rq)
>>> +{
>>> +     struct i915_dependency *p;
>>> +
>>> +     list_for_each_entry(p, &rq->sched.signalers_list, signal_link)
>>> +             if (!(p->flags & I915_DEPENDENCY_WEAK))
>>> +                     return false;
>>> +
>>> +     return true;
>>> +}
>>> +
>>>    static void eb_request_add(struct i915_execbuffer *eb)
>>>    {
>>>        struct i915_request *rq = eb->request;
>>> @@ -2604,7 +2615,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
>>>                 * Allow interactive/synchronous clients to jump ahead of
>>>                 * the bulk clients. (FQ_CODEL)
>>>                 */
>>> -             if (list_empty(&rq->sched.signalers_list))
>>> +             if (new_client(rq))
>>>                        attr.priority |= I915_PRIORITY_WAIT;
>>>        } else {
>>>                /* Serialise with context_close via the add_to_timeline */
>>>
>>
>> Did absence of this have any functional effect? I hope not, but anyway:
> 
> Bah, I have a new test case where this WAIT bumping is still upsetting us.
> 
> I don't think I have any choice but to rip it out if we have timeslicing
> enabled.
> 
> Would you prefer a complete remission of I915_PRIORITY_WAIT or keep it
> under if (!intel_engine_has_timeslicing(rq->engine)) ?

Doesn't feel worthwhile to keep it for just BDW right?

Regards,

Tvrtko