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

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

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