On 6/3/26 19:54, Kuehling, Felix wrote:
...
>> + r = vm->update_funcs->prepare(¶ms, sync,
>> + AMDGPU_KERNEL_JOB_ID_VM_UNMAP_RANGE);
>> + if (r)
>> + goto error_free;
>> +
>> + amdgpu_vm_update_leaves(¶ms, start, last, 0, flags);
>> +
>> + r = vm->update_funcs->commit(¶ms, fence);
>> + if (r)
>> + goto error_free;
>
> So this wraps amdgpu_vm_update_leaves with all the stuff that's necessary to
> actually execute the page table update.
Yes, exactly that.
> I don't understand how amdgpu_vm_update_leaves works without this when you
> call it directly from amdgpu_vm_handle_fault (in patch 6).
It does mostly the same, but not 100%.
> Shouldn't you use amdgpu_vm_unmap_range there instead?
No, the main difference is that calling amdgpu_vm_update_leaves() from the
fault handler needs to bypass the normal sequencial handling of VM updates.
Background is that this is a band aid for a dma_fence based submission to avoid
a full GPU lockup and instead just continue even when the rendering is
incorrect.
> And if that's true, then maybe you can use the name amdgpu_vm_update_leaves
> for this instead.
I'm certainly open for better naming.
Maybe we should call the function in amdgpu_vm_pt.c
amdgpu_vm_pt_update_leaves() and the higher level function in amdgpu_vm.c just
amdgpu_vm_update_leaves() ?
Thanks for the review,
Christian.
>
> Regards,
> Felix
>
>
>> +
>> + amdgpu_vm_tlb_flush(¶ms, fence, tlb_cb);
>> + amdgpu_vm_pt_free_list(adev, ¶ms);
>> + tlb_cb = NULL;
>> +
>> +error_free:
>> + kfree(tlb_cb);
>> + amdgpu_vm_eviction_unlock(vm);
>> + drm_dev_exit(idx);
>> + return r;
>> +}
>> +
>> void amdgpu_vm_get_memory(struct amdgpu_vm *vm,
>> struct amdgpu_mem_stats stats[__AMDGPU_PL_NUM])
>> {
>> @@ -1362,11 +1434,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>> struct amdgpu_bo_va *bo_va,
>> trace_amdgpu_vm_bo_update(mapping);
>> - r = amdgpu_vm_update_range(adev, vm, false, flush_tlb,
>> - !uncached, &sync, mapping->start,
>> - mapping->last, update_flags,
>> - mapping->offset, vram_base, mem,
>> - pages_addr, last_update);
>> + r = amdgpu_vm_map_range(adev, vm, flush_tlb, !uncached, &sync,
>> + mapping->start, mapping->last,
>> + update_flags, mapping->offset,
>> + vram_base, mem, pages_addr,
>> + last_update);
>> if (r)
>> goto error_free;
>> }
>> @@ -1565,9 +1637,9 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>> struct amdgpu_bo_va_mapping, list);
>> list_del(&mapping->list);
>> - r = amdgpu_vm_update_range(adev, vm, false, true, false,
>> - &sync, mapping->start, mapping->last,
>> - 0, 0, 0, NULL, NULL, &f);
>> + r = amdgpu_vm_map_range(adev, vm, true, false,
>> + &sync, mapping->start, mapping->last,
>> + 0, 0, 0, NULL, NULL, &f);
>> amdgpu_vm_free_mapping(adev, vm, mapping, f);
>> if (r) {
>> dma_fence_put(f);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 3e86a2a470f0..561f2873d2ec 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -529,12 +529,16 @@ int amdgpu_vm_flush_compute_tlb(struct amdgpu_device
>> *adev,
>> uint32_t xcc_mask);
>> void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
>> struct amdgpu_vm *vm, struct amdgpu_bo *bo);
>> -int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>> - bool unlocked, bool flush_tlb, bool allow_override,
>> +int amdgpu_vm_map_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>> + bool flush_tlb, bool allow_override,
>> + struct amdgpu_sync *sync, uint64_t start,
>> + uint64_t last, uint64_t flags, uint64_t offset,
>> + uint64_t vram_base, struct ttm_resource *res,
>> + dma_addr_t *pages_addr, struct dma_fence **fence);
>> +int amdgpu_vm_unmap_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>> struct amdgpu_sync *sync, uint64_t start,
>> - uint64_t last, uint64_t flags, uint64_t offset,
>> - uint64_t vram_base, struct ttm_resource *res,
>> - dma_addr_t *pages_addr, struct dma_fence **fence);
>> + uint64_t last, uint64_t flags,
>> + struct dma_fence **fence);
>> int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>> struct amdgpu_bo_va *bo_va,
>> bool clear);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>> index 6f5415d5a1bc..ac3f3e31e2e2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>> @@ -553,7 +553,6 @@ void amdgpu_vm_pt_free_list(struct amdgpu_device *adev,
>> struct amdgpu_vm_update_params *params)
>> {
>> struct amdgpu_vm_bo_base *entry, *next;
>> - bool unlocked = params->unlocked;
>> if (list_empty(¶ms->tlb_flush_waitlist))
>> return;
>> @@ -561,7 +560,7 @@ void amdgpu_vm_pt_free_list(struct amdgpu_device *adev,
>> /*
>> * unlocked unmap clear page table leaves, warning to free the page
>> entry.
>> */
>> - WARN_ON(unlocked);
>> + WARN_ON(params->unlocked);
>> list_for_each_entry_safe(entry, next, ¶ms->tlb_flush_waitlist,
>> vm_status)
>> amdgpu_vm_pt_free(entry);
>> @@ -801,24 +800,17 @@ int amdgpu_vm_ptes_update(struct
>> amdgpu_vm_update_params *params,
>> uint64_t incr, entry_end, pe_start;
>> struct amdgpu_bo *pt;
>> - if (!params->unlocked) {
>> - /* make sure that the page tables covering the
>> - * address range are actually allocated
>> - */
>> - r = amdgpu_vm_pt_alloc(params->adev, params->vm,
>> - &cursor);
>> - if (r)
>> - return r;
>> - }
>> + /* make sure that the page tables covering the
>> + * address range are actually allocated
>> + */
>> + r = amdgpu_vm_pt_alloc(params->adev, params->vm, &cursor);
>> + if (r)
>> + return r;
>> shift = amdgpu_vm_pt_level_shift(adev, cursor.level);
>> parent_shift = amdgpu_vm_pt_level_shift(adev, cursor.level - 1);
>> - if (params->unlocked) {
>> - /* Unlocked updates are only allowed on the leaves */
>> - if (amdgpu_vm_pt_descendant(adev, &cursor))
>> - continue;
>> - } else if (adev->asic_type < CHIP_VEGA10 &&
>> - (flags & AMDGPU_PTE_VALID)) {
>> + if (adev->asic_type < CHIP_VEGA10 &&
>> + (flags & AMDGPU_PTE_VALID)) {
>> /* No huge page support before GMC v9 */
>> if (cursor.level != AMDGPU_VM_PTB) {
>> if (!amdgpu_vm_pt_descendant(adev, &cursor))
>> @@ -864,14 +856,7 @@ int amdgpu_vm_ptes_update(struct
>> amdgpu_vm_update_params *params,
>> mask = amdgpu_vm_pt_entries_mask(adev, cursor.level);
>> pe_start = ((cursor.pfn >> shift) & mask) * 8;
>> - if (cursor.level < AMDGPU_VM_PTB && params->unlocked)
>> - /*
>> - * MMU notifier callback unlocked unmap huge page, leave is PDE
>> entry,
>> - * only clear one entry. Next entry search again for PDE or PTE
>> leave.
>> - */
>> - entry_end = 1ULL << shift;
>> - else
>> - entry_end = ((uint64_t)mask + 1) << shift;
>> + entry_end = ((uint64_t)mask + 1) << shift;
>> entry_end += cursor.pfn & ~(entry_end - 1);
>> entry_end = min(entry_end, end);
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> index 37b5166e9a14..d0ea20dea3e1 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> @@ -1372,9 +1372,8 @@ svm_range_unmap_from_gpu(struct amdgpu_device *adev,
>> struct amdgpu_vm *vm,
>> return -EINVAL;
>> }
>> - return amdgpu_vm_update_range(adev, vm, true, true, false, NULL,
>> gpu_start,
>> - gpu_end, init_pte_value, 0, 0, NULL, NULL,
>> - fence);
>> + return amdgpu_vm_unmap_range(adev, vm, NULL, gpu_start, gpu_end,
>> + init_pte_value, fence);
>> }
>> static int
>> @@ -1489,12 +1488,11 @@ svm_range_map_to_gpu(struct kfd_process_device *pdd,
>> struct svm_range *prange,
>> (last_domain == SVM_RANGE_VRAM_DOMAIN) ? 1 : 0,
>> pte_flags);
>> - r = amdgpu_vm_update_range(adev, vm, false, flush_tlb, true,
>> - NULL, gpu_start, gpu_end,
>> - pte_flags,
>> - (last_start - prange->start) << PAGE_SHIFT,
>> - bo_adev ? bo_adev->vm_manager.vram_base_offset : 0,
>> - NULL, dma_addr, &vm->last_update);
>> + r = amdgpu_vm_map_range(adev, vm, flush_tlb, true, NULL,
>> + gpu_start, gpu_end, pte_flags,
>> + (last_start - prange->start) << PAGE_SHIFT,
>> + bo_adev ? bo_adev->vm_manager.vram_base_offset : 0,
>> + NULL, dma_addr, &vm->last_update);
>> for (j = last_start - prange->start; j <= i; j++)
>> dma_addr[j] |= last_domain;
