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@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.
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@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.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
