On Tue, Apr 27, 2021 at 4:32 AM Daniel Vetter daniel@ffwll.ch wrote:

On Fri, Apr 23, 2021 at 05:31:11PM -0500, Jason Ekstrand wrote:

...

This reverts commit 88be76cdafc7 ("drm/i915: Allow userspace to specify ringsize on construction"). This API was originally added for OpenCL but the compute-runtime PR has sat open for a year without action so we can still pull it out if we want. I argue we should drop it for three reasons:

1. If the compute-runtime PR has sat open for a year, this clearly isn't that important.

2. It's a very leaky API. Ring size is an implementation detail of the current execlist scheduler and really only makes sense there. It can't apply to the older ring-buffer scheduler on pre-execlist hardware because that's shared across all contexts and it won't apply to the GuC scheduler that's in the pipeline.

3. Having userspace set a ring size in bytes is a bad solution to the problem of having too small a ring. There is no way that userspace has the information to know how to properly set the ring size so it's just going to detect the feature and always set it to the maximum of 512K. This is what the compute-runtime PR does. The scheduler in i915, on the other hand, does have the information to make an informed choice. It could detect if the ring size is a problem and grow it itself. Or, if that's too hard, we could just increase the default size from 16K to 32K or even 64K instead of relying on userspace to do it.

Let's drop this API for now and, if someone decides they really care about solving this problem, they can do it properly.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net

Two things:

• I'm assuming you have an igt change to make sure we get EINVAL for both set and getparam now? Just to make sure.

I've written up some quick tests. I'll send them out in the next version of the IGT series or as a separate series if that one gets reviewed without comment (unlikely).

• intel_context->ring is either a ring pointer when CONTEXT_ALLOC_BIT is set in ce->flags, or the size of the ring stored in the pointer if not. I'm seriously hoping you get rid of this complexity with your proto-context series, and also delete __intel_context_ring_size() in the end. That function has no business existing imo.

I hadn't done that yet, no. But I typed up a patch today which I'll send out with the next version of this series which does this.

--Jason

If not, please make sure that's the case.

Aside from these patch looks good.

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

...

---

drivers/gpu/drm/i915/Makefile | 1 - drivers/gpu/drm/i915/gem/i915_gem_context.c | 85 +------------------ drivers/gpu/drm/i915/gt/intel_context_param.c | 63 -------------- drivers/gpu/drm/i915/gt/intel_context_param.h | 3 - include/uapi/drm/i915_drm.h | 20 +---- 5 files changed, 4 insertions(+), 168 deletions(-) delete mode 100644 drivers/gpu/drm/i915/gt/intel_context_param.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index d0d936d9137bc..afa22338fa343 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -88,7 +88,6 @@ gt-y += \ gt/gen8_ppgtt.o \ gt/intel_breadcrumbs.o \ gt/intel_context.o \

• gt/intel_context_param.o \
  gt/intel_context_sseu.o \
  gt/intel_engine_cs.o \
  gt/intel_engine_heartbeat.o \
  

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index fd8ee52e17a47..e52b85b8f923d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1335,63 +1335,6 @@ static int set_ppgtt(struct drm_i915_file_private *file_priv, return err; }

-static int __apply_ringsize(struct intel_context *ce, void *sz) -{

• return intel_context_set_ring_size(ce, (unsigned long)sz);
  

-}

-static int set_ringsize(struct i915_gem_context *ctx,

•                 struct drm_i915_gem_context_param *args)
  

-{

• if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
  

•         return -ENODEV;
  

• if (args->size)
  

•         return -EINVAL;
  

• if (!IS_ALIGNED(args->value, I915_GTT_PAGE_SIZE))
  

•         return -EINVAL;
  

• if (args->value < I915_GTT_PAGE_SIZE)
  

•         return -EINVAL;
  

• if (args->value > 128 * I915_GTT_PAGE_SIZE)
  

•         return -EINVAL;
  

• return context_apply_all(ctx,
  

•                          __apply_ringsize,
  

•                          __intel_context_ring_size(args->value));
  

-}

-static int __get_ringsize(struct intel_context *ce, void *arg) -{

• long sz;
  

• sz = intel_context_get_ring_size(ce);
  

• GEM_BUG_ON(sz > INT_MAX);
  

• return sz; /* stop on first engine */
  

-}

-static int get_ringsize(struct i915_gem_context *ctx,

•                 struct drm_i915_gem_context_param *args)
  

-{

• int sz;
  

• if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
  

•         return -ENODEV;
  

• if (args->size)
  

•         return -EINVAL;
  

• sz = context_apply_all(ctx, __get_ringsize, NULL);
  

• if (sz < 0)
  

•         return sz;
  

• args->value = sz;
  

• return 0;
  

-}

int i915_gem_user_to_context_sseu(struct intel_gt *gt, const struct drm_i915_gem_context_param_sseu *user, @@ -2037,11 +1980,8 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv, ret = set_persistence(ctx, args); break;

• case I915_CONTEXT_PARAM_RINGSIZE:
  

•         ret = set_ringsize(ctx, args);
  

•         break;
  

• case I915_CONTEXT_PARAM_BAN_PERIOD:
  

• case I915_CONTEXT_PARAM_RINGSIZE:
  default:
          ret = -EINVAL;
          break;
  

@@ -2069,18 +2009,6 @@ static int create_setparam(struct i915_user_extension __user *ext, void *data) return ctx_setparam(arg->fpriv, arg->ctx, &local.param); }

-static int copy_ring_size(struct intel_context *dst,

•                   struct intel_context *src)
  

-{

• long sz;
  

• sz = intel_context_get_ring_size(src);
  

• if (sz < 0)
  

•         return sz;
  

• return intel_context_set_ring_size(dst, sz);
  

-}

static int clone_engines(struct i915_gem_context *dst, struct i915_gem_context *src) { @@ -2125,12 +2053,6 @@ static int clone_engines(struct i915_gem_context *dst, }

          intel_context_set_gem(clone->engines[n], dst);

•         /* Copy across the preferred ringsize */
  

•         if (copy_ring_size(clone->engines[n], e->engines[n])) {
  

•                 __free_engines(clone, n + 1);
  

•                 goto err_unlock;
  

•         }
  }
  clone->num_engines = n;
  i915_sw_fence_complete(&e->fence);
  

@@ -2490,11 +2412,8 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, args->value = i915_gem_context_is_persistent(ctx); break;

• case I915_CONTEXT_PARAM_RINGSIZE:
  

•         ret = get_ringsize(ctx, args);
  

•         break;
  

• case I915_CONTEXT_PARAM_BAN_PERIOD:
  

• case I915_CONTEXT_PARAM_RINGSIZE:
  default:
          ret = -EINVAL;
          break;
  

diff --git a/drivers/gpu/drm/i915/gt/intel_context_param.c b/drivers/gpu/drm/i915/gt/intel_context_param.c deleted file mode 100644 index 65dcd090245d6..0000000000000 --- a/drivers/gpu/drm/i915/gt/intel_context_param.c +++ /dev/null @@ -1,63 +0,0 @@ -// SPDX-License-Identifier: MIT -/*

• • Copyright © 2019 Intel Corporation
• */

-#include "i915_active.h" -#include "intel_context.h" -#include "intel_context_param.h" -#include "intel_ring.h"

-int intel_context_set_ring_size(struct intel_context *ce, long sz) -{

• int err;
  

• if (intel_context_lock_pinned(ce))
  

•         return -EINTR;
  

• err = i915_active_wait(&ce->active);
  

• if (err < 0)
  

•         goto unlock;
  

• if (intel_context_is_pinned(ce)) {
  

•         err = -EBUSY; /* In active use, come back later! */
  

•         goto unlock;
  

• }
  

• if (test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
  

•         struct intel_ring *ring;
  

•         /* Replace the existing ringbuffer */
  

•         ring = intel_engine_create_ring(ce->engine, sz);
  

•         if (IS_ERR(ring)) {
  

•                 err = PTR_ERR(ring);
  

•                 goto unlock;
  

•         }
  

•         intel_ring_put(ce->ring);
  

•         ce->ring = ring;
  

•         /* Context image will be updated on next pin */
  

• } else {
  

•         ce->ring = __intel_context_ring_size(sz);
  

• }
  

-unlock:

• intel_context_unlock_pinned(ce);
  

• return err;
  

-}

-long intel_context_get_ring_size(struct intel_context *ce) -{

• long sz = (unsigned long)READ_ONCE(ce->ring);
  

• if (test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
  

•         if (intel_context_lock_pinned(ce))
  

•                 return -EINTR;
  

•         sz = ce->ring->size;
  

•         intel_context_unlock_pinned(ce);
  

• }
  

• return sz;
  

-} diff --git a/drivers/gpu/drm/i915/gt/intel_context_param.h b/drivers/gpu/drm/i915/gt/intel_context_param.h index 3ecacc675f414..dffedd983693d 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_param.h +++ b/drivers/gpu/drm/i915/gt/intel_context_param.h @@ -10,9 +10,6 @@

#include "intel_context.h"

-int intel_context_set_ring_size(struct intel_context *ce, long sz); -long intel_context_get_ring_size(struct intel_context *ce);

static inline int intel_context_set_watchdog_us(struct intel_context *ce, u64 timeout_us) { diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 6a34243a7646a..6eefbc6dec01f 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1721,24 +1721,8 @@ struct drm_i915_gem_context_param { */ #define I915_CONTEXT_PARAM_PERSISTENCE 0xb

-/*

• • I915_CONTEXT_PARAM_RINGSIZE:
• • Sets the size of the CS ringbuffer to use for logical ring contexts. This
• • applies a limit of how many batches can be queued to HW before the caller
• • is blocked due to lack of space for more commands.
• • Only reliably possible to be set prior to first use, i.e. during
• • construction. At any later point, the current execution must be flushed as
• • the ring can only be changed while the context is idle. Note, the ringsize
• • can be specified as a constructor property, see
• • I915_CONTEXT_CREATE_EXT_SETPARAM, but can also be set later if required.
• • Only applies to the current set of engine and lost when those engines
• • are replaced by a new mapping (see I915_CONTEXT_PARAM_ENGINES).
• • Must be between 4 - 512 KiB, in intervals of page size [4 KiB].
• • Default is 16 KiB.

+/* This API has been removed. On the off chance someone somewhere has

• • attempted to use it, never re-use this context param number.
  */

#define I915_CONTEXT_PARAM_RINGSIZE 0xc /* Must be kept compact -- no holes and well documented */ -- 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

On Fri, Apr 23, 2021 at 05:31:12PM -0500, Jason Ekstrand wrote:

The idea behind this param is to support OpenCL drivers with relocations because OpenCL reserves 0x0 for NULL and, if we placed memory there, it would confuse CL kernels. It was originally sent out as part of a patch series including libdrm [1] and Beignet [2] support. However, the libdrm and Beignet patches never landed in their respective upstream projects so this API has never been used. It's never been used in Mesa or any other driver, either.

Dropping this API allows us to delete a small bit of code.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net

Same thing about an igt making sure we reject these. Maybe an entire wash-up igt which validates all the new restrictions on get/setparam (including that after execbuf it's even more strict). -Daniel

---

drivers/gpu/drm/i915/gem/i915_gem_context.c | 16 ++-------------- .../gpu/drm/i915/gem/i915_gem_context_types.h | 1 - drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 -------- include/uapi/drm/i915_drm.h | 4 ++++ 4 files changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index e52b85b8f923d..35bcdeddfbf3f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1922,15 +1922,6 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv, int ret = 0;

switch (args->param) {

• case I915_CONTEXT_PARAM_NO_ZEROMAP:

• if (args->size)
  

• 	ret = -EINVAL;
  

• else if (args->value)
  

• 	set_bit(UCONTEXT_NO_ZEROMAP, &ctx->user_flags);
  

• else
  

• 	clear_bit(UCONTEXT_NO_ZEROMAP, &ctx->user_flags);
  

• break;
  

• case I915_CONTEXT_PARAM_NO_ERROR_CAPTURE: if (args->size) ret = -EINVAL;

@@ -1980,6 +1971,7 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv, ret = set_persistence(ctx, args); break;

• case I915_CONTEXT_PARAM_NO_ZEROMAP: case I915_CONTEXT_PARAM_BAN_PERIOD: case I915_CONTEXT_PARAM_RINGSIZE: default:

@@ -2360,11 +2352,6 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, return -ENOENT;

switch (args->param) {

• case I915_CONTEXT_PARAM_NO_ZEROMAP:

• args->size = 0;
  

• args->value = test_bit(UCONTEXT_NO_ZEROMAP, &ctx->user_flags);
  

• break;
  

• case I915_CONTEXT_PARAM_GTT_SIZE: args->size = 0; rcu_read_lock();

@@ -2412,6 +2399,7 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, args->value = i915_gem_context_is_persistent(ctx); break;

• case I915_CONTEXT_PARAM_NO_ZEROMAP: case I915_CONTEXT_PARAM_BAN_PERIOD: case I915_CONTEXT_PARAM_RINGSIZE: default:

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h index 340473aa70de0..5ae71ec936f7c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -129,7 +129,6 @@ struct i915_gem_context { * @user_flags: small set of booleans controlled by the user */ unsigned long user_flags; -#define UCONTEXT_NO_ZEROMAP 0 #define UCONTEXT_NO_ERROR_CAPTURE 1 #define UCONTEXT_BANNABLE 2 #define UCONTEXT_RECOVERABLE 3 diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 297143511f99b..b812f313422a9 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -290,7 +290,6 @@ struct i915_execbuffer { struct intel_context *reloc_context;

u64 invalid_flags; /** Set of execobj.flags that are invalid */

• u32 context_flags; /** Set of execobj.flags to insert from the ctx */

  u64 batch_len; /** Length of batch within object */ u32 batch_start_offset; /** Location within object of batch */

@@ -541,9 +540,6 @@ eb_validate_vma(struct i915_execbuffer *eb, entry->flags |= EXEC_OBJECT_NEEDS_GTT | __EXEC_OBJECT_NEEDS_MAP; }

• if (!(entry->flags & EXEC_OBJECT_PINNED))

• entry->flags |= eb->context_flags;
  

• return 0;

}

@@ -750,10 +746,6 @@ static int eb_select_context(struct i915_execbuffer *eb) if (rcu_access_pointer(ctx->vm)) eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT;

• eb->context_flags = 0;
• if (test_bit(UCONTEXT_NO_ZEROMAP, &ctx->user_flags))

• eb->context_flags |= __EXEC_OBJECT_NEEDS_BIAS;
  

• return 0;

}

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 6eefbc6dec01f..a0aaa8298f28d 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1637,6 +1637,10 @@ struct drm_i915_gem_context_param { __u32 size; __u64 param; #define I915_CONTEXT_PARAM_BAN_PERIOD 0x1 +/* I915_CONTEXT_PARAM_NO_ZEROMAP has been removed. On the off chance

• • someone somewhere has attempted to use it, never re-use this context
• • param number.
• */

#define I915_CONTEXT_PARAM_NO_ZEROMAP 0x2 #define I915_CONTEXT_PARAM_GTT_SIZE 0x3

#define I915_CONTEXT_PARAM_NO_ERROR_CAPTURE 0x4

2.31.1

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx

On Fri, Apr 23, 2021 at 05:31:13PM -0500, Jason Ekstrand wrote:

Instead of handling it like a context param, unconditionally set it when intel_contexts are created. This doesn't fix anything but does simplify the code a bit.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net

So the idea here is that since years we've had a watchdog uapi floating about. Aim was for media, so that they could set very tight deadlines for their transcodes jobs, so that if you have a corrupt bitstream (especially for decoding) you don't hang your desktop unecessarily wrong.

But it's been stuck in limbo since forever, plus I get how this gets a bit in the way of the proto ctx work, so makes sense to remove this prep work again.

Maybe include the above in the commit message for a notch more context.

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/i915/gem/i915_gem_context.c | 43 +++---------------- .../gpu/drm/i915/gem/i915_gem_context_types.h | 4 -- drivers/gpu/drm/i915/gt/intel_context_param.h | 3 +- 3 files changed, 6 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 35bcdeddfbf3f..1091cc04a242a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -233,7 +233,11 @@ static void intel_context_set_gem(struct intel_context *ce, intel_engine_has_timeslices(ce->engine)) __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);

• intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us);

• if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) &&

•    ctx->i915->params.request_timeout_ms) {
  

• unsigned int timeout_ms = ctx->i915->params.request_timeout_ms;
  

• intel_context_set_watchdog_us(ce, (u64)timeout_ms * 1000);
  

• }

}

static void __free_engines(struct i915_gem_engines *e, unsigned int count) @@ -792,41 +796,6 @@ static void __assign_timeline(struct i915_gem_context *ctx, context_apply_all(ctx, __apply_timeline, timeline); }

-static int __apply_watchdog(struct intel_context *ce, void *timeout_us) -{

• return intel_context_set_watchdog_us(ce, (uintptr_t)timeout_us);

-}

-static int -__set_watchdog(struct i915_gem_context *ctx, unsigned long timeout_us) -{

• int ret;
• ret = context_apply_all(ctx, __apply_watchdog,

• 		(void *)(uintptr_t)timeout_us);
  

• if (!ret)

• ctx->watchdog.timeout_us = timeout_us;
  

• return ret;

-}

-static void __set_default_fence_expiry(struct i915_gem_context *ctx) -{

• struct drm_i915_private *i915 = ctx->i915;
• int ret;
• if (!IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) ||

•    !i915->params.request_timeout_ms)
  

• return;
  

• /* Default expiry for user fences. */
• ret = __set_watchdog(ctx, i915->params.request_timeout_ms * 1000);
• if (ret)

• drm_notice(&i915->drm,
  

• 	   "Failed to configure default fence expiry! (%d)",
  

• 	   ret);
  

-}

static struct i915_gem_context * i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) { @@ -871,8 +840,6 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) intel_timeline_put(timeline); }

• __set_default_fence_expiry(ctx);

• trace_i915_context_create(ctx);

  return ctx;

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h index 5ae71ec936f7c..676592e27e7d2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -153,10 +153,6 @@ struct i915_gem_context { */ atomic_t active_count;

• struct {

• u64 timeout_us;
  

• } watchdog;
• /**
  • @hang_timestamp: The last time(s) this context caused a GPU hang
  */

diff --git a/drivers/gpu/drm/i915/gt/intel_context_param.h b/drivers/gpu/drm/i915/gt/intel_context_param.h index dffedd983693d..0c69cb42d075c 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_param.h +++ b/drivers/gpu/drm/i915/gt/intel_context_param.h @@ -10,11 +10,10 @@

#include "intel_context.h"

-static inline int +static inline void intel_context_set_watchdog_us(struct intel_context *ce, u64 timeout_us) { ce->watchdog.timeout_us = timeout_us;

• return 0;

}

#endif /* INTEL_CONTEXT_PARAM_H */

2.31.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, Apr 28, 2021 at 10:55 AM Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

On 23/04/2021 23:31, Jason Ekstrand wrote:

...

Instead of handling it like a context param, unconditionally set it when intel_contexts are created. This doesn't fix anything but does simplify the code a bit.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net

drivers/gpu/drm/i915/gem/i915_gem_context.c | 43 +++---------------- .../gpu/drm/i915/gem/i915_gem_context_types.h | 4 -- drivers/gpu/drm/i915/gt/intel_context_param.h | 3 +- 3 files changed, 6 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 35bcdeddfbf3f..1091cc04a242a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -233,7 +233,11 @@ static void intel_context_set_gem(struct intel_context *ce, intel_engine_has_timeslices(ce->engine)) __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);

• intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us);
  

• if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) &&
  

•     ctx->i915->params.request_timeout_ms) {
  

•         unsigned int timeout_ms = ctx->i915->params.request_timeout_ms;
  

•         intel_context_set_watchdog_us(ce, (u64)timeout_ms * 1000);
  

Blank line between declarations and code please, or just lose the local.

Otherwise looks okay. Slight change that same GEM context can now have a mix of different request expirations isn't interesting I think. At least the change goes away by the end of the series.

In order for that to happen, I think you'd have to have a race between CREATE_CONTEXT and someone smashing the request_timeout_ms param via sysfs. Or am I missing something? Given that timeouts are really per-engine anyway, I don't think we need to care too much about that.

--Jason

Regards,

Tvrtko

...

• }
  

  }

  static void __free_engines(struct i915_gem_engines *e, unsigned int count)

@@ -792,41 +796,6 @@ static void __assign_timeline(struct i915_gem_context *ctx, context_apply_all(ctx, __apply_timeline, timeline); }

-static int __apply_watchdog(struct intel_context *ce, void *timeout_us) -{

• return intel_context_set_watchdog_us(ce, (uintptr_t)timeout_us);
  

-}

-static int -__set_watchdog(struct i915_gem_context *ctx, unsigned long timeout_us) -{

• int ret;
  

• ret = context_apply_all(ctx, __apply_watchdog,
  

•                         (void *)(uintptr_t)timeout_us);
  

• if (!ret)
  

•         ctx->watchdog.timeout_us = timeout_us;
  

• return ret;
  

-}

-static void __set_default_fence_expiry(struct i915_gem_context *ctx) -{

• struct drm_i915_private *i915 = ctx->i915;
  

• int ret;
  

• if (!IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) ||
  

•     !i915->params.request_timeout_ms)
  

•         return;
  

• /* Default expiry for user fences. */
  

• ret = __set_watchdog(ctx, i915->params.request_timeout_ms * 1000);
  

• if (ret)
  

•         drm_notice(&i915->drm,
  

•                    "Failed to configure default fence expiry! (%d)",
  

•                    ret);
  

-}

• static struct i915_gem_context * i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) {

@@ -871,8 +840,6 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) intel_timeline_put(timeline); }

• __set_default_fence_expiry(ctx);
  

• trace_i915_context_create(ctx);
  
  return ctx;
  

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h index 5ae71ec936f7c..676592e27e7d2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -153,10 +153,6 @@ struct i915_gem_context { */ atomic_t active_count;

• struct {
  

•         u64 timeout_us;
  

• } watchdog;
  

• /**
   * @hang_timestamp: The last time(s) this context caused a GPU hang
   */
  

diff --git a/drivers/gpu/drm/i915/gt/intel_context_param.h b/drivers/gpu/drm/i915/gt/intel_context_param.h index dffedd983693d..0c69cb42d075c 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_param.h +++ b/drivers/gpu/drm/i915/gt/intel_context_param.h @@ -10,11 +10,10 @@

#include "intel_context.h"

-static inline int +static inline void intel_context_set_watchdog_us(struct intel_context *ce, u64 timeout_us) { ce->watchdog.timeout_us = timeout_us;

• return 0;
  

  }

  #endif /* INTEL_CONTEXT_PARAM_H */

On Thu, Apr 29, 2021 at 3:04 AM Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

On 28/04/2021 18:24, Jason Ekstrand wrote:

...

On Wed, Apr 28, 2021 at 10:55 AM Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 23/04/2021 23:31, Jason Ekstrand wrote:

...

Instead of handling it like a context param, unconditionally set it when intel_contexts are created. This doesn't fix anything but does simplify the code a bit.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net

drivers/gpu/drm/i915/gem/i915_gem_context.c | 43 +++---------------- .../gpu/drm/i915/gem/i915_gem_context_types.h | 4 -- drivers/gpu/drm/i915/gt/intel_context_param.h | 3 +- 3 files changed, 6 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 35bcdeddfbf3f..1091cc04a242a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -233,7 +233,11 @@ static void intel_context_set_gem(struct intel_context *ce, intel_engine_has_timeslices(ce->engine)) __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);

• intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us);
  

• if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) &&
  

•     ctx->i915->params.request_timeout_ms) {
  

•         unsigned int timeout_ms = ctx->i915->params.request_timeout_ms;
  

•         intel_context_set_watchdog_us(ce, (u64)timeout_ms * 1000);
  

Blank line between declarations and code please, or just lose the local.

Otherwise looks okay. Slight change that same GEM context can now have a mix of different request expirations isn't interesting I think. At least the change goes away by the end of the series.

In order for that to happen, I think you'd have to have a race between CREATE_CONTEXT and someone smashing the request_timeout_ms param via sysfs. Or am I missing something? Given that timeouts are really per-engine anyway, I don't think we need to care too much about that.

We don't care, no.

For completeness only - by the end of the series it is what you say. But at _this_ point in the series though it is if modparam changes at any point between context create and replacing engines. Which is a change compared to before this patch, since modparam was cached in the GEM context so far. So one GEM context was a single request_timeout_ms.

I've added the following to the commit message:

It also means that sync files exported from different engines on a SINGLE_TIMELINE context will have different fence contexts. This is visible to userspace if it looks at the obj_name field of sync_fence_info.

How's that sound?

--Jason

Regards,

Tvrtko

...

--Jason

...

Regards,

Tvrtko

...

• }
  

  }

  static void __free_engines(struct i915_gem_engines *e, unsigned int count)

@@ -792,41 +796,6 @@ static void __assign_timeline(struct i915_gem_context *ctx, context_apply_all(ctx, __apply_timeline, timeline); }

-static int __apply_watchdog(struct intel_context *ce, void *timeout_us) -{

• return intel_context_set_watchdog_us(ce, (uintptr_t)timeout_us);
  

-}

-static int -__set_watchdog(struct i915_gem_context *ctx, unsigned long timeout_us) -{

• int ret;
  

• ret = context_apply_all(ctx, __apply_watchdog,
  

•                         (void *)(uintptr_t)timeout_us);
  

• if (!ret)
  

•         ctx->watchdog.timeout_us = timeout_us;
  

• return ret;
  

-}

-static void __set_default_fence_expiry(struct i915_gem_context *ctx) -{

• struct drm_i915_private *i915 = ctx->i915;
  

• int ret;
  

• if (!IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) ||
  

•     !i915->params.request_timeout_ms)
  

•         return;
  

• /* Default expiry for user fences. */
  

• ret = __set_watchdog(ctx, i915->params.request_timeout_ms * 1000);
  

• if (ret)
  

•         drm_notice(&i915->drm,
  

•                    "Failed to configure default fence expiry! (%d)",
  

•                    ret);
  

-}

• static struct i915_gem_context * i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) {

@@ -871,8 +840,6 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) intel_timeline_put(timeline); }

• __set_default_fence_expiry(ctx);
  

•  trace_i915_context_create(ctx);
  
   return ctx;
  

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h index 5ae71ec936f7c..676592e27e7d2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -153,10 +153,6 @@ struct i915_gem_context { */ atomic_t active_count;

• struct {
  

•         u64 timeout_us;
  

• } watchdog;
  

•  /**
    * @hang_timestamp: The last time(s) this context caused a GPU hang
    */
  

diff --git a/drivers/gpu/drm/i915/gt/intel_context_param.h b/drivers/gpu/drm/i915/gt/intel_context_param.h index dffedd983693d..0c69cb42d075c 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_param.h +++ b/drivers/gpu/drm/i915/gt/intel_context_param.h @@ -10,11 +10,10 @@

#include "intel_context.h"

-static inline int +static inline void intel_context_set_watchdog_us(struct intel_context *ce, u64 timeout_us) { ce->watchdog.timeout_us = timeout_us;

• return 0;
  

  }

  #endif /* INTEL_CONTEXT_PARAM_H */

On Thu, Apr 29, 2021 at 07:12:05PM +0200, Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 09:54:15AM -0500, Jason Ekstrand wrote:

...

On Thu, Apr 29, 2021 at 3:04 AM Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 28/04/2021 18:24, Jason Ekstrand wrote:

...

On Wed, Apr 28, 2021 at 10:55 AM Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 23/04/2021 23:31, Jason Ekstrand wrote:

...

Instead of handling it like a context param, unconditionally set it when intel_contexts are created. This doesn't fix anything but does simplify the code a bit.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net

drivers/gpu/drm/i915/gem/i915_gem_context.c | 43 +++---------------- .../gpu/drm/i915/gem/i915_gem_context_types.h | 4 -- drivers/gpu/drm/i915/gt/intel_context_param.h | 3 +- 3 files changed, 6 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 35bcdeddfbf3f..1091cc04a242a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -233,7 +233,11 @@ static void intel_context_set_gem(struct intel_context *ce, intel_engine_has_timeslices(ce->engine)) __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);

• intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us);
  

• if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) &&
  

•     ctx->i915->params.request_timeout_ms) {
  

•         unsigned int timeout_ms = ctx->i915->params.request_timeout_ms;
  

•         intel_context_set_watchdog_us(ce, (u64)timeout_ms * 1000);
  

Blank line between declarations and code please, or just lose the local.

Otherwise looks okay. Slight change that same GEM context can now have a mix of different request expirations isn't interesting I think. At least the change goes away by the end of the series.

In order for that to happen, I think you'd have to have a race between CREATE_CONTEXT and someone smashing the request_timeout_ms param via sysfs. Or am I missing something? Given that timeouts are really per-engine anyway, I don't think we need to care too much about that.

We don't care, no.

For completeness only - by the end of the series it is what you say. But at _this_ point in the series though it is if modparam changes at any point between context create and replacing engines. Which is a change compared to before this patch, since modparam was cached in the GEM context so far. So one GEM context was a single request_timeout_ms.

I've added the following to the commit message:

It also means that sync files exported from different engines on a SINGLE_TIMELINE context will have different fence contexts. This is visible to userspace if it looks at the obj_name field of sync_fence_info.

How's that sound?

If you add "Which media-driver as the sole user of this doesn't do" then I think it's perfect.

Uh I think you replied to the wrong thread :-)

This here is about watchdog, not timeline. -Daniel

On 29/04/2021 15:54, Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 3:04 AM Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 28/04/2021 18:24, Jason Ekstrand wrote:

...

On Wed, Apr 28, 2021 at 10:55 AM Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 23/04/2021 23:31, Jason Ekstrand wrote:

...

Instead of handling it like a context param, unconditionally set it when intel_contexts are created. This doesn't fix anything but does simplify the code a bit.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net

drivers/gpu/drm/i915/gem/i915_gem_context.c   | 43 +++----------------
.../gpu/drm/i915/gem/i915_gem_context_types.h |  4 --
drivers/gpu/drm/i915/gt/intel_context_param.h |  3 +-
3 files changed, 6 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 35bcdeddfbf3f..1091cc04a242a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -233,7 +233,11 @@ static void intel_context_set_gem(struct intel_context *ce, intel_engine_has_timeslices(ce->engine)) __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);

• intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us);
  

• if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) &&
  

•     ctx->i915->params.request_timeout_ms) {
  

•         unsigned int timeout_ms = ctx->i915->params.request_timeout_ms;
  

•         intel_context_set_watchdog_us(ce, (u64)timeout_ms * 1000);
  

Blank line between declarations and code please, or just lose the local.

Otherwise looks okay. Slight change that same GEM context can now have a mix of different request expirations isn't interesting I think. At least the change goes away by the end of the series.

In order for that to happen, I think you'd have to have a race between CREATE_CONTEXT and someone smashing the request_timeout_ms param via sysfs. Or am I missing something? Given that timeouts are really per-engine anyway, I don't think we need to care too much about that.

We don't care, no.

For completeness only - by the end of the series it is what you say. But at _this_ point in the series though it is if modparam changes at any point between context create and replacing engines. Which is a change compared to before this patch, since modparam was cached in the GEM context so far. So one GEM context was a single request_timeout_ms.

I've added the following to the commit message:

It also means that sync files exported from different engines on a SINGLE_TIMELINE context will have different fence contexts. This is visible to userspace if it looks at the obj_name field of sync_fence_info.

How's that sound?

Wrong thread but sounds good.

I haven't looked into the fence merge logic apart from noticing context is used there. So I'd suggest a quick look there on top, just to make sure merging logic does not hold any surprises if contexts start to differ. Probably just results with more inefficiency somewhere, in theory.

Regards,

Tvrtko

On Tue, Apr 27, 2021 at 4:49 AM Daniel Vetter daniel@ffwll.ch wrote:

On Fri, Apr 23, 2021 at 05:31:15PM -0500, Jason Ekstrand wrote:

...

This API allows one context to grab bits out of another context upon creation. It can be used as a short-cut for setparam(getparam()) for things like I915_CONTEXT_PARAM_VM. However, it's never been used by any real userspace. It's used by a few IGT tests and that's it. Since it doesn't add any real value (most of the stuff you can CLONE you can copy in other ways), drop it.

There is one thing that this API allows you to clone which you cannot clone via getparam/setparam: timelines. However, timelines are an implementation detail of i915 and not really something that needs to be exposed to userspace. Also, sharing timelines between contexts isn't obviously useful and supporting it has the potential to complicate i915 internally. It also doesn't add any functionality that the client can't get in other ways. If a client really wants a shared timeline, they can use a syncobj and set it as an in and out fence on every submit.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_context.c | 199 +------------------- include/uapi/drm/i915_drm.h | 16 +- 2 files changed, 6 insertions(+), 209 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 8a77855123cec..2c2fefa912805 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1958,207 +1958,14 @@ static int create_setparam(struct i915_user_extension __user *ext, void *data) return ctx_setparam(arg->fpriv, arg->ctx, &local.param); }

-static int clone_engines(struct i915_gem_context *dst,

•                  struct i915_gem_context *src)
  

+static int invalid_ext(struct i915_user_extension __user *ext, void *data) {

• struct i915_gem_engines *clone, *e;
  

• bool user_engines;
  

• unsigned long n;
  

• e = __context_engines_await(src, &user_engines);
  

• if (!e)
  

•         return -ENOENT;
  

• clone = alloc_engines(e->num_engines);
  

• if (!clone)
  

•         goto err_unlock;
  

• for (n = 0; n < e->num_engines; n++) {
  

•         struct intel_engine_cs *engine;
  

•         if (!e->engines[n]) {
  

•                 clone->engines[n] = NULL;
  

•                 continue;
  

•         }
  

•         engine = e->engines[n]->engine;
  

•         /*
  

•          * Virtual engines are singletons; they can only exist
  

•          * inside a single context, because they embed their
  

•          * HW context... As each virtual context implies a single
  

•          * timeline (each engine can only dequeue a single request
  

•          * at any time), it would be surprising for two contexts
  

•          * to use the same engine. So let's create a copy of
  

•          * the virtual engine instead.
  

•          */
  

•         if (intel_engine_is_virtual(engine))
  

•                 clone->engines[n] =
  

•                         intel_execlists_clone_virtual(engine);
  

You forgot to gc this function here ^^

Done, with pleasure!

...

•         else
  

•                 clone->engines[n] = intel_context_create(engine);
  

•         if (IS_ERR_OR_NULL(clone->engines[n])) {
  

•                 __free_engines(clone, n);
  

•                 goto err_unlock;
  

•         }
  

•         intel_context_set_gem(clone->engines[n], dst);
  

Not peeked ahead, but I'm really hoping intel_context_set_gem gets removed eventually too ...

I've not gotten rid of it yet but it's on my list of things to clean up. The problem is that there are a pile of parameters we want to set for user engines which we don't set for internal engines:

- VM - priority - hangcheck timeout - gem_context back-pointer (I'd love to drop this one!) - a bunch more when we start shifting more stuff into intel_context

And there are a bunch of places where we create non-user engines. The end result being that we have four ugly options:

1. Set them after the fact as per intel_context_set_gem 2. Touch all 79 instances of intel_context_create( for each new create param we add 3. Add a new struct intel_context_create_args which contains all the extra stuff and make NULL mean "use the defaults" 4. Add a new struct i915_gem_engine which is used for client-visible engines. When we switch to an engine-based uAPI, this is probably what would be exposed to userspace.

I'm happy to hear opinions on which of those is the best option. 2. is clearly a bad idea.

...

• }
  

• clone->num_engines = n;
  

• i915_sw_fence_complete(&e->fence);
  

• /* Serialised by constructor */
  

• engines_idle_release(dst, rcu_replace_pointer(dst->engines, clone, 1));
  

• if (user_engines)
  

•         i915_gem_context_set_user_engines(dst);
  

• else
  

•         i915_gem_context_clear_user_engines(dst);
  

• return 0;
  

-err_unlock:

• i915_sw_fence_complete(&e->fence);
  

• return -ENOMEM;
  

-}

-static int clone_flags(struct i915_gem_context *dst,

•                struct i915_gem_context *src)
  

-{

• dst->user_flags = src->user_flags;
  

• return 0;
  

-}

-static int clone_schedattr(struct i915_gem_context *dst,

•                    struct i915_gem_context *src)
  

-{

• dst->sched = src->sched;
  

• return 0;
  

-}

-static int clone_sseu(struct i915_gem_context *dst,

•               struct i915_gem_context *src)
  

-{

• struct i915_gem_engines *e = i915_gem_context_lock_engines(src);
  

• struct i915_gem_engines *clone;
  

• unsigned long n;
  

• int err;
  

• /* no locking required; sole access under constructor*/
  

• clone = __context_engines_static(dst);
  

• if (e->num_engines != clone->num_engines) {
  

•         err = -EINVAL;
  

•         goto unlock;
  

• }
  

• for (n = 0; n < e->num_engines; n++) {
  

•         struct intel_context *ce = e->engines[n];
  

•         if (clone->engines[n]->engine->class != ce->engine->class) {
  

•                 /* Must have compatible engine maps! */
  

•                 err = -EINVAL;
  

•                 goto unlock;
  

•         }
  

•         /* serialises with set_sseu */
  

•         err = intel_context_lock_pinned(ce);
  

•         if (err)
  

•                 goto unlock;
  

•         clone->engines[n]->sseu = ce->sseu;
  

•         intel_context_unlock_pinned(ce);
  

• }
  

• err = 0;
  

-unlock:

• i915_gem_context_unlock_engines(src);
  

• return err;
  

-}

-static int clone_timeline(struct i915_gem_context *dst,

•                   struct i915_gem_context *src)
  

-{

• if (src->timeline)
  

•         __assign_timeline(dst, src->timeline);
  

• return 0;
  

-}

-static int clone_vm(struct i915_gem_context *dst,

•             struct i915_gem_context *src)
  

-{

• struct i915_address_space *vm;
  

• int err = 0;
  

• if (!rcu_access_pointer(src->vm))
  

•         return 0;
  

• rcu_read_lock();
  

• vm = context_get_vm_rcu(src);
  

• rcu_read_unlock();
  

• if (!mutex_lock_interruptible(&dst->mutex)) {
  

•         __assign_ppgtt(dst, vm);
  

•         mutex_unlock(&dst->mutex);
  

• } else {
  

•         err = -EINTR;
  

• }
  

• i915_vm_put(vm);
  

• return err;
  

-}

-static int create_clone(struct i915_user_extension __user *ext, void *data) -{

• static int (* const fn[])(struct i915_gem_context *dst,
  

•                           struct i915_gem_context *src) = {
  

-#define MAP(x, y) [ilog2(I915_CONTEXT_CLONE_##x)] = y

•         MAP(ENGINES, clone_engines),
  

•         MAP(FLAGS, clone_flags),
  

•         MAP(SCHEDATTR, clone_schedattr),
  

•         MAP(SSEU, clone_sseu),
  

•         MAP(TIMELINE, clone_timeline),
  

•         MAP(VM, clone_vm),
  

-#undef MAP

• };
  

• struct drm_i915_gem_context_create_ext_clone local;
  

• const struct create_ext *arg = data;
  

• struct i915_gem_context *dst = arg->ctx;
  

• struct i915_gem_context *src;
  

• int err, bit;
  

• if (copy_from_user(&local, ext, sizeof(local)))
  

•         return -EFAULT;
  

• BUILD_BUG_ON(GENMASK(BITS_PER_TYPE(local.flags) - 1, ARRAY_SIZE(fn)) !=
  

•              I915_CONTEXT_CLONE_UNKNOWN);
  

• if (local.flags & I915_CONTEXT_CLONE_UNKNOWN)
  

•         return -EINVAL;
  

• if (local.rsvd)
  

•         return -EINVAL;
  

• rcu_read_lock();
  

• src = __i915_gem_context_lookup_rcu(arg->fpriv, local.clone_id);
  

• rcu_read_unlock();
  

• if (!src)
  

•         return -ENOENT;
  

• GEM_BUG_ON(src == dst);
  

• for (bit = 0; bit < ARRAY_SIZE(fn); bit++) {
  

•         if (!(local.flags & BIT(bit)))
  

•                 continue;
  

•         err = fn[bit](dst, src);
  

•         if (err)
  

•                 return err;
  

• }
  

• return 0;
  

• return -EINVAL;
  

}

static const i915_user_extension_fn create_extensions[] = { [I915_CONTEXT_CREATE_EXT_SETPARAM] = create_setparam,

• [I915_CONTEXT_CREATE_EXT_CLONE] = create_clone,
  

• [I915_CONTEXT_CREATE_EXT_CLONE] = invalid_ext,
  

};

static bool client_is_banned(struct drm_i915_file_private *file_priv) diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index a0aaa8298f28d..75a71b6756ed8 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1887,20 +1887,10 @@ struct drm_i915_gem_context_create_ext_setparam { struct drm_i915_gem_context_param param; };

-struct drm_i915_gem_context_create_ext_clone { +/* This API has been removed. On the off chance someone somewhere has

• • attempted to use it, never re-use this extension number.
• */

#define I915_CONTEXT_CREATE_EXT_CLONE 1

I think we need to put these somewhere else now, here it's just plain lost. I think in the kerneldoc for drm_i915_gem_context_create_ext_setparam would be best, with the #define right above and in the kerneldoc an enumeration of all the values and what they're for.

I fully agree it's not great. But I'm not sure create_ext_setparam makes sense either. This is it's own extension that's unrelated to ext_setparam.

--Jason

I think I'll need to sign up Matt B or you for doing some kerneldoc polish on these so they're all collected together. -Daniel

...

• struct i915_user_extension base;
  

• __u32 clone_id;
  

• __u32 flags;
  

-#define I915_CONTEXT_CLONE_ENGINES (1u << 0) -#define I915_CONTEXT_CLONE_FLAGS (1u << 1) -#define I915_CONTEXT_CLONE_SCHEDATTR (1u << 2) -#define I915_CONTEXT_CLONE_SSEU (1u << 3) -#define I915_CONTEXT_CLONE_TIMELINE (1u << 4) -#define I915_CONTEXT_CLONE_VM (1u << 5) -#define I915_CONTEXT_CLONE_UNKNOWN -(I915_CONTEXT_CLONE_VM << 1)

• __u64 rsvd;
  

-};

struct drm_i915_gem_context_destroy { __u32 ctx_id; -- 2.31.1

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On 23/04/2021 23:31, Jason Ekstrand wrote:

This API is entirely unnecessary and I'd love to get rid of it. If userspace wants a single timeline across multiple contexts, they can either use implicit synchronization or a syncobj, both of which existed at the time this feature landed. The justification given at the time was that it would help GL drivers which are inherently single-timeline. However, neither of our GL drivers actually wanted the feature. i965 was already in maintenance mode at the time and iris uses syncobj for everything.

Unfortunately, as much as I'd love to get rid of it, it is used by the media driver so we can't do that. We can, however, do the next-best thing which is to embed a syncobj in the context and do exactly what we'd expect from userspace internally. This isn't an entirely identical implementation because it's no longer atomic if userspace races with itself by calling execbuffer2 twice simultaneously from different threads. It won't crash in that case; it just doesn't guarantee any ordering between those two submits.

1)

Please also mention the difference in context/timeline name when observed via the sync file API.

2)

I don't remember what we have concluded in terms of observable effects in sync_file_merge?

Regards,

Tvrtko

Moving SINGLE_TIMELINE to a syncobj emulation has a couple of technical advantages beyond mere annoyance. One is that intel_timeline is no longer an api-visible object and can remain entirely an implementation detail. This may be advantageous as we make scheduler changes going forward. Second is that, together with deleting the CLONE_CONTEXT API, we should now have a 1:1 mapping between intel_context and intel_timeline which may help us reduce locking.

v2 (Jason Ekstrand):

• Update the comment on i915_gem_context::syncobj to mention that it's an emulation and the possible race if userspace calls execbuffer2 twice on the same context concurrently.
• Wrap the checks for eb.gem_context->syncobj in unlikely()
• Drop the dma_fence reference
• Improved commit message

v3 (Jason Ekstrand):

• Move the dma_fence_put() to before the error exit

Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_context.c | 49 +++++-------------- .../gpu/drm/i915/gem/i915_gem_context_types.h | 14 +++++- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 16 ++++++ 3 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 2c2fefa912805..a72c9b256723b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -67,6 +67,8 @@ #include <linux/log2.h> #include <linux/nospec.h>

+#include <drm/drm_syncobj.h>

• #include "gt/gen6_ppgtt.h" #include "gt/intel_context.h" #include "gt/intel_context_param.h"

@@ -225,10 +227,6 @@ static void intel_context_set_gem(struct intel_context *ce, ce->vm = vm; }

• GEM_BUG_ON(ce->timeline);
• if (ctx->timeline)

• ce->timeline = intel_timeline_get(ctx->timeline);
  

• if (ctx->sched.priority >= I915_PRIORITY_NORMAL && intel_engine_has_timeslices(ce->engine)) __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);

@@ -351,9 +349,6 @@ void i915_gem_context_release(struct kref *ref) mutex_destroy(&ctx->engines_mutex); mutex_destroy(&ctx->lut_mutex);

• if (ctx->timeline)

• intel_timeline_put(ctx->timeline);
  

• put_pid(ctx->pid); mutex_destroy(&ctx->mutex);

@@ -570,6 +565,9 @@ static void context_close(struct i915_gem_context *ctx) if (vm) i915_vm_close(vm);

• if (ctx->syncobj)

• drm_syncobj_put(ctx->syncobj);
  

• ctx->file_priv = ERR_PTR(-EBADF);

  /*

@@ -765,33 +763,11 @@ static void __assign_ppgtt(struct i915_gem_context *ctx, i915_vm_close(vm); }

-static void __set_timeline(struct intel_timeline **dst,

• 	   struct intel_timeline *src)
  

-{

• struct intel_timeline *old = *dst;
• *dst = src ? intel_timeline_get(src) : NULL;
• if (old)

• intel_timeline_put(old);
  

-}

-static void __apply_timeline(struct intel_context *ce, void *timeline) -{

• __set_timeline(&ce->timeline, timeline);

-}

-static void __assign_timeline(struct i915_gem_context *ctx,

• 	      struct intel_timeline *timeline)
  

-{

• __set_timeline(&ctx->timeline, timeline);
• context_apply_all(ctx, __apply_timeline, timeline);

-}

• static struct i915_gem_context * i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) { struct i915_gem_context *ctx;

• int ret;

  if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE && !HAS_EXECLISTS(i915))

@@ -820,16 +796,13 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) }

if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) {

• struct intel_timeline *timeline;
  

• timeline = intel_timeline_create(&i915->gt);
  

• if (IS_ERR(timeline)) {
  

• ret = drm_syncobj_create(&ctx->syncobj,
  

• 			 DRM_SYNCOBJ_CREATE_SIGNALED,
  

• 			 NULL);
  

• if (ret) {
  context_close(ctx);
  

• 	return ERR_CAST(timeline);
  

• 	return ERR_PTR(ret);
  

  }

• __assign_timeline(ctx, timeline);
  

• intel_timeline_put(timeline);
  

  }

  trace_i915_context_create(ctx);

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h index 676592e27e7d2..df76767f0c41b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -83,7 +83,19 @@ struct i915_gem_context { struct i915_gem_engines __rcu *engines; struct mutex engines_mutex; /* guards writes to engines */

• struct intel_timeline *timeline;

• /**

• * @syncobj: Shared timeline syncobj
  

• *
  

• * When the SHARED_TIMELINE flag is set on context creation, we
  

• * emulate a single timeline across all engines using this syncobj.
  

• * For every execbuffer2 call, this syncobj is used as both an in-
  

• * and out-fence.  Unlike the real intel_timeline, this doesn't
  

• * provide perfect atomic in-order guarantees if the client races
  

• * with itself by calling execbuffer2 twice concurrently.  However,
  

• * if userspace races with itself, that's not likely to yield well-
  

• * defined results anyway so we choose to not care.
  

• */
  

• struct drm_syncobj *syncobj;

  /**

  • @vm: unique address space (GTT)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index b812f313422a9..d640bba6ad9ab 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -3460,6 +3460,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, goto err_vma; }

• if (unlikely(eb.gem_context->syncobj)) {

• struct dma_fence *fence;
  

• fence = drm_syncobj_fence_get(eb.gem_context->syncobj);
  

• err = i915_request_await_dma_fence(eb.request, fence);
  

• dma_fence_put(fence);
  

• if (err)
  

• 	goto err_ext;
  

• }
• if (in_fence) { if (args->flags & I915_EXEC_FENCE_SUBMIT) err = i915_request_await_execution(eb.request,

@@ -3517,6 +3527,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, fput(out_fence->file); } }

• if (unlikely(eb.gem_context->syncobj)) {

• drm_syncobj_replace_fence(eb.gem_context->syncobj,
  

• 			  &eb.request->fence);
  

• }

• i915_request_put(eb.request);

  err_vma:

On 28/04/2021 18:26, Jason Ekstrand wrote:

On Wed, Apr 28, 2021 at 10:49 AM Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 23/04/2021 23:31, Jason Ekstrand wrote:

...

This API is entirely unnecessary and I'd love to get rid of it. If userspace wants a single timeline across multiple contexts, they can either use implicit synchronization or a syncobj, both of which existed at the time this feature landed. The justification given at the time was that it would help GL drivers which are inherently single-timeline. However, neither of our GL drivers actually wanted the feature. i965 was already in maintenance mode at the time and iris uses syncobj for everything.

Unfortunately, as much as I'd love to get rid of it, it is used by the media driver so we can't do that. We can, however, do the next-best thing which is to embed a syncobj in the context and do exactly what we'd expect from userspace internally. This isn't an entirely identical implementation because it's no longer atomic if userspace races with itself by calling execbuffer2 twice simultaneously from different threads. It won't crash in that case; it just doesn't guarantee any ordering between those two submits.

Please also mention the difference in context/timeline name when observed via the sync file API.

I don't remember what we have concluded in terms of observable effects in sync_file_merge?

I don't see how either of these are observable since this syncobj is never exposed to userspace in any way. Please help me understand what I'm missing here.

Single timeline context - two execbufs - return two out fences.

Before the patch those two had the same fence context, with the patch they have different ones.

Fence context is visible to userspace via sync file info (timeline name at least) and rules in sync_file_merge.

Regards,

Tvrtko

--Jason

...

Regards,

Tvrtko

...

Moving SINGLE_TIMELINE to a syncobj emulation has a couple of technical advantages beyond mere annoyance. One is that intel_timeline is no longer an api-visible object and can remain entirely an implementation detail. This may be advantageous as we make scheduler changes going forward. Second is that, together with deleting the CLONE_CONTEXT API, we should now have a 1:1 mapping between intel_context and intel_timeline which may help us reduce locking.

v2 (Jason Ekstrand):

• Update the comment on i915_gem_context::syncobj to mention that it's an emulation and the possible race if userspace calls execbuffer2 twice on the same context concurrently.
• Wrap the checks for eb.gem_context->syncobj in unlikely()
• Drop the dma_fence reference
• Improved commit message

v3 (Jason Ekstrand):

• Move the dma_fence_put() to before the error exit

Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_context.c | 49 +++++-------------- .../gpu/drm/i915/gem/i915_gem_context_types.h | 14 +++++- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 16 ++++++ 3 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 2c2fefa912805..a72c9b256723b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -67,6 +67,8 @@ #include <linux/log2.h> #include <linux/nospec.h>

+#include <drm/drm_syncobj.h>

• #include "gt/gen6_ppgtt.h" #include "gt/intel_context.h" #include "gt/intel_context_param.h"

@@ -225,10 +227,6 @@ static void intel_context_set_gem(struct intel_context *ce, ce->vm = vm; }

• GEM_BUG_ON(ce->timeline);
  

• if (ctx->timeline)
  

•         ce->timeline = intel_timeline_get(ctx->timeline);
  

•  if (ctx->sched.priority >= I915_PRIORITY_NORMAL &&
       intel_engine_has_timeslices(ce->engine))
           __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);
  

@@ -351,9 +349,6 @@ void i915_gem_context_release(struct kref *ref) mutex_destroy(&ctx->engines_mutex); mutex_destroy(&ctx->lut_mutex);

• if (ctx->timeline)
  

•         intel_timeline_put(ctx->timeline);
  

•  put_pid(ctx->pid);
   mutex_destroy(&ctx->mutex);
  
  

@@ -570,6 +565,9 @@ static void context_close(struct i915_gem_context *ctx) if (vm) i915_vm_close(vm);

• if (ctx->syncobj)
  

•         drm_syncobj_put(ctx->syncobj);
  

•  ctx->file_priv = ERR_PTR(-EBADF);
  
   /*
  

@@ -765,33 +763,11 @@ static void __assign_ppgtt(struct i915_gem_context *ctx, i915_vm_close(vm); }

-static void __set_timeline(struct intel_timeline **dst,

•                    struct intel_timeline *src)
  

-{

• struct intel_timeline *old = *dst;
  

• *dst = src ? intel_timeline_get(src) : NULL;
  

• if (old)
  

•         intel_timeline_put(old);
  

-}

-static void __apply_timeline(struct intel_context *ce, void *timeline) -{

• __set_timeline(&ce->timeline, timeline);
  

-}

-static void __assign_timeline(struct i915_gem_context *ctx,

•                       struct intel_timeline *timeline)
  

-{

• __set_timeline(&ctx->timeline, timeline);
  

• context_apply_all(ctx, __apply_timeline, timeline);
  

-}

• static struct i915_gem_context * i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) { struct i915_gem_context *ctx;

• int ret;
  
   if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE &&
       !HAS_EXECLISTS(i915))
  

@@ -820,16 +796,13 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) }

   if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) {

•         struct intel_timeline *timeline;
  

•         timeline = intel_timeline_create(&i915->gt);
  

•         if (IS_ERR(timeline)) {
  

•         ret = drm_syncobj_create(&ctx->syncobj,
  

•                                  DRM_SYNCOBJ_CREATE_SIGNALED,
  

•                                  NULL);
  

•         if (ret) {
                   context_close(ctx);
  

•                 return ERR_CAST(timeline);
  

•                 return ERR_PTR(ret);
           }
  

•         __assign_timeline(ctx, timeline);
  

•         intel_timeline_put(timeline);
   }
  
   trace_i915_context_create(ctx);
  

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h index 676592e27e7d2..df76767f0c41b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -83,7 +83,19 @@ struct i915_gem_context { struct i915_gem_engines __rcu *engines; struct mutex engines_mutex; /* guards writes to engines */

• struct intel_timeline *timeline;
  

• /**
  

•  * @syncobj: Shared timeline syncobj
  

•  *
  

•  * When the SHARED_TIMELINE flag is set on context creation, we
  

•  * emulate a single timeline across all engines using this syncobj.
  

•  * For every execbuffer2 call, this syncobj is used as both an in-
  

•  * and out-fence.  Unlike the real intel_timeline, this doesn't
  

•  * provide perfect atomic in-order guarantees if the client races
  

•  * with itself by calling execbuffer2 twice concurrently.  However,
  

•  * if userspace races with itself, that's not likely to yield well-
  

•  * defined results anyway so we choose to not care.
  

•  */
  

• struct drm_syncobj *syncobj;
  
   /**
    * @vm: unique address space (GTT)
  

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index b812f313422a9..d640bba6ad9ab 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -3460,6 +3460,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, goto err_vma; }

• if (unlikely(eb.gem_context->syncobj)) {
  

•         struct dma_fence *fence;
  

•         fence = drm_syncobj_fence_get(eb.gem_context->syncobj);
  

•         err = i915_request_await_dma_fence(eb.request, fence);
  

•         dma_fence_put(fence);
  

•         if (err)
  

•                 goto err_ext;
  

• }
  

•  if (in_fence) {
           if (args->flags & I915_EXEC_FENCE_SUBMIT)
                   err = i915_request_await_execution(eb.request,
  

@@ -3517,6 +3527,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, fput(out_fence->file); } }

• if (unlikely(eb.gem_context->syncobj)) {
  

•         drm_syncobj_replace_fence(eb.gem_context->syncobj,
  

•                                   &eb.request->fence);
  

• }
  

•  i915_request_put(eb.request);
  
  

  err_vma:

On Thu, Apr 29, 2021 at 7:08 AM Daniel Vetter daniel@ffwll.ch wrote:

On Thu, Apr 29, 2021 at 09:06:47AM +0100, Tvrtko Ursulin wrote:

...

On 28/04/2021 18:26, Jason Ekstrand wrote:

...

On Wed, Apr 28, 2021 at 10:49 AM Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 23/04/2021 23:31, Jason Ekstrand wrote:

...

This API is entirely unnecessary and I'd love to get rid of it. If userspace wants a single timeline across multiple contexts, they can either use implicit synchronization or a syncobj, both of which existed at the time this feature landed. The justification given at the time was that it would help GL drivers which are inherently single-timeline. However, neither of our GL drivers actually wanted the feature. i965 was already in maintenance mode at the time and iris uses syncobj for everything.

Unfortunately, as much as I'd love to get rid of it, it is used by the media driver so we can't do that. We can, however, do the next-best thing which is to embed a syncobj in the context and do exactly what we'd expect from userspace internally. This isn't an entirely identical implementation because it's no longer atomic if userspace races with itself by calling execbuffer2 twice simultaneously from different threads. It won't crash in that case; it just doesn't guarantee any ordering between those two submits.

Please also mention the difference in context/timeline name when observed via the sync file API.

I don't remember what we have concluded in terms of observable effects in sync_file_merge?

I don't see how either of these are observable since this syncobj is never exposed to userspace in any way. Please help me understand what I'm missing here.

Single timeline context - two execbufs - return two out fences.

Before the patch those two had the same fence context, with the patch they have different ones.

Fence context is visible to userspace via sync file info (timeline name at least) and rules in sync_file_merge.

Thanks! How about adding this to the commit message:

It also means that sync files exported from different engines on a SINGLE_TIMELINE context will have different fence contexts. This is visible to userspace if it looks at the obj_name field of sync_fence_info.

I don't think sync_file_merge is significantly affected. If there are ever multiple fences in a sync_file, it gets a driver name of "dma_fence_array" and an object name of "unbound".

--Jason

Good point worth mentioninig in the commit message.

media-driver doesn't use any of this in combination with single_timeline, so we just dont care. -Daniel

...

Regards,

Tvrtko

...

--Jason

...

Regards,

Tvrtko

...

Moving SINGLE_TIMELINE to a syncobj emulation has a couple of technical advantages beyond mere annoyance. One is that intel_timeline is no longer an api-visible object and can remain entirely an implementation detail. This may be advantageous as we make scheduler changes going forward. Second is that, together with deleting the CLONE_CONTEXT API, we should now have a 1:1 mapping between intel_context and intel_timeline which may help us reduce locking.

v2 (Jason Ekstrand):

• Update the comment on i915_gem_context::syncobj to mention that it's an emulation and the possible race if userspace calls execbuffer2 twice on the same context concurrently.
• Wrap the checks for eb.gem_context->syncobj in unlikely()
• Drop the dma_fence reference
• Improved commit message

v3 (Jason Ekstrand):

• Move the dma_fence_put() to before the error exit

Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_context.c | 49 +++++-------------- .../gpu/drm/i915/gem/i915_gem_context_types.h | 14 +++++- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 16 ++++++ 3 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 2c2fefa912805..a72c9b256723b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -67,6 +67,8 @@ #include <linux/log2.h> #include <linux/nospec.h>

+#include <drm/drm_syncobj.h>

• #include "gt/gen6_ppgtt.h" #include "gt/intel_context.h" #include "gt/intel_context_param.h"

@@ -225,10 +227,6 @@ static void intel_context_set_gem(struct intel_context *ce, ce->vm = vm; }

• GEM_BUG_ON(ce->timeline);
  

• if (ctx->timeline)
  

•         ce->timeline = intel_timeline_get(ctx->timeline);
  

•  if (ctx->sched.priority >= I915_PRIORITY_NORMAL &&
       intel_engine_has_timeslices(ce->engine))
           __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);
  

@@ -351,9 +349,6 @@ void i915_gem_context_release(struct kref *ref) mutex_destroy(&ctx->engines_mutex); mutex_destroy(&ctx->lut_mutex);

• if (ctx->timeline)
  

•         intel_timeline_put(ctx->timeline);
  

•  put_pid(ctx->pid);
   mutex_destroy(&ctx->mutex);
  
  

@@ -570,6 +565,9 @@ static void context_close(struct i915_gem_context *ctx) if (vm) i915_vm_close(vm);

• if (ctx->syncobj)
  

•         drm_syncobj_put(ctx->syncobj);
  

•  ctx->file_priv = ERR_PTR(-EBADF);
  
   /*
  

@@ -765,33 +763,11 @@ static void __assign_ppgtt(struct i915_gem_context *ctx, i915_vm_close(vm); }

-static void __set_timeline(struct intel_timeline **dst,

•                    struct intel_timeline *src)
  

-{

• struct intel_timeline *old = *dst;
  

• *dst = src ? intel_timeline_get(src) : NULL;
  

• if (old)
  

•         intel_timeline_put(old);
  

-}

-static void __apply_timeline(struct intel_context *ce, void *timeline) -{

• __set_timeline(&ce->timeline, timeline);
  

-}

-static void __assign_timeline(struct i915_gem_context *ctx,

•                       struct intel_timeline *timeline)
  

-{

• __set_timeline(&ctx->timeline, timeline);
  

• context_apply_all(ctx, __apply_timeline, timeline);
  

-}

• static struct i915_gem_context * i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) { struct i915_gem_context *ctx;

• int ret;
  
   if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE &&
       !HAS_EXECLISTS(i915))
  

@@ -820,16 +796,13 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags) }

   if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) {

•         struct intel_timeline *timeline;
  

•         timeline = intel_timeline_create(&i915->gt);
  

•         if (IS_ERR(timeline)) {
  

•         ret = drm_syncobj_create(&ctx->syncobj,
  

•                                  DRM_SYNCOBJ_CREATE_SIGNALED,
  

•                                  NULL);
  

•         if (ret) {
                   context_close(ctx);
  

•                 return ERR_CAST(timeline);
  

•                 return ERR_PTR(ret);
           }
  

•         __assign_timeline(ctx, timeline);
  

•         intel_timeline_put(timeline);
   }
  
   trace_i915_context_create(ctx);
  

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h index 676592e27e7d2..df76767f0c41b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -83,7 +83,19 @@ struct i915_gem_context { struct i915_gem_engines __rcu *engines; struct mutex engines_mutex; /* guards writes to engines */

• struct intel_timeline *timeline;
  

• /**
  

•  * @syncobj: Shared timeline syncobj
  

•  *
  

•  * When the SHARED_TIMELINE flag is set on context creation, we
  

•  * emulate a single timeline across all engines using this syncobj.
  

•  * For every execbuffer2 call, this syncobj is used as both an in-
  

•  * and out-fence.  Unlike the real intel_timeline, this doesn't
  

•  * provide perfect atomic in-order guarantees if the client races
  

•  * with itself by calling execbuffer2 twice concurrently.  However,
  

•  * if userspace races with itself, that's not likely to yield well-
  

•  * defined results anyway so we choose to not care.
  

•  */
  

• struct drm_syncobj *syncobj;
  
   /**
    * @vm: unique address space (GTT)
  

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index b812f313422a9..d640bba6ad9ab 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -3460,6 +3460,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, goto err_vma; }

• if (unlikely(eb.gem_context->syncobj)) {
  

•         struct dma_fence *fence;
  

•         fence = drm_syncobj_fence_get(eb.gem_context->syncobj);
  

•         err = i915_request_await_dma_fence(eb.request, fence);
  

•         dma_fence_put(fence);
  

•         if (err)
  

•                 goto err_ext;
  

• }
  

•  if (in_fence) {
           if (args->flags & I915_EXEC_FENCE_SUBMIT)
                   err = i915_request_await_execution(eb.request,
  

@@ -3517,6 +3527,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, fput(out_fence->file); } }

• if (unlikely(eb.gem_context->syncobj)) {
  

•         drm_syncobj_replace_fence(eb.gem_context->syncobj,
  

•                                   &eb.request->fence);
  

• }
  

•  i915_request_put(eb.request);
  
  

  err_vma:

---

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

On Fri, Apr 23, 2021 at 05:31:17PM -0500, Jason Ekstrand wrote:

This has never been used by any userspace except IGT and provides no real functionality beyond parroting back parameters userspace passed in as part of context creation or via setparam. If the context is in legacy mode (where you use I915_EXEC_RENDER and friends), it returns success with zero data so it's not useful for discovering what engines are in the context. It's also not a replacement for the recently removed I915_CONTEXT_CLONE_ENGINES because it doesn't return any of the balancing or bonding information.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net

drivers/gpu/drm/i915/gem/i915_gem_context.c | 77 +-------------------- 1 file changed, 1 insertion(+), 76 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index a72c9b256723b..e8179918fa306 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1725,78 +1725,6 @@ set_engines(struct i915_gem_context *ctx, return 0; }

-static int -get_engines(struct i915_gem_context *ctx,

•    struct drm_i915_gem_context_param *args)
  

-{

• struct i915_context_param_engines __user *user;
• struct i915_gem_engines *e;
• size_t n, count, size;
• bool user_engines;
• int err = 0;
• e = __context_engines_await(ctx, &user_engines);
• if (!e)

• return -ENOENT;
  

• if (!user_engines) {

• i915_sw_fence_complete(&e->fence);
  

• args->size = 0;
  

• return 0;
  

• }
• count = e->num_engines;
• /* Be paranoid in case we have an impedance mismatch */
• if (!check_struct_size(user, engines, count, &size)) {

• err = -EINVAL;
  

• goto err_free;
  

• }
• if (overflows_type(size, args->size)) {

• err = -EINVAL;
  

• goto err_free;
  

• }
• if (!args->size) {

• args->size = size;
  

• goto err_free;
  

• }
• if (args->size < size) {

• err = -EINVAL;
  

• goto err_free;
  

• }
• user = u64_to_user_ptr(args->value);
• if (put_user(0, &user->extensions)) {

• err = -EFAULT;
  

• goto err_free;
  

• }
• for (n = 0; n < count; n++) {

• struct i915_engine_class_instance ci = {
  

• 	.engine_class = I915_ENGINE_CLASS_INVALID,
  

• 	.engine_instance = I915_ENGINE_CLASS_INVALID_NONE,
  

• };
  

• if (e->engines[n]) {
  

• 	ci.engine_class = e->engines[n]->engine->uabi_class;
  

• 	ci.engine_instance = e->engines[n]->engine->uabi_instance;
  

• }
  

• if (copy_to_user(&user->engines[n], &ci, sizeof(ci))) {
  

• 	err = -EFAULT;
  

• 	goto err_free;
  

• }
  

• }
• args->size = size;

-err_free:

• i915_sw_fence_complete(&e->fence);
• return err;

-}

static int set_persistence(struct i915_gem_context *ctx, const struct drm_i915_gem_context_param *args) @@ -2127,10 +2055,6 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, ret = get_ppgtt(file_priv, ctx, args); break;

• case I915_CONTEXT_PARAM_ENGINES:

• ret = get_engines(ctx, args);
  

• break;
  

• case I915_CONTEXT_PARAM_PERSISTENCE: args->size = 0; args->value = i915_gem_context_is_persistent(ctx);

@@ -2138,6 +2062,7 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,

case I915_CONTEXT_PARAM_NO_ZEROMAP: case I915_CONTEXT_PARAM_BAN_PERIOD:

• case I915_CONTEXT_PARAM_ENGINES: case I915_CONTEXT_PARAM_RINGSIZE:

I like how this list keeps growing. Same thing as usual about "pls check igt coverage".

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

default: ret = -EINVAL; -- 2.31.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

This adds a bunch of complexity which the media driver has never actually used. The media driver does technically bond a balanced engine to another engine but the balanced engine only has one engine in the sibling set. This doesn't actually result in a virtual engine.

Unless some userspace badly wants it, there's no good reason to support this case. This makes I915_CONTEXT_ENGINES_EXT_BOND a total no-op. We leave the validation code in place in case we ever decide we want to do something interesting with the bonding information.

v2 (Jason Ekstrand): - Don't delete quite as much code. Some of it was necessary.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 +- .../drm/i915/gt/intel_execlists_submission.c | 83 ------- .../drm/i915/gt/intel_execlists_submission.h | 4 - drivers/gpu/drm/i915/gt/selftest_execlists.c | 229 ------------------ 4 files changed, 6 insertions(+), 328 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index e8179918fa306..5f8d0faf783aa 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1553,6 +1553,12 @@ set_engines__bond(struct i915_user_extension __user *base, void *data) } virtual = set->engines->engines[idx]->engine;

+ if (intel_engine_is_virtual(virtual)) { + drm_dbg(&i915->drm, + "Bonding with virtual engines not allowed\n"); + return -EINVAL; + } + err = check_user_mbz(&ext->flags); if (err) return err; @@ -1593,18 +1599,6 @@ set_engines__bond(struct i915_user_extension __user *base, void *data) n, ci.engine_class, ci.engine_instance); return -EINVAL; } - - /* - * A non-virtual engine has no siblings to choose between; and - * a submit fence will always be directed to the one engine. - */ - if (intel_engine_is_virtual(virtual)) { - err = intel_virtual_engine_attach_bond(virtual, - master, - bond); - if (err) - return err; - } }

return 0; diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index de124870af44d..a6204c60b59cb 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -181,18 +181,6 @@ struct virtual_engine { int prio; } nodes[I915_NUM_ENGINES];

- /* - * Keep track of bonded pairs -- restrictions upon on our selection - * of physical engines any particular request may be submitted to. - * If we receive a submit-fence from a master engine, we will only - * use one of sibling_mask physical engines. - */ - struct ve_bond { - const struct intel_engine_cs *master; - intel_engine_mask_t sibling_mask; - } *bonds; - unsigned int num_bonds; - /* And finally, which physical engines this virtual engine maps onto. */ unsigned int num_siblings; struct intel_engine_cs *siblings[]; @@ -3307,7 +3295,6 @@ static void rcu_virtual_context_destroy(struct work_struct *wrk) intel_breadcrumbs_free(ve->base.breadcrumbs); intel_engine_free_request_pool(&ve->base);

- kfree(ve->bonds); kfree(ve); }

@@ -3560,33 +3547,13 @@ static void virtual_submit_request(struct i915_request *rq) spin_unlock_irqrestore(&ve->base.active.lock, flags); }

-static struct ve_bond * -virtual_find_bond(struct virtual_engine *ve, - const struct intel_engine_cs *master) -{ - int i; - - for (i = 0; i < ve->num_bonds; i++) { - if (ve->bonds[i].master == master) - return &ve->bonds[i]; - } - - return NULL; -} - static void virtual_bond_execute(struct i915_request *rq, struct dma_fence *signal) { - struct virtual_engine *ve = to_virtual_engine(rq->engine); intel_engine_mask_t allowed, exec; - struct ve_bond *bond;

allowed = ~to_request(signal)->engine->mask;

- bond = virtual_find_bond(ve, to_request(signal)->engine); - if (bond) - allowed &= bond->sibling_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)) @@ -3747,59 +3714,9 @@ intel_execlists_clone_virtual(struct intel_engine_cs *src) if (IS_ERR(dst)) return dst;

- if (se->num_bonds) { - struct virtual_engine *de = to_virtual_engine(dst->engine); - - de->bonds = kmemdup(se->bonds, - sizeof(*se->bonds) * se->num_bonds, - GFP_KERNEL); - if (!de->bonds) { - intel_context_put(dst); - return ERR_PTR(-ENOMEM); - } - - de->num_bonds = se->num_bonds; - } - return dst; }

-int intel_virtual_engine_attach_bond(struct intel_engine_cs *engine, - const struct intel_engine_cs *master, - const struct intel_engine_cs *sibling) -{ - struct virtual_engine *ve = to_virtual_engine(engine); - struct ve_bond *bond; - int n; - - /* Sanity check the sibling is part of the virtual engine */ - for (n = 0; n < ve->num_siblings; n++) - if (sibling == ve->siblings[n]) - break; - if (n == ve->num_siblings) - return -EINVAL; - - bond = virtual_find_bond(ve, master); - if (bond) { - bond->sibling_mask |= sibling->mask; - return 0; - } - - bond = krealloc(ve->bonds, - sizeof(*bond) * (ve->num_bonds + 1), - GFP_KERNEL); - if (!bond) - return -ENOMEM; - - bond[ve->num_bonds].master = master; - bond[ve->num_bonds].sibling_mask = sibling->mask; - - ve->bonds = bond; - ve->num_bonds++; - - return 0; -} - void intel_execlists_show_requests(struct intel_engine_cs *engine, struct drm_printer *m, void (*show_request)(struct drm_printer *m, diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.h b/drivers/gpu/drm/i915/gt/intel_execlists_submission.h index fd61dae820e9e..80cec37a56ba9 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.h +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.h @@ -39,10 +39,6 @@ intel_execlists_create_virtual(struct intel_engine_cs **siblings, struct intel_context * intel_execlists_clone_virtual(struct intel_engine_cs *src);

-int intel_virtual_engine_attach_bond(struct intel_engine_cs *engine, - const struct intel_engine_cs *master, - const struct intel_engine_cs *sibling); - bool intel_engine_in_execlists_submission_mode(const struct intel_engine_cs *engine);

diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c index 1081cd36a2bd3..f03446d587160 100644 --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c @@ -4311,234 +4311,6 @@ static int live_virtual_preserved(void *arg) return 0; }

-static int bond_virtual_engine(struct intel_gt *gt, - unsigned int class, - struct intel_engine_cs **siblings, - unsigned int nsibling, - unsigned int flags) -#define BOND_SCHEDULE BIT(0) -{ - struct intel_engine_cs *master; - struct i915_request *rq[16]; - enum intel_engine_id id; - struct igt_spinner spin; - unsigned long n; - int err; - - /* - * A set of bonded requests is intended to be run concurrently - * across a number of engines. We use one request per-engine - * and a magic fence to schedule each of the bonded requests - * at the same time. A consequence of our current scheduler is that - * we only move requests to the HW ready queue when the request - * becomes ready, that is when all of its prerequisite fences have - * been signaled. As one of those fences is the master submit fence, - * there is a delay on all secondary fences as the HW may be - * currently busy. Equally, as all the requests are independent, - * they may have other fences that delay individual request - * submission to HW. Ergo, we do not guarantee that all requests are - * immediately submitted to HW at the same time, just that if the - * rules are abided by, they are ready at the same time as the - * first is submitted. Userspace can embed semaphores in its batch - * to ensure parallel execution of its phases as it requires. - * Though naturally it gets requested that perhaps the scheduler should - * take care of parallel execution, even across preemption events on - * different HW. (The proper answer is of course "lalalala".) - * - * With the submit-fence, we have identified three possible phases - * of synchronisation depending on the master fence: queued (not - * ready), executing, and signaled. The first two are quite simple - * and checked below. However, the signaled master fence handling is - * contentious. Currently we do not distinguish between a signaled - * fence and an expired fence, as once signaled it does not convey - * any information about the previous execution. It may even be freed - * and hence checking later it may not exist at all. Ergo we currently - * do not apply the bonding constraint for an already signaled fence, - * as our expectation is that it should not constrain the secondaries - * and is outside of the scope of the bonded request API (i.e. all - * userspace requests are meant to be running in parallel). As - * it imposes no constraint, and is effectively a no-op, we do not - * check below as normal execution flows are checked extensively above. - * - * XXX Is the degenerate handling of signaled submit fences the - * expected behaviour for userpace? - */ - - GEM_BUG_ON(nsibling >= ARRAY_SIZE(rq) - 1); - - if (igt_spinner_init(&spin, gt)) - return -ENOMEM; - - err = 0; - rq[0] = ERR_PTR(-ENOMEM); - for_each_engine(master, gt, id) { - struct i915_sw_fence fence = {}; - struct intel_context *ce; - - if (master->class == class) - continue; - - ce = intel_context_create(master); - if (IS_ERR(ce)) { - err = PTR_ERR(ce); - goto out; - } - - memset_p((void *)rq, ERR_PTR(-EINVAL), ARRAY_SIZE(rq)); - - rq[0] = igt_spinner_create_request(&spin, ce, MI_NOOP); - intel_context_put(ce); - if (IS_ERR(rq[0])) { - err = PTR_ERR(rq[0]); - goto out; - } - i915_request_get(rq[0]); - - if (flags & BOND_SCHEDULE) { - onstack_fence_init(&fence); - err = i915_sw_fence_await_sw_fence_gfp(&rq[0]->submit, - &fence, - GFP_KERNEL); - } - - i915_request_add(rq[0]); - if (err < 0) - goto out; - - if (!(flags & BOND_SCHEDULE) && - !igt_wait_for_spinner(&spin, rq[0])) { - err = -EIO; - goto out; - } - - for (n = 0; n < nsibling; n++) { - struct intel_context *ve; - - ve = intel_execlists_create_virtual(siblings, nsibling); - if (IS_ERR(ve)) { - err = PTR_ERR(ve); - onstack_fence_fini(&fence); - goto out; - } - - err = intel_virtual_engine_attach_bond(ve->engine, - master, - siblings[n]); - if (err) { - intel_context_put(ve); - onstack_fence_fini(&fence); - goto out; - } - - err = intel_context_pin(ve); - intel_context_put(ve); - if (err) { - onstack_fence_fini(&fence); - goto out; - } - - rq[n + 1] = i915_request_create(ve); - intel_context_unpin(ve); - if (IS_ERR(rq[n + 1])) { - err = PTR_ERR(rq[n + 1]); - onstack_fence_fini(&fence); - goto out; - } - i915_request_get(rq[n + 1]); - - err = i915_request_await_execution(rq[n + 1], - &rq[0]->fence, - ve->engine->bond_execute); - i915_request_add(rq[n + 1]); - if (err < 0) { - onstack_fence_fini(&fence); - goto out; - } - } - onstack_fence_fini(&fence); - intel_engine_flush_submission(master); - igt_spinner_end(&spin); - - if (i915_request_wait(rq[0], 0, HZ / 10) < 0) { - pr_err("Master request did not execute (on %s)!\n", - rq[0]->engine->name); - err = -EIO; - goto out; - } - - for (n = 0; n < nsibling; n++) { - if (i915_request_wait(rq[n + 1], 0, - MAX_SCHEDULE_TIMEOUT) < 0) { - err = -EIO; - goto out; - } - - if (rq[n + 1]->engine != siblings[n]) { - pr_err("Bonded request did not execute on target engine: expected %s, used %s; master was %s\n", - siblings[n]->name, - rq[n + 1]->engine->name, - rq[0]->engine->name); - err = -EINVAL; - goto out; - } - } - - for (n = 0; !IS_ERR(rq[n]); n++) - i915_request_put(rq[n]); - rq[0] = ERR_PTR(-ENOMEM); - } - -out: - for (n = 0; !IS_ERR(rq[n]); n++) - i915_request_put(rq[n]); - if (igt_flush_test(gt->i915)) - err = -EIO; - - igt_spinner_fini(&spin); - return err; -} - -static int live_virtual_bond(void *arg) -{ - static const struct phase { - const char *name; - unsigned int flags; - } phases[] = { - { "", 0 }, - { "schedule", BOND_SCHEDULE }, - { }, - }; - struct intel_gt *gt = arg; - struct intel_engine_cs *siblings[MAX_ENGINE_INSTANCE + 1]; - unsigned int class; - int err; - - if (intel_uc_uses_guc_submission(&gt->uc)) - return 0; - - for (class = 0; class <= MAX_ENGINE_CLASS; class++) { - const struct phase *p; - int nsibling; - - nsibling = select_siblings(gt, class, siblings); - if (nsibling < 2) - continue; - - for (p = phases; p->name; p++) { - err = bond_virtual_engine(gt, - class, siblings, nsibling, - p->flags); - if (err) { - pr_err("%s(%s): failed class=%d, nsibling=%d, err=%d\n", - __func__, p->name, class, nsibling, err); - return err; - } - } - } - - return 0; -} - static int reset_virtual_engine(struct intel_gt *gt, struct intel_engine_cs **siblings, unsigned int nsibling) @@ -4712,7 +4484,6 @@ int intel_execlists_live_selftests(struct drm_i915_private *i915) SUBTEST(live_virtual_mask), SUBTEST(live_virtual_preserved), SUBTEST(live_virtual_slice), - SUBTEST(live_virtual_bond), SUBTEST(live_virtual_reset), };

On Fri, Apr 23, 2021 at 5:31 PM Jason Ekstrand jason@jlekstrand.net wrote:

This adds a bunch of complexity which the media driver has never actually used. The media driver does technically bond a balanced engine to another engine but the balanced engine only has one engine in the sibling set. This doesn't actually result in a virtual engine.

Unless some userspace badly wants it, there's no good reason to support this case. This makes I915_CONTEXT_ENGINES_EXT_BOND a total no-op. We leave the validation code in place in case we ever decide we want to do something interesting with the bonding information.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net

drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 +- .../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 | 100 -------- .../drm/i915/gt/intel_execlists_submission.h | 4 - drivers/gpu/drm/i915/gt/selftest_execlists.c | 229 ------------------ 6 files changed, 7 insertions(+), 353 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index e8179918fa306..5f8d0faf783aa 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1553,6 +1553,12 @@ set_engines__bond(struct i915_user_extension __user *base, void *data) } virtual = set->engines->engines[idx]->engine;

•   if (intel_engine_is_virtual(virtual)) {
  

•           drm_dbg(&i915->drm,
  

•                   "Bonding with virtual engines not allowed\n");
  

•           return -EINVAL;
  

•   }
  

•   err = check_user_mbz(&ext->flags);
    if (err)
            return err;
  

@@ -1593,18 +1599,6 @@ set_engines__bond(struct i915_user_extension __user *base, void *data) n, ci.engine_class, ci.engine_instance); return -EINVAL; }

•           /*
  

•            * A non-virtual engine has no siblings to choose between; and
  

•            * a submit fence will always be directed to the one engine.
  

•            */
  

•           if (intel_engine_is_virtual(virtual)) {
  

•                   err = intel_virtual_engine_attach_bond(virtual,
  

•                                                          master,
  

•                                                          bond);
  

•                   if (err)
  

•                           return err;
  

•           }
    }
  
    return 0;
  

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 de124870af44d..b6e2b59f133b7 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -181,18 +181,6 @@ struct virtual_engine { int prio; } nodes[I915_NUM_ENGINES];

•   /*
  

•    * Keep track of bonded pairs -- restrictions upon on our selection
  

•    * of physical engines any particular request may be submitted to.
  

•    * If we receive a submit-fence from a master engine, we will only
  

•    * use one of sibling_mask physical engines.
  

•    */
  

•   struct ve_bond {
  

•           const struct intel_engine_cs *master;
  

•           intel_engine_mask_t sibling_mask;
  

•   } *bonds;
  

•   unsigned int num_bonds;
  

•   /* And finally, which physical engines this virtual engine maps onto. */
    unsigned int num_siblings;
    struct intel_engine_cs *siblings[];
  

@@ -3307,7 +3295,6 @@ static void rcu_virtual_context_destroy(struct work_struct *wrk) intel_breadcrumbs_free(ve->base.breadcrumbs); intel_engine_free_request_pool(&ve->base);

•   kfree(ve->bonds);
    kfree(ve);
  

}

@@ -3560,42 +3547,6 @@ static void virtual_submit_request(struct i915_request *rq) spin_unlock_irqrestore(&ve->base.active.lock, flags); }

-static struct ve_bond * -virtual_find_bond(struct virtual_engine *ve,

•             const struct intel_engine_cs *master)
  

-{

•   int i;
  

•   for (i = 0; i < ve->num_bonds; i++) {
  

•           if (ve->bonds[i].master == master)
  

•                   return &ve->bonds[i];
  

•   }
  

•   return NULL;
  

-}

-static void -virtual_bond_execute(struct i915_request *rq, struct dma_fence *signal) -{

•   struct virtual_engine *ve = to_virtual_engine(rq->engine);
  

•   intel_engine_mask_t allowed, exec;
  

•   struct ve_bond *bond;
  

•   allowed = ~to_request(signal)->engine->mask;
  

•   bond = virtual_find_bond(ve, to_request(signal)->engine);
  

•   if (bond)
  

•           allowed &= bond->sibling_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;