Re: [PATCH] drm/amdgpu: bring back the ring parameter for update_spm_vmid()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/amdgpu: ring emission in update_spm_vmid() gated on SR-IOV
  1-VF mode
--

commit 90ca59b6e99e6b8e5c6b2f133b97efb1b24aedb0
Author: Alex Deucher <alexander.deucher@amd.com>

drm/amdgpu: bring back the ring parameter for update_spm_vmid()

This commit restores the ring parameter to the update_spm_vmid() callback
and passes the ring from amdgpu_ib_schedule() so that GFX11 and GFX12 can
emit the SPM VMID update on the ring instead of using MMIO.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 6210edf1937aa..a24cdf5852a32 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -213,7 +213,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, struct amdgpu_job *job,
>  		amdgpu_ring_emit_pipeline_sync(ring);
>  
>  	if (emit_spm_needed)
> -		adev->gfx.rlc.funcs->update_spm_vmid(adev, ring->xcc_id,
> +		adev->gfx.rlc.funcs->update_spm_vmid(adev, ring->xcc_id, ring,
>  						     adev->vm_manager.spm_vmid);
>  
>  	if (emit_gds_needed)

[ ... ]

> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> index 5d068a9d9c72c..ac814c12d60f2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c

[ ... ]

> @@ -5615,12 +5616,21 @@ static int gfx_v11_0_update_gfx_clock_gating(struct amdgpu_device *adev,
>  }
>  
>  static void gfx_v11_0_update_spm_vmid(struct amdgpu_device *adev, int xcc_id,
> -		unsigned int vmid)
> +		struct amdgpu_ring *ring, unsigned vmid)
>  {
>  	u32 reg, pre_data, data;
>  
> -	amdgpu_gfx_off_ctrl(adev, false);
>  	reg = SOC15_REG_OFFSET(GC, 0, regRLC_SPM_MC_CNTL);
> +
> +	if (ring && amdgpu_sriov_is_pp_one_vf(adev) &&
> +	    ((ring->funcs->type == AMDGPU_RING_TYPE_GFX) ||
> +	     (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE))) {
> +		data = (vmid & RLC_SPM_MC_CNTL__RLC_SPM_VMID_MASK) << RLC_SPM_MC_CNTL__RLC_SPM_VMID__SHIFT;
> +		amdgpu_ring_emit_wreg(ring, reg, data);
> +		return;
> +	}
> +
> +	amdgpu_gfx_off_ctrl(adev, false);
>  	if (amdgpu_sriov_is_pp_one_vf(adev) && !amdgpu_sriov_runtime(adev))
>  		pre_data = RREG32_NO_KIQ(reg);
>  	else
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> index df2fb09bbcd86..07f981f27bcc8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c

[ ... ]

> @@ -3978,12 +3979,22 @@ static void gfx_v12_0_update_perf_clk(struct amdgpu_device *adev,
>  
>  static void gfx_v12_0_update_spm_vmid(struct amdgpu_device *adev,
>  				      int xcc_id,
> +				      struct amdgpu_ring *ring,
>  				      unsigned vmid)
>  {
>  	u32 reg, data;
>  
> -	amdgpu_gfx_off_ctrl(adev, false);
>  	reg = SOC15_REG_OFFSET(GC, 0, regRLC_SPM_MC_CNTL);
> +
> +	if (ring && amdgpu_sriov_is_pp_one_vf(adev) &&
> +	    ((ring->funcs->type == AMDGPU_RING_TYPE_GFX) ||
> +	     (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE))) {
> +		data = (vmid & RLC_SPM_MC_CNTL__RLC_SPM_VMID_MASK) << RLC_SPM_MC_CNTL__RLC_SPM_VMID__SHIFT;
> +		amdgpu_ring_emit_wreg(ring, reg, data);
> +		return;
> +	}
> +
> +	amdgpu_gfx_off_ctrl(adev, false);

[Severity: High]
Does gating the ring emission on amdgpu_sriov_is_pp_one_vf(adev) prevent
gfx_v11_0_update_spm_vmid() and gfx_v12_0_update_spm_vmid() from emitting
regRLC_SPM_MC_CNTL on bare-metal systems?

amdgpu_sriov_is_pp_one_vf(adev) checks AMDGIM_FEATURE_PP_ONE_VF, which is
only set on SR-IOV single-VF guests:

drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h:
#define amdgpu_sriov_is_pp_one_vf(adev) \
	((adev)->virt.gim_feature & AMDGIM_FEATURE_PP_ONE_VF)

On bare metal, calling update_spm_vmid() with a non-NULL ring from
amdgpu_ib_schedule() skips amdgpu_ring_emit_wreg() and falls through to
amdgpu_gfx_off_ctrl() and CPU MMIO on every GFX and compute IB submission.
Because GFXOFF is re-enabled on the CPU before the ring is committed and
executed by the GPU, GFXOFF can re-engage and clear regRLC_SPM_MC_CNTL
without any packet on the ring to restore it.

In addition, the update_spm_vmid() callbacks in gfx_v7_0, gfx_v8_0,
gfx_v9_0, gfx_v9_4_3, and gfx_v10_0 ignore the ring parameter and run the
MMIO path on every IB submission, while the ring path in gfx_v11_0 and
gfx_v12_0 overwrites regRLC_SPM_MC_CNTL without preserving other bits in
the register.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112214802.1774412-1-alexander.deucher@amd.com?part=1