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

[Niranjana Vishwanathapura]

On Tue, Sep 27, 2022 at 10:28:03AM +0100, Tvrtko Ursulin wrote:
>
>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.
>

Thanks. Yah, separating it out is looking lot cleaner. Yah, I have further split the patches including patch 6 and move part it to before patch 4.
Everything is looking much cleaner now. Will be posting updated series soon.

Regards,
Niranjana

>Regards,
>
>Tvrtko