Re: [PATCH 5/6] [v3] drm/i915: Use the new vm [un]bind functions

[Ben Widawsky <ben@bwidawsk.net>]

On Wed, Sep 18, 2013 at 09:30:17AM +0100, Chris Wilson wrote:
> On Tue, Sep 17, 2013 at 05:02:03PM -0700, Ben Widawsky wrote:
> > > The code does
> > > 
> > > 	exec_start = i915_gem_obj_offset(batch_obj, vm) +
> > > 			args->batch_start_offset;
> > > 	exec_len = args->batch_len;
> > > 	...
> > > 	ret = ring->dispatch_execbuffer(ring,
> > > 					exec_start, exec_len,
> > > 					flags);
> > > 	if (ret)
> > > 		goto err;
> > > 
> > > So we lookup the address of the batch buffer in the wrong vm for
> > > I915_DISPATCH_SECURE.
> > > -Chris
> > > 
> > 
> > But this is very easily solved, no?
> > 
> > http://cgit.freedesktop.org/~bwidawsk/drm-intel/tree/drivers/gpu/drm/i915/i915_gem_execbuffer.c?h=ppgtt#n1083
> 
> No, just because the batch once had a ggtt entry doesn't mean the CS is
> going to use the ggtt for this execution...
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

I guess your use of the term, "CS" here is a bit confusing to me, since
in my head the CS better do whatever we tell it to do.

If you're trying to say, "just because batch_obj has a ggtt binding;
that doesn't necessarily mean it's the one to use at dispatch." I think
that statement is true, but it's still pretty simple to solve, just use
the I915_DISPATCH_SECURE flag to check instead of
obj->has_global_gtt_mapping. Right?

I'm really sorry about being so dense here.

As a side note, I tried really hard to think of how we could end up with
a ggtt mapping for batch_obj, and not want to use that one. I'm not
actually sure it's possible, but I can't prove it as such, so I'm
willing to assume it is possible. Excluding SNB, so few objects actually
will get a ggtt mapping, I don't believe any of them should be reused
for a batch BO - however, IGT can probably make it happen.

-- 
Ben Widawsky, Intel Open Source Technology Center