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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
