On 11/6/25 18:25, Kuehling, Felix wrote:
>
> 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.
Can I take that as reviewed-by you for this patch?
Thanks,
Christian.
>
> Regards,
> Felix
>
>
>>
>> Regards,
>> Christian.
>>
>>> Regards,
>>> Felix
>>>
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Regards,
>>>>> Felix
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Christian.
>>>>>>
>>>>>>> P.
>>>>>>>
>>>>>>>> Signed-off-by: Christian König <[email protected]>
>>>>>>>> ---
>>>>>>>> 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;
>>
