On 12/01/2014 05:16 PM, Daniel Vetter wrote:
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 ...
Since patch(1) sorts out simple mismatches with "fuzz" to me it is a
synonym for trivial. :)
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).
Ah yes, I overlooked how the current obj_to_ggtt looks. Or to better say
I only looked at my version and assumed I only added the view type check.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
