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

[Chris Wilson]

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.

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

> >>>>+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!)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre