On 10/13/26 10:21, Khatri, Sunil wrote:
>
> On 13-10-2026 01:42 pm, Christian König wrote:
>> On 10/13/26 09:56, Sunil Khatri wrote:
>>> MES userqueues bypass amdgpu_vm_flush(), so a reservation granted to a
>>> userqueue never got its VMID's page table programmed. Add
>>> amdgpu_vm_spm_update(), called from amdgpu_vm_validate(), to bind the
>>> fixed SPM VMID's page-table-base to the reserving VM's root PD when
>>> that VM holds the reservation; it is a no-op for every other VM.
>>> amdgpu_vmid_free_reserved() unbinds it directly to a dummy address
>>> under the same lock that clears the reservation, so stray SPM accesses
>>> fault instead of racing a new reservation's binding.
>>>
>>> Signed-off-by: Sunil Khatri <[email protected]>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 6 ++++
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37 +++++++++++++++++++++++++
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 +
>>> 3 files changed, 44 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
>>> index 992f7cb3a3102..0ad94b09c6b46 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
>>> @@ -581,6 +581,12 @@ void amdgpu_vmid_free_reserved(struct amdgpu_device
>>> *adev, struct amdgpu_vm *vm,
>>> if (vm->reserved_vmid[vmhub]) {
>>> vm->reserved_vmid[vmhub] = NULL;
>>> id_mgr->reserved_vmid = false;
>>> +
>>> + if (adev->vm_manager.spm_vmid && adev->gfxhub.funcs) {
>>> + adev->gfxhub.funcs->setup_vm_pt_regs(adev,
>>> adev->vm_manager.spm_vmid, 0);
>>> + amdgpu_gmc_flush_gpu_tlb(adev,
>>> adev->vm_manager.spm_vmid,
>>> + AMDGPU_GFXHUB(0),
>>> TLB_FLUSH_LEGACY);
>>> + }
>>> }
>>> mutex_unlock(&id_mgr->lock);
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index de992405ed718..3967dadb8d425 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -568,6 +568,41 @@ uint64_t amdgpu_vm_generation(struct amdgpu_device
>>> *adev, struct amdgpu_vm *vm)
>>> return result;
>>> }
>>>
>>> +/**
>>> + * amdgpu_vm_spm_update - bind the SPM VMID's page table base to @vm
>>> + *
>>> + * @adev: amdgpu device pointer
>>> + * @vm: vm to check against the SPM reservation
>>> + *
>>> + * The SPM VMID is fixed (adev->vm_manager.spm_vmid), so its
>>> page-table-base
>>> + * register must track whichever VM currently holds the reservation instead
>>> + * of being set per-job like a normal VMID. Bind it to @vm's real root PD
>>> + * address if @vm holds the reservation; otherwise do nothing, since @vm is
>>> + * not the one that could legitimately change the current binding.
>>> + */
>>> +void amdgpu_vm_spm_update(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>>> +{
>>> + struct amdgpu_kq_vmid_mgr *id_mgr =
>>> + &adev->vm_manager.id_mgr[AMDGPU_GFXHUB(0)];
>>> + uint64_t pd_addr;
>>> +
>>> + if (!adev->vm_manager.spm_vmid || !adev->gfxhub.funcs)
>>> + return;
>>> +
>>> + mutex_lock(&id_mgr->lock);
>>> + if (vm->reserved_vmid[AMDGPU_GFXHUB(0)] !=
>>> + &id_mgr->ids[adev->vm_manager.spm_vmid] || !vm->root.bo) {
>>> + mutex_unlock(&id_mgr->lock);
>>> + return;
>>> + }
>>> +
>>> + pd_addr = amdgpu_gmc_pd_addr(vm->root.bo);
>>> + adev->gfxhub.funcs->setup_vm_pt_regs(adev, adev->vm_manager.spm_vmid,
>>> pd_addr);
>> That function seriously needs a wrapper in amdgpu_gmc.c.
> You mean to move this function totally in the amdgpu_gmc.c or let this be
> here and create a wrapper in gmc layet calling this function?
Create a wrapper for this in the gmc layer and call that here.
>>> + amdgpu_gmc_flush_gpu_tlb(adev, adev->vm_manager.spm_vmid,
>>> AMDGPU_GFXHUB(0),
>>> + TLB_FLUSH_LEGACY);
>>> + mutex_unlock(&id_mgr->lock);
>>> +}
>>> +
>>> /**
>>> * amdgpu_vm_validate - validate evicted BOs tracked in the VM
>>> *
>>> @@ -630,6 +665,8 @@ int amdgpu_vm_validate(struct amdgpu_device *adev,
>>> struct amdgpu_vm *vm,
>>> amdgpu_vm_bo_needs_update(bo_base);
>>> }
>>>
>>> + amdgpu_vm_spm_update(adev, vm);
>>> +
>> Wrong place for that, this needs to be in the caller of
>> amdgpu_vmid_alloc_reserved() and in
>> amdgpu_userq_vm_validate_and_restore_queue().
> If i understood it correctly, amdgpu_vm_ioctl -> amdgpu_vmid_alloc_reserved
> after this call returns when we should call amdgpu_vm_spm_update?
Yes.
> For the userqueues,
>
> amdgpu_userq_vm_validate_and_restore_queue { ... ret =
> amdgpu_vm_update_pdes(adev, vm, false); if (ret) goto unlock_all;
> is it safe to add after this point ? amdgpu_vm_spm_update(adev, vm);
> }
I think that should be before amdgpu_evf_mgr_rearm().
Regards,
Christian.
>
>> And while at it please rename amdgpu_vmid_alloc_reserved() and
>> amdgpu_vmid_free_reserved() into amdgpu_vmid_alloc_spm() and
>> amdgpu_vmid_free_spm().
>
> Noted
>
> Regards
> Sunil Khatri
>
>> Regards,
>> Christian.
>>
>>> if (!ticket)
>>> return 0;
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index b8aa45db1d844..2a2a2ccbaa2cc 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -443,6 +443,7 @@ int amdgpu_vm_lock_individual(struct amdgpu_vm *vm,
>>> struct drm_exec *exec,
>>> unsigned int num_fences);
>>> bool amdgpu_vm_ready(struct amdgpu_vm *vm);
>>> uint64_t amdgpu_vm_generation(struct amdgpu_device *adev, struct amdgpu_vm
>>> *vm);
>>> +void amdgpu_vm_spm_update(struct amdgpu_device *adev, struct amdgpu_vm
>>> *vm);
>>> int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>> struct ww_acquire_ctx *ticket,
>>> int (*callback)(void *p, struct amdgpu_bo *bo),
