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