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