[Intel-gfx] [PATCH 01/23] drm/i915: Drop vm.ref for duplicate vma on construction

[Chris Wilson]

Quoting Andi Shyti (2020-07-02 21:25:45)
> Hi Chris,
> 
> > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> > index 1f63c4a1f055..7fe1f317cd2b 100644
> > --- a/drivers/gpu/drm/i915/i915_vma.c
> > +++ b/drivers/gpu/drm/i915/i915_vma.c
> > @@ -198,6 +198,7 @@ vma_create(struct drm_i915_gem_object *obj,
> >               cmp = i915_vma_compare(pos, vm, view);
> >               if (cmp == 0) {
> >                       spin_unlock(&obj->vma.lock);
> > +                     i915_vm_put(vm);
> >                       i915_vma_free(vma);
> 
> You are forgettin one return without dereferencing it.
> 
> would this be a solution:
> 
> @@ -106,6 +106,7 @@ vma_create(struct drm_i915_gem_object *obj,
>  {
>         struct i915_vma *vma;
>         struct rb_node *rb, **p;
> +       struct i915_vma *pos = ERR_PTR(-E2BIG);
>  
>         /* The aliasing_ppgtt should never be used directly! */
>         GEM_BUG_ON(vm == &vm->gt->ggtt->alias->vm);
> @@ -185,7 +186,6 @@ vma_create(struct drm_i915_gem_object *obj,
>         rb = NULL;
>         p = &obj->vma.tree.rb_node;
>         while (*p) {
> -               struct i915_vma *pos;
>                 long cmp;
>  
>                 rb = *p;
> @@ -197,12 +197,8 @@ vma_create(struct drm_i915_gem_object *obj,
>                  * and dispose of ours.
>                  */
>                 cmp = i915_vma_compare(pos, vm, view);
> -               if (cmp == 0) {
> -                       spin_unlock(&obj->vma.lock);
> -                       i915_vm_put(vm);
> -                       i915_vma_free(vma);
> -                       return pos;
> -               }
> +               if (!cmp)
> +                       goto err_unlock;

Yeah, but you might as well do

if (cmp < 0)
 	p = right;
else if (cmp > 0)
 	p = left;
else
	goto err_unlock;
 
>                 if (cmp < 0)
>                         p = &rb->rb_right;
> @@ -230,8 +226,9 @@ vma_create(struct drm_i915_gem_object *obj,
>  err_unlock:
>         spin_unlock(&obj->vma.lock);
>  err_vma:
> +       i915_vm_put(vm);
>         i915_vma_free(vma);
> -       return ERR_PTR(-E2BIG);
> +       return pos;
>  }
> 
> Andi

Ta, going to send that as a patch?
-Chris