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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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@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?

>         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?

/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);