On 9/23/26 16:18, Thadeu Lima de Souza Cascardo wrote:
...
>>
>
> What about the other 3 patches? If there is less controversy around them, I
> can submit them as a new patchset version.
Patch #1 is simply not the right direction we need, but see comments on patch
#4 as well.
Patch #2 is a good cleanup, that could potentially be applied immediately.
Patch #3 is what I'm already working on. We actually need to change the code so
that failures simply can't occur (except for device hot plug).
If any failure occurs here the only real doable recovery is to kill all ongoing
work and disconnect the device.
Patch #4 is an absolute no-go, committing partial page table update can cause
tons of problems.
Essentially we need to rework the code so that page table allocation happens
before we kick of any HW update.
I already discussed that with Natalie quite a while ago on our weekly meeting,
you are basically the third or forth person to stumble over that problem
already.
Regards,
Christian.
>
> Thanks.
> Cascardo.
>
>>>
>>>> Re-mapping the two fragments before and after the unmapped area is
>>>> supposed to allocate the new PDs.
>>>>
>>>> Without that we run into tons of problems with userptrs and MMU notifiers.
>>>>
>>>> I will CC you on the patches I have so far.
>>>>
>>>
>>> Please, do. I will be happy to test it and review it.
>>>
>>> Thanks.
>>> Cascardo.
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> The other reproducer that I have required that I used REPLACE instead of
>>>>> MAP in order to trigger the tlb_flush_waitlist issue. REPLACE does call
>>>>> amdgpu_vm_clear_mappings too. Notice that RADV Mesa driver uses REPLACE,
>>>>> instead of MAP, so that explains that we can see this in production.
>>>>>
>>>>> Regards.
>>>>> Cascardo.
>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>> Regards.
>>>>>>> Cascardo.
>>>>>>>
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Christian.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>>>>>>>>>> Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>>>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>>>> index 09037df76400..5bde36754607 100644
>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>>>> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct
>>>>>>>>>> amdgpu_device *adev,
>>>>>>>>>> while (!list_empty(&vm->freed)) {
>>>>>>>>>> mapping = list_first_entry(&vm->freed,
>>>>>>>>>> struct amdgpu_bo_va_mapping, list);
>>>>>>>>>> - list_del(&mapping->list);
>>>>>>>>>> r = amdgpu_vm_update_range(adev, vm, false, false,
>>>>>>>>>> true, false,
>>>>>>>>>> &sync, mapping->start,
>>>>>>>>>> mapping->last,
>>>>>>>>>> 0, 0, 0, NULL, NULL, &f);
>>>>>>>>>> - amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>>>>>>>> if (r) {
>>>>>>>>>> dma_fence_put(f);
>>>>>>>>>> goto error_free;
>>>>>>>>>> }
>>>>>>>>>> + list_del(&mapping->list);
>>>>>>>>>> + amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>>>>>>>> }
>>>>>>>>>> if (fence && f) {
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
