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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
