[PATCH] drm/amdgpu: lock the eviction fence before signaling it

[Liang, Prike]

[Public]

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig at amd.com>
> Sent: Thursday, May 8, 2025 9:56 PM
> To: Liang, Prike <Prike.Liang at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
> Subject: Re: [PATCH] drm/amdgpu: lock the eviction fence before signaling it
>
>
>
> On 5/8/25 15:44, Prike Liang wrote:
> > Lock and refer to the eviction fence before trying to signal it.
> >
> > Signed-off-by: Prike Liang <Prike.Liang at amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > index 1a7469543db5..dd272c1fcbb4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > @@ -108,13 +108,16 @@ amdgpu_eviction_fence_suspend_worker(struct
> work_struct *work)
> >     struct amdgpu_eviction_fence *ev_fence;
> >
> >     mutex_lock(&uq_mgr->userq_mutex);
> > -   ev_fence = evf_mgr->ev_fence;
> > +   spin_lock(&evf_mgr->ev_fence_lock);
> > +   ev_fence = (struct amdgpu_eviction_fence *)dma_fence_get(&evf_mgr-
> >ev_fence->base);
>
> That case is not a good approach, instead put the dma_fence_get on a separate
> line.
Thank for the suggestion, as such change can benefit on the readability?
Do you mean something like the following change?

    struct amdgpu_eviction_fence *ev_fence;
+       struct amdgpu_eviction_fence *ev_fence = NULL;
+       struct dma_fence *base_fence;

        mutex_lock(&uq_mgr->userq_mutex);
-       ev_fence = evf_mgr->ev_fence;
+       spin_lock(&evf_mgr->ev_fence_lock);
+       base_fence = dma_fence_get(&evf_mgr->ev_fence->base);
+       if (base_fence)
+               ev_fence = (struct amdgpu_eviction_fence *)base_fence;
+       spin_unlock(&evf_mgr->ev_fence_lock);
        if (!ev_fence)
                goto unlock;

        amdgpu_userq_evict(uq_mgr, ev_fence);

 unlock:
+       dma_fence_put(base_fence);

> Apart from that it looks good to me.
>
> Christian.
>
> > +   spin_unlock(&evf_mgr->ev_fence_lock);
> >     if (!ev_fence)
> >             goto unlock;
> >
> >     amdgpu_userq_evict(uq_mgr, ev_fence);
> >
> >  unlock:
> > +   dma_fence_put(&evf_mgr->ev_fence->base);
> >     mutex_unlock(&uq_mgr->userq_mutex);
> >  }
> >