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

[Goel, Akash]

On 3/9/2016 8:32 PM, Chris Wilson wrote:
> On Wed, Mar 09, 2016 at 08:20:07PM +0530, Goel, Akash wrote:
>>> 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() ?
>
> Exactly. At the least lockdep should warn if we hit a fault along this
> path (due to the illegal nesting of mmap_sem inside struct_mtuex).
>

I hope it won't look ungainly to unlock the struct_mutex before 
copy_from_user and lock it back right after that.

>>
>>>
>>>> @@ -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 ?
>
> Yes. All other paths need to report .size = 0 (as they don't write
> through a pointer).
>

Fine will add the args->size = 0 for all the other cases.

>>>> +	/* 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.
>
> Yes. But make sure you have combinations of inactive, active, and
> hanging objects inside the to-be-evicted segment. Those cover the most
> frequent errors we have to handle (and easiest to reproduce).
>
Fine, will refer other tests logic to see how to ensure that previously 
soft pinned object is still marked as active, when the eviction happens 
on enabling TR-TT.

Sorry what is the hanging object type ?

>>>> +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.
>
> Why can't we? We can construct and submit a request setting the
> registers inside the right context image at that point, and they never
> change after that point.

Ok yes a new request can be constructed & submitted for the Context, 
emitting the LRIs to update the TRTT params in the Context image.
But won't that be relatively cumbersome considering that we are able to 
easily defer & conflate that with next batch submission, through an 
extra flag trtt_info.update_trtt_params.

Best regards
Akash


> -Chris
>