Re: [PATCH] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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":

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 ?

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

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx