AMD General
> -----Original Message-----
> From: amd-gfx <[email protected]> On Behalf Of Mukul
> Joshi
> Sent: Thursday, October 1, 2026 9:36 AM
> To: Kuehling, Felix <[email protected]>; Koenig, Christian
> <[email protected]>; Timur Kristóf <[email protected]>;
> [email protected]; Huang, Honglei1 <[email protected]>; Deucher,
> Alexander <[email protected]>; Yang, Philip
> <[email protected]>; [email protected]; [email protected]
> Cc: [email protected]
> Subject: Re: [PATCH 2/9] drm/amdgpu: fix cleared PDE/PTE flag generation
>
>
> On 9/30/2026 12:12 PM, Kuehling, Felix wrote:
> > 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?
> >
> yes we have been testing this and it works fine. Having said that, the patch
> looks
> good to me.
>
> However, I have a question on one of the change in the patch below.
>
>
> >
> >>
> >> 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 <[email protected]>
> >>>>> Reviewed-by: Timur Kristóf <[email protected]>
> >>>>>
> >>>>>> ---
> >>>>>> 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(¶ms, vmbo, addr, 0,
> >>>>>> entries,
> >>>>>> - value, flags);
> >>>>>> + flags = amdgpu_vm_pt_clear_flags(adev, vm, level);
> >>>>>> + r = vm->update_funcs->update(¶ms, vmbo, 0, 0, entries,
> >>>>>> +0,
> >>>>> flags);
>
> We were passing addr before to the update() call but with this change, we are
> passing 0 now for the addr argument now.
>
Sorry I meant to ask about the v"alue" args. Earlier, we would get the value
from amdgpu_gmc_get_vm_pde()
But now we are ignoring it and sending the value args as 0.
Why is that? Should we keep it the same as before?
Regards,
Mukul
> Why is that?
>
> Regards,
>
> Mukul
>
> >>>>>> 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);
> >>>>>> }
> >>>>>>
> >>>>>> /*
> >>>>>
> >>>>>
