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

[Jason Ekstrand <jason@jlekstrand.net>]

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