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@xxxxxxxxxxxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > ---
> > +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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
