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
>
> > this by comparing the parents parallel fence values and skipping the
> parents -> parent's
>
> > 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
> 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?
>
> 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?
>
> John.
>
> >
> > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > ---
> > 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)'?
>
Missed this one. That isn't necessary as if from is not a parallel
submit the following compare of parents will always return false. I
could add if you insist as either way works.
Matt
> > + 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)
>
