[Intel-gfx] [PATCH] [v9] drm/i915: Use the new vm [un]bind functions

[Chris Wilson]

On Sun, Sep 22, 2013 at 11:46:00AM -0700, Ben Widawsky wrote:
> From: Ben Widawsky <ben at bwidawsk.net>
> 
> 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 at chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>

Minor comments inline,
(for the series) Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

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