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