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 12:57:20AM +0100, Chris Wilson wrote:
> On Tue, Sep 17, 2013 at 04:48:50PM -0700, Ben Widawsky wrote:
> > On Wed, Sep 18, 2013 at 12:33:32AM +0100, Chris Wilson wrote:
> > > On Tue, Sep 17, 2013 at 04:14:43PM -0700, Ben Widawsky wrote:
> > > > On Tue, Sep 17, 2013 at 09:55:35PM +0100, Chris Wilson wrote:
> > > > > On Tue, Sep 17, 2013 at 10:01:33AM -0700, Ben Widawsky wrote:
> > > > > > @@ -1117,8 +1109,13 @@ 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 &&
> > > > > > +	    !batch_obj->has_global_gtt_mapping) {
> > > > > > +		const struct i915_address_space *ggtt = obj_to_ggtt(batch_obj);
> > > > > > +		struct i915_vma *vma = i915_gem_obj_to_ggtt(batch_obj);
> > > > > > +		BUG_ON(!vma);
> > > > > > +		ggtt->bind_vma(vma, batch_obj->cache_level, GLOBAL_BIND);
> > > > > > +	}
> > > > > 
> > > > > The issue here is that if we don't set the USE_PPGTT/USE_SECURE flag in
> > > > > the dispatch, the CS will use the GGTT (hence our binding) but so we
> > > > > then need to use the GGTT offset for the dispatch as well.
> > > > > 
> > > > > Is that as concisely as we can write bind_to_ggtt? :(
> > > > > -Chris
> > > > > 
> > > > 
> > > > Resuming the conversation started on irc... what do you want from me?
> > > 
> > > I think we need to pass the ggtt offset to dispatch for
> > > I915_DISPATCH_SECURE -- which offset to use might even depend upon the
> > > implementation and hw generation in intel_ringbuffer.c. But at the very
> > > least, I think SNB/IVB will be executing the wrong address come full
> > > ppgtt.
> > > 
> > > dev_priv->ggtt.base.bind_vma(i915_gem_obj_to_ggtt(batch_obj),
> > >                              batch_obj->cache_level,
> > > 			     GLOBAL_BIND);
> > > 
> > > #define i915_vm_bind(vm__, vma__, cache_level__, flags__) \
> > >  (vm__)->bind_vma((vma__), (cache_level__), (flags__))
> > > 
> > > i915_vm_bind(&dev_priv->ggtt.base,
> > >              i915_gem_obj_to_ggtt(batch_obj),
> > > 	     batch_obj->cache_level,
> > > 	     GLOBAL_BIND);
> > > -Chris
> > > 
> > 
> > I915_DISPATCH_SECURE is a special case. If we see the flag, we look up
> > the GGTT offset as opposed to the offset in the VM being used at
> > execbuf. We can either bind the batchbuffer into both the PPGTT, and
> > GGTT, or it's only even in the GGTT - in either case, we'll have to have
> > done the bind (and found space in the drm_mm). It just seems like this
> > is duplicating the already existing i915_gem_object_bind_to_vm code
> > that's in place.
> > 
> > Sorry if I am not following what you're asking. I'm just failing to see
> > a problem, or maybe you're just trying to solve problems that I haven't
> > yet conceived; or solved in a different way.  It's pretty darn hard to
> > discuss this given the piecemeal nature of the thing.
> 
> 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

-- 
Ben Widawsky, Intel Open Source Technology Center