Re: [PATCH 2/3] drm/i915: Allow userspace to specify ringsize on construction

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Janusz Krzysztofik (2019-11-18 11:14:12)
> Hi Chris,
> 
> Only some minor comments from me, mostly out of my curiosity.
> 
> On Friday, November 15, 2019 5:05:45 PM CET Chris Wilson wrote:
> > No good reason why we must always use a static ringsize, so let
> > userspace select one during construction.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> > +static int __apply_ringsize(struct intel_context *ce, void *sz)
> > +{
> > +     int err;
> > +
> > +     err = i915_active_wait(&ce->active);
> > +     if (err < 0)
> > +             return err;
> > +
> > +     if (intel_context_lock_pinned(ce))
> > +             return -EINTR;
> > +
> > +     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,
> > +                                             (unsigned long)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 = sz;
> > +     }
> > +
> > +unlock:
> > +     intel_context_unlock_pinned(ce);
> > +     return err;
> > +}
> 
> I'm wondering if this function (and __get_ringsize() below as well), with its 
> dependency on intel_context internals, especially on that dual meaning of 
> ce->ring which depends on (ce->flags & CONTEXT_ALLOC_BIT), would better fit 
> into drivers/gpu/drm/i915/gt/intel_context.c.

Possibly, but at the same time it's currently only implementing a
feature of the GEM context.

I hear you, I'm just resisting, mainly because I don't want to have to
think of a good name :)

intel_context_param.c
intel_context_ring.c

I might be able to find friends for either.

> > +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)
> > +{
> > +     int num_pages;
> > +
> > +     if (intel_context_lock_pinned(ce))
> > +             return -EINTR;
> > +
> > +     if (test_bit(CONTEXT_ALLOC_BIT, &ce->flags))
> > +             num_pages = ce->ring->size / I915_GTT_PAGE_SIZE;
> > +     else
> > +             num_pages = (uintptr_t)ce->ring / I915_GTT_PAGE_SIZE;
> > +
> > +     intel_context_unlock_pinned(ce);
> > +     return num_pages; /* stop on first engine */
> 
> Location of this comment seems not perfect to me as it is not quite obvious 
> how that works without examining how this function is used, but having spent a 
> while looking around, I'm not able to suggest a better place.

Yeah, the comment is for the intent of returning the positive, so
there's definitely a case for explaining the unusual pattern here.

The calling loop is just a standard if (err) return err; propagation so
that hardly merits a long winded explanation.

> > +static int get_ringsize(struct i915_gem_context *ctx,
> > +                     struct drm_i915_gem_context_param *args)
> > +{
> > +     int num_pages;
> > +
> > +     if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
> > +             return -ENODEV;
> > +
> > +     if (args->size)
> > +             return -EINVAL;
> > +
> > +     num_pages = context_apply_all(ctx, __get_ringsize, NULL);
> > +     if (num_pages < 0)
> > +             return num_pages;
> > +
> > +     args->value = (u64)num_pages * I915_GTT_PAGE_SIZE;
> 
> Do you convert to num_pages inside __get_ringsize() then back to size in bytes 
> to avoid an overflow?  Or any other reason?  Something that may be useful in 
> the future?

Just being prudent in making sure we have sufficient bits across all the
type-narrowing.

> > @@ -2003,6 +2113,19 @@ static int clone_engines(struct i915_gem_context *dst,
> >                       __free_engines(clone, n);
> >                       goto err_unlock;
> >               }
> > +
> > +             /* Copy across the preferred ringsize */
> > +             clone->engines[n]->ring = e->engines[n]->ring;
> > +             if (test_bit(CONTEXT_ALLOC_BIT, &e->engines[n]->flags)) {
> > +                     if (intel_context_lock_pinned(e->engines[n])) {
> > +                             __free_engines(clone, n + 1);
> > +                             goto err_unlock;
> > +                     }
> > +
> > +                     clone->engines[n]->ring =
> > +                             __intel_context_ring_size(e->engines[n]->ring->size);
> > +                     intel_context_unlock_pinned(e->engines[n]);
> > +             }
> 
> Another candidate for a helper located in 
> drivers/gpu/drm/i915/gt/intel_context.c?

This is much less of a candidate for potential reuse as I feel it is very
peculiar to the GEM->engines[], and not a fit for ugpu. At least as
currently written; an intel_context_get_ring_size() to go along with
intel_context_set_ring_size(), maybe.

> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index 5400d7e057f1..ae7cd681b075 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -1587,6 +1587,25 @@ struct drm_i915_gem_context_param {
> >   * By default, new contexts allow persistence.
> >   */
> >  #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.
> > + *
> > + * 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.
> > + */
> > +#define I915_CONTEXT_PARAM_RINGSIZE  0xc
> 
> I know it looked like that already before, but having other documented flags 
> separated by blank lines from each other, Is there any reason for not putting 
> another blank line after the last one?
> 
> >  /* Must be kept compact -- no holes and well documented */

You mean before the /* Must be... */? My intent is to make it conflict
and force people to take notice of the instruction.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx