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 2 potential
issues to consider:
- [High] drm/amdgpu: update_spm_vmid() ring emission gated on SR-IOV
  one-VF mode
- [Medium] drm/amdgpu: unskipped MMIO path and shadowed reg in
  gfx_v12_1_update_spm_vmid()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] drm/amdgpu: null pointer dereference in amdgpu_vm_flush()
  [1]
--

commit 6992ee0f5dfcb4a6c2339de5ec059b2331ca4d48
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 RLC update_spm_vmid() callback
so that amdgpu_ib_schedule() can emit the SPM VMID update on the ring instead
of via 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)

[Severity: Critical]
This problem wasn't introduced by this patch, but can [1]
amdgpu_ib_schedule() dereference a NULL adev->gfx.rlc.funcs pointer when
initializing emit_spm_needed?

drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c:amdgpu_ib_schedule() {
    ...
	bool emit_spm_needed = adev->gfx.rlc.funcs->update_spm_vmid &&
		(ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE ||
		 ring->funcs->type == AMDGPU_RING_TYPE_GFX);
    ...
}

On hardware configurations where adev->gfx.rlc.funcs is not initialized,
evaluating adev->gfx.rlc.funcs->update_spm_vmid during IB submission causes a
NULL pointer dereference and kernel panic.

[ ... ]

> 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);

[Severity: High]
Does gating the ring emission on amdgpu_sriov_is_pp_one_vf(adev) prevent
gfx_v11_0_update_spm_vmid(), gfx_v12_0_update_spm_vmid(), and
gfx_v12_1_update_spm_vmid() from ever emitting the SPM update on the ring or
skipping the MMIO path on bare metal?

amdgpu_sriov_is_pp_one_vf(adev) checks AMDGIM_FEATURE_PP_ONE_VF, which is
only set in SR-IOV single-VF mode and is false on bare metal.  When
amdgpu_ib_schedule() passes a non-NULL ring on bare-metal systems, this
condition evaluates to false and execution falls through to the synchronous
amdgpu_gfx_off_ctrl() and MMIO path on every GFX and compute IB submission.
If GFXOFF engages after amdgpu_ib_schedule() re-enables GFXOFF on the CPU and
before the GPU executes the queued IB, regRLC_SPM_MC_CNTL is cleared by
GFXOFF and never restored by the ring.

[ ... ]

> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_1.c b/drivers/gpu/drm/amd/amdgpu/gfx_v12_1.c
> index 1f57d0fa2c044..2bbe83d98563a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v12_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v12_1.c

[ ... ]

> @@ -2945,7 +2945,8 @@ static void gfx_v12_1_update_perf_clk(struct amdgpu_device *adev,
>  
>  static void gfx_v12_1_update_spm_vmid(struct amdgpu_device *adev,
>  				      int xcc_id,
> -				      unsigned int vmid)
> +				      struct amdgpu_ring *ring,
> +				      unsigned vmid)
>  {
>  	u32 reg, data;
>  
> @@ -2962,6 +2963,14 @@ static void gfx_v12_1_update_spm_vmid(struct amdgpu_device *adev,
>  		WREG32_SOC15_NO_KIQ(GC, GET_INST(GC, xcc_id), regRLC_SPM_MC_CNTL, data);
>  	else
>  		WREG32_SOC15(GC, GET_INST(GC, xcc_id), regRLC_SPM_MC_CNTL, data);
> +
> +	if (ring
> +	    && amdgpu_sriov_is_pp_one_vf(adev)
> +	    && ((ring->funcs->type == AMDGPU_RING_TYPE_GFX)
> +		|| (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE))) {
> +		uint32_t reg = SOC15_REG_OFFSET(GC, GET_INST(GC, xcc_id), regRLC_SPM_MC_CNTL);
> +		amdgpu_ring_emit_wreg(ring, reg, data);
> +	}
>  }

[Severity: Medium]
Should the ring emission check in gfx_v12_1_update_spm_vmid() run and return
before the MMIO read and write, as in gfx_v11_0_update_spm_vmid() and
gfx_v12_0_update_spm_vmid()?

Placing the ring check at the end of the function leaves the MMIO accesses to
regRLC_SPM_MC_CNTL unconditional whenever a ring is passed from
amdgpu_ib_schedule().  The inner uint32_t reg declaration also shadows the
outer u32 reg variable that is already initialized to the same offset.

[1] https://sashiko.dev/#/bug/linux-c7bb3824-c8ad-4019-945c-0cd80d5a9da3

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