Re: [PATCH 1/9] drm/i915: Assign every HW context a unique ID

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 19/04/16 07:49, Chris Wilson wrote:
> The hardware tracks contexts and expects all live contexts (those active
> on the hardware) to have a unique identifier. This is used by the
> hardware to assign pagefaults and the like to a particular context.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
>   drivers/gpu/drm/i915/i915_drv.h         |  4 ++++
>   drivers/gpu/drm/i915/i915_gem_context.c | 32 ++++++++++++++++++++++++++++++++
>   3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index ffc4734cb2dc..cacbe45b7938 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2014,7 +2014,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
>   		    ctx->legacy_hw_ctx.rcs_state == NULL)
>   			continue;
>
> -		seq_puts(m, "HW context ");
> +		seq_printf(m, "HW context %d ", ctx->hw_id);

Should be %u.

>   		describe_ctx(m, ctx);
>   		if (ctx == dev_priv->kernel_context)
>   			seq_printf(m, "(kernel context) ");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index dc3bb5c10957..368fd7090189 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -851,6 +851,7 @@ struct intel_context {
>   	struct i915_ctx_hang_stats hang_stats;
>   	struct i915_hw_ppgtt *ppgtt;
>
> +	unsigned hw_id;
>   	unsigned flags;
>   #define CONTEXT_NO_ZEROMAP		(1<<0)
>   #define CONTEXT_NO_ERROR_CAPTURE	(1<<1)
> @@ -1810,6 +1811,9 @@ struct drm_i915_private {
>   	DECLARE_HASHTABLE(mm_structs, 7);
>   	struct mutex mm_lock;
>
> +	struct ida context_hw_ida;
> +#define MAX_CONTEXT_HW_ID (1<<20)

BUILD_BUG_ON somewhere if someone in the future is tempted to increase 
this without realizing ids returned by ida_simple_get are limited to 
0x80000000 ?

> +
>   	/* Kernel Modesetting */
>
>   	struct drm_crtc *plane_to_crtc_mapping[I915_MAX_PIPES];
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index d7d9763a9818..db53d811370d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -169,6 +169,8 @@ void i915_gem_context_free(struct kref *ctx_ref)
>   	if (ctx->legacy_hw_ctx.rcs_state)
>   		drm_gem_object_unreference(&ctx->legacy_hw_ctx.rcs_state->base);
>   	list_del(&ctx->link);
> +
> +	ida_simple_remove(&ctx->i915->context_hw_ida, ctx->hw_id);
>   	kfree(ctx);
>   }
>
> @@ -209,6 +211,28 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
>   	return obj;
>   }
>
> +static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out)
> +{
> +	int ret;
> +
> +	ret = ida_simple_get(&dev_priv->context_hw_ida,
> +			     0, MAX_CONTEXT_HW_ID, GFP_KERNEL);
> +	if (ret < 0) {
> +		/* Contexts are only released when no longer active.
> +		 * Flush any pending retires to hopefully release some
> +		 * stale contexts and try again.
> +		 */
> +		i915_gem_retire_requests(dev_priv->dev);
> +		ret = ida_simple_get(&dev_priv->context_hw_ida,
> +				     0, MAX_CONTEXT_HW_ID, GFP_KERNEL);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	*out = ret;
> +	return 0;
> +}
> +
>   static struct intel_context *
>   __create_hw_context(struct drm_device *dev,
>   		    struct drm_i915_file_private *file_priv)
> @@ -225,6 +249,12 @@ __create_hw_context(struct drm_device *dev,
>   	list_add_tail(&ctx->link, &dev_priv->context_list);
>   	ctx->i915 = dev_priv;
>
> +	ret = assign_hw_id(dev_priv, &ctx->hw_id);
> +	if (ret) {
> +		kfree(ctx);
> +		return ERR_PTR(ret);
> +	}
> +
>   	if (dev_priv->hw_context_size) {
>   		struct drm_i915_gem_object *obj =
>   				i915_gem_alloc_context_obj(dev, dev_priv->hw_context_size);
> @@ -375,6 +405,8 @@ int i915_gem_context_init(struct drm_device *dev)
>   		}
>   	}
>
> +	ida_init(&dev_priv->context_hw_ida);
> +
>   	if (i915.enable_execlists) {
>   		/* NB: intentionally left blank. We will allocate our own
>   		 * backing objects as we need them, thank you very much */
>

Otherwise looks fine. With the two minors above added:

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

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx