[Intel-gfx] [PATCH] drm/i915: Infrastructure for supporting different GGTT views per object

[Daniel Vetter]

On Mon, Dec 01, 2014 at 04:39:36PM +0000, Tvrtko Ursulin wrote:
> 
> On 12/01/2014 04:01 PM, Daniel Vetter wrote:
> >On Mon, Dec 01, 2014 at 02:46:29PM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 12/01/2014 11:32 AM, Tvrtko Ursulin wrote:
> >>>>>@@ -5430,9 +5434,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct
> >>>>>drm_i915_gem_object *obj)
> >>>>>  {
> >>>>>      struct i915_vma *vma;
> >>>>>
> >>>>>-    vma = list_first_entry(&obj->vma_list, typeof(*vma), vma_link);
> >>>>>-    if (vma->vm != i915_obj_to_ggtt(obj))
> >>>>>-        return NULL;
> >>>>>+    list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >>>>>+        if (vma->vm != i915_obj_to_ggtt(obj))
> >>>>>+            continue;
> >>>>>+        if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> >>>>>+            return vma;
> >>>>>+    }
> >>>>
> >>>>We fairly put the ggtt vma into the head of the list. Imo better to keep
> >>>>the head slot reserved for the ggtt normal view, might be some random
> >>>>code
> >>>>relying upon this.
> >>>
> >>>Ok.
> >>
> >>Although on a second thought - I am not sure this makes sense since
> >>alternative views can exist without the normal one. Thoughts?
> >
> >Yeah, hence we need to put the normal ggtt view at the front and
> >everything else at the back. I'm just somewhat afraid of something
> >expecting the normal ggtt view to be the first one and which then
> >accidentally breaks.
> 
> No, my point was that we can't guarantee that since the first entry will be
> an alternative view when it is the only item on the list. So in the light of
> that does it make sense to bother with this special casing at all?
> 
> >But if you think this is too much fuzz then please split this change out
> >into a separate patch (i.e. the change to the lookup loop + no longer
> >inserting ggtt vmas a the front). That way when any regression bisects to
> >this patch it's clear what's going on.
> 
> To double check if I understand what you mean, you lost me with "fuzz":

Hm I guess my understanding of fuzz isn't quite in line with common usage.
I've meant "hairy work". Oh well ...

> If you agree with my comment above, then the recommendation is for another
> prep patch to do what you say here? Ie. remove the special case for GGTT VMA
> list_add ?

Yes.

> i915_gem_obj_ti_ggtt can remain untouched in that prep patch, in my view,
> since it will be the only GGTT VMA, no?

If you don't insert the ggtt view any more at the front the current
obj_to_ggtt won't work any more. So you must change that in the same patch
(otherwise you break bisectability, which is the entire point of the
split-out).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch