Re: [PATCH 2/9] drm/amdgpu: fix cleared PDE/PTE flag generation

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

On 9/30/26 16:44, Kuehling, Felix wrote:
> [+Mukul]
> 
> On 2026-09-30 05:02, Christian König wrote:
>> @Felix and @Philip any objections to this patch?
>>
>> It is actually a bug fix for the NPA support.
> 
> As I understand it, this fixes the flags used when unmapping memory from the NPA VM. We passed adev->gmc.noretry_flags from amdgpu_ualink_unmap_npa_addr to amdgpu_vm_update_range explicitly in the flags parameter. I guess that's no longer needed with your fix. But I think the end result would be the same, right?

Not quite, passing adev->gmc.noretry_flags to amdgpu_vm_update_range() is completely broken as well and also need fixing.

The flags parameter to amdgpu_vm_update_range() can't contain the AMDGPU_PTE_TF nor the AMDGPU_PDE_PTE flag because those are overwritten by the PTE callbacks.

That's why setting the AMDGPU_PTE_IS_PTE for gfx12 through the init_pte_flags is completely broken as well.

We seriously need to stop doing such hacks.

Regards,
Christian.

> 
> Regards,
>   Felix
> 
> 
>>
>> Regards,
>> Christian.
>>
>> On 9/28/26 21:08, Timur Kristóf wrote:
>>> On 2026. szeptember 28., hétfő 11:10:34 keleti államokbeli nyári idő Christian
>>> König wrote:
>>>> That was broken since adding the NPA support.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
>>>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 64 ++++++++++++-----------
>>>>   1 file changed, 33 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c index
>>>> c03327f1242d3..e8f441e018839 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>>>> @@ -346,6 +346,33 @@ static void amdgpu_vm_pt_next_dfs(struct amdgpu_device
>>>> *adev, amdgpu_vm_pt_continue_dfs((start), (entry));           
>>> \
>>>>            (entry) = (cursor).entry, amdgpu_vm_pt_next_dfs((adev),
>>> &(cursor)))
>>>> +/* Return the flags used for cleared PDEs/PTES */
>>>> +static uint64_t amdgpu_vm_pt_clear_flags(struct amdgpu_device *adev,
>>>> +                    struct amdgpu_vm *vm,
>>>> +                    unsigned int level)
>>>> +{
>>>> +    uint64_t flags;
>>>> +
>>>> +    if (adev->asic_type < CHIP_VEGA10)
>>>> +        return 0;
>>>> +
>>>> +    if (level != AMDGPU_VM_PTB) {
>>>> +        uint64_t value = 0;
>>>> +
>>>> +        flags = AMDGPU_PDE_PTE_FLAG(adev);
>>>> +        if (vm->is_npa)
>>>> +            flags |= adev->gmc.noretry_flags;
>>>> +        amdgpu_gmc_get_vm_pde(adev, level, &value, &flags);
>>>> +    } else if (vm->is_npa) {
>>>> +        flags = adev->gmc.noretry_flags;
>>>> +    } else {
>>>> +        /* Workaround for fault priority problem on GMC9 */
>>>> +        flags = AMDGPU_PTE_EXECUTABLE | adev->gmc.init_pte_flags;
>>>> +    }
>>>> +
>>>> +    return flags;
>>>> +}
>>>> +
>>>>   /**
>>>>    * amdgpu_vm_pt_clear - initially clear the PDs/PTs
>>>>    *
>>>> @@ -366,10 +393,9 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev,
>>>> struct amdgpu_vm *vm, struct ttm_operation_ctx ctx = { true, false };
>>>>       struct amdgpu_vm_update_params params;
>>>>       struct amdgpu_bo *ancestor = &vmbo->bo;
>>>> -    unsigned int entries;
>>>>       struct amdgpu_bo *bo = &vmbo->bo;
>>>> -    uint64_t value = 0, flags = 0;
>>>> -    uint64_t addr;
>>>> +    unsigned int entries;
>>>> +    uint64_t flags;
>>>>       int r, idx;
>>>>
>>>>       /* Figure out our place in the hierarchy */
>>>> @@ -404,26 +430,8 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev,
>>>> struct amdgpu_vm *vm, if (r)
>>>>           goto exit;
>>>>
>>>> -    addr = 0;
>>>> -
>>>> -    if (adev->asic_type >= CHIP_VEGA10) {
>>>> -        if (level != AMDGPU_VM_PTB) {
>>>> -            if (vm->is_npa)
>>>> -                flags = adev->gmc.noretry_flags;
>>>> -            /* Handle leaf PDEs as PTEs */
>>>> -            flags |= AMDGPU_PDE_PTE_FLAG(adev);
>>>> -            amdgpu_gmc_get_vm_pde(adev, level,
>>>> -                          &value, &flags);
>>>> -        } else if (vm->is_npa) {
>>>> -            flags = adev->gmc.noretry_flags;
>>>> -        } else {
>>>> -            /* Workaround for fault priority problem on
>>> GMC9 */
>>>> -            flags = AMDGPU_PTE_EXECUTABLE | adev-
>>>> gmc.init_pte_flags;
>>>> -        }
>>>> -    }
>>>> -
>>>> -    r = vm->update_funcs->update(&params, vmbo, addr, 0, entries,
>>>> -                     value, flags);
>>>> +    flags = amdgpu_vm_pt_clear_flags(adev, vm, level);
>>>> +    r = vm->update_funcs->update(&params, vmbo, 0, 0, entries, 0,
>>> flags);
>>>>       if (r)
>>>>           goto exit;
>>>>
>>>> @@ -712,15 +720,9 @@ static void amdgpu_vm_pte_update_flags(struct
>>>> amdgpu_vm_update_params *params, flags |=
>>>> AMDGPU_PDE_PTE_FLAG(params->adev);
>>>>           amdgpu_gmc_get_vm_pde(adev, level, &addr, &flags);
>>>>
>>>> -    } else if (adev->asic_type >= CHIP_VEGA10 &&
>>>> -           !(flags & AMDGPU_PTE_VALID) &&
>>>> +    } else if (!(flags & AMDGPU_PTE_VALID) &&
>>>>              !(flags & AMDGPU_PTE_PRT_FLAG(params->adev))) {
>>>> -
>>>> -        /* Workaround for fault priority problem on GMC9 and
>>> GFX12,
>>>> -         * EXECUTABLE for GMC9 fault priority and init_pte_flags
>>>> -         * (e.g. AMDGPU_PTE_IS_PTE on GFX12)
>>>> -         */
>>>> -        flags |= AMDGPU_PTE_EXECUTABLE | adev-
>>>> gmc.init_pte_flags;
>>>> +        flags |= amdgpu_vm_pt_clear_flags(adev, params->vm,
>>> level);
>>>>       }
>>>>
>>>>       /*
>>>
>>>
>>>