From: "Timur Kristóf" <timur.kristof@gmail.com>
To: amd-gfx@lists.freedesktop.org, "Marek Olšák" <maraeo@gmail.com>,
"Alex Deucher" <alexander.deucher@amd.com>,
"Christian König" <christian.koenig@amd.com>,
"Tvrtko Ursulin" <tursulin@ursulin.net>,
pierre-eric.pelloux-prayer@amd.com,
"Natalie Vock" <nat@pixelcluster.dev>,
"Lijo Lazar" <lijo.lazar@amd.com>,
"Felix Kuehling" <Felix.Kuehling@amd.com>
Cc: "Timur Kristóf" <timur.kristof@gmail.com>
Subject: [PATCH 3/9] drm/amdgpu/sdma: Fix executing duplicate commands after recovery on SDMA v4.4.2
Date: Tue, 8 Sep 2026 20:10:46 +0200 [thread overview]
Message-ID: <20260908181052.381126-4-timur.kristof@gmail.com> (raw)
In-Reply-To: <20260908181052.381126-1-timur.kristof@gmail.com>
SDMA v4.4.2 had a custom recovery implementation that
tried to keep ring contents and restore the rptr after
an SDMA soft reset. Later it was changed to use the
more common amdgpu_sdma_reset_engine() implementation.
Currently, SDMA v4.4.2 may execute some commands twice
after a recovery:
1. From the old ring buffer contents at the restored rptr
2. From the contents restored by amdgpu_sdma_reset_engine()
that calls the ring reset helper functions.
Fix that by removing the code that restores rptr.
This is not needed anymore because amdgpu_sdma_reset_engine()
already saves and restores the contents of both the SDMA gfx
and paging rings, so there is no need to do anything extra.
Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 2 -
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 2 -
drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 76 +++++-------------------
3 files changed, 14 insertions(+), 66 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 686c92e96025..132ca5ce3a6d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -347,8 +347,6 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
ring->buf_mask = (ring->ring_size / 4) - 1;
ring->ptr_mask = ring->funcs->support_64bit_ptrs ?
0xffffffffffffffff : ring->buf_mask;
- /* Initialize cached_rptr to 0 */
- ring->cached_rptr = 0;
if (!ring->ring_backup) {
ring->ring_backup = kvzalloc(ring->ring_size, GFP_KERNEL);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 6b6ee4083c8d..61e30de1977b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -423,8 +423,6 @@ struct amdgpu_ring {
bool is_sw_ring;
unsigned int entry_index;
- /* store the cached rptr to restore after reset */
- uint64_t cached_rptr;
};
#define amdgpu_ring_parse_cs(r, p, job, ib) ((r)->funcs->parse_cs((p), (job), (ib)))
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
index 7e8d528cf422..53c816043eb8 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
@@ -660,12 +660,11 @@ static uint32_t sdma_v4_4_2_rb_cntl(struct amdgpu_ring *ring, uint32_t rb_cntl)
*
* @adev: amdgpu_device pointer
* @i: instance to resume
- * @restore: used to restore wptr when restart
*
* Set up the gfx DMA ring buffers and enable them.
* Returns 0 for success, error for failure.
*/
-static void sdma_v4_4_2_gfx_resume(struct amdgpu_device *adev, unsigned int i, bool restore)
+static void sdma_v4_4_2_gfx_resume(struct amdgpu_device *adev, unsigned int i)
{
struct amdgpu_ring *ring = &adev->sdma.instance[i].ring;
u32 rb_cntl, ib_cntl, wptr_poll_cntl;
@@ -673,7 +672,6 @@ static void sdma_v4_4_2_gfx_resume(struct amdgpu_device *adev, unsigned int i, b
u32 doorbell;
u32 doorbell_offset;
u64 wptr_gpu_addr;
- u64 rwptr;
wb_offset = (ring->rptr_offs * 4);
@@ -693,32 +691,16 @@ static void sdma_v4_4_2_gfx_resume(struct amdgpu_device *adev, unsigned int i, b
WREG32_SDMA(i, regSDMA_GFX_RB_BASE, ring->gpu_addr >> 8);
WREG32_SDMA(i, regSDMA_GFX_RB_BASE_HI, ring->gpu_addr >> 40);
- if (!restore)
- ring->wptr = 0;
+ ring->wptr = 0;
/* before programing wptr to a less value, need set minor_ptr_update first */
WREG32_SDMA(i, regSDMA_GFX_MINOR_PTR_UPDATE, 1);
- /* For the guilty queue, set RPTR to the current wptr to skip bad commands,
- * It is not a guilty queue, restore cache_rptr and continue execution.
- */
- if (adev->sdma.instance[i].gfx_guilty)
- rwptr = ring->wptr;
- else
- rwptr = ring->cached_rptr;
-
/* Initialize the ring buffer's read and write pointers */
- if (restore) {
- WREG32_SDMA(i, regSDMA_GFX_RB_RPTR, lower_32_bits(rwptr << 2));
- WREG32_SDMA(i, regSDMA_GFX_RB_RPTR_HI, upper_32_bits(rwptr << 2));
- WREG32_SDMA(i, regSDMA_GFX_RB_WPTR, lower_32_bits(rwptr << 2));
- WREG32_SDMA(i, regSDMA_GFX_RB_WPTR_HI, upper_32_bits(rwptr << 2));
- } else {
- WREG32_SDMA(i, regSDMA_GFX_RB_RPTR, 0);
- WREG32_SDMA(i, regSDMA_GFX_RB_RPTR_HI, 0);
- WREG32_SDMA(i, regSDMA_GFX_RB_WPTR, 0);
- WREG32_SDMA(i, regSDMA_GFX_RB_WPTR_HI, 0);
- }
+ WREG32_SDMA(i, regSDMA_GFX_RB_RPTR, 0);
+ WREG32_SDMA(i, regSDMA_GFX_RB_RPTR_HI, 0);
+ WREG32_SDMA(i, regSDMA_GFX_RB_WPTR, 0);
+ WREG32_SDMA(i, regSDMA_GFX_RB_WPTR_HI, 0);
doorbell = RREG32_SDMA(i, regSDMA_GFX_DOORBELL);
doorbell_offset = RREG32_SDMA(i, regSDMA_GFX_DOORBELL_OFFSET);
@@ -771,7 +753,7 @@ static void sdma_v4_4_2_gfx_resume(struct amdgpu_device *adev, unsigned int i, b
* Set up the page DMA ring buffers and enable them.
* Returns 0 for success, error for failure.
*/
-static void sdma_v4_4_2_page_resume(struct amdgpu_device *adev, unsigned int i, bool restore)
+static void sdma_v4_4_2_page_resume(struct amdgpu_device *adev, unsigned int i)
{
struct amdgpu_ring *ring = &adev->sdma.instance[i].page;
u32 rb_cntl, ib_cntl, wptr_poll_cntl;
@@ -779,7 +761,6 @@ static void sdma_v4_4_2_page_resume(struct amdgpu_device *adev, unsigned int i,
u32 doorbell;
u32 doorbell_offset;
u64 wptr_gpu_addr;
- u64 rwptr;
wb_offset = (ring->rptr_offs * 4);
@@ -787,26 +768,11 @@ static void sdma_v4_4_2_page_resume(struct amdgpu_device *adev, unsigned int i,
rb_cntl = sdma_v4_4_2_rb_cntl(ring, rb_cntl);
WREG32_SDMA(i, regSDMA_PAGE_RB_CNTL, rb_cntl);
- /* For the guilty queue, set RPTR to the current wptr to skip bad commands,
- * It is not a guilty queue, restore cache_rptr and continue execution.
- */
- if (adev->sdma.instance[i].page_guilty)
- rwptr = ring->wptr;
- else
- rwptr = ring->cached_rptr;
-
/* Initialize the ring buffer's read and write pointers */
- if (restore) {
- WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR, lower_32_bits(rwptr << 2));
- WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR_HI, upper_32_bits(rwptr << 2));
- WREG32_SDMA(i, regSDMA_PAGE_RB_WPTR, lower_32_bits(rwptr << 2));
- WREG32_SDMA(i, regSDMA_PAGE_RB_WPTR_HI, upper_32_bits(rwptr << 2));
- } else {
- WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR, 0);
- WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR_HI, 0);
- WREG32_SDMA(i, regSDMA_PAGE_RB_WPTR, 0);
- WREG32_SDMA(i, regSDMA_PAGE_RB_WPTR_HI, 0);
- }
+ WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR, 0);
+ WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR_HI, 0);
+ WREG32_SDMA(i, regSDMA_PAGE_RB_WPTR, 0);
+ WREG32_SDMA(i, regSDMA_PAGE_RB_WPTR_HI, 0);
/* set the wb address whether it's enabled or not */
WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR_ADDR_HI,
@@ -820,8 +786,7 @@ static void sdma_v4_4_2_page_resume(struct amdgpu_device *adev, unsigned int i,
WREG32_SDMA(i, regSDMA_PAGE_RB_BASE, ring->gpu_addr >> 8);
WREG32_SDMA(i, regSDMA_PAGE_RB_BASE_HI, ring->gpu_addr >> 40);
- if (!restore)
- ring->wptr = 0;
+ ring->wptr = 0;
/* before programing wptr to a less value, need set minor_ptr_update first */
WREG32_SDMA(i, regSDMA_PAGE_MINOR_PTR_UPDATE, 1);
@@ -959,9 +924,9 @@ static int sdma_v4_4_2_inst_start(struct amdgpu_device *adev,
uint32_t temp;
WREG32_SDMA(i, regSDMA_SEM_WAIT_FAIL_TIMER_CNTL, 0);
- sdma_v4_4_2_gfx_resume(adev, i, restore);
+ sdma_v4_4_2_gfx_resume(adev, i);
if (adev->sdma.has_page_queue)
- sdma_v4_4_2_page_resume(adev, i, restore);
+ sdma_v4_4_2_page_resume(adev, i);
/* set utc l1 enable flag always to 1 */
temp = RREG32_SDMA(i, regSDMA_CNTL);
@@ -1645,7 +1610,6 @@ static int sdma_v4_4_2_stop_queue(struct amdgpu_ring *ring)
struct amdgpu_device *adev = ring->adev;
u32 instance_id = ring->me;
u32 inst_mask;
- uint64_t rptr;
if (amdgpu_sriov_vf(adev))
return -EINVAL;
@@ -1657,18 +1621,6 @@ static int sdma_v4_4_2_stop_queue(struct amdgpu_ring *ring)
adev->sdma.instance[instance_id].page_guilty =
sdma_v4_4_2_is_queue_selected(adev, instance_id, true);
- /* Cache the rptr before reset, after the reset,
- * all of the registers will be reset to 0
- */
- rptr = amdgpu_ring_get_rptr(ring);
- ring->cached_rptr = rptr;
- /* Cache the rptr for the page queue if it exists */
- if (adev->sdma.has_page_queue) {
- struct amdgpu_ring *page_ring = &adev->sdma.instance[instance_id].page;
- rptr = amdgpu_ring_get_rptr(page_ring);
- page_ring->cached_rptr = rptr;
- }
-
/* stop queue */
inst_mask = 1 << ring->me;
sdma_v4_4_2_inst_gfx_stop(adev, inst_mask);
--
2.55.0
next prev parent reply other threads:[~2026-09-08 18:11 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-08 18:10 [PATCH 0/9] drm/amdgpu/sdma: Improve existing SDMA queue resets (v4) Timur Kristóf
2026-09-08 18:10 ` [PATCH 1/9] drm/amdgpu/sdma: Remove unimplemented soft_reset() for SDMA and SI DMA Timur Kristóf
2026-09-08 18:10 ` [PATCH 2/9] drm/amdgpu/sdma: Remove superfluous rlc_resume and rlc_stop functions Timur Kristóf
2026-09-08 18:10 ` Timur Kristóf [this message]
2026-09-08 18:10 ` [PATCH 4/9] drm/amdgpu/sdma: Remove unnecessary guilty tracking of SDMA queues Timur Kristóf
2026-09-08 18:10 ` [PATCH 5/9] drm/amdgpu: Add amdgpu_ring_clear_ring_and_ptrs() Timur Kristóf
2026-09-08 18:10 ` [PATCH 6/9] drm/amdgpu/sdma: Clear SDMA rings after reset before starting them Timur Kristóf
2026-09-08 18:10 ` [PATCH 7/9] drm/amdgpu/sdma: Move SDMA v5.x queue reset to common code Timur Kristóf
2026-09-08 18:10 ` [PATCH 8/9] drm/amdgpu/sdma: Always handle kernel queues in amdgpu_sdma_reset_engine() Timur Kristóf
2026-09-08 18:10 ` [PATCH 9/9] drm/amdgpu/sdma: Use common SDMA legacy queue reset on SDMA v4.4.2 Timur Kristóf
2026-09-11 18:44 ` [PATCH 0/9] drm/amdgpu/sdma: Improve existing SDMA queue resets (v4) Alex Deucher
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=20260908181052.381126-4-timur.kristof@gmail.com \
--to=timur.kristof@gmail.com \
--cc=Felix.Kuehling@amd.com \
--cc=alexander.deucher@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=christian.koenig@amd.com \
--cc=lijo.lazar@amd.com \
--cc=maraeo@gmail.com \
--cc=nat@pixelcluster.dev \
--cc=pierre-eric.pelloux-prayer@amd.com \
--cc=tursulin@ursulin.net \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox