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

[Niranjana Vishwanathapura]

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).

Regards,
Niranjana

>Regards,
>
>Tvrtko
>
>>
>>Regards,
>>Niranjana
>>
>>>Regards,
>>>
>>>Tvrtko
>>>
>>>>+
>>>>     rb = NULL;
>>>>     p = &obj->vma.tree.rb_node;
>>>>     while (*p) {
>>>>@@ -221,6 +225,7 @@ vma_create(struct drm_i915_gem_object *obj,
>>>>     rb_link_node(&vma->obj_node, rb, p);
>>>>     rb_insert_color(&vma->obj_node, &obj->vma.tree);
>>>>+skip_rb_insert:
>>>>     if (i915_vma_is_ggtt(vma))
>>>>         /*
>>>>          * We put the GGTT vma at the start of the vma-list, followed
>>>>@@ -279,6 +284,7 @@ i915_vma_lookup(struct drm_i915_gem_object *obj,
>>>>  * @obj: parent &struct drm_i915_gem_object to be mapped
>>>>  * @vm: address space in which the mapping is located
>>>>  * @view: additional mapping requirements
>>>>+ * @persistent: Whether the vma is persistent
>>>>  *
>>>>  * i915_vma_instance() looks up an existing VMA of the @obj in 
>>>>the @vm with
>>>>  * the same @view characteristics. If a match is not found, one 
>>>>is created.