[Intel-gfx] [PATCH v4] drm/i915: Support to enable TRTT on GEN9

[Chris Wilson]

On Wed, Mar 09, 2016 at 05:00:24PM +0530, akash.goel at intel.com wrote:
> +static int
> +intel_context_get_trtt(struct intel_context *ctx,
> +		       struct drm_i915_gem_context_param *args)
> +{
> +	struct drm_i915_gem_context_trtt_param trtt_params;
> +	struct drm_device *dev = ctx->i915->dev;
> +
> +	if (!HAS_TRTT(dev) || !USES_FULL_48BIT_PPGTT(dev)) {

Both of these actually inspect dev_priv (and magically convert dev into
dev_priv).

> +		return -ENODEV;
> +	} else if (args->size < sizeof(trtt_params)) {
> +		args->size = sizeof(trtt_params);
> +	} else {
> +		trtt_params.segment_base_addr =
> +			ctx->trtt_info.segment_base_addr;
> +		trtt_params.l3_table_address =
> +			ctx->trtt_info.l3_table_address;
> +		trtt_params.null_tile_val =
> +			ctx->trtt_info.null_tile_val;
> +		trtt_params.invd_tile_val =
> +			ctx->trtt_info.invd_tile_val;
> +
> +		if (__copy_to_user(to_user_ptr(args->value),
> +				   &trtt_params,
> +				   sizeof(trtt_params)))
> +			return -EFAULT;

args->size = sizeof(trtt_params);

in case the use passed in size > sizeof(trtt_params) we want to report
how many bytes we wrote.

> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +intel_context_set_trtt(struct intel_context *ctx,
> +		       struct drm_i915_gem_context_param *args)
> +{
> +	struct drm_i915_gem_context_trtt_param trtt_params;
> +	struct drm_device *dev = ctx->i915->dev;
> +
> +	if (!HAS_TRTT(dev) || !USES_FULL_48BIT_PPGTT(dev))

Ditto (dev_priv)

> +		return -ENODEV;
> +	else if (ctx->flags & CONTEXT_USE_TRTT)
> +		return -EEXIST;

What locks are we holding here?

> +	else if (args->size < sizeof(trtt_params))
> +		return -EINVAL;
> +	else if (copy_from_user(&trtt_params,
> +				to_user_ptr(args->value),
> +				sizeof(trtt_params)))

Because whatever they are, we can't hold them here!

(Imagine/write a test that passes in the trtt_params inside a GTT mmaping.)

> @@ -923,7 +1015,6 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
>  		return PTR_ERR(ctx);
>  	}
>  
> -	args->size = 0;

Awooga. Does every path then set it?

> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7b8de85..8de0319 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2169,6 +2169,17 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
>  {
>  	gtt_write_workarounds(dev);
>  
> +	if (HAS_TRTT(dev) && USES_FULL_48BIT_PPGTT(dev)) {
> +		struct drm_i915_private *dev_priv = dev->dev_private;
> +		/*
> +		 * Globally enable TR-TT support in Hw.
> +		 * Still TR-TT enabling on per context basis is required.
> +		 * Non-trtt contexts are not affected by this setting.
> +		 */
> +		I915_WRITE(GEN9_TR_CHICKEN_BIT_VECTOR,
> +			   GEN9_TRTT_BYPASS_DISABLE);
> +	}
> +
>  	/* In the case of execlists, PPGTT is enabled by the context descriptor
>  	 * and the PDPs are contained within the context itself.  We don't
>  	 * need to do anything here. */
> @@ -3368,6 +3379,57 @@ i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
>  
>  }
>  
> +void intel_trtt_context_destroy_vma(struct i915_vma *vma)
> +{
> +	struct i915_address_space *vm = vma->vm;
> +
> +	WARN_ON(!list_empty(&vma->obj_link));
> +	WARN_ON(!list_empty(&vma->vm_link));
> +	WARN_ON(!list_empty(&vma->exec_list));

WARN_ON(!vma->pin_count);

> +
> +	drm_mm_remove_node(&vma->node);
> +	i915_ppgtt_put(i915_vm_to_ppgtt(vm));
> +	kmem_cache_free(to_i915(vm->dev)->vmas, vma);
> +}
> +
> +struct i915_vma *
> +intel_trtt_context_allocate_vma(struct i915_address_space *vm,
> +				uint64_t segment_base_addr)
> +{
> +	struct i915_vma *vma;
> +	int ret;
> +
> +	vma = kmem_cache_zalloc(to_i915(vm->dev)->vmas, GFP_KERNEL);
> +	if (!vma)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&vma->obj_link);
> +	INIT_LIST_HEAD(&vma->vm_link);
> +	INIT_LIST_HEAD(&vma->exec_list);
> +	vma->vm = vm;
> +	i915_ppgtt_get(i915_vm_to_ppgtt(vm));
> +
> +	/* Mark the vma as permanently pinned */
> +	vma->pin_count = 1;
> +
> +	/* Reserve from the 48 bit PPGTT space */
> +	vma->node.start = segment_base_addr;
> +	vma->node.size = GEN9_TRTT_SEGMENT_SIZE;
> +	ret = drm_mm_reserve_node(&vm->mm, &vma->node);
> +	if (ret) {
> +		ret = i915_gem_evict_for_vma(vma);

Given that this has a known GPF, you need a test case that tries to
evict an active/hanging object in order to make room for the trtt.

> +static int gen9_init_context_trtt(struct drm_i915_gem_request *req)

Since TRTT is render only, call this gen9_init_rcs_context_trtt()

>  static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
>  {
>  	struct i915_hw_ppgtt *ppgtt = req->ctx->ppgtt;
> @@ -1693,6 +1757,20 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
>  		req->ctx->ppgtt->pd_dirty_rings &= ~intel_ring_flag(req->ring);
>  	}
>  
> +	/*
> +	 * Emitting LRIs to update the TRTT registers is most reliable, instead
> +	 * of directly updating the context image, as this will ensure that
> +	 * update happens in a serialized manner for the context and also
> +	 * lite-restore scenario will get handled.
> +	 */
> +	if ((req->ring->id == RCS) && req->ctx->trtt_info.update_trtt_params) {
> +		ret = gen9_emit_trtt_regs(req);
> +		if (ret)
> +			return ret;
> +
> +		req->ctx->trtt_info.update_trtt_params = false;

Bah. Since we can only update the params once (EEXIST otherwise),
we emit the change when the user sets the new params.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre