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

[Ben Widawsky]

On Mon, Sep 23, 2013 at 09:39:31AM +0100, Chris Wilson wrote:
> 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.

Which bug are we talking about again? I'll update the commit message,
but I never saw a bug other than vm != ggtt (and ones I introduced while
trying to make you happy since v3).

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

I used the XXX because it is a bit tricky to understand for the casual
observer. Maybe "NB" was more appropriate. Anyway, removed.

> 
> > +		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;
>   }
> 

You're missing a step here, I assume you mean:
i915_gem_obj_ggtt_pin(...)
vma = i915_gem_obj_to_ggtt(...)
if (IS_ERR)...

Or had you something else in mind?

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

I am fine with this change - though I don't find it personally more
clear in stressing your point. Earlier requests were heavily in favor is
using "ggtt" where it couldn't be a generic VM. I think therefore the
excessive use of i915_gem_obj_to_ggtt makes the code look less generic,
which it is. Ie. I don't see this as abusing the interface.

I got all the other nitpicks. Once you ack the "missing step" I shall
resend.

-- 
Ben Widawsky, Intel Open Source Technology Center