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

-- 
Ben Widawsky, Intel Open Source Technology Center