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

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

On 2026-09-30 10:59, Christian König wrote:
> 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.

I'm pretty sure Mukul tested this and got something that worked as 
expected, at least for a time. I'm not sure if it regressed since, maybe 
during upstreaming. @Mukul, can you comment?


>
> 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.

Is there any documentation that tells us what flags can be used where. 
This isn't obvious at all.

Thanks,
   Felix


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