On Mon, Dec 06, 2021 at 12:01:04PM -0800, John Harrison wrote:
> On 11/11/2021 13:20, Matthew Brost wrote:
> > A weak implementation of parallel submission (multi-bb execbuf IOCTL) for
> > execlists. Doing as little as possible to support this interface for
> > execlists - basically just passing submit fences between each request
> > generated and virtual engines are not allowed. This is on par with what
> > is there for the existing (hopefully soon deprecated) bonding interface.
> >
> > We perma-pin these execlists contexts to align with GuC implementation.
> >
> > v2:
> > (John Harrison)
> > - Drop siblings array as num_siblings must be 1
> > v3:
> > (John Harrison)
> > - Drop single submission
> >
> > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_context.c | 10 +++--
> > drivers/gpu/drm/i915/gt/intel_context.c | 4 +-
> > .../drm/i915/gt/intel_execlists_submission.c | 40 +++++++++++++++++++
> > drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +
> > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 -
> > 5 files changed, 50 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index ebd775cb1661c..d7bf6c8f70b7b 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -570,10 +570,6 @@ set_proto_ctx_engines_parallel_submit(struct i915_user_extension __user *base,
> > struct intel_engine_cs **siblings = NULL;
> > intel_engine_mask_t prev_mask;
> > - /* FIXME: This is NIY for execlists */
> > - if (!(intel_uc_uses_guc_submission(&i915->gt.uc)))
> > - return -ENODEV;
> > -
> > if (get_user(slot, &ext->engine_index))
> > return -EFAULT;
> > @@ -583,6 +579,12 @@ set_proto_ctx_engines_parallel_submit(struct i915_user_extension __user *base,
> > if (get_user(num_siblings, &ext->num_siblings))
> > return -EFAULT;
> > + if (!intel_uc_uses_guc_submission(&i915->gt.uc) && num_siblings != 1) {
> > + drm_dbg(&i915->drm, "Only 1 sibling (%d) supported in non-GuC mode\n",
> > + num_siblings);
> > + return -EINVAL;
> > + }
> > +
> > if (slot >= set->num_engines) {
> > drm_dbg(&i915->drm, "Invalid placement value, %d >= %d\n",
> > slot, set->num_engines);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> > index 5634d14052bc9..1bec92e1d8e63 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > @@ -79,7 +79,8 @@ static int intel_context_active_acquire(struct intel_context *ce)
> > __i915_active_acquire(&ce->active);
> > - if (intel_context_is_barrier(ce) || intel_engine_uses_guc(ce->engine))
> > + if (intel_context_is_barrier(ce) || intel_engine_uses_guc(ce->engine) ||
> > + intel_context_is_parallel(ce))
> > return 0;
> > /* Preallocate tracking nodes */
> > @@ -563,7 +564,6 @@ void intel_context_bind_parent_child(struct intel_context *parent,
> > * Callers responsibility to validate that this function is used
> > * correctly but we use GEM_BUG_ON here ensure that they do.
> > */
> > - GEM_BUG_ON(!intel_engine_uses_guc(parent->engine));
> > GEM_BUG_ON(intel_context_is_pinned(parent));
> > GEM_BUG_ON(intel_context_is_child(parent));
> > GEM_BUG_ON(intel_context_is_pinned(child));
> > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > index ca03880fa7e49..5fd49ee47096d 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > @@ -2598,6 +2598,45 @@ static void execlists_context_cancel_request(struct intel_context *ce,
> > current->comm);
> > }
> > +static struct intel_context *
> > +execlists_create_parallel(struct intel_engine_cs **engines,
> > + unsigned int num_siblings,
> > + unsigned int width)
> > +{
> > + struct intel_context *parent = NULL, *ce, *err;
> > + int i;
> > +
> > + GEM_BUG_ON(num_siblings != 1);
> > +
> > + for (i = 0; i < width; ++i) {
> > + ce = intel_context_create(engines[i]);
> > + if (!ce) {
> > + err = ERR_PTR(-ENOMEM);
> intel_context_create already checks for null and returns -ENOMEM. This needs
> to check for IS_ERR(ce).
>
Yep.
> > + goto unwind;
> > + }
> > +
> > + if (i == 0)
> > + parent = ce;
> > + else
> > + intel_context_bind_parent_child(parent, ce);
> > + }
> > +
> > + parent->parallel.fence_context = dma_fence_context_alloc(1);
> > +
> > + intel_context_set_nopreempt(parent);
> > + for_each_child(parent, ce) {
> > + intel_context_set_nopreempt(ce);
> > + intel_context_set_single_submission(ce);
> I thought the single submission thing wasn't wanted anymore?
>
Yep.
> > + }
> > +
> > + return parent;
> > +
> > +unwind:
> > + if (parent)
> > + intel_context_put(parent);
> > + return err;
> > +}
> > +
> > static const struct intel_context_ops execlists_context_ops = {
> > .flags = COPS_HAS_INFLIGHT,
> > @@ -2616,6 +2655,7 @@ static const struct intel_context_ops execlists_context_ops = {
> > .reset = lrc_reset,
> > .destroy = lrc_destroy,
> > + .create_parallel = execlists_create_parallel,
> > .create_virtual = execlists_create_virtual,
> > };
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 56156cf18c413..70f4b309522d3 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1065,6 +1065,8 @@ lrc_pin(struct intel_context *ce,
> > void lrc_unpin(struct intel_context *ce)
> > {
> > + if (unlikely(ce->parallel.last_rq))
> > + i915_request_put(ce->parallel.last_rq);
> Should set this to null after to prevent the possibility of a double put?
>
Not needed as parallel contexts are perma-pinnned and only unpinned
once in their lifetime.
That being said, will set it to NULL anyways to be safe.
Matt
> John.
>
> > check_redzone((void *)ce->lrc_reg_state - LRC_STATE_OFFSET,
> > ce->engine);
> > }
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index 5cc49c0b38897..cd1784953d679 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -3235,8 +3235,6 @@ static void guc_parent_context_unpin(struct intel_context *ce)
> > GEM_BUG_ON(!intel_context_is_parent(ce));
> > GEM_BUG_ON(!intel_engine_is_virtual(ce->engine));
> > - if (ce->parallel.last_rq)
> > - i915_request_put(ce->parallel.last_rq);
> > unpin_guc_id(guc, ce);
> > lrc_unpin(ce);
> > }
>
