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@xxxxxxxxxxxxxx>
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.
- 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.
If not, please make sure that's the case.
Aside from these patch looks good.
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
