From: sashiko-bot@kernel.org
To: "Alex Deucher" <alexander.deucher@amd.com>,
amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/3] drm/amdgpu: bring back the ring parameter for update_spm_vmid()
Date: Fri, 13 Nov 2026 16:15:38 +0000 [thread overview]
Message-ID: <sashiko-outbox-213179@kernel.org> (raw)
In-Reply-To: <20261113154954.1865439-3-alexander.deucher@amd.com>
Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/amdgpu: update_spm_vmid() runs MMIO on every IB on
GFX7/8/9.4.3/12.1
- [Medium] drm/amdgpu: missing update_spm_vmid() dwords in GFX9/10/12
emit_frame_size
--
commit ffe1064b12164c8a48cc5a74a5943685b8d92ee1
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 update_spm_vmid() and calls it
from amdgpu_ib_schedule() on GFX and compute rings so chips with GFXOFF
emit the SPM VMID update via the ring instead of MMIO.
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index f98a7c6797b3c..ae028eeacaed7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -121,6 +121,10 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, struct amdgpu_job *job,
> struct dma_fence **f)
> {
> struct amdgpu_device *adev = ring->adev;
> + bool emit_spm_needed = adev->gfx.rlc.funcs &&
> + adev->gfx.rlc.funcs->update_spm_vmid &&
> + (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE ||
> + ring->funcs->type == AMDGPU_RING_TYPE_GFX);
> struct amdgpu_ib *ib;
> struct dma_fence *tmp = NULL;
> struct amdgpu_fence *af;
> @@ -209,6 +213,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, ring,
> + 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/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index 7683e8a0a4380..9d58af0c678c8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -8187,8 +8187,17 @@ static void gfx_v10_0_update_spm_vmid_internal(struct amdgpu_device *adev,
> }
>
> static void gfx_v10_0_update_spm_vmid(struct amdgpu_device *adev, int xcc_id,
> - unsigned int vmid)
> -{
> + struct amdgpu_ring *ring, unsigned int vmid)
> +{
> + if (ring &&
> + ((ring->funcs->type == AMDGPU_RING_TYPE_GFX) ||
> + (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE))) {
> + u32 reg = SOC15_REG_OFFSET(GC, 0, mmRLC_SPM_MC_CNTL);
> + u32 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);
>
> gfx_v10_0_update_spm_vmid_internal(adev, vmid);
[ ... ]
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> index df2fb09bbcd86..b01e239db195f 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 &&
> + ((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))
> data = RREG32_NO_KIQ(reg);
> else
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_1.c b/drivers/gpu/drm/amd/amdgpu/gfx_v12_1.c
> index 1f57d0fa2c044..bf69d71e6b065 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;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> index 72c84b61fda16..43dea0810137b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
[ ... ]
> @@ -3483,7 +3483,7 @@ static int gfx_v7_0_rlc_resume(struct amdgpu_device *adev)
> }
>
> static void gfx_v7_0_update_spm_vmid(struct amdgpu_device *adev, int xcc_id,
> - unsigned int vmid)
> + struct amdgpu_ring *ring, unsigned vmid)
> {
> u32 data;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index 4666aa5b38c75..360a958f7577d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
[ ... ]
> @@ -5484,7 +5484,7 @@ static void gfx_v8_0_unset_safe_mode(struct amdgpu_device *adev, int xcc_id)
> }
>
> static void gfx_v8_0_update_spm_vmid(struct amdgpu_device *adev, int xcc_id,
> - unsigned int vmid)
> + struct amdgpu_ring *ring, unsigned vmid)
> {
> u32 data;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 2e2673a6dbbf5..7d001d7e3a0d0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -5227,8 +5227,17 @@ static void gfx_v9_0_update_spm_vmid_internal(struct amdgpu_device *adev,
> }
>
> static void gfx_v9_0_update_spm_vmid(struct amdgpu_device *adev, int xcc_id,
> - unsigned int vmid)
> -{
> + struct amdgpu_ring *ring, unsigned int vmid)
> +{
> + if (ring &&
> + ((ring->funcs->type == AMDGPU_RING_TYPE_GFX) ||
> + (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE))) {
> + u32 reg = SOC15_REG_OFFSET(GC, 0, mmRLC_SPM_MC_CNTL);
> + u32 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);
>
> gfx_v9_0_update_spm_vmid_internal(adev, vmid);
[Severity: Medium]
Do gfx_v9_0, gfx_v10_0, and gfx_v12_0 need 5 additional DWords in
.emit_frame_size for the new amdgpu_ring_emit_wreg() call in
update_spm_vmid()?
While gfx_v11_0_ring_funcs_gfx and gfx_v11_0_ring_funcs_compute already
include 5 DWords for update_spm_vmid in .emit_frame_size, that accounting
is missing from gfx_v9_0_ring_funcs_gfx, gfx_v9_0_sw_ring_funcs_gfx,
gfx_v9_0_ring_funcs_compute, gfx_v10_0_ring_funcs_gfx,
gfx_v10_0_ring_funcs_compute, gfx_v12_0_ring_funcs_gfx, and
gfx_v12_0_ring_funcs_compute.
When a submission emits the full frame and alloc_size lands within 4 DWords
below a 256-DWord alignment boundary (for example, 15 IBs on
gfx_v10_0_ring_funcs_gfx gives 196 + 60 = 256 DWords), emitting 261 DWords
underflows ring->count_dw in amdgpu_ring_commit(), logging an error and
padding with NOPs into the next unallocated 256-DWord block. On
gfx_v9_0_ring_funcs_compute, 125 IBs also pushes the total from 1020 to
1025 DWords, exceeding the 1024-DWord max_dw passed to amdgpu_ring_init().
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> index da031caa4e9e6..30c8b751849cd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
[ ... ]
> @@ -1609,7 +1609,7 @@ static int gfx_v9_4_3_rlc_resume(struct amdgpu_device *adev)
> }
>
> static void gfx_v9_4_3_update_spm_vmid(struct amdgpu_device *adev,
> - int inst, unsigned int vmid)
> + int inst, struct amdgpu_ring *ring, unsigned int vmid)
> {
> u32 reg, pre_data, data;
[Severity: High]
Should gfx_v7_0_update_spm_vmid(), gfx_v8_0_update_spm_vmid(),
gfx_v9_4_3_update_spm_vmid(), and gfx_v12_1_update_spm_vmid() return early
when ring is non-NULL?
Because amdgpu_ib_schedule() now calls update_spm_vmid() on every GFX and
compute IB submission whenever the callback is present, and these four
implementations ignore the ring parameter, they run synchronous MMIO on
every IB submission. On GFX7 and GFX8, every submission calls
amdgpu_gfx_off_ctrl(), RREG32(), and WREG32(); on GFX9.4.3, every compute
submission runs RREG32() (which dispatches and polls KIQ under SR-IOV
runtime); and on GFX12.1 when kernel compute queues are enabled via
amdgpu_user_queue=0, concurrent per-ring scheduler threads perform
unserialized RREG32() and WREG32_SOC15() read-modify-writes without
disengaging GFXOFF.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261113154954.1865439-1-alexander.deucher@amd.com?part=3
next prev parent reply other threads:[~2026-11-13 16:15 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-11-13 15:49 [PATCH 1/3] Revert "drm/amdgpu: re-enable SPM updates in the the IB path" Alex Deucher
2026-11-13 15:49 ` [PATCH 2/3] Revert "drm/amdgpu: reemit RLC SPM register in ring reemit" Alex Deucher
2026-11-13 15:55 ` sashiko-bot
2026-11-13 15:49 ` [PATCH 3/3] drm/amdgpu: bring back the ring parameter for update_spm_vmid() Alex Deucher
2026-11-13 16:15 ` sashiko-bot [this message]
2026-11-13 15:58 ` [PATCH 1/3] Revert "drm/amdgpu: re-enable SPM updates in the the IB path" sashiko-bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=sashiko-outbox-213179@kernel.org \
--to=sashiko-bot@kernel.org \
--cc=alexander.deucher@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.