On 10/29/26 08:03, Jesse Zhang wrote:
> Commit 38e07c072833 ("drm/amdgpu: remove the now-redundant runtime SPM
> VMID update") dropped the RLC_SPM_VMID update on submission, as the SPM
> VMID is fixed and programmed once at init. RLC_SPM_MC_CNTL doesn't keep
> its value over GFXOFF though, the RLC brings it back as 0 when it powers
> GFX up again. So once the GPU idled, the job of a process which reserved
> the VMID for SPM runs with the SPM writing through VMID 0.
That sounds like a pretty clear bug in the RLC handling to me.
@Alex do you know where the save/restore register list can be found?
This here is not really an option since we want to completely nuke the
update_spm_vmid() function.
Regards,
Christian.
>
> On navi48 RLC_SPM_MC_CNTL reads 0xf right after hw_init, 0 once GFXOFF
> was entered, and stays 0xf with GFXOFF disabled. A COPY_DATA of the
> register from the IB of a process with a reserved VMID gives 0 instead of
> the reserved VMID 15:
>
> $ umr -i 0 -go 0; umr -i 0 -r '*.*.regRLC_SPM_MC_CNTL'
> gfx1201.regRLC_SPM_MC_CNTL => 0x00000000
> $ amd_spm --run-subtest reserved-vmid-spm-vmid
> RLC_SPM_MC_CNTL in the job: 0x00000000
>
> Update it on submission again, but only for the jobs on the reserved VMID
> and with the fixed SPM VMID. Unlike before that commit, a GPU reset must
> not force the update for every job: RLC_SPM_MC_CNTL is shared, and a job
> of another VM would retarget the SPM of the profiled process to its own
> VMID. The reserved path requests the update on every submission anyway.
> With that it's back to 15 inside the job.
>
> Fixes: 38e07c072833 ("drm/amdgpu: remove the now-redundant runtime SPM VMID
> update")
> Signed-off-by: Jesse Zhang <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 8 +++++++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_job.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 15 ++++++++++++++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 3 ++-
> 5 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index f98a7c6797b3..ec0883df91b1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -126,6 +126,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, struct
> amdgpu_job *job,
> struct amdgpu_fence *af;
> struct amdgpu_fence *vm_af;
> bool need_ctx_switch;
> + bool emit_spm_needed = false;
> bool emit_gds_needed = false;
> uint64_t fence_ctx;
> uint32_t status = 0, alloc_size;
> @@ -194,7 +195,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, struct
> amdgpu_job *job,
>
> /* VM sequence */
> vm_af->ib_wptr = ring->wptr;
> - amdgpu_vm_flush(ring, job, &need_pipe_sync, &emit_gds_needed);
> + amdgpu_vm_flush(ring, job, &need_pipe_sync, &emit_spm_needed,
> + &emit_gds_needed);
> vm_af->ib_dw_size =
> amdgpu_ring_get_dw_distance(ring, vm_af->ib_wptr, ring->wptr);
>
> @@ -209,6 +211,10 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, struct
> amdgpu_job *job,
> if (need_pipe_sync)
> amdgpu_ring_emit_pipeline_sync(ring);
>
> + if (emit_spm_needed)
> + adev->gfx.rlc.funcs->update_spm_vmid(adev, ring->xcc_id,
> + adev->vm_manager.spm_vmid);
> +
> if (emit_gds_needed)
> amdgpu_ring_emit_gds_switch(ring, job->vmid, job->gds_base,
> job->gds_size, job->gws_base,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 9c879ac68352..124fb875c651 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -434,6 +434,7 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm,
> return r;
>
> job->vm_needs_flush = needs_flush;
> + job->spm_update_needed = true;
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> index f6577f82e6b9..ae569b4497f6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> @@ -75,6 +75,7 @@ struct amdgpu_job {
> uint32_t preemption_status;
> bool vm_needs_flush;
> bool gds_switch_needed;
> + bool spm_update_needed;
> uint64_t vm_pd_addr;
> unsigned vmid;
> unsigned pasid;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 000c982816a7..7a8e363449a6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -809,12 +809,14 @@ bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring
> *ring,
> * @ring: ring to use for flush
> * @job: related job
> * @need_pipe_sync: is pipe sync needed
> + * @emit_spm_needed: does the caller need to emit spm
> * @emit_gds_needed: does the caller need to emit gds
> *
> * Emit a VM flush when it is necessary.
> */
> void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> - bool *need_pipe_sync, bool *emit_gds_needed)
> + bool *need_pipe_sync, bool *emit_spm_needed,
> + bool *emit_gds_needed)
> {
> struct amdgpu_device *adev = ring->adev;
> struct amdgpu_isolation *isolation =
> @@ -823,6 +825,8 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct
> amdgpu_job *job,
> unsigned vmhub = ring->vm_hub;
> struct amdgpu_kq_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
> struct amdgpu_vmid *id = &id_mgr->ids[job->vmid];
> + bool spm_update_needed = adev->gfx.rlc.funcs->update_spm_vmid &&
> + job->spm_update_needed;
> bool gds_switch_needed = ring->funcs->emit_gds_switch &&
> job->gds_switch_needed;
> bool vm_flush_needed = job->vm_needs_flush;
> @@ -845,6 +849,7 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct
> amdgpu_job *job,
> mutex_unlock(&id_mgr->lock);
>
> gds_switch_needed &= !!ring->funcs->emit_gds_switch;
> + spm_update_needed &= !!adev->gfx.rlc.funcs->update_spm_vmid;
> vm_flush_needed &= !!ring->funcs->emit_vm_flush &&
> job->vm_pd_addr != AMDGPU_BO_INVALID_OFFSET;
> pasid_mapping_needed &= adev->gmc.gmc_funcs->emit_pasid_mapping &&
> @@ -858,6 +863,10 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct
> amdgpu_job *job,
> emit_fence = vm_flush_needed || pasid_mapping_needed ||
> cleaner_shader_needed;
>
> + *emit_spm_needed = spm_update_needed;
> + if (spm_update_needed && emit_fence)
> + *emit_spm_needed = false;
> +
> *emit_gds_needed = gds_switch_needed;
> if (gds_switch_needed && emit_fence)
> *emit_gds_needed = false;
> @@ -897,6 +906,10 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct
> amdgpu_job *job,
> if (pasid_mapping_needed)
> amdgpu_gmc_emit_pasid_mapping(ring, job->vmid, job->pasid);
>
> + if (spm_update_needed)
> + adev->gfx.rlc.funcs->update_spm_vmid(adev, ring->xcc_id,
> + adev->vm_manager.spm_vmid);
> +
> if (gds_switch_needed)
> amdgpu_ring_emit_gds_switch(ring, job->vmid, job->gds_base,
> job->gds_size,
> job->gws_base,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 06e0ddd60c7b..973ad7576d9e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -449,7 +449,8 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct
> amdgpu_vm *vm,
> int (*callback)(void *p, struct amdgpu_bo *bo),
> void *param);
> void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> - bool *need_pipe_sync, bool *emit_gds_needed);
> + bool *need_pipe_sync, bool *emit_spm_needed,
> + bool *emit_gds_needed);
> int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
> struct amdgpu_vm *vm, bool immediate);
> int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
