Re: [PATCH v2 06/11] drm/i915: Introduce a preempt context

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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@chris-wilson.co.uk>
> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

<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.

> +	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).

> +{
> +	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?

> @@ -441,23 +456,30 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
>  	BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
>  	ida_init(&dev_priv->contexts.hw_ida);
>  
> -	ctx = i915_gem_create_context(dev_priv, NULL);
> +	/* lowest priority; idle task */
> +	ctx = create_kernel_context(dev_priv, I915_PRIORITY_MIN);
>  	if (IS_ERR(ctx)) {
>  		DRM_ERROR("Failed to create default global context (error %ld)\n",
>  			  PTR_ERR(ctx));
>  		return PTR_ERR(ctx);
>  	}
>  
> -	/* For easy recognisablity, we want the kernel context to be 0 and then
> +	/*
> +	 * For easy recognisablity, we want the kernel context to be 0 and then
>  	 * all user contexts will have non-zero hw_id.
>  	 */
>  	GEM_BUG_ON(ctx->hw_id);
> -
> -	i915_gem_context_clear_bannable(ctx);
> -	ctx->priority = I915_PRIORITY_MIN; /* lowest priority; idle task */
> +	GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
>  	dev_priv->kernel_context = ctx;
>  
> -	GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> +	/* highest priority; preempting task */
> +	ctx = create_kernel_context(dev_priv, INT_MAX);
> +	if (IS_ERR(ctx)) {
> +		DRM_ERROR("Failed to create default preempt context (error %ld)\n",
> +			  PTR_ERR(ctx));

There's no onion teardown so kernel_context is not freed. Pleas add

> +++ 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.

> +
>  	/* 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.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx