Re: [PATCH 10/27] drm/i915/gem: Remove engine auto-magic with FENCE_SUBMIT

[Jason Ekstrand <jason@jlekstrand.net>]

On Tue, May 4, 2021 at 3:56 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, May 03, 2021 at 10:57:31AM -0500, Jason Ekstrand wrote:
> > Even though FENCE_SUBMIT is only documented to wait until the request in
> > the in-fence starts instead of waiting until it completes, it has a bit
> > more magic than that.  If FENCE_SUBMIT is used to submit something to a
> > balanced engine, we would wait to assign engines until the primary
> > request was ready to start and then attempt to assign it to a different
> > engine than the primary.  There is an IGT test which exercises this by
> > submitting a primary batch to a specific VCS and then using FENCE_SUBMIT
> > to submit a secondary which can run on any VCS and have i915 figure out
> > which VCS to run it on such that they can run in parallel.
> >
> > However, this functionality has never been used in the real world.  The
> > media driver (the only user of FENCE_SUBMIT) always picks exactly two
> > physical engines to bond and never asks us to pick which to use.
>
> Maybe reference the specific igt you're break (and removing in the igt
> series to match this) here. Just for the record and all that.

Done.

> -Daniel
>
> >
> > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c  |  2 +-
> >  drivers/gpu/drm/i915/gt/intel_engine_types.h    |  7 -------
> >  .../drm/i915/gt/intel_execlists_submission.c    | 17 -----------------
> >  3 files changed, 1 insertion(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index d640bba6ad9ab..efb2fa3522a42 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -3474,7 +3474,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> >               if (args->flags & I915_EXEC_FENCE_SUBMIT)
> >                       err = i915_request_await_execution(eb.request,
> >                                                          in_fence,
> > -                                                        eb.engine->bond_execute);
> > +                                                        NULL);
> >               else
> >                       err = i915_request_await_dma_fence(eb.request,
> >                                                          in_fence);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > index 883bafc449024..68cfe5080325c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > @@ -446,13 +446,6 @@ struct intel_engine_cs {
> >        */
> >       void            (*submit_request)(struct i915_request *rq);
> >
> > -     /*
> > -      * Called on signaling of a SUBMIT_FENCE, passing along the signaling
> > -      * request down to the bonded pairs.
> > -      */
> > -     void            (*bond_execute)(struct i915_request *rq,
> > -                                     struct dma_fence *signal);
> > -
> >       /*
> >        * Call when the priority on a request has changed and it and its
> >        * dependencies may need rescheduling. Note the request itself may
> > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > index 14378b28169b7..635d6d2494d26 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > @@ -3547,22 +3547,6 @@ static void virtual_submit_request(struct i915_request *rq)
> >       spin_unlock_irqrestore(&ve->base.active.lock, flags);
> >  }
> >
> > -static void
> > -virtual_bond_execute(struct i915_request *rq, struct dma_fence *signal)
> > -{
> > -     intel_engine_mask_t allowed, exec;
> > -
> > -     allowed = ~to_request(signal)->engine->mask;
> > -
> > -     /* Restrict the bonded request to run on only the available engines */
> > -     exec = READ_ONCE(rq->execution_mask);
> > -     while (!try_cmpxchg(&rq->execution_mask, &exec, exec & allowed))
> > -             ;
> > -
> > -     /* Prevent the master from being re-run on the bonded engines */
> > -     to_request(signal)->execution_mask &= ~allowed;
> > -}
> > -
> >  struct intel_context *
> >  intel_execlists_create_virtual(struct intel_engine_cs **siblings,
> >                              unsigned int count)
> > @@ -3616,7 +3600,6 @@ intel_execlists_create_virtual(struct intel_engine_cs **siblings,
> >
> >       ve->base.schedule = i915_schedule;
> >       ve->base.submit_request = virtual_submit_request;
> > -     ve->base.bond_execute = virtual_bond_execute;
> >
> >       INIT_LIST_HEAD(virtual_queue(ve));
> >       ve->base.execlists.queue_priority_hint = INT_MIN;
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch