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

["Christian König" <christian.koenig@amd.com>]

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