Quoting Joonas Lahtinen (2017-09-28 13:32:09)
> On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> > Add another perma-pinned context for using for preemption at any time.
> > We cannot just reuse the existing kernel context, as first and foremost
> > we need to ensure that we can preempt the kernel context itself, so
> > require a distinct context id. Similar to the kernel context, we may
> > want to interrupt execution and switch to the preempt context at any
> > time, and so it needs to be permanently pinned and available.
> >
> > To compensate for yet another permanent allocation, we shrink the
> > existing context and the new context by reducing their ringbuffer to the
> > minimum.
> >
> > v2: Assert that we never allocate a request from the preemption context.
> > v3: Limit perma-pin to engines that may preempt.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Reviewed-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
>
> <SNIP>
>
> > @@ -2250,8 +2251,9 @@ struct drm_i915_private {
> > wait_queue_head_t gmbus_wait_queue;
> >
> > struct pci_dev *bridge_dev;
> > - struct i915_gem_context *kernel_context;
> > struct intel_engine_cs *engine[I915_NUM_ENGINES];
>
> First to touch old code gets to add kerneldoc, just formulate what's in
> the commit message into here.
/* Nothing to see here, please move along. */
> > + struct i915_gem_context *kernel_context;
> > + struct i915_gem_context *preempt_context;
> > struct i915_vma *semaphore;
> >
> > struct drm_dma_handle *status_page_dmah;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 921ee369c74d..1518e110fd04 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -416,14 +416,29 @@ i915_gem_context_create_gvt(struct drm_device *dev)
> > return ctx;
> > }
> >
> > +static struct i915_gem_context *
> > +create_kernel_context(struct drm_i915_private *i915, int prio)
>
> I may vote for 'create_internal_context' because 'kernel context' is
> quite an established term. But I've got no hard option, just gotta keep
> the terminology coherent (eg. in the above requested kerneldoc).
The contexts return by this function pass i915_gem_context_is_kernel().
Atm, I think kernel_context is still the consistent terminology.
> > +{
> > + struct i915_gem_context *ctx;
> > +
> > + ctx = i915_gem_create_context(i915, NULL);
> > + if (IS_ERR(ctx))
> > + return ctx;
> > +
> > + i915_gem_context_clear_bannable(ctx);
> > + ctx->priority = prio;
> > + ctx->ring_size = PAGE_SIZE;
> > +
> > + return ctx;
> > +}
> > +
> > int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> > {
> > struct i915_gem_context *ctx;
> >
> > /* Init should only be called once per module load. Eventually the
> > * restriction on the context_disabled check can be loosened. */
>
> Commit seems to be out of date now?
Got to keep some humour around.
> > +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> > @@ -587,6 +587,13 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
> >
> > lockdep_assert_held(&dev_priv->drm.struct_mutex);
> >
> > + /*
> > + * Preempt contexts are reserved for exclusive use to inject a
> > + * preemption context switch. They are never to be used for any trivial
> > + * request!
> > + */
> > + GEM_BUG_ON(ctx == dev_priv->preempt_context);
>
> Maybe check the prio here too.
More a task for ->schedule().
>
> > +
> > /* ABI: Before userspace accesses the GPU (e.g. execbuffer), report
> > * EIO if the GPU is already wedged.
> > */
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index 21e037fc21b7..02eb25ed1064 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -613,9 +613,22 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
> > if (IS_ERR(ring))
> > return PTR_ERR(ring);
> >
> > + /*
> > + * Similarly the preempt context must always be available so that
> > + * we can interrupt the engine at any time.
> > + */
> > + if (INTEL_INFO(engine->i915)->has_logical_ring_preemption) {
> > + ring = engine->context_pin(engine,
> > + engine->i915->preempt_context);
> > + if (IS_ERR(ring)) {
> > + ret = PTR_ERR(ring);
> > + goto err_unpin_kernel;
> > + }
> > + }
>
> Maybe add helper for the internal/kernel_context_pin and _free too,
> it's unnecessary code duplication.
What's the duplication? I'm not clear on what may be saved.
engine->context_pin()
engine->context_unpin()
??
The cleanup path I have more sympathy for, I considered doing it. But I
keep getting side tracked by the desire to eliminate the context_close()
for the kernel contexts and creating them in a closed state.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
