[Intel-gfx] [RFC 09/10] drm/i915/vm_bind: Skip vma_lookup for persistent vmas

[Niranjana Vishwanathapura]

On Tue, Jul 05, 2022 at 10:57:17AM +0200, Thomas Hellström wrote:
>On Fri, 2022-07-01 at 15:50 -0700, Niranjana Vishwanathapura wrote:
>> vma_lookup is tied to segment of the object instead of section
>> 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.
>>
>> Signed-off-by: Niranjana Vishwanathapura
>> <niranjana.vishwanathapura at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_vma.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_vma.c
>> b/drivers/gpu/drm/i915/i915_vma.c
>> index 6adb013579be..9aa38b772b5b 100644
>> --- a/drivers/gpu/drm/i915/i915_vma.c
>> +++ b/drivers/gpu/drm/i915/i915_vma.c
>> @@ -197,6 +197,10 @@ vma_create(struct drm_i915_gem_object *obj,
>>                 __set_bit(I915_VMA_GGTT_BIT, __i915_vma_flags(vma));
>>         }
>>  
>> +       if (!i915_vma_is_ggtt(vma) &&
>> +           (view && view->type == I915_GGTT_VIEW_PARTIAL))
>> +               goto skip_rb_insert;
>> +
>
>Rather than guessing that a vma with this signature is a persistent
>vma, which is confusing to the reader, could we have an argument saying
>we want to create a persistent vma?

Yah, sounds good. We probably can even check for vm->vm_bind_mode here
instead of passing a new argument. I think i915 won't create any
internal vmas for this VM, so, should be good to check vm->vm_bind_mode.

>
>>         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
>> @@ -292,13 +297,16 @@ i915_vma_instance(struct drm_i915_gem_object
>> *obj,
>>                   struct i915_address_space *vm,
>>                   const struct i915_ggtt_view *view)
>>  {
>> -       struct i915_vma *vma;
>> +       struct i915_vma *vma = NULL;
>>  
>>         GEM_BUG_ON(!kref_read(&vm->ref));
>>  
>> -       spin_lock(&obj->vma.lock);
>> -       vma = i915_vma_lookup(obj, vm, view);
>> -       spin_unlock(&obj->vma.lock);
>> +       if (i915_is_ggtt(vm) || !view ||
>> +           view->type != I915_GGTT_VIEW_PARTIAL) {
>
>Same here?

We probably can remove this code and have vm_bind ioctl directly
call vma_create.

Niranjana

>
>/Thomas
>
>
>> +               spin_lock(&obj->vma.lock);
>> +               vma = i915_vma_lookup(obj, vm, view);
>> +               spin_unlock(&obj->vma.lock);
>> +       }
>>  
>>         /* vma_create() will resolve the race if another creates the
>> vma */
>>         if (unlikely(!vma))
>> @@ -1670,7 +1678,8 @@ static void release_references(struct i915_vma
>> *vma, bool vm_ddestroy)
>>  
>>         spin_lock(&obj->vma.lock);
>>         list_del(&vma->obj_link);
>> -       if (!RB_EMPTY_NODE(&vma->obj_node))
>> +       if (!i915_vma_is_persistent(vma) &&
>> +           !RB_EMPTY_NODE(&vma->obj_node))
>>                 rb_erase(&vma->obj_node, &obj->vma.tree);
>>  
>>         spin_unlock(&obj->vma.lock);
>