Re: [PATCH v4] drm/i915: Support to enable TRTT on GEN9

["Goel, Akash" <akash.goel@intel.com>]

On 3/9/2016 5:34 PM, Chris Wilson wrote:
> On Wed, Mar 09, 2016 at 05:00:24PM +0530, akash.goel@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).

Sorry, my bad. Missed the __I915__ macro.
>
>> +		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.

fine will add this.
>
>> +	}
>> +
>> +	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!
>
The struct_mutex lock was taken in the caller, ioctl function.
Ok, so need to release that before invoking copy_from_user.

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

This could cause a recursive locking of struct_mutex from the gem_fault() ?

>
>> @@ -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?
>

It is being set only for the TRTT case. For the other existing cases, 
should it be explicitly set to 0, is that really needed ?

>> 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);

Thanks, will add.

>
>> +
>> +	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.
>
In the new test case, will soft pin objects in TR-TT segment first. Then 
later on enabling TR-TT, those objects should get evicted.

>> +static int gen9_init_context_trtt(struct drm_i915_gem_request *req)
>
> Since TRTT is render only, call this gen9_init_rcs_context_trtt()
>
Thanks, will change.

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

Sorry couldn't get this point. We can't emit the params right away when 
User sets them (only once). We need to emit/apply the params (onetime) 
in a deferred manner on the next submission.

Best regards
Akash

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