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

[Ben Widawsky]

On Wed, Sep 18, 2013 at 03:53:37PM +0100, Chris Wilson wrote:
> On Wed, Sep 18, 2013 at 07:47:45AM -0700, Ben Widawsky wrote:
> > 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.
> 
> Exactly, we tell the CS to use the ggtt for the SECURE batch (at least
> on some generations).
>  
> > 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?
> 
> Yes. With the same caveat that it may change.

What may change? Dispatch secure always means use the GGTT offset, does
it not? Or do you think we'll want privileged batches running from the
PPGTT? If the latter is true, why ever use GGTT?

>  
> > 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.
> 
> It's trivial for a batch to end up with a ggtt entry - userspace can
> just access it through the GTT. Or any bo prior to reusing it as a
> batch.
> -Chris

Trivial, perhaps on the gtt mapping. It's not really relevant, but is
any userspace currently doing that? As for BO reuse, that's a separate
problem - are we handing back BOs with their mappings intact? That seems
like a security problem.

> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ben Widawsky, Intel Open Source Technology Center