[PATCH 1/3] drm/amdgpu: program the SPM VMID for jobs on the reserved VMID again

[PATCH 1/3] drm/amdgpu: program the SPM VMID for jobs on the reserved VMID again

[Jesse Zhang]

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.

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 <Jesse.Zhang@amd.com>
---
 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,
-- 
2.50.1

[PATCH 2/3] drm/amdgpu/gfx12: disallow GFXOFF while updating the SPM VMID

[Jesse Zhang]

gfx_v12_0_update_spm_vmid() does a read-modify-write of RLC_SPM_MC_CNTL
over MMIO without keeping GFX powered, unlike gfx11. When GFX is in
GFXOFF at that point the update doesn't stick, and with the SPM VMID
programmed on submission again, the job of a process with a reserved
VMID can still run with the SPM writing through VMID 0.

On navi48, after a few other submissions and a VM fault on the same VM,
a COPY_DATA of RLC_SPM_MC_CNTL from such a job reads 0 although the
update was called for it:

  $ run testcase: cs-secure-ib-compute,gem-mapping-info-high-va,reserved-vmid-spm-vmid
  amdgpu 0000:03:00.0: [gfxhub] page fault (src_id:0 ring:40 vmid:7 pasid:185)
  amdgpu 0000:03:00.0: 	 Faulty UTCL2 client ID: SDMA1 (0xe)
  RLC_SPM_MC_CNTL in the job: 0x00000000

With GFXOFF disallowed around the update it reads the reserved VMID 15.

Fixes: 52cb80c12e8a ("drm/amdgpu: Add gfx v12_0 ip block support (v6)")
Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
index d334fb805020..9bdd82cc202d 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
@@ -3963,6 +3963,7 @@ static void gfx_v12_0_update_spm_vmid(struct amdgpu_device *adev,
 {
 	u32 reg, data;
 
+	amdgpu_gfx_off_ctrl(adev, false);
 	reg = SOC15_REG_OFFSET(GC, 0, regRLC_SPM_MC_CNTL);
 	if (amdgpu_sriov_is_pp_one_vf(adev))
 		data = RREG32_NO_KIQ(reg);
@@ -3976,6 +3977,7 @@ static void gfx_v12_0_update_spm_vmid(struct amdgpu_device *adev,
 		WREG32_SOC15_NO_KIQ(GC, 0, regRLC_SPM_MC_CNTL, data);
 	else
 		WREG32_SOC15(GC, 0, regRLC_SPM_MC_CNTL, data);
+	amdgpu_gfx_off_ctrl(adev, true);
 }
 
 static const struct amdgpu_rlc_funcs gfx_v12_0_rlc_funcs = {
-- 
2.50.1

[PATCH 3/3] drm/amdgpu: program the SH_MEM registers of the SPM VMID

[Jesse Zhang]

Commit 02232367be23 ("drm/amdgpu: add spm_vmid and exclude it from the
MES, KFD and kq pools") took the SPM VMID out of the kernel queue and the
user queue VMID masks, and since commit 8a9263fc8812 ("drm/amdgpu: use
dedicated SPM VMID from the reserved path") the kernel queue jobs of a VM
with a reserved VMID run on it. The gfx constants_init() only programs
SH_MEM_CONFIG and SH_MEM_BASES for VMID 0 and the kernel queue mask
though, and init_compute_vmid() for the user queue mask, so the SPM VMID
keeps the reset defaults: no unaligned access mode and no apertures.

On navi48 VMID 15 has SH_MEM_CONFIG 0xc000 and SH_MEM_BASES 0 while the
other graphics VMIDs have 0xc00c and 0x20001000. A shader storing a dword
to an address with a byte offset of 1 writes it there in a normal
process, but at the dword aligned address in one which reserved a VMID,
e.g. for profiling with SPM:

  $ umr -i 0 -sb 0 0 0 15 -r '*.*.regSH_MEM_CONFIG'
  gfx1201.regSH_MEM_CONFIG => 0x0000c000
  $ amd_pmc --run-subtest reserved-vmid-unaligned-store
  unaligned store: 0x0000000000002a00 normally, 0x000000000000002a with a reserved VMID

Include the SPM VMID in for_each_vmid_and_zero() for the GFXHUB so the
constants_init() of all gfx generations program it like the other
kernel queue VMIDs.

Fixes: 8a9263fc8812 ("drm/amdgpu: use dedicated SPM VMID from the reserved path")
Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
index c134000eecfd..6d0f302c1344 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
@@ -45,14 +45,17 @@
  * Equivalent to OR-ing BIT(0) into id_mgr[hub].vmid_mask and walking
  * the result with for_each_set_bit(). Used by the gfx_v*_constants_init()
  * paths which must always touch VMID 0 (system) in addition to the regular
- * VMIDs owned by the hub.
+ * VMIDs owned by the hub. On the GFXHUB this includes the SPM VMID, which is
+ * kept out of the pool but runs the kernel queue jobs of the VM reserving it.
  *
  * Relies on AMDGPU_NUM_VMID (16) fitting in a single unsigned long, i.e.
  * vmid_mask[] being a 1-element bitmap.
  */
 #define for_each_vmid_and_zero(vmid, adev, hub)				\
 	for (unsigned long __vmid_mask =				\
-		(adev)->vm_manager.id_mgr[(hub)].vmid_mask[0] | BIT(0);	\
+		(adev)->vm_manager.id_mgr[(hub)].vmid_mask[0] | BIT(0) |	\
+		((hub) == AMDGPU_GFXHUB(0) && (adev)->vm_manager.spm_vmid ?	\
+		 BIT((adev)->vm_manager.spm_vmid) : 0);			\
 	     __vmid_mask; __vmid_mask = 0)				\
 		for_each_set_bit((vmid), &__vmid_mask, AMDGPU_NUM_VMID)
 
-- 
2.50.1

Re: [PATCH 1/3] drm/amdgpu: program the SPM VMID for jobs on the reserved VMID again

[Lazar, Lijo]

On 29-Oct-26 12:33 PM, 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.
> 
> 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 <Jesse.Zhang@amd.com>
> ---
>   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);

ring->xcc_id is not filled for all rings. Hence, if you want to collect 
some cache perf counters when say sdma (part of GFX domain) job is 
running, this doesn't work.

I had posted a patch earlier for this, but I thought the refactor 
already took care of this

drm/amdgpu: Derive RLC SPM xcc from GFXHUB

Thanks,
Lijo

> +
>   	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,

Re: [PATCH 2/3] drm/amdgpu/gfx12: disallow GFXOFF while updating the SPM VMID

[Alex Deucher]

On Thu, Oct 29, 2026 at 4:21 AM Jesse Zhang <Jesse.Zhang@amd.com> wrote:
>
> gfx_v12_0_update_spm_vmid() does a read-modify-write of RLC_SPM_MC_CNTL
> over MMIO without keeping GFX powered, unlike gfx11. When GFX is in
> GFXOFF at that point the update doesn't stick, and with the SPM VMID
> programmed on submission again, the job of a process with a reserved
> VMID can still run with the SPM writing through VMID 0.

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Additionally, since the default value for this register is 0 and we
only change the vmid, we could simplify these functions and just write
the value with the updated vmid and avoid the RMW.

Alex

>
> On navi48, after a few other submissions and a VM fault on the same VM,
> a COPY_DATA of RLC_SPM_MC_CNTL from such a job reads 0 although the
> update was called for it:
>
>   $ run testcase: cs-secure-ib-compute,gem-mapping-info-high-va,reserved-vmid-spm-vmid
>   amdgpu 0000:03:00.0: [gfxhub] page fault (src_id:0 ring:40 vmid:7 pasid:185)
>   amdgpu 0000:03:00.0:   Faulty UTCL2 client ID: SDMA1 (0xe)
>   RLC_SPM_MC_CNTL in the job: 0x00000000
>
> With GFXOFF disallowed around the update it reads the reserved VMID 15.
>
> Fixes: 52cb80c12e8a ("drm/amdgpu: Add gfx v12_0 ip block support (v6)")
> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> index d334fb805020..9bdd82cc202d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> @@ -3963,6 +3963,7 @@ static void gfx_v12_0_update_spm_vmid(struct amdgpu_device *adev,
>  {
>         u32 reg, data;
>
> +       amdgpu_gfx_off_ctrl(adev, false);
>         reg = SOC15_REG_OFFSET(GC, 0, regRLC_SPM_MC_CNTL);
>         if (amdgpu_sriov_is_pp_one_vf(adev))
>                 data = RREG32_NO_KIQ(reg);
> @@ -3976,6 +3977,7 @@ static void gfx_v12_0_update_spm_vmid(struct amdgpu_device *adev,
>                 WREG32_SOC15_NO_KIQ(GC, 0, regRLC_SPM_MC_CNTL, data);
>         else
>                 WREG32_SOC15(GC, 0, regRLC_SPM_MC_CNTL, data);
> +       amdgpu_gfx_off_ctrl(adev, true);
>  }
>
>  static const struct amdgpu_rlc_funcs gfx_v12_0_rlc_funcs = {
> --
> 2.50.1
>

Re: [PATCH 3/3] drm/amdgpu: program the SH_MEM registers of the SPM VMID

[Alex Deucher]

On Thu, Oct 29, 2026 at 3:41 AM Jesse Zhang <Jesse.Zhang@amd.com> wrote:
>
> Commit 02232367be23 ("drm/amdgpu: add spm_vmid and exclude it from the
> MES, KFD and kq pools") took the SPM VMID out of the kernel queue and the
> user queue VMID masks, and since commit 8a9263fc8812 ("drm/amdgpu: use
> dedicated SPM VMID from the reserved path") the kernel queue jobs of a VM
> with a reserved VMID run on it. The gfx constants_init() only programs
> SH_MEM_CONFIG and SH_MEM_BASES for VMID 0 and the kernel queue mask
> though, and init_compute_vmid() for the user queue mask, so the SPM VMID
> keeps the reset defaults: no unaligned access mode and no apertures.

This is fine for now, but I think it would be better to unify
constants_init() and init_compute_vmid() and update all the vmids in
one place.  The reserved vmid will need to reflect the configuration
of whoever uses it, which could be gfx or compute.

Alex

>
> On navi48 VMID 15 has SH_MEM_CONFIG 0xc000 and SH_MEM_BASES 0 while the
> other graphics VMIDs have 0xc00c and 0x20001000. A shader storing a dword
> to an address with a byte offset of 1 writes it there in a normal
> process, but at the dword aligned address in one which reserved a VMID,
> e.g. for profiling with SPM:
>
>   $ umr -i 0 -sb 0 0 0 15 -r '*.*.regSH_MEM_CONFIG'
>   gfx1201.regSH_MEM_CONFIG => 0x0000c000
>   $ amd_pmc --run-subtest reserved-vmid-unaligned-store
>   unaligned store: 0x0000000000002a00 normally, 0x000000000000002a with a reserved VMID
>
> Include the SPM VMID in for_each_vmid_and_zero() for the GFXHUB so the
> constants_init() of all gfx generations program it like the other
> kernel queue VMIDs.
>
> Fixes: 8a9263fc8812 ("drm/amdgpu: use dedicated SPM VMID from the reserved path")
> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> index c134000eecfd..6d0f302c1344 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> @@ -45,14 +45,17 @@
>   * Equivalent to OR-ing BIT(0) into id_mgr[hub].vmid_mask and walking
>   * the result with for_each_set_bit(). Used by the gfx_v*_constants_init()
>   * paths which must always touch VMID 0 (system) in addition to the regular
> - * VMIDs owned by the hub.
> + * VMIDs owned by the hub. On the GFXHUB this includes the SPM VMID, which is
> + * kept out of the pool but runs the kernel queue jobs of the VM reserving it.
>   *
>   * Relies on AMDGPU_NUM_VMID (16) fitting in a single unsigned long, i.e.
>   * vmid_mask[] being a 1-element bitmap.
>   */
>  #define for_each_vmid_and_zero(vmid, adev, hub)                                \
>         for (unsigned long __vmid_mask =                                \
> -               (adev)->vm_manager.id_mgr[(hub)].vmid_mask[0] | BIT(0); \
> +               (adev)->vm_manager.id_mgr[(hub)].vmid_mask[0] | BIT(0) |        \
> +               ((hub) == AMDGPU_GFXHUB(0) && (adev)->vm_manager.spm_vmid ?     \
> +                BIT((adev)->vm_manager.spm_vmid) : 0);                 \
>              __vmid_mask; __vmid_mask = 0)                              \
>                 for_each_set_bit((vmid), &__vmid_mask, AMDGPU_NUM_VMID)
>
> --
> 2.50.1
>

Re: [PATCH 1/3] drm/amdgpu: program the SPM VMID for jobs on the reserved VMID again

[Christian König]

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 <Jesse.Zhang@amd.com>
> ---
>  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,