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

From: Daniel Vetter <daniel@ffwll.ch>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 07/11] drm/i915: Fix up map and fenceable for VMA
Date: Wed, 10 Jul 2013 19:08:53 +0200	[thread overview]
Message-ID: <20130710170853.GF6143@phenom.ffwll.local> (raw)
In-Reply-To: <20130710163900.GA1221@bwidawsk.net>

On Wed, Jul 10, 2013 at 09:39:00AM -0700, Ben Widawsky wrote:
> 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?

Yeah, I'm ok with that, since the above hunk that started my thinking will
go away anyway.

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

If you want to keep it, please add a comment explaining why and how
exactly it will get fixed up. In case of doubt just create a new patch to
highlight the special case imo.
-Daniel

> 
> 
> > > @@ -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

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