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