This is v4 of https://lore.kernel.org/lkml/[email protected]/T/
The goal of this patch series is fixing a deadlock upon locking the dma reservation
of a DRM gem object when pinning it, at a prime import operation.
Changelog:
v3:
- Split driver fixes into separate commits for Panfrost and Lima
- Make drivers call drm_gem_shmem_pin_locked instead of drm_gem_shmem_object_pin
- Improved commit message for first patch to explain why dma resv locking in the
pin callback is no longer necessary.
v2:
- Removed comment explaining reason why an already-locked
pin function replaced the locked variant inside Panfrost's
object pin callback.
- Moved already-assigned attachment warning into generic
already-locked gem object pin function
Adrián Larumbe (3):
drm/panfrost: Fix dma_resv deadlock at drm object pin time
drm/lima: Fix dma_resv deadlock at drm object pin time
drm/gem-shmem: Add import attachment warning to locked pin function
drivers/gpu/drm/drm_gem_shmem_helper.c | 2 ++
drivers/gpu/drm/lima/lima_gem.c | 2 +-
drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +-
3 files changed, 4 insertions(+), 2 deletions(-)
base-commit: 7acacca1b157fcb258cfd781603425f73bc7370b
--
2.45.1
Commit a78027847226 ("drm/gem: Acquire reservation lock in
drm_gem_{pin/unpin}()") moved locking the DRM object's dma reservation to
drm_gem_pin(), but Lima's pin callback kept calling drm_gem_shmem_pin,
which also tries to lock the same dma_resv, leading to a double lock
situation.
As was already done for Panfrost in the previous commit, fix it by
replacing drm_gem_shmem_pin() with its locked variant.
Cc: Thomas Zimmermann <[email protected]>
Cc: Dmitry Osipenko <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Steven Price <[email protected]>
Fixes: a78027847226 ("drm/gem: Acquire reservation lock in drm_gem_{pin/unpin}()")
Signed-off-by: Adrián Larumbe <[email protected]>
---
drivers/gpu/drm/lima/lima_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
index 7ea244d876ca..9bb997dbb4b9 100644
--- a/drivers/gpu/drm/lima/lima_gem.c
+++ b/drivers/gpu/drm/lima/lima_gem.c
@@ -185,7 +185,7 @@ static int lima_gem_pin(struct drm_gem_object *obj)
if (bo->heap_size)
return -EINVAL;
- return drm_gem_shmem_pin(&bo->base);
+ return drm_gem_shmem_pin_locked(&bo->base);
}
static int lima_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map)
--
2.45.1
Commit ec144244a43f ("drm/gem-shmem: Acquire reservation lock in GEM
pin/unpin callbacks") moved locking DRM object's dma reservation to
drm_gem_shmem_object_pin, and made drm_gem_shmem_pin_locked public, so we
need to make sure the non-NULL check warning is also added to the latter.
Cc: Thomas Zimmermann <[email protected]>
Cc: Dmitry Osipenko <[email protected]>
Cc: Boris Brezillon <[email protected]>
Fixes: a78027847226 ("drm/gem: Acquire reservation lock in drm_gem_{pin/unpin}()")
Signed-off-by: Adrián Larumbe <[email protected]>
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 177773bcdbfd..ad5d9f704e15 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -233,6 +233,8 @@ int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem)
dma_resv_assert_held(shmem->base.resv);
+ drm_WARN_ON(shmem->base.dev, shmem->base.import_attach);
+
ret = drm_gem_shmem_get_pages(shmem);
return ret;
--
2.45.1
When Panfrost must pin an object that is being prepared a dma-buf
attachment for on behalf of another driver, the core drm gem object pinning
code already takes a lock on the object's dma reservation.
However, Panfrost GEM object's pinning callback would eventually try taking
the lock on the same dma reservation when delegating pinning of the object
onto the shmem subsystem, which led to a deadlock.
This can be shown by enabling CONFIG_DEBUG_WW_MUTEX_SLOWPATH, which throws
the following recursive locking situation:
weston/3440 is trying to acquire lock:
ffff000000e235a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_gem_shmem_pin+0x34/0xb8 [drm_shmem_helper]
but task is already holding lock:
ffff000000e235a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_gem_pin+0x2c/0x80 [drm]
Fix it by replacing drm_gem_shmem_pin with its locked version, as the lock
had already been taken by drm_gem_pin().
Cc: Thomas Zimmermann <[email protected]>
Cc: Dmitry Osipenko <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Steven Price <[email protected]>
Fixes: a78027847226 ("drm/gem: Acquire reservation lock in drm_gem_{pin/unpin}()")
Signed-off-by: Adrián Larumbe <[email protected]>
---
drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index d47b40b82b0b..8e0ff3efede7 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -192,7 +192,7 @@ static int panfrost_gem_pin(struct drm_gem_object *obj)
if (bo->is_heap)
return -EINVAL;
- return drm_gem_shmem_pin(&bo->base);
+ return drm_gem_shmem_pin_locked(&bo->base);
}
static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj)
--
2.45.1
On Thu, 23 May 2024 12:32:17 +0100
Adrián Larumbe <[email protected]> wrote:
> When Panfrost must pin an object that is being prepared a dma-buf
> attachment for on behalf of another driver, the core drm gem object pinning
> code already takes a lock on the object's dma reservation.
>
> However, Panfrost GEM object's pinning callback would eventually try taking
> the lock on the same dma reservation when delegating pinning of the object
> onto the shmem subsystem, which led to a deadlock.
>
> This can be shown by enabling CONFIG_DEBUG_WW_MUTEX_SLOWPATH, which throws
> the following recursive locking situation:
>
> weston/3440 is trying to acquire lock:
> ffff000000e235a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_gem_shmem_pin+0x34/0xb8 [drm_shmem_helper]
> but task is already holding lock:
> ffff000000e235a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_gem_pin+0x2c/0x80 [drm]
>
> Fix it by replacing drm_gem_shmem_pin with its locked version, as the lock
> had already been taken by drm_gem_pin().
>
> Cc: Thomas Zimmermann <[email protected]>
> Cc: Dmitry Osipenko <[email protected]>
> Cc: Boris Brezillon <[email protected]>
> Cc: Steven Price <[email protected]>
> Fixes: a78027847226 ("drm/gem: Acquire reservation lock in drm_gem_{pin/unpin}()")
> Signed-off-by: Adrián Larumbe <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
> ---
> drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index d47b40b82b0b..8e0ff3efede7 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -192,7 +192,7 @@ static int panfrost_gem_pin(struct drm_gem_object *obj)
> if (bo->is_heap)
> return -EINVAL;
>
> - return drm_gem_shmem_pin(&bo->base);
> + return drm_gem_shmem_pin_locked(&bo->base);
> }
>
> static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj)
On Thu, 23 May 2024 12:32:18 +0100
Adrián Larumbe <[email protected]> wrote:
> Commit a78027847226 ("drm/gem: Acquire reservation lock in
> drm_gem_{pin/unpin}()") moved locking the DRM object's dma reservation to
> drm_gem_pin(), but Lima's pin callback kept calling drm_gem_shmem_pin,
> which also tries to lock the same dma_resv, leading to a double lock
> situation.
>
> As was already done for Panfrost in the previous commit, fix it by
> replacing drm_gem_shmem_pin() with its locked variant.
>
> Cc: Thomas Zimmermann <[email protected]>
> Cc: Dmitry Osipenko <[email protected]>
> Cc: Boris Brezillon <[email protected]>
> Cc: Steven Price <[email protected]>
> Fixes: a78027847226 ("drm/gem: Acquire reservation lock in drm_gem_{pin/unpin}()")
> Signed-off-by: Adrián Larumbe <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
> ---
> drivers/gpu/drm/lima/lima_gem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
> index 7ea244d876ca..9bb997dbb4b9 100644
> --- a/drivers/gpu/drm/lima/lima_gem.c
> +++ b/drivers/gpu/drm/lima/lima_gem.c
> @@ -185,7 +185,7 @@ static int lima_gem_pin(struct drm_gem_object *obj)
> if (bo->heap_size)
> return -EINVAL;
>
> - return drm_gem_shmem_pin(&bo->base);
> + return drm_gem_shmem_pin_locked(&bo->base);
> }
>
> static int lima_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map)
On Thu, 23 May 2024 12:32:19 +0100
Adrián Larumbe <[email protected]> wrote:
> Commit ec144244a43f ("drm/gem-shmem: Acquire reservation lock in GEM
> pin/unpin callbacks") moved locking DRM object's dma reservation to
> drm_gem_shmem_object_pin, and made drm_gem_shmem_pin_locked public, so we
> need to make sure the non-NULL check warning is also added to the latter.
>
> Cc: Thomas Zimmermann <[email protected]>
> Cc: Dmitry Osipenko <[email protected]>
> Cc: Boris Brezillon <[email protected]>
> Fixes: a78027847226 ("drm/gem: Acquire reservation lock in drm_gem_{pin/unpin}()")
> Signed-off-by: Adrián Larumbe <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 177773bcdbfd..ad5d9f704e15 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -233,6 +233,8 @@ int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem)
>
> dma_resv_assert_held(shmem->base.resv);
>
> + drm_WARN_ON(shmem->base.dev, shmem->base.import_attach);
> +
> ret = drm_gem_shmem_get_pages(shmem);
>
> return ret;
On Thu, May 23 2024 at 12:32:18 +01:00:00, Adri?n Larumbe
<[email protected]> wrote:
> Commit a78027847226 ("drm/gem: Acquire reservation lock in
> drm_gem_{pin/unpin}()") moved locking the DRM object's dma
> reservation to
> drm_gem_pin(), but Lima's pin callback kept calling drm_gem_shmem_pin,
> which also tries to lock the same dma_resv, leading to a double lock
> situation.
>
> As was already done for Panfrost in the previous commit, fix it by
> replacing drm_gem_shmem_pin() with its locked variant.
Hi, just found this while dealing with compositor lockups upon
launching a GL client on an old Rockchip RK3066 tablet, and it did fix
the problem :) Thank you.
Tested-by: Val Packett <[email protected]>
On Thu, 23 May 2024 12:32:16 +0100
Adrián Larumbe <[email protected]> wrote:
> This is v4 of https://lore.kernel.org/lkml/[email protected]/T/
>
> The goal of this patch series is fixing a deadlock upon locking the dma reservation
> of a DRM gem object when pinning it, at a prime import operation.
>
> Changelog:
> v3:
> - Split driver fixes into separate commits for Panfrost and Lima
> - Make drivers call drm_gem_shmem_pin_locked instead of drm_gem_shmem_object_pin
> - Improved commit message for first patch to explain why dma resv locking in the
> pin callback is no longer necessary.
> v2:
> - Removed comment explaining reason why an already-locked
> pin function replaced the locked variant inside Panfrost's
> object pin callback.
> - Moved already-assigned attachment warning into generic
> already-locked gem object pin function
>
>
> Adrián Larumbe (3):
> drm/panfrost: Fix dma_resv deadlock at drm object pin time
> drm/lima: Fix dma_resv deadlock at drm object pin time
> drm/gem-shmem: Add import attachment warning to locked pin function
Queued to drm-misc-fixes.
Thanks!
Boris