Re: [Intel-gfx] [RFC v4 13/14] drm/i915/vm_bind: Skip vma_lookup for persistent vmas

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

On 26/09/2022 18:09, Niranjana Vishwanathapura wrote:
> On Mon, Sep 26, 2022 at 05:26:12PM +0100, Tvrtko Ursulin wrote:
>>
>> On 24/09/2022 05:30, Niranjana Vishwanathapura wrote:
>>> On Fri, Sep 23, 2022 at 09:40:20AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 21/09/2022 08:09, Niranjana Vishwanathapura wrote:
>>>>> vma_lookup is tied to segment of the object instead of section
>>>>
>>>> Can be, but not only that. It would be more accurate to say it is 
>>>> based of gtt views.
>>>
>>> Yah, but new code is also based on gtt views, the only difference
>>> is that now there can be multiple mappings (at different VAs)
>>> to the same gtt_view of the object.
>>>
>>>>
>>>>> of VA space. Hence, it do not support aliasing (ie., multiple
>>>>> bindings to the same section of the object).
>>>>> Skip vma_lookup for persistent vmas as it supports aliasing.
>>>>
>>>> What's broken without this patch? If something is, should it go 
>>>> somewhere earlier in the series? If so should be mentioned in the 
>>>> commit message.
>>>>
>>>> Or is it just a performance optimisation to skip unused tracking? If 
>>>> so should also be mentioned in the commit message.
>>>>
>>>
>>> No, it is not a performance optimization.
>>> The vma_lookup is based on the fact that there can be only one mapping
>>> for a given gtt_view of the object.
>>> So, it was looking for gtt_view to find the mapping.
>>>
>>> But now, as I mentioned above, there can be multiple mappings for a
>>> given gtt_view of the object. Hence the vma_lookup method won't work
>>> here. Hence, it is being skipped for persistent vmas.
>>
>> Right, so in that case isn't this patch too late in the series? 
>> Granted you only allow _userspace_ to use vm bind in 14/14, but the 
>> kernel infrastructure is there and if there was a selftest it would be 
>> able to fail without this patch, no?
>>
> 
> Yes it is incorrect patch ordering. I am fixing it by moving this patch
> to early in the series and adding a new i915_vma_create_persistent()
> function and avoid touching i915_vma_instance() everywhere (as you
> suggested).
> 
> <snip>
> 
>>>>> --- a/drivers/gpu/drm/i915/i915_vma.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_vma.c
>>>>> @@ -110,7 +110,8 @@ static void __i915_vma_retire(struct 
>>>>> i915_active *ref)
>>>>>  static struct i915_vma *
>>>>>  vma_create(struct drm_i915_gem_object *obj,
>>>>>         struct i915_address_space *vm,
>>>>> -       const struct i915_gtt_view *view)
>>>>> +       const struct i915_gtt_view *view,
>>>>> +       bool persistent)
>>>>>  {
>>>>>      struct i915_vma *pos = ERR_PTR(-E2BIG);
>>>>>      struct i915_vma *vma;
>>>>> @@ -197,6 +198,9 @@ vma_create(struct drm_i915_gem_object *obj,
>>>>>          __set_bit(I915_VMA_GGTT_BIT, __i915_vma_flags(vma));
>>>>>      }
>>>>> +    if (persistent)
>>>>> +        goto skip_rb_insert;
>>>>
>>>> Oh so you don't use the gtt_view's fully at all. I now have 
>>>> reservations whether that was the right approach. Since you are not 
>>>> using the existing rb tree tracking I mean..
>>>>
>>>> You know if a vma is persistent right? So you could have just added 
>>>> special case for persistent vmas to __i915_vma_get_pages and still 
>>>> call intel_partial_pages from there. Maybe union over struct 
>>>> i915_gtt_view in i915_vma for either the view or struct 
>>>> intel_partial_info for persistent ones.
>>>>
>>>
>>> We are using the gtt_view fully in this patch for persistent vmas.
>>
>> I guess yours and mine definition of fully are different. :)
>>
>>> But as mentioned above, now we have support multiple mappings
>>> for the same gtt_view of the object. For this, the current
>>> vma_lookup() falls short. So, we are skipping it.
>>
>> I get it - but then, having only now noticed how it will be used, I am 
>> less convinced touching the ggtt_view code was the right approach.
>>
>> What about what I proposed above? That you just add code to 
>> __i915_vma_get_pages, which in case of a persistent VMA would call 
>> intel_partial_pages from there.
>>
>> If that works I think it's cleaner and we'd just revert the ggtt_view 
>> to gtt_view rename.
>>
> 
> I don't think that is any cleaner. We need to store the partial view
> information somewhere for the persistent vmas as well. Why not use
> the existing gtt_view for that instead of a new data structure?
> In fact long back I had such an implementation and it was looking
> odd and was suggested to use the existing infrastructure (gtt_view).
> 
> Besides, I think the current i915_vma_lookup method is no longer valid.
> (Ever since we had softpinning, lookup should have be based on the VA
> and not the vma's view of the object).

As a side note I don't think soft pinning was a problem. That did not establish a partial VMA concept, nor had any interaction view ggtt_views. It was still one obj - one vma per vm relationship.

But okay, it is okay to do it like this. I think when you change to separate create/lookup entry points for persistent it will become much cleaner. I do acknowledge you have to "hide" them from normal lookup to avoid confusing the legacy code paths.

One more note - I think patch 6 should be before or together with patch 4. In general infrastructure to handle vm bind should all be in place before code starts using it.

Regards,

Tvrtko