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

From: Chris Wilson <chris@chris-wilson.co.uk>
To: "Goel, Akash" <akash.goel@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v4] drm/i915: Support to enable TRTT on GEN9
Date: Wed, 9 Mar 2016 15:02:55 +0000	[thread overview]
Message-ID: <20160309150255.GI1405@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <56E0381F.2080603@intel.com>

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

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

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

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

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx