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

From: "Goel, Akash" <akash.goel@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	intel-gfx@lists.freedesktop.org,
	Michel Thierry <michel.thierry@intel.com>,
	akash.goel@intel.com
Subject: Re: [PATCH v4] drm/i915: Support to enable TRTT on GEN9
Date: Wed, 9 Mar 2016 22:08:39 +0530	[thread overview]
Message-ID: <56E0518F.1080700@intel.com> (raw)
In-Reply-To: <20160309162153.GM1405@nuc-i3427.alporthouse.com>

On 3/9/2016 9:51 PM, Chris Wilson wrote:
> On Wed, Mar 09, 2016 at 09:26:08PM +0530, Goel, Akash wrote:
>>
>>
>> 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.
>
> It what's we have to do. However, we have to make sure that we do not
> lose state, or the user doesn't interfere, across the unlock. i.e. make
> sure we have a reference on the context, double check that the state is
> still valid (so do the EEXISTS check after the copy) etc.
>

Thanks for the inputs, will keep them in mind.

>>>> 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 ?
>
> Submit a recursive batch using the vma inside your trtt region.
> See igt_hang_ctx() if you are free to select the trtt region using the
> offset generated by igt_hang_ctx() (and for this test you are), then it
> is very simple. See gem_softpin, test_evict_hang() and
> test_evict_active().
>

Thanks for suggesting these tests, will refer them.

>>>>>> +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.
>
> A conditional on every batch vs a one-off ?
>
> request = i915_gem_request_alloc(&dev_priv->ring[RCS], ctx);
> if (IS_ERR(request))
> 	return ERR_PTR(request);
>
> ret = gen9_emit_trtt_regs(request);
> if (ret) {
> 	i915_gem_request_cancel(request);
> 	return ret;
> }
>
> i915_add_request(request);
> return 0;
>
> Complain to whoever sold you your kernel if it is not that simple. (And
> that is quite byzantine compared to how it should be!)

Fine, thanks much for the required code snippet, will update the patch.

Sorry actually was bit skeptical about introducing a new non-execbuffer 
path, from the where the request allocation & submission happens.

Best regards
Akash

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