[PATCH 13/87] drm/i915: Simplify testing for am-I-the-kernel-context?

[Tvrtko Ursulin]

On 05/01/2017 23:19, Chris Wilson wrote:
> The kernel context (dev_priv->kernel_context) is unique in that it is
> not associated with any user filp - it is the only one with
> ctx->file_priv == NULL. This is a simpler test than comparing it against
> dev_priv->kernel_context which involves some pointer dancing.
>
> In checking that this is true, we notice that the gvt context is
> allocating itself a i915_hw_ppgtt it doesn't use and not flagging that
> its file_priv should be invalid.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         |  2 +-
>  drivers/gpu/drm/i915/i915_gem_context.c | 13 ++++++++++++-
>  drivers/gpu/drm/i915/i915_gem_context.h |  5 +++++
>  drivers/gpu/drm/i915/intel_lrc.c        |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  4 ++--
>  5 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d2ca6f729f42..f6f4ec894a7f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4204,7 +4204,7 @@ static void assert_kernel_context_is_current(struct drm_i915_private *dev_priv)
>  	enum intel_engine_id id;
>
>  	for_each_engine(engine, dev_priv, id)
> -		GEM_BUG_ON(engine->last_retired_context != dev_priv->kernel_context);
> +		GEM_BUG_ON(!i915_gem_context_is_kernel(engine->last_retired_context));
>  }
>
>  int i915_gem_suspend(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 07ac81103f44..40a6939e3956 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -413,14 +413,17 @@ i915_gem_context_create_gvt(struct drm_device *dev)
>  	if (ret)
>  		return ERR_PTR(ret);
>
> -	ctx = i915_gem_create_context(to_i915(dev), NULL);
> +	ctx = __create_hw_context(to_i915(dev), NULL);
>  	if (IS_ERR(ctx))
>  		goto out;
>
> +	ctx->file_priv = ERR_PTR(-EBADF);
>  	i915_gem_context_set_closed(ctx); /* not user accessible */
>  	i915_gem_context_clear_bannable(ctx);
>  	i915_gem_context_set_force_single_submission(ctx);
>  	ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */
> +
> +	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
>  out:
>  	mutex_unlock(&dev->struct_mutex);
>  	return ctx;
> @@ -472,6 +475,8 @@ int i915_gem_context_init(struct drm_i915_private *dev_priv)
>  	ctx->priority = I915_PRIORITY_MIN; /* lowest priority; idle task */
>  	dev_priv->kernel_context = ctx;
>
> +	GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> +
>  	DRM_DEBUG_DRIVER("%s context support initialized\n",
>  			i915.enable_execlists ? "LR" :
>  			dev_priv->hw_context_size ? "HW" : "fake");
> @@ -524,6 +529,8 @@ void i915_gem_context_fini(struct drm_i915_private *dev_priv)
>
>  	lockdep_assert_held(&dev_priv->drm.struct_mutex);
>
> +	GEM_BUG_ON(!i915_gem_context_is_kernel(dctx));
> +
>  	context_close(dctx);
>  	dev_priv->kernel_context = NULL;
>
> @@ -549,6 +556,8 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
>  	ctx = i915_gem_create_context(to_i915(dev), file_priv);
>  	mutex_unlock(&dev->struct_mutex);
>
> +	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
> +
>  	if (IS_ERR(ctx)) {
>  		idr_destroy(&file_priv->context_idr);
>  		return PTR_ERR(ctx);
> @@ -968,6 +977,8 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  	if (IS_ERR(ctx))
>  		return PTR_ERR(ctx);
>
> +	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
> +
>  	args->ctx_id = ctx->user_handle;
>  	DRM_DEBUG("HW context %d created\n", args->ctx_id);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index 89f6764fb338..0ac750b90f3d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -246,6 +246,11 @@ static inline bool i915_gem_context_is_default(const struct i915_gem_context *c)
>  	return c->user_handle == DEFAULT_CONTEXT_HANDLE;
>  }
>
> +static inline bool i915_gem_context_is_kernel(struct i915_gem_context *ctx)
> +{
> +	return !ctx->file_priv;
> +}
> +
>  /* i915_gem_context.c */
>  int __must_check i915_gem_context_init(struct drm_i915_private *dev_priv);
>  void i915_gem_context_lost(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index a9eefb171170..6db246ad2f13 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -786,7 +786,7 @@ static int execlists_context_pin(struct intel_engine_cs *engine,
>  	flags = PIN_GLOBAL;
>  	if (ctx->ggtt_offset_bias)
>  		flags |= PIN_OFFSET_BIAS | ctx->ggtt_offset_bias;
> -	if (ctx == ctx->i915->kernel_context)
> +	if (i915_gem_context_is_kernel(ctx))
>  		flags |= PIN_HIGH;
>
>  	ret = i915_vma_pin(ce->state, 0, GEN8_LR_CONTEXT_ALIGN, flags);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e84d488e167a..0971ac396b60 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1974,7 +1974,7 @@ static int intel_ring_context_pin(struct intel_engine_cs *engine,
>  		unsigned int flags;
>
>  		flags = 0;
> -		if (ctx == ctx->i915->kernel_context)
> +		if (i915_gem_context_is_kernel(ctx))
>  			flags = PIN_HIGH;
>
>  		ret = context_pin(ctx, flags);
> @@ -1989,7 +1989,7 @@ static int intel_ring_context_pin(struct intel_engine_cs *engine,
>  	 * as during eviction we cannot allocate and pin the renderstate in
>  	 * order to initialise the context.
>  	 */
> -	if (ctx == ctx->i915->kernel_context)
> +	if (i915_gem_context_is_kernel(ctx))
>  		ce->initialised = true;
>
>  	i915_gem_context_get(ctx);
>

Looks OK, checked the GVT code as well.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko