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@intel.com

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); }