On Sun, Sep 22, 2013 at 11:46:00AM -0700, Ben Widawsky wrote:
> From: Ben Widawsky <ben@xxxxxxxxxxxx>
>
> Building on the last patch which created the new function pointers in
> the VM for bind/unbind, here we actually put those new function pointers
> to use.
>
> Split out as a separate patch to aid in review. I'm fine with squashing
> into the previous patch if people request it.
>
> v2: Updated to address the smart ggtt which can do aliasing as needed
> Make sure we bind to global gtt when mappable and fenceable. I thought
> we could get away without this initialy, but we cannot.
>
> v3: Make the global GTT binding explicitly use the ggtt VM for
> bind_vma(). While at it, use the new ggtt_vma helper (Chris)
>
> v4: Make the code support the secure dispatch flag, which requires
> special handling during execbuf. This was fixed (incorrectly) later in
> the series, but having it here earlier in the series should be perfectly
> acceptable. (Chris)
> Move do_switch over to the new, special ggtt_vma interface.
>
> v5: Don't use a local variable (or assertion) when setting the batch
> object to the global GTT during secure dispatch (Chris)
>
> v6: Caclulate the exec offset for the secure case (Bug fix missed on
> v4). (Chris)
> Remove redundant check for has_global_gtt_mapping, since it is done in
> bind_vma.
>
> v7: Remove now unused dev_priv in do_switch
> Don't pass the vm to ggtt_offset (error from v6 which I should have
> caught before sending).
>
> v8: Assert, and rework the SNB workaround (to make it a bit clearer)
> code to make sure the VM can't be anything but the GGTT.
>
> v9: Fixing more bugs which can't exist yet on the behest of Chris. Make
> sure that the batch object is properly bound, and added to the global
> VM's active list - for when we use non-global VMs. (Chris)
Not quite, the patch introduced an outright bug in addition to potential
issue of vm != ggtt.
> CC: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
Minor comments inline,
(for the series) Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> @@ -1118,8 +1115,32 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> * batch" bit. Hence we need to pin secure batches into the global gtt.
> * hsw should have this fixed, but let's be paranoid and do it
> * unconditionally for now. */
> - if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping)
> - i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level);
> + if (flags & I915_DISPATCH_SECURE) {
> + struct i915_address_space *ggtt = obj_to_ggtt(batch_obj);
Please leave whitespace after variable declarations.
> + /* Assuming all privileged batches are in the global GTT means
> + * we need to make sure we have a global gtt offset, as well as
> + * the PTEs mapped. As mentioned above, we can forego this on
> + * HSW, but don't.
> + */
And a line of whitespace here since this is a block comment and not
closely coupled to the next line of code.
> + ret = i915_gem_obj_ggtt_pin(batch_obj, 0, false, false);
> + if (ret)
> + goto err;
> +
> + ggtt->bind_vma(i915_gem_obj_to_ggtt(batch_obj),
> + batch_obj->cache_level,
> + GLOBAL_BIND);
> +
> + /* XXX: Since the active list is per VM, we need to make sure
> + * this VMA ends up on the GGTT's active list to avoid premature
> + * eviction.
> + */
No XXX required, unless you have a magical plan; the reasoning is sound.
> + i915_vma_move_to_active(i915_gem_obj_to_ggtt(batch_obj), ring);
> +
> + i915_gem_object_unpin(batch_obj);
I think this interface violates Rusty's rules (API should be easy to
use but hard to misuse).
vma = i915_gem_object_pin(batch_obj, ggtt, 0, false, false);
if (IS_ERR(vm)) {
ret = PTR_ERR(vm);
goto err;
}
ggtt->bind_vma(vma, batch_obj->cache_level, GLOBAL_BIND); // this would a flag to a future pin()
i915_vma_move_to_active(vma, ring);
exec_start += vma->node.start;
i915_gem_object_unpin(batch_obj, vma);
What I am stressing here is that the vma->node is only valid whilst the
object is pinned, and that access should be through the vma rather than
the object. However, that interface is a little too idealistic.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
