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

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

[Prike Liang]

Lock and refer to the eviction fence before trying to signal it.

Signed-off-by: Prike Liang <Prike.Liang@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);
+	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);
 }
 
-- 
2.34.1

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

[Christian König]

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@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.

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);
>  }
>  

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

[Liang, Prike]

[Public]

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Thursday, May 8, 2025 9:56 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@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@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);
> >  }
> >

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

[Christian König]

On 5/9/25 08:31, Liang, Prike wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Thursday, May 8, 2025 9:56 PM
>> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@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@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;

No what I mean was this just the other way around:

ev_fence = evf_mgr->ev_fence;
if (ev_fence)
	dma_fence_get(&ev_fence->base);

E.g. try to avoid the return value of dma_fence_get(). That is only meant to be used in macros or when we pass the value to functions who take ownership of the reference.

Regards,
Christian.


> +       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);
>>>  }
>>>
>