Re: [PATCH 6/9] drm/i915: Wrap VMA binding

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, May 07, 2014 at 08:54:56AM -0700, Ben Widawsky wrote:
> On Wed, May 07, 2014 at 09:55:49AM +0200, Daniel Vetter wrote:
> > On Tue, May 06, 2014 at 10:21:35PM -0700, Ben Widawsky wrote:
> > > This will be useful for some upcoming patches which do more platform
> > > specific work. Having it in one central place just makes things a bit
> > > cleaner and easier.
> > > 
> > > There is a small functional change here. There are more calls to the
> > > tracepoints.
> > > 
> > > NOTE: I didn't actually end up using this patch for the intended purpose, but I
> > > thought it was a nice patch to keep around.
> > > 
> > > v2: s/i915_gem_bind_vma/i915_gem_vma_bind/
> > > s/i915_gem_unbind_vma/i915_gem_vma_unbind/
> > > (Chris)
> > > 
> > > v3: Missed one spot
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > You change the semantics of the vma bind/unbind tracepoints - previously
> > they've meant we reserve/unreserve a ppgtt address for an object, not
> > they're called every time we touch the ptes (which is what
> > vma_bind|unbind actually do). That doesn't look too terribly useful for
> > tracing any more.
> > 
> > What's the use case for these added tracepoints?
> > -Daniel
> > 
> 
> You're right. The tracepoint is incredibly useful for debug purposes,
> but not as an actual tracepoint. The current tracepoint is badly named,
> IMO, but that's fine. I am not sure it's worth fixing though, since
> nobody has really spoken up for the patch.

Yeah I think a new tracepoint we use every time we frob ptes could be
useful indeed. But conflating that operation with the vm operation isn't
good, especially since the vm operation lacks crucial information like the
actual caching bits we're using with the ptes.
-Daniel
> 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h            |  3 +++
> > >  drivers/gpu/drm/i915/i915_gem.c            | 13 ++++++-------
> > >  drivers/gpu/drm/i915/i915_gem_context.c    |  2 +-
> > >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 +++--
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c        | 16 ++++++++++++++--
> > >  5 files changed, 27 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index c1d2bea..30cde3d 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2264,6 +2264,9 @@ bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> > >  			struct i915_address_space *vm);
> > >  unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> > >  				struct i915_address_space *vm);
> > > +void i915_gem_vma_bind(struct i915_vma *vma, enum i915_cache_level,
> > > +		       unsigned flags);
> > > +void i915_gem_vma_unbind(struct i915_vma *vma);
> > >  struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > >  				     struct i915_address_space *vm);
> > >  struct i915_vma *
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index e9599e9..1567911 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -2771,7 +2771,7 @@ int i915_vma_unbind(struct i915_vma *vma)
> > >  
> > >  	trace_i915_vma_unbind(vma);
> > >  
> > > -	vma->unbind_vma(vma);
> > > +	i915_gem_vma_unbind(vma);
> > >  
> > >  	i915_gem_gtt_finish_object(obj);
> > >  
> > > @@ -3342,9 +3342,8 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> > >  
> > >  	WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
> > >  
> > > -	trace_i915_vma_bind(vma, flags);
> > > -	vma->bind_vma(vma, obj->cache_level,
> > > -		      flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
> > > +	i915_gem_vma_bind(vma, obj->cache_level,
> > > +			  flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
> > >  
> > >  	i915_gem_verify_gtt(dev);
> > >  	return vma;
> > > @@ -3548,8 +3547,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > >  
> > >  		list_for_each_entry(vma, &obj->vma_list, vma_link)
> > >  			if (drm_mm_node_allocated(&vma->node))
> > > -				vma->bind_vma(vma, cache_level,
> > > -					      obj->has_global_gtt_mapping ? GLOBAL_BIND : 0);
> > > +				i915_gem_vma_bind(vma, cache_level,
> > > +						  obj->has_global_gtt_mapping ? GLOBAL_BIND : 0);
> > >  	}
> > >  
> > >  	list_for_each_entry(vma, &obj->vma_list, vma_link)
> > > @@ -3914,7 +3913,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> > >  	}
> > >  
> > >  	if (flags & PIN_GLOBAL && !obj->has_global_gtt_mapping)
> > > -		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
> > > +		i915_gem_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
> > >  
> > >  	vma->pin_count++;
> > >  	if (flags & PIN_MAPPABLE)
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > > index f77b4c1..00c7d88 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > @@ -655,7 +655,7 @@ static int do_switch(struct intel_ring_buffer *ring,
> > >  	if (!to->obj->has_global_gtt_mapping) {
> > >  		struct i915_vma *vma = i915_gem_obj_to_vma(to->obj,
> > >  							   &dev_priv->gtt.base);
> > > -		vma->bind_vma(vma, to->obj->cache_level, GLOBAL_BIND);
> > > +		i915_gem_vma_bind(vma, to->obj->cache_level, GLOBAL_BIND);
> > >  	}
> > >  
> > >  	if (!to->is_initialized || i915_gem_context_is_default(to))
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > index 6cc004f..211bbbd 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -369,7 +369,8 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
> > >  		struct i915_vma *vma =
> > >  			list_first_entry(&target_i915_obj->vma_list,
> > >  					 typeof(*vma), vma_link);
> > > -		vma->bind_vma(vma, target_i915_obj->cache_level, GLOBAL_BIND);
> > > +		i915_gem_vma_bind(vma, target_i915_obj->cache_level,
> > > +				  GLOBAL_BIND);
> > >  	}
> > >  
> > >  	/* Validate that the target is in a valid r/w GPU domain */
> > > @@ -1266,7 +1267,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > >  		 * allocate space first */
> > >  		struct i915_vma *vma = i915_gem_obj_to_ggtt(batch_obj);
> > >  		BUG_ON(!vma);
> > > -		vma->bind_vma(vma, batch_obj->cache_level, GLOBAL_BIND);
> > > +		i915_gem_vma_bind(vma, batch_obj->cache_level, GLOBAL_BIND);
> > >  	}
> > >  
> > >  	if (flags & I915_DISPATCH_SECURE)
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index 79ae83b..a66cb6a 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -1319,10 +1319,9 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
> > >  		 * without telling our object about it. So we need to fake it.
> > >  		 */
> > >  		obj->has_global_gtt_mapping = 0;
> > > -		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
> > > +		i915_gem_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
> > >  	}
> > >  
> > > -
> > >  	if (INTEL_INFO(dev)->gen >= 8) {
> > >  		gen8_setup_private_ppat(dev_priv);
> > >  		return;
> > > @@ -1986,6 +1985,19 @@ int i915_gem_gtt_init(struct drm_device *dev)
> > >  	return 0;
> > >  }
> > >  
> > > +void i915_gem_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> > > +		       unsigned flags)
> > > +{
> > > +	trace_i915_vma_bind(vma, flags);
> > > +	vma->bind_vma(vma, cache_level, flags);
> > > +}
> > > +
> > > +void i915_gem_vma_unbind(struct i915_vma *vma)
> > > +{
> > > +	trace_i915_vma_unbind(vma);
> > > +	vma->unbind_vma(vma);
> > > +}
> > > +
> > >  static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> > >  					      struct i915_address_space *vm)
> > >  {
> > > -- 
> > > 1.9.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch