On Wed, Mar 09, 2016 at 05:00:24PM +0530, akash.goel@xxxxxxxxx wrote:
> +static int
> +intel_context_get_trtt(struct intel_context *ctx,
> + struct drm_i915_gem_context_param *args)
> +{
> + struct drm_i915_gem_context_trtt_param trtt_params;
> + struct drm_device *dev = ctx->i915->dev;
> +
> + if (!HAS_TRTT(dev) || !USES_FULL_48BIT_PPGTT(dev)) {
Both of these actually inspect dev_priv (and magically convert dev into
dev_priv).
> + return -ENODEV;
> + } else if (args->size < sizeof(trtt_params)) {
> + args->size = sizeof(trtt_params);
> + } else {
> + trtt_params.segment_base_addr =
> + ctx->trtt_info.segment_base_addr;
> + trtt_params.l3_table_address =
> + ctx->trtt_info.l3_table_address;
> + trtt_params.null_tile_val =
> + ctx->trtt_info.null_tile_val;
> + trtt_params.invd_tile_val =
> + ctx->trtt_info.invd_tile_val;
> +
> + if (__copy_to_user(to_user_ptr(args->value),
> + &trtt_params,
> + sizeof(trtt_params)))
> + return -EFAULT;
args->size = sizeof(trtt_params);
in case the use passed in size > sizeof(trtt_params) we want to report
how many bytes we wrote.
> + }
> +
> + return 0;
> +}
> +
> +static int
> +intel_context_set_trtt(struct intel_context *ctx,
> + struct drm_i915_gem_context_param *args)
> +{
> + struct drm_i915_gem_context_trtt_param trtt_params;
> + struct drm_device *dev = ctx->i915->dev;
> +
> + if (!HAS_TRTT(dev) || !USES_FULL_48BIT_PPGTT(dev))
Ditto (dev_priv)
> + return -ENODEV;
> + else if (ctx->flags & CONTEXT_USE_TRTT)
> + return -EEXIST;
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!
(Imagine/write a test that passes in the trtt_params inside a GTT mmaping.)
> @@ -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?
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7b8de85..8de0319 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2169,6 +2169,17 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
> {
> gtt_write_workarounds(dev);
>
> + if (HAS_TRTT(dev) && USES_FULL_48BIT_PPGTT(dev)) {
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + /*
> + * Globally enable TR-TT support in Hw.
> + * Still TR-TT enabling on per context basis is required.
> + * Non-trtt contexts are not affected by this setting.
> + */
> + I915_WRITE(GEN9_TR_CHICKEN_BIT_VECTOR,
> + GEN9_TRTT_BYPASS_DISABLE);
> + }
> +
> /* In the case of execlists, PPGTT is enabled by the context descriptor
> * and the PDPs are contained within the context itself. We don't
> * need to do anything here. */
> @@ -3368,6 +3379,57 @@ i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
>
> }
>
> +void intel_trtt_context_destroy_vma(struct i915_vma *vma)
> +{
> + struct i915_address_space *vm = vma->vm;
> +
> + WARN_ON(!list_empty(&vma->obj_link));
> + WARN_ON(!list_empty(&vma->vm_link));
> + WARN_ON(!list_empty(&vma->exec_list));
WARN_ON(!vma->pin_count);
> +
> + drm_mm_remove_node(&vma->node);
> + i915_ppgtt_put(i915_vm_to_ppgtt(vm));
> + kmem_cache_free(to_i915(vm->dev)->vmas, vma);
> +}
> +
> +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.
> +static int gen9_init_context_trtt(struct drm_i915_gem_request *req)
Since TRTT is render only, call this gen9_init_rcs_context_trtt()
> 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.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
