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

[Matthew Brost <matthew.brost@intel.com>]

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.

Matt
 
> John.
> 
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@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)
>