Re: [PATCH 07/11] drm/i915: Fix up map and fenceable for VMA

[Ben Widawsky <ben@bwidawsk.net>]

On Tue, Jul 09, 2013 at 09:16:54AM +0200, Daniel Vetter wrote:
> On Mon, Jul 08, 2013 at 11:08:38PM -0700, Ben Widawsky wrote:
> > formerly: "drm/i915: Create VMAs (part 3.5) - map and fenceable
> > tracking"
> > 
> > The map_and_fenceable tracking is per object. GTT mapping, and fences
> > only apply to global GTT. As such,  object operations which are not
> > performed on the global GTT should not effect mappable or fenceable
> > characteristics.
> > 
> > Functionally, this commit could very well be squashed in to the previous
> > patch which updated object operations to take a VM argument.  This
> > commit is split out because it's a bit tricky (or at least it was for
> > me).
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 21015cd..501c590 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2635,7 +2635,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj,
> >  
> >  	trace_i915_gem_object_unbind(obj, vm);
> >  
> > -	if (obj->has_global_gtt_mapping)
> > +	if (obj->has_global_gtt_mapping && i915_is_ggtt(vm))
> >  		i915_gem_gtt_unbind_object(obj);
> 
> Wont this part be done as part of the global gtt clear_range callback?
> 
> >  	if (obj->has_aliasing_ppgtt_mapping) {
> >  		i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj);
> 
> And I have a hunch that we should shovel the aliasing ppgtt clearing into
> the ggtt write_ptes/clear_range callbacks, too. Once all this has settled
> at least.
> 

Addressing both comments at once:

First, this is a rebase mistake AFAICT because this hunk doesn't really
belong in this patch anyway.

Eventually, I'd want to kill i915_gem_gtt_unbind_object, and
i915_ppgtt_unbind_object. In the 66 patch series, I killed the latter,
but decided to leave the former to make it clear that is a special case.

In the original 66 patch series, I did not move clear_range which is
probably why this was left like this. I believe bind was fixed to just
be vm->bleh()

If you're good with the idea, I'll add a new patch to remove those and
use the i915_address_space. I'll do the same in other applicable places.
It's easiest if I do that as a patch 12, I think, if you don't mind?

I do think this hunk belongs in another patch though until I do the
above. I'm not really sure where to put that.


> > @@ -2646,7 +2646,8 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj,
> >  
> >  	list_del(&obj->mm_list);
> >  	/* Avoid an unnecessary call to unbind on rebind. */
> > -	obj->map_and_fenceable = true;
> > +	if (i915_is_ggtt(vm))
> > +		obj->map_and_fenceable = true;
> >  
> >  	vma = i915_gem_obj_to_vma(obj, vm);
> >  	list_del(&vma->vma_link);
> > @@ -3213,7 +3214,9 @@ search_free:
> >  		i915_is_ggtt(vm) &&
> >  		vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end;
> >  
> > -	obj->map_and_fenceable = mappable && fenceable;
> > +	/* Map and fenceable only changes if the VM is the global GGTT */
> > +	if (i915_is_ggtt(vm))
> > +		obj->map_and_fenceable = mappable && fenceable;
> >  
> >  	trace_i915_gem_object_bind(obj, vm, map_and_fenceable);
> >  	i915_gem_verify_gtt(dev);
> > -- 
> > 1.8.3.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