[Intel-gfx] [PATCH 23/26] drm/i915: Make request conflict tracking understand parallel submits

[John Harrison]

On 10/12/2021 17:32, Matthew Brost wrote:
> On Tue, Oct 12, 2021 at 03:08:05PM -0700, John Harrison wrote:
>> On 10/4/2021 15:06, Matthew Brost wrote:
>>> If an object in the excl or shared slot is a composite fence from a
>>> parallel submit and the current request in the conflict tracking is from
>>> the same parallel context there is no need to enforce ordering as the
>>> ordering already implicit. Make the request conflict tracking understand
>> ordering already -> ordering is already
>>
> Yep.
>
>>> this by comparing the parents parallel fence values and skipping the
>> parents -> parent's
>>
> Yep.
>
>>> conflict insertion if the values match.
>> Presumably, this is to cope with the fact that the parallel submit fences do
>> not look like regular submission fences. And hence the existing code that
>> says 'new fence belongs to same context as old fence, so safe to ignore'
>> does not work with parallel submission. However, this change does not appear
> Yes. The check for 'if (fence->context == rq->fence.context)' doesn't
> work with parallel submission as each rq->fence.context corresponds to a
> timeline. With parallel submission each intel_context in the parallel
> submit has its own timeline (seqno) so the compare fails for different
> intel_context within the same parallel submit. This is the reason for
> the additional compare on parallel submits parents, if they have the
> same parent it is the same parallel submission and there is no need to
> enforce additional ordering.
>
>> to be adding parallel submit support to an existing 'same context' check. It
>> seems to be a brand new check that does not exist for single submission.
>> What makes parallel submit different? If we aren't skipping same context
>> fences for single submits, why do we need it for parallel? Conversely, if we
>> need it for parallel then why don't we need it for single?
>>
> I'm confused by what you are asking here. The existing same context
> check is fine for parallel submits - it will just return true when we
> compare requests with the same intel_context and new additional check
> only true parallel submissions with the same parent.
>
>> And if the single submission version is simply somewhere else in the code,
>> why do the parallel version here instead of at the same place?
>>
> Again I'm confused by what you are asking. We might just need to sync on
> a quick call.
That's okay. I think I had partly confused myself ;).

I was just meaning that the parallel compliant version of the 'ctxtA == 
ctxtB -> skip' test should be coded adjacent to the single submission 
version of the same test. I had somehow completely missed that the 
single submission version is indeed the line above in 
i915_request_await_execution(). So the two are indeed very definitely 
next to each other.

It's all good :).

John.


>
> Matt
>   
>> John.
>>
>>> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_request.c | 43 +++++++++++++++++++----------
>>>    1 file changed, 29 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>>> index e9bfa32f9270..cf89624020ad 100644
>>> --- a/drivers/gpu/drm/i915/i915_request.c
>>> +++ b/drivers/gpu/drm/i915/i915_request.c
>>> @@ -1325,6 +1325,25 @@ i915_request_await_external(struct i915_request *rq, struct dma_fence *fence)
>>>    	return err;
>>>    }
>>> +static inline bool is_parallel_rq(struct i915_request *rq)
>>> +{
>>> +	return intel_context_is_parallel(rq->context);
>>> +}
>>> +
>>> +static inline struct intel_context *request_to_parent(struct i915_request *rq)
>>> +{
>>> +	return intel_context_to_parent(rq->context);
>>> +}
>>> +
>>> +static bool is_same_parallel_context(struct i915_request *to,
>>> +				     struct i915_request *from)
>>> +{
>>> +	if (is_parallel_rq(to))
>> Should this not say '&& is_parallel_rq(from)'?
>>
>>> +		return request_to_parent(to) == request_to_parent(from);
>>> +
>>> +	return false;
>>> +}
>>> +
>>>    int
>>>    i915_request_await_execution(struct i915_request *rq,
>>>    			     struct dma_fence *fence)
>>> @@ -1356,11 +1375,14 @@ i915_request_await_execution(struct i915_request *rq,
>>>    		 * want to run our callback in all cases.
>>>    		 */
>>> -		if (dma_fence_is_i915(fence))
>>> +		if (dma_fence_is_i915(fence)) {
>>> +			if (is_same_parallel_context(rq, to_request(fence)))
>>> +				continue;
>>>    			ret = __i915_request_await_execution(rq,
>>>    							     to_request(fence));
>>> -		else
>>> +		} else {
>>>    			ret = i915_request_await_external(rq, fence);
>>> +		}
>>>    		if (ret < 0)
>>>    			return ret;
>>>    	} while (--nchild);
>>> @@ -1461,10 +1483,13 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
>>>    						 fence))
>>>    			continue;
>>> -		if (dma_fence_is_i915(fence))
>>> +		if (dma_fence_is_i915(fence)) {
>>> +			if (is_same_parallel_context(rq, to_request(fence)))
>>> +				continue;
>>>    			ret = i915_request_await_request(rq, to_request(fence));
>>> -		else
>>> +		} else {
>>>    			ret = i915_request_await_external(rq, fence);
>>> +		}
>>>    		if (ret < 0)
>>>    			return ret;
>>> @@ -1539,16 +1564,6 @@ i915_request_await_object(struct i915_request *to,
>>>    	return ret;
>>>    }
>>> -static inline bool is_parallel_rq(struct i915_request *rq)
>>> -{
>>> -	return intel_context_is_parallel(rq->context);
>>> -}
>>> -
>>> -static inline struct intel_context *request_to_parent(struct i915_request *rq)
>>> -{
>>> -	return intel_context_to_parent(rq->context);
>>> -}
>>> -
>>>    static struct i915_request *
>>>    __i915_request_ensure_parallel_ordering(struct i915_request *rq,
>>>    					struct intel_timeline *timeline)