[PATCH] drm/i915: Force ww lock for i915_gem_object_ggtt_pin_ww, v2.

[Matthew Auld]

On Tue, 30 Nov 2021 at 09:21, Maarten Lankhorst
<maarten.lankhorst at linux.intel.com> wrote:
>
> We will need the lock to unbind the vma, and wait for bind to complete.
> Remove the special casing for the !ww path, and force ww locking for all.
>
> Changes since v1:
> - Pass err to for_i915_gem_ww handling for -EDEADLK handling.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  7 ++-----
>  drivers/gpu/drm/i915/i915_gem.c | 30 ++++++++++++++++++++++++++----
>  2 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1bfadd9127fc..8322abe194da 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1842,13 +1842,10 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
>                             const struct i915_ggtt_view *view,
>                             u64 size, u64 alignment, u64 flags);
>
> -static inline struct i915_vma * __must_check
> +struct i915_vma * __must_check
>  i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
>                          const struct i915_ggtt_view *view,
> -                        u64 size, u64 alignment, u64 flags)
> -{
> -       return i915_gem_object_ggtt_pin_ww(obj, NULL, view, size, alignment, flags);
> -}
> +                        u64 size, u64 alignment, u64 flags);
>
>  int i915_gem_object_unbind(struct drm_i915_gem_object *obj,
>                            unsigned long flags);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 527228d4da7e..6002045bd194 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -877,6 +877,8 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
>         struct i915_vma *vma;
>         int ret;
>
> +       GEM_WARN_ON(!ww);

Return something here, or GEM_BUG_ON()? I assume it's going to crash
and burn anyway?

> +
>         if (flags & PIN_MAPPABLE &&
>             (!view || view->type == I915_GGTT_VIEW_NORMAL)) {
>                 /*
> @@ -936,10 +938,7 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
>                         return ERR_PTR(ret);
>         }
>
> -       if (ww)
> -               ret = i915_vma_pin_ww(vma, ww, size, alignment, flags | PIN_GLOBAL);
> -       else
> -               ret = i915_vma_pin(vma, size, alignment, flags | PIN_GLOBAL);
> +       ret = i915_vma_pin_ww(vma, ww, size, alignment, flags | PIN_GLOBAL);
>
>         if (ret)
>                 return ERR_PTR(ret);
> @@ -959,6 +958,29 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
>         return vma;
>  }
>
> +struct i915_vma * __must_check
> +i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> +                        const struct i915_ggtt_view *view,
> +                        u64 size, u64 alignment, u64 flags)
> +{
> +       struct i915_gem_ww_ctx ww;
> +       struct i915_vma *ret;
> +       int err;
> +
> +       for_i915_gem_ww(&ww, err, true) {
> +               err = i915_gem_object_lock(obj, &ww);
> +               if (err)
> +                       continue;
> +
> +               ret = i915_gem_object_ggtt_pin_ww(obj, &ww, view, size,
> +                                                 alignment, flags);
> +               if (IS_ERR(ret))
> +                       err = PTR_ERR(ret);

Maybe s/ret/vma/ ? Seeing ret makes me think it's an integer.

Anyway,
Reviewed-by: Matthew Auld <matthew.auld at intel.com>

> +       }
> +
> +       return err ? ERR_PTR(err) : ret;
> +}
> +
>  int
>  i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
>                        struct drm_file *file_priv)
> --
> 2.34.1
>