Re: [PATCH 01/21] drm/i915: Drop I915_CONTEXT_PARAM_RINGSIZE

[Daniel Vetter <daniel@ffwll.ch>]

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.

- 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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel