Re: [PATCH 11/20] drm/amdgpu: fix KFD eviction fence enable_signaling path

["Kuehling, Felix" <felix.kuehling@amd.com>]

On 2025-11-06 12:09, Christian König wrote:
>
> On 11/6/25 18:07, Kuehling, Felix wrote:
>> On 2025-11-06 11:46, Christian König wrote:
>>> On 11/6/25 17:37, Kuehling, Felix wrote:
>>>> On 2025-11-06 08:43, Christian König wrote:
>>>>> On 11/4/25 17:28, Philipp Stanner wrote:
>>>>>> On Fri, 2025-10-31 at 14:16 +0100, Christian König wrote:
>>>>>>> Calling dma_fence_is_signaled() here is illegal!
>>>>>> The series was sent as a v2. But is this still an RFC?
>>>>> I think when Matthew came up with the XE patches we pretty much agreed that this is the way to go.
>>>>>
>>>>>> If not, more detailed commit messages are a desirable thing.
>>>>> Good point, how about:
>>>>>
>>>>> The enable_signaling callback is called with the same irqsave spinlock held than dma_fence_is_signaled() tries to grab. That will 100% reliable deadlock if that happens.
>>>> I guess we could use dma_fence_is_signaled_locked instead. That said, it only tries to take the lock (in dma_fence_signal) if fence->ops->signal is set, which isn't the case for these fences. That's why this has never caused a problem up till now.
>>> But when fence->ops->signal isn't set then why are we calling this?
>> There is no need to enable-signaling (and trigger a preemption), if the eviction fence has already signaled.
> But when the evicted fence has already been signaled then enable_signaling is not called in the first place:
>
>          if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>                  return false;
>
>          if (!was_set && fence->ops->enable_signaling) {
>                  trace_dma_fence_enable_signal(fence);
>
>                  if (!fence->ops->enable_signaling(fence)) {
> ...
>
> So the extra check is actually completely superfluous as far as I can see.

Thanks. I agree.

Regards,
   Felix


>
> Regards,
> Christian.
>
>> Regards,
>>    Felix
>>
>>
>>> Regards,
>>> Christian.
>>>
>>>> Regards,
>>>>     Felix
>>>>
>>>>
>>>>> Thanks,
>>>>> Christian.
>>>>>
>>>>>> P.
>>>>>>
>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 6 ------
>>>>>>>     1 file changed, 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
>>>>>>> index 1ef758ac5076..09c919f72b6c 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
>>>>>>> @@ -120,12 +120,6 @@ static bool amdkfd_fence_enable_signaling(struct
>>>>>>> dma_fence *f)
>>>>>>>     {
>>>>>>>         struct amdgpu_amdkfd_fence *fence =
>>>>>>> to_amdgpu_amdkfd_fence(f);
>>>>>>>     -    if (!fence)
>>>>>>> -        return false;
>>>>>>> -
>>>>>>> -    if (dma_fence_is_signaled(f))
>>>>>>> -        return true;
>>>>>>> -
>>>>>>>         if (!fence->svm_bo) {
>>>>>>>             if
>>>>>>> (!kgd2kfd_schedule_evict_and_restore_process(fence->mm, f))
>>>>>>>                 return true;
>