On Tue, Jul 2, 2024 at 5:12 AM Zack Rusin <zack.rusin@xxxxxxxxxxxx> wrote:
>
> Introduce a version of the fence ops that on release doesn't remove
> the fence from the pending list, and thus doesn't require a lock to
> fix poll->fence wait->fence unref deadlocks.
>
> vmwgfx overwrites the wait callback to iterate over the list of all
> fences and update their status, to do that it holds a lock to prevent
> the list modifcations from other threads. The fence destroy callback
> both deletes the fence and removes it from the list of pending
> fences, for which it holds a lock.
>
> dma buf polling cb unrefs a fence after it's been signaled: so the poll
> calls the wait, which signals the fences, which are being destroyed.
> The destruction tries to acquire the lock on the pending fences list
> which it can never get because it's held by the wait from which it
> was called.
>
> Old bug, but not a lot of userspace apps were using dma-buf polling
> interfaces. Fix those, in particular this fixes KDE stalls/deadlock.
>
> Signed-off-by: Zack Rusin <zack.rusin@xxxxxxxxxxxx>
> Fixes: 2298e804e96e ("drm/vmwgfx: rework to new fence interface, v2")
> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@xxxxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: <stable@xxxxxxxxxxxxxxx> # v6.2+
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> index 5efc6a766f64..588d50ababf6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> @@ -32,7 +32,6 @@
> #define VMW_FENCE_WRAP (1 << 31)
>
> struct vmw_fence_manager {
> - int num_fence_objects;
> struct vmw_private *dev_priv;
> spinlock_t lock;
> struct list_head fence_list;
> @@ -124,13 +123,13 @@ static void vmw_fence_obj_destroy(struct dma_fence *f)
> {
> struct vmw_fence_obj *fence =
> container_of(f, struct vmw_fence_obj, base);
> -
> struct vmw_fence_manager *fman = fman_from_fence(fence);
>
> - spin_lock(&fman->lock);
> - list_del_init(&fence->head);
> - --fman->num_fence_objects;
> - spin_unlock(&fman->lock);
> + if (!list_empty(&fence->head)) {
> + spin_lock(&fman->lock);
> + list_del_init(&fence->head);
> + spin_unlock(&fman->lock);
> + }
> fence->destroy(fence);
> }
>
> @@ -257,7 +256,6 @@ static const struct dma_fence_ops vmw_fence_ops = {
> .release = vmw_fence_obj_destroy,
> };
>
> -
> /*
> * Execute signal actions on fences recently signaled.
> * This is done from a workqueue so we don't have to execute
> @@ -355,7 +353,6 @@ static int vmw_fence_obj_init(struct vmw_fence_manager *fman,
> goto out_unlock;
> }
> list_add_tail(&fence->head, &fman->fence_list);
> - ++fman->num_fence_objects;
>
> out_unlock:
> spin_unlock(&fman->lock);
> @@ -403,7 +400,7 @@ static bool vmw_fence_goal_new_locked(struct vmw_fence_manager *fman,
> u32 passed_seqno)
> {
> u32 goal_seqno;
> - struct vmw_fence_obj *fence;
> + struct vmw_fence_obj *fence, *next_fence;
>
> if (likely(!fman->seqno_valid))
> return false;
> @@ -413,7 +410,7 @@ static bool vmw_fence_goal_new_locked(struct vmw_fence_manager *fman,
> return false;
>
> fman->seqno_valid = false;
> - list_for_each_entry(fence, &fman->fence_list, head) {
> + list_for_each_entry_safe(fence, next_fence, &fman->fence_list, head) {
> if (!list_empty(&fence->seq_passed_actions)) {
> fman->seqno_valid = true;
> vmw_fence_goal_write(fman->dev_priv,
> --
> 2.43.0
>
LGTM
Reviewed-by: Martin Krastev <martin.krastev@xxxxxxxxxxxx>
Regards,
Martin
