Re: [PATCH 09/13] drm/gem: Acquire reservation lock in drm_gem_{pin/unpin}()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 27, 2024 at 6:39 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Acquire the buffer object's reservation lock in drm_gem_pin() and
> remove locking the drivers' GEM callbacks where necessary. Same for
> unpin().
>
> DRM drivers and memory managers modified by this patch will now have
> correct dma-buf locking semantics: the caller is responsible for
> holding the reservation lock when calling the pin or unpin callback.
>
> DRM drivers and memory managers that are not modified will now be
> protected against concurent invocation of their pin and unpin callbacks.
>
> PRIME does not implement struct dma_buf_ops.pin, which requires
> the caller to hold the reservation lock. It does implement struct
> dma_buf_ops.attach, which requires to callee to acquire the
> reservation lock. The PRIME code uses drm_gem_pin(), so locks
> are now taken as specified. Same for unpin and detach.
>
> The patch harmonizes GEM pin and unpin to have non-interruptible
> reservation locking across all drivers, as is already the case for
> vmap and vunmap. This affects gem-shmem, gem-vram, loongson, qxl and
> radeon.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
>  drivers/gpu/drm/drm_gem.c               | 22 ++++++++++++++++++++--
>  drivers/gpu/drm/drm_gem_vram_helper.c   | 15 +--------------
>  drivers/gpu/drm/drm_internal.h          |  2 ++
>  drivers/gpu/drm/loongson/lsdc_gem.c     | 13 ++-----------
>  drivers/gpu/drm/msm/msm_gem_prime.c     |  4 ----
>  drivers/gpu/drm/nouveau/nouveau_prime.c | 11 -----------
>  drivers/gpu/drm/qxl/qxl_prime.c         | 14 +-------------
>  drivers/gpu/drm/radeon/radeon_prime.c   | 11 -----------
>  drivers/gpu/drm/vmwgfx/vmwgfx_gem.c     | 25 ++++++-------------------
>  include/drm/drm_gem_shmem_helper.h      | 11 +----------
>  10 files changed, 33 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 44a948b80ee14..e0f80c6a7096f 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1161,7 +1161,7 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
>                 obj->funcs->print_info(p, indent, obj);
>  }
>
> -int drm_gem_pin(struct drm_gem_object *obj)
> +int drm_gem_pin_locked(struct drm_gem_object *obj)
>  {
>         if (obj->funcs->pin)
>                 return obj->funcs->pin(obj);
> @@ -1169,12 +1169,30 @@ int drm_gem_pin(struct drm_gem_object *obj)
>         return 0;
>  }
>
> -void drm_gem_unpin(struct drm_gem_object *obj)
> +void drm_gem_unpin_locked(struct drm_gem_object *obj)
>  {
>         if (obj->funcs->unpin)
>                 obj->funcs->unpin(obj);
>  }
>
> +int drm_gem_pin(struct drm_gem_object *obj)
> +{
> +       int ret;
> +
> +       dma_resv_lock(obj->resv, NULL);
> +       ret = drm_gem_pin_locked(obj);
> +       dma_resv_unlock(obj->resv);
> +
> +       return ret;
> +}
> +
> +void drm_gem_unpin(struct drm_gem_object *obj)
> +{
> +       dma_resv_lock(obj->resv, NULL);
> +       drm_gem_unpin_locked(obj);
> +       dma_resv_unlock(obj->resv);
> +}
> +
>  int drm_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map)
>  {
>         int ret;
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index 15029d89badf8..5a16b3e0a4134 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -774,11 +774,6 @@ EXPORT_SYMBOL(drm_gem_vram_simple_display_pipe_cleanup_fb);
>  static int drm_gem_vram_object_pin(struct drm_gem_object *gem)
>  {
>         struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
> -       int ret;
> -
> -       ret = ttm_bo_reserve(&gbo->bo, true, false, NULL);
> -       if (ret)
> -               return ret;
>
>         /*
>          * Fbdev console emulation is the use case of these PRIME
> @@ -789,10 +784,7 @@ static int drm_gem_vram_object_pin(struct drm_gem_object *gem)
>          * the buffer to be pinned to VRAM, implement a callback that
>          * sets the flags accordingly.
>          */
> -       ret = drm_gem_vram_pin_locked(gbo, 0);
> -       ttm_bo_unreserve(&gbo->bo);
> -
> -       return ret;
> +       return drm_gem_vram_pin_locked(gbo, 0);
>  }
>
>  /**
> @@ -803,13 +795,8 @@ static int drm_gem_vram_object_pin(struct drm_gem_object *gem)
>  static void drm_gem_vram_object_unpin(struct drm_gem_object *gem)
>  {
>         struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
> -       int ret;
>
> -       ret = ttm_bo_reserve(&gbo->bo, true, false, NULL);
> -       if (ret)
> -               return;
>         drm_gem_vram_unpin_locked(gbo);
> -       ttm_bo_unreserve(&gbo->bo);
>  }
>
>  /**
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 8e4faf0a28e6c..40b2d3a274d6c 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -170,6 +170,8 @@ void drm_gem_release(struct drm_device *dev, struct drm_file *file_private);
>  void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
>                         const struct drm_gem_object *obj);
>
> +int drm_gem_pin_locked(struct drm_gem_object *obj);
> +void drm_gem_unpin_locked(struct drm_gem_object *obj);
>  int drm_gem_pin(struct drm_gem_object *obj);
>  void drm_gem_unpin(struct drm_gem_object *obj);
>  int drm_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map);
> diff --git a/drivers/gpu/drm/loongson/lsdc_gem.c b/drivers/gpu/drm/loongson/lsdc_gem.c
> index 04293df2f0de0..a720d8f532093 100644
> --- a/drivers/gpu/drm/loongson/lsdc_gem.c
> +++ b/drivers/gpu/drm/loongson/lsdc_gem.c
> @@ -19,33 +19,24 @@ static int lsdc_gem_prime_pin(struct drm_gem_object *obj)
>         struct lsdc_bo *lbo = gem_to_lsdc_bo(obj);
>         int ret;
>
> -       ret = lsdc_bo_reserve(lbo);
> -       if (unlikely(ret))
> -               return ret;
> +       dma_resv_assert_held(obj->resv);
>
>         ret = lsdc_bo_pin(lbo, LSDC_GEM_DOMAIN_GTT, NULL);
>         if (likely(ret == 0))
>                 lbo->sharing_count++;
>
> -       lsdc_bo_unreserve(lbo);
> -
>         return ret;
>  }
>
>  static void lsdc_gem_prime_unpin(struct drm_gem_object *obj)
>  {
>         struct lsdc_bo *lbo = gem_to_lsdc_bo(obj);
> -       int ret;
>
> -       ret = lsdc_bo_reserve(lbo);
> -       if (unlikely(ret))
> -               return;
> +       dma_resv_assert_held(obj->resv);
>
>         lsdc_bo_unpin(lbo);
>         if (lbo->sharing_count)
>                 lbo->sharing_count--;
> -
> -       lsdc_bo_unreserve(lbo);
>  }
>
>  static struct sg_table *lsdc_gem_prime_get_sg_table(struct drm_gem_object *obj)
> diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
> index 0d22df53ab98a..ee267490c9359 100644
> --- a/drivers/gpu/drm/msm/msm_gem_prime.c
> +++ b/drivers/gpu/drm/msm/msm_gem_prime.c
> @@ -53,11 +53,9 @@ int msm_gem_prime_pin(struct drm_gem_object *obj)
>         if (obj->import_attach)
>                 return 0;
>
> -       msm_gem_lock(obj);
>         pages = msm_gem_pin_pages_locked(obj);
>         if (IS_ERR(pages))
>                 ret = PTR_ERR(pages);
> -       msm_gem_unlock(obj);
>
>         return ret;
>  }
> @@ -67,7 +65,5 @@ void msm_gem_prime_unpin(struct drm_gem_object *obj)
>         if (obj->import_attach)
>                 return;
>
> -       msm_gem_lock(obj);
>         msm_gem_unpin_pages_locked(obj);
> -       msm_gem_unlock(obj);
>  }
> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c
> index 774f9bd031102..b58ab595faf82 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
> @@ -86,17 +86,12 @@ struct drm_gem_object *nouveau_gem_prime_import_sg_table(struct drm_device *dev,
>  int nouveau_gem_prime_pin(struct drm_gem_object *obj)
>  {
>         struct nouveau_bo *nvbo = nouveau_gem_object(obj);
> -       struct ttm_buffer_object *bo = &nvbo->bo;
>         int ret;
>
> -       ret = ttm_bo_reserve(bo, false, false, NULL);
> -       if (ret)
> -               return -EINVAL;
>         /* pin buffer into GTT */
>         ret = nouveau_bo_pin_locked(nvbo, NOUVEAU_GEM_DOMAIN_GART, false);
>         if (ret)
>                 ret = -EINVAL;
> -       ttm_bo_unreserve(bo);
>
>         return ret;
>  }
> @@ -104,14 +99,8 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj)
>  void nouveau_gem_prime_unpin(struct drm_gem_object *obj)
>  {
>         struct nouveau_bo *nvbo = nouveau_gem_object(obj);
> -       struct ttm_buffer_object *bo = &nvbo->bo;
> -       int ret;
>
> -       ret = ttm_bo_reserve(bo, false, false, NULL);
> -       if (ret)
> -               return;
>         nouveau_bo_unpin_locked(nvbo);
> -       ttm_bo_unreserve(bo);
>  }
>
>  struct dma_buf *nouveau_gem_prime_export(struct drm_gem_object *gobj,
> diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
> index f2646603e12eb..19bf551a7b311 100644
> --- a/drivers/gpu/drm/qxl/qxl_prime.c
> +++ b/drivers/gpu/drm/qxl/qxl_prime.c
> @@ -31,27 +31,15 @@
>  int qxl_gem_prime_pin(struct drm_gem_object *obj)
>  {
>         struct qxl_bo *bo = gem_to_qxl_bo(obj);
> -       int r;
>
> -       r = qxl_bo_reserve(bo);
> -       if (r)
> -               return r;
> -       r = qxl_bo_pin_locked(bo);
> -       qxl_bo_unreserve(bo);
> -
> -       return r;
> +       return qxl_bo_pin_locked(bo);
>  }
>
>  void qxl_gem_prime_unpin(struct drm_gem_object *obj)
>  {
>         struct qxl_bo *bo = gem_to_qxl_bo(obj);
> -       int r;
>
> -       r = qxl_bo_reserve(bo);
> -       if (r)
> -               return;
>         qxl_bo_unpin_locked(bo);
> -       qxl_bo_unreserve(bo);
>  }
>
>  struct sg_table *qxl_gem_prime_get_sg_table(struct drm_gem_object *obj)
> diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c
> index b3cfc99f4d7ed..a77881f035e7a 100644
> --- a/drivers/gpu/drm/radeon/radeon_prime.c
> +++ b/drivers/gpu/drm/radeon/radeon_prime.c
> @@ -73,32 +73,21 @@ int radeon_gem_prime_pin(struct drm_gem_object *obj)
>         struct radeon_bo *bo = gem_to_radeon_bo(obj);
>         int ret = 0;
>
> -       ret = radeon_bo_reserve(bo, false);
> -       if (unlikely(ret != 0))
> -               return ret;
> -
>         /* pin buffer into GTT */
>         ret = radeon_bo_pin(bo, RADEON_GEM_DOMAIN_GTT, NULL);
>         if (likely(ret == 0))
>                 bo->prime_shared_count++;
>
> -       radeon_bo_unreserve(bo);
>         return ret;
>  }
>
>  void radeon_gem_prime_unpin(struct drm_gem_object *obj)
>  {
>         struct radeon_bo *bo = gem_to_radeon_bo(obj);
> -       int ret = 0;
> -
> -       ret = radeon_bo_reserve(bo, false);
> -       if (unlikely(ret != 0))
> -               return;
>
>         radeon_bo_unpin(bo);
>         if (bo->prime_shared_count)
>                 bo->prime_shared_count--;
> -       radeon_bo_unreserve(bo);
>  }
>
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
> index 12787bb9c111d..186150f41fbcc 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
> @@ -48,33 +48,20 @@ static void vmw_gem_object_close(struct drm_gem_object *obj,
>  {
>  }
>
> -static int vmw_gem_pin_private(struct drm_gem_object *obj, bool do_pin)
> +static int vmw_gem_object_pin(struct drm_gem_object *obj)
>  {
> -       struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(obj);
>         struct vmw_bo *vbo = to_vmw_bo(obj);
> -       int ret;
> -
> -       ret = ttm_bo_reserve(bo, false, false, NULL);
> -       if (unlikely(ret != 0))
> -               goto err;
> -
> -       vmw_bo_pin_reserved(vbo, do_pin);
> -
> -       ttm_bo_unreserve(bo);
> -
> -err:
> -       return ret;
> -}
>
> +       vmw_bo_pin_reserved(vbo, true);
>
> -static int vmw_gem_object_pin(struct drm_gem_object *obj)
> -{
> -       return vmw_gem_pin_private(obj, true);
> +       return 0;
>  }
>
>  static void vmw_gem_object_unpin(struct drm_gem_object *obj)
>  {
> -       vmw_gem_pin_private(obj, false);
> +       struct vmw_bo *vbo = to_vmw_bo(obj);
> +
> +       vmw_bo_pin_reserved(vbo, false);
>  }
>
>  static struct sg_table *vmw_gem_object_get_sg_table(struct drm_gem_object *obj)
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index eb12aa9a8c556..efbc9f27312b5 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -175,15 +175,8 @@ static inline void drm_gem_shmem_object_print_info(struct drm_printer *p, unsign
>  static inline int drm_gem_shmem_object_pin(struct drm_gem_object *obj)
>  {
>         struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> -       int ret;
>
> -       ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
> -       if (ret)
> -               return ret;
> -       ret = drm_gem_shmem_pin_locked(shmem);
> -       dma_resv_unlock(shmem->base.resv);
> -
> -       return ret;
> +       return drm_gem_shmem_pin_locked(shmem);
>  }
>
>  /**
> @@ -197,9 +190,7 @@ static inline void drm_gem_shmem_object_unpin(struct drm_gem_object *obj)
>  {
>         struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>
> -       dma_resv_lock(shmem->base.resv, NULL);
>         drm_gem_shmem_unpin_locked(shmem);
> -       dma_resv_unlock(shmem->base.resv);
>  }
>
>

Ah, I see. Looks great.

Reviewed-by: Zack Rusin <zack.rusin@xxxxxxxxxxxx>





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux