* [PATCH] drm/amdgpu: deduplicate ring preempt ib function
@ 2026-04-21 20:03 Leonardo Cesar
2026-04-22 6:58 ` Christian König
0 siblings, 1 reply; 5+ messages in thread
From: Leonardo Cesar @ 2026-04-21 20:03 UTC (permalink / raw)
To: alexander.deucher, christian.koenig, airlied, simona
Cc: Leonardo Cesar, amd-gfx, dri-devel
The ring preemption function is identical for both gfx_v11_0 and
gfx_v12_0. This patch refactors the code by moving the core logic
into a generic function inside amdgpu_gfx.c to reduce code
duplication and simplify future maintenance.
Signed-off-by: Leonardo Cesar <leonardocesar@usp.br>
---
v1 -> v2:
- Removed wrapper functions for gfx_v11 and gfx_v12 and updated call sites directly.
drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | ...
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 51 ++++++++++++++++++++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 2 +
drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c | 52 +------------------------
drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c | 52 +------------------------
4 files changed, 55 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 2956e45c9..a157cbd8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -2684,3 +2684,54 @@ void amdgpu_debugfs_compute_sched_mask_init(struct amdgpu_device *adev)
#endif
}
+int amdgpu_gfx_ring_preempt_ib(struct amdgpu_ring *ring)
+{
+ int i, r = 0;
+ struct amdgpu_device *adev = ring->adev;
+ struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
+ struct amdgpu_ring *kiq_ring = &kiq->ring;
+ unsigned long flags;
+
+ if (adev->enable_mes)
+ return -EINVAL;
+
+ if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
+ return -EINVAL;
+
+ spin_lock_irqsave(&kiq->ring_lock, flags);
+
+ if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size)) {
+ spin_unlock_irqrestore(&kiq->ring_lock, flags);
+ return -ENOMEM;
+ }
+
+ /* assert preemption condition */
+ amdgpu_ring_set_preempt_cond_exec(ring, false);
+
+ /* assert IB preemption, emit the trailing fence */
+ kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
+ ring->trail_fence_gpu_addr,
+ ++ring->trail_seq);
+ amdgpu_ring_commit(kiq_ring);
+
+ spin_unlock_irqrestore(&kiq->ring_lock, flags);
+
+ /* poll the trailing fence */
+ for (i = 0; i < adev->usec_timeout; i++) {
+ if (ring->trail_seq ==
+ le32_to_cpu(*(ring->trail_fence_cpu_addr)))
+ break;
+ udelay(1);
+ }
+
+ if (i >= adev->usec_timeout) {
+ r = -EINVAL;
+ DRM_ERROR("ring %d failed to preempt ib\n", ring->idx);
+ }
+
+ /* deassert preemption condition */
+ amdgpu_ring_set_preempt_cond_exec(ring, true);
+ return r;
+}
+
+
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
index a0cf0a3b4..77050f988 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
@@ -664,6 +664,8 @@ void amdgpu_gfx_csb_preamble_end(u32 *buffer, u32 count);
void amdgpu_debugfs_gfx_sched_mask_init(struct amdgpu_device *adev);
void amdgpu_debugfs_compute_sched_mask_init(struct amdgpu_device *adev);
+int amdgpu_gfx_ring_preempt_ib(struct amdgpu_ring *ring);
+
static inline const char *amdgpu_gfx_compute_mode_desc(int mode)
{
switch (mode) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
index 5097de940..1ba848bfa 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
@@ -6206,56 +6206,6 @@ static void gfx_v11_0_ring_emit_gfx_shadow(struct amdgpu_ring *ring,
ring->set_q_mode_offs = offs;
}
-static int gfx_v11_0_ring_preempt_ib(struct amdgpu_ring *ring)
-{
- int i, r = 0;
- struct amdgpu_device *adev = ring->adev;
- struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
- struct amdgpu_ring *kiq_ring = &kiq->ring;
- unsigned long flags;
-
- if (adev->enable_mes)
- return -EINVAL;
-
- if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
- return -EINVAL;
-
- spin_lock_irqsave(&kiq->ring_lock, flags);
-
- if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size)) {
- spin_unlock_irqrestore(&kiq->ring_lock, flags);
- return -ENOMEM;
- }
-
- /* assert preemption condition */
- amdgpu_ring_set_preempt_cond_exec(ring, false);
-
- /* assert IB preemption, emit the trailing fence */
- kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
- ring->trail_fence_gpu_addr,
- ++ring->trail_seq);
- amdgpu_ring_commit(kiq_ring);
-
- spin_unlock_irqrestore(&kiq->ring_lock, flags);
-
- /* poll the trailing fence */
- for (i = 0; i < adev->usec_timeout; i++) {
- if (ring->trail_seq ==
- le32_to_cpu(*(ring->trail_fence_cpu_addr)))
- break;
- udelay(1);
- }
-
- if (i >= adev->usec_timeout) {
- r = -EINVAL;
- DRM_ERROR("ring %d failed to preempt ib\n", ring->idx);
- }
-
- /* deassert preemption condition */
- amdgpu_ring_set_preempt_cond_exec(ring, true);
- return r;
-}
-
static void gfx_v11_0_ring_emit_de_meta(struct amdgpu_ring *ring, bool resume)
{
struct amdgpu_device *adev = ring->adev;
@@ -7295,7 +7245,7 @@ static const struct amdgpu_ring_funcs gfx_v11_0_ring_funcs_gfx = {
.emit_cntxcntl = gfx_v11_0_ring_emit_cntxcntl,
.emit_gfx_shadow = gfx_v11_0_ring_emit_gfx_shadow,
.init_cond_exec = gfx_v11_0_ring_emit_init_cond_exec,
- .preempt_ib = gfx_v11_0_ring_preempt_ib,
+ .preempt_ib = amdgpu_gfx_ring_preempt_ib,
.emit_frame_cntl = gfx_v11_0_ring_emit_frame_cntl,
.emit_wreg = gfx_v11_0_ring_emit_wreg,
.emit_reg_wait = gfx_v11_0_ring_emit_reg_wait,
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
index 65c33823a..6cf244349 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
@@ -4611,56 +4611,6 @@ static unsigned gfx_v12_0_ring_emit_init_cond_exec(struct amdgpu_ring *ring,
return ret;
}
-static int gfx_v12_0_ring_preempt_ib(struct amdgpu_ring *ring)
-{
- int i, r = 0;
- struct amdgpu_device *adev = ring->adev;
- struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
- struct amdgpu_ring *kiq_ring = &kiq->ring;
- unsigned long flags;
-
- if (adev->enable_mes)
- return -EINVAL;
-
- if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
- return -EINVAL;
-
- spin_lock_irqsave(&kiq->ring_lock, flags);
-
- if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size)) {
- spin_unlock_irqrestore(&kiq->ring_lock, flags);
- return -ENOMEM;
- }
-
- /* assert preemption condition */
- amdgpu_ring_set_preempt_cond_exec(ring, false);
-
- /* assert IB preemption, emit the trailing fence */
- kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
- ring->trail_fence_gpu_addr,
- ++ring->trail_seq);
- amdgpu_ring_commit(kiq_ring);
-
- spin_unlock_irqrestore(&kiq->ring_lock, flags);
-
- /* poll the trailing fence */
- for (i = 0; i < adev->usec_timeout; i++) {
- if (ring->trail_seq ==
- le32_to_cpu(*(ring->trail_fence_cpu_addr)))
- break;
- udelay(1);
- }
-
- if (i >= adev->usec_timeout) {
- r = -EINVAL;
- DRM_ERROR("ring %d failed to preempt ib\n", ring->idx);
- }
-
- /* deassert preemption condition */
- amdgpu_ring_set_preempt_cond_exec(ring, true);
- return r;
-}
-
static void gfx_v12_0_ring_emit_rreg(struct amdgpu_ring *ring, uint32_t reg,
uint32_t reg_val_offs)
{
@@ -5539,7 +5489,7 @@ static const struct amdgpu_ring_funcs gfx_v12_0_ring_funcs_gfx = {
.pad_ib = amdgpu_ring_generic_pad_ib,
.emit_cntxcntl = gfx_v12_0_ring_emit_cntxcntl,
.init_cond_exec = gfx_v12_0_ring_emit_init_cond_exec,
- .preempt_ib = gfx_v12_0_ring_preempt_ib,
+ .preempt_ib = amdgpu_gfx_ring_preempt_ib,
.emit_wreg = gfx_v12_0_ring_emit_wreg,
.emit_reg_wait = gfx_v12_0_ring_emit_reg_wait,
.emit_reg_write_reg_wait = gfx_v12_0_ring_emit_reg_write_reg_wait,
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/amdgpu: deduplicate ring preempt ib function
2026-04-21 20:03 [PATCH] drm/amdgpu: deduplicate ring preempt ib function Leonardo Cesar
@ 2026-04-22 6:58 ` Christian König
2026-04-22 12:55 ` Alex Deucher
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Christian König @ 2026-04-22 6:58 UTC (permalink / raw)
To: Leonardo Cesar, alexander.deucher, airlied, simona; +Cc: amd-gfx, dri-devel
On 4/21/26 22:03, Leonardo Cesar wrote:
> The ring preemption function is identical for both gfx_v11_0 and
> gfx_v12_0. This patch refactors the code by moving the core logic
> into a generic function inside amdgpu_gfx.c to reduce code
> duplication and simplify future maintenance.
Yeah that one looks reasonable. As far as I can see there isn't anything HW generation specific in the function.
Question is rather why we have that for gfx12 in the first place. @Alex IIRC we support preemption only for a very narrow use case on gfx11, could that just be accidentially be copied over?
>
> Signed-off-by: Leonardo Cesar <leonardocesar@usp.br>
>
> ---
> v1 -> v2:
> - Removed wrapper functions for gfx_v11 and gfx_v12 and updated call sites directly.
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | ...
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 51 ++++++++++++++++++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 2 +
> drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c | 52 +------------------------
> drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c | 52 +------------------------
> 4 files changed, 55 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 2956e45c9..a157cbd8e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -2684,3 +2684,54 @@ void amdgpu_debugfs_compute_sched_mask_init(struct amdgpu_device *adev)
> #endif
> }
>
> +int amdgpu_gfx_ring_preempt_ib(struct amdgpu_ring *ring)
> +{
> + int i, r = 0;
Just a style nit: Variables like "i" or "r" last please and don't initialize vairables like "r" while defining it.
Regards,
Christian.
> + struct amdgpu_device *adev = ring->adev;
> + struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
> + struct amdgpu_ring *kiq_ring = &kiq->ring;
> + unsigned long flags;
> +
> + if (adev->enable_mes)
> + return -EINVAL;
> +
> + if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
> + return -EINVAL;
> +
> + spin_lock_irqsave(&kiq->ring_lock, flags);
> +
> + if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size)) {
> + spin_unlock_irqrestore(&kiq->ring_lock, flags);
> + return -ENOMEM;
> + }
> +
> + /* assert preemption condition */
> + amdgpu_ring_set_preempt_cond_exec(ring, false);
> +
> + /* assert IB preemption, emit the trailing fence */
> + kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
> + ring->trail_fence_gpu_addr,
> + ++ring->trail_seq);
> + amdgpu_ring_commit(kiq_ring);
> +
> + spin_unlock_irqrestore(&kiq->ring_lock, flags);
> +
> + /* poll the trailing fence */
> + for (i = 0; i < adev->usec_timeout; i++) {
> + if (ring->trail_seq ==
> + le32_to_cpu(*(ring->trail_fence_cpu_addr)))
> + break;
> + udelay(1);
> + }
> +
> + if (i >= adev->usec_timeout) {
> + r = -EINVAL;
> + DRM_ERROR("ring %d failed to preempt ib\n", ring->idx);
> + }
> +
> + /* deassert preemption condition */
> + amdgpu_ring_set_preempt_cond_exec(ring, true);
> + return r;
> +}
> +
> +
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> index a0cf0a3b4..77050f988 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> @@ -664,6 +664,8 @@ void amdgpu_gfx_csb_preamble_end(u32 *buffer, u32 count);
> void amdgpu_debugfs_gfx_sched_mask_init(struct amdgpu_device *adev);
> void amdgpu_debugfs_compute_sched_mask_init(struct amdgpu_device *adev);
>
> +int amdgpu_gfx_ring_preempt_ib(struct amdgpu_ring *ring);
> +
> static inline const char *amdgpu_gfx_compute_mode_desc(int mode)
> {
> switch (mode) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> index 5097de940..1ba848bfa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> @@ -6206,56 +6206,6 @@ static void gfx_v11_0_ring_emit_gfx_shadow(struct amdgpu_ring *ring,
> ring->set_q_mode_offs = offs;
> }
>
> -static int gfx_v11_0_ring_preempt_ib(struct amdgpu_ring *ring)
> -{
> - int i, r = 0;
> - struct amdgpu_device *adev = ring->adev;
> - struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
> - struct amdgpu_ring *kiq_ring = &kiq->ring;
> - unsigned long flags;
> -
> - if (adev->enable_mes)
> - return -EINVAL;
> -
> - if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
> - return -EINVAL;
> -
> - spin_lock_irqsave(&kiq->ring_lock, flags);
> -
> - if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size)) {
> - spin_unlock_irqrestore(&kiq->ring_lock, flags);
> - return -ENOMEM;
> - }
> -
> - /* assert preemption condition */
> - amdgpu_ring_set_preempt_cond_exec(ring, false);
> -
> - /* assert IB preemption, emit the trailing fence */
> - kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
> - ring->trail_fence_gpu_addr,
> - ++ring->trail_seq);
> - amdgpu_ring_commit(kiq_ring);
> -
> - spin_unlock_irqrestore(&kiq->ring_lock, flags);
> -
> - /* poll the trailing fence */
> - for (i = 0; i < adev->usec_timeout; i++) {
> - if (ring->trail_seq ==
> - le32_to_cpu(*(ring->trail_fence_cpu_addr)))
> - break;
> - udelay(1);
> - }
> -
> - if (i >= adev->usec_timeout) {
> - r = -EINVAL;
> - DRM_ERROR("ring %d failed to preempt ib\n", ring->idx);
> - }
> -
> - /* deassert preemption condition */
> - amdgpu_ring_set_preempt_cond_exec(ring, true);
> - return r;
> -}
> -
> static void gfx_v11_0_ring_emit_de_meta(struct amdgpu_ring *ring, bool resume)
> {
> struct amdgpu_device *adev = ring->adev;
> @@ -7295,7 +7245,7 @@ static const struct amdgpu_ring_funcs gfx_v11_0_ring_funcs_gfx = {
> .emit_cntxcntl = gfx_v11_0_ring_emit_cntxcntl,
> .emit_gfx_shadow = gfx_v11_0_ring_emit_gfx_shadow,
> .init_cond_exec = gfx_v11_0_ring_emit_init_cond_exec,
> - .preempt_ib = gfx_v11_0_ring_preempt_ib,
> + .preempt_ib = amdgpu_gfx_ring_preempt_ib,
> .emit_frame_cntl = gfx_v11_0_ring_emit_frame_cntl,
> .emit_wreg = gfx_v11_0_ring_emit_wreg,
> .emit_reg_wait = gfx_v11_0_ring_emit_reg_wait,
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> index 65c33823a..6cf244349 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> @@ -4611,56 +4611,6 @@ static unsigned gfx_v12_0_ring_emit_init_cond_exec(struct amdgpu_ring *ring,
> return ret;
> }
>
> -static int gfx_v12_0_ring_preempt_ib(struct amdgpu_ring *ring)
> -{
> - int i, r = 0;
> - struct amdgpu_device *adev = ring->adev;
> - struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
> - struct amdgpu_ring *kiq_ring = &kiq->ring;
> - unsigned long flags;
> -
> - if (adev->enable_mes)
> - return -EINVAL;
> -
> - if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
> - return -EINVAL;
> -
> - spin_lock_irqsave(&kiq->ring_lock, flags);
> -
> - if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size)) {
> - spin_unlock_irqrestore(&kiq->ring_lock, flags);
> - return -ENOMEM;
> - }
> -
> - /* assert preemption condition */
> - amdgpu_ring_set_preempt_cond_exec(ring, false);
> -
> - /* assert IB preemption, emit the trailing fence */
> - kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
> - ring->trail_fence_gpu_addr,
> - ++ring->trail_seq);
> - amdgpu_ring_commit(kiq_ring);
> -
> - spin_unlock_irqrestore(&kiq->ring_lock, flags);
> -
> - /* poll the trailing fence */
> - for (i = 0; i < adev->usec_timeout; i++) {
> - if (ring->trail_seq ==
> - le32_to_cpu(*(ring->trail_fence_cpu_addr)))
> - break;
> - udelay(1);
> - }
> -
> - if (i >= adev->usec_timeout) {
> - r = -EINVAL;
> - DRM_ERROR("ring %d failed to preempt ib\n", ring->idx);
> - }
> -
> - /* deassert preemption condition */
> - amdgpu_ring_set_preempt_cond_exec(ring, true);
> - return r;
> -}
> -
> static void gfx_v12_0_ring_emit_rreg(struct amdgpu_ring *ring, uint32_t reg,
> uint32_t reg_val_offs)
> {
> @@ -5539,7 +5489,7 @@ static const struct amdgpu_ring_funcs gfx_v12_0_ring_funcs_gfx = {
> .pad_ib = amdgpu_ring_generic_pad_ib,
> .emit_cntxcntl = gfx_v12_0_ring_emit_cntxcntl,
> .init_cond_exec = gfx_v12_0_ring_emit_init_cond_exec,
> - .preempt_ib = gfx_v12_0_ring_preempt_ib,
> + .preempt_ib = amdgpu_gfx_ring_preempt_ib,
> .emit_wreg = gfx_v12_0_ring_emit_wreg,
> .emit_reg_wait = gfx_v12_0_ring_emit_reg_wait,
> .emit_reg_write_reg_wait = gfx_v12_0_ring_emit_reg_write_reg_wait,
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/amdgpu: deduplicate ring preempt ib function
2026-04-22 6:58 ` Christian König
@ 2026-04-22 12:55 ` Alex Deucher
2026-04-22 21:52 ` Claude review: " Claude Code Review Bot
2026-04-22 21:52 ` Claude Code Review Bot
2 siblings, 0 replies; 5+ messages in thread
From: Alex Deucher @ 2026-04-22 12:55 UTC (permalink / raw)
To: Christian König
Cc: Leonardo Cesar, alexander.deucher, airlied, simona, amd-gfx,
dri-devel
On Wed, Apr 22, 2026 at 3:09 AM Christian König
<christian.koenig@amd.com> wrote:
>
> On 4/21/26 22:03, Leonardo Cesar wrote:
> > The ring preemption function is identical for both gfx_v11_0 and
> > gfx_v12_0. This patch refactors the code by moving the core logic
> > into a generic function inside amdgpu_gfx.c to reduce code
> > duplication and simplify future maintenance.
>
> Yeah that one looks reasonable. As far as I can see there isn't anything HW generation specific in the function.
>
> Question is rather why we have that for gfx12 in the first place. @Alex IIRC we support preemption only for a very narrow use case on gfx11, could that just be accidentially be copied over?
>
Yeah, probably just copy and pasted over when we brought up gfx12.
Alex
> >
> > Signed-off-by: Leonardo Cesar <leonardocesar@usp.br>
> >
> > ---
> > v1 -> v2:
> > - Removed wrapper functions for gfx_v11 and gfx_v12 and updated call sites directly.
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | ...
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 51 ++++++++++++++++++++++++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 2 +
> > drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c | 52 +------------------------
> > drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c | 52 +------------------------
> > 4 files changed, 55 insertions(+), 102 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > index 2956e45c9..a157cbd8e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > @@ -2684,3 +2684,54 @@ void amdgpu_debugfs_compute_sched_mask_init(struct amdgpu_device *adev)
> > #endif
> > }
> >
> > +int amdgpu_gfx_ring_preempt_ib(struct amdgpu_ring *ring)
> > +{
> > + int i, r = 0;
>
> Just a style nit: Variables like "i" or "r" last please and don't initialize vairables like "r" while defining it.
>
> Regards,
> Christian.
>
> > + struct amdgpu_device *adev = ring->adev;
> > + struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
> > + struct amdgpu_ring *kiq_ring = &kiq->ring;
> > + unsigned long flags;
> > +
> > + if (adev->enable_mes)
> > + return -EINVAL;
> > +
> > + if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
> > + return -EINVAL;
> > +
> > + spin_lock_irqsave(&kiq->ring_lock, flags);
> > +
> > + if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size)) {
> > + spin_unlock_irqrestore(&kiq->ring_lock, flags);
> > + return -ENOMEM;
> > + }
> > +
> > + /* assert preemption condition */
> > + amdgpu_ring_set_preempt_cond_exec(ring, false);
> > +
> > + /* assert IB preemption, emit the trailing fence */
> > + kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
> > + ring->trail_fence_gpu_addr,
> > + ++ring->trail_seq);
> > + amdgpu_ring_commit(kiq_ring);
> > +
> > + spin_unlock_irqrestore(&kiq->ring_lock, flags);
> > +
> > + /* poll the trailing fence */
> > + for (i = 0; i < adev->usec_timeout; i++) {
> > + if (ring->trail_seq ==
> > + le32_to_cpu(*(ring->trail_fence_cpu_addr)))
> > + break;
> > + udelay(1);
> > + }
> > +
> > + if (i >= adev->usec_timeout) {
> > + r = -EINVAL;
> > + DRM_ERROR("ring %d failed to preempt ib\n", ring->idx);
> > + }
> > +
> > + /* deassert preemption condition */
> > + amdgpu_ring_set_preempt_cond_exec(ring, true);
> > + return r;
> > +}
> > +
> > +
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > index a0cf0a3b4..77050f988 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > @@ -664,6 +664,8 @@ void amdgpu_gfx_csb_preamble_end(u32 *buffer, u32 count);
> > void amdgpu_debugfs_gfx_sched_mask_init(struct amdgpu_device *adev);
> > void amdgpu_debugfs_compute_sched_mask_init(struct amdgpu_device *adev);
> >
> > +int amdgpu_gfx_ring_preempt_ib(struct amdgpu_ring *ring);
> > +
> > static inline const char *amdgpu_gfx_compute_mode_desc(int mode)
> > {
> > switch (mode) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> > index 5097de940..1ba848bfa 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> > @@ -6206,56 +6206,6 @@ static void gfx_v11_0_ring_emit_gfx_shadow(struct amdgpu_ring *ring,
> > ring->set_q_mode_offs = offs;
> > }
> >
> > -static int gfx_v11_0_ring_preempt_ib(struct amdgpu_ring *ring)
> > -{
> > - int i, r = 0;
> > - struct amdgpu_device *adev = ring->adev;
> > - struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
> > - struct amdgpu_ring *kiq_ring = &kiq->ring;
> > - unsigned long flags;
> > -
> > - if (adev->enable_mes)
> > - return -EINVAL;
> > -
> > - if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
> > - return -EINVAL;
> > -
> > - spin_lock_irqsave(&kiq->ring_lock, flags);
> > -
> > - if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size)) {
> > - spin_unlock_irqrestore(&kiq->ring_lock, flags);
> > - return -ENOMEM;
> > - }
> > -
> > - /* assert preemption condition */
> > - amdgpu_ring_set_preempt_cond_exec(ring, false);
> > -
> > - /* assert IB preemption, emit the trailing fence */
> > - kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
> > - ring->trail_fence_gpu_addr,
> > - ++ring->trail_seq);
> > - amdgpu_ring_commit(kiq_ring);
> > -
> > - spin_unlock_irqrestore(&kiq->ring_lock, flags);
> > -
> > - /* poll the trailing fence */
> > - for (i = 0; i < adev->usec_timeout; i++) {
> > - if (ring->trail_seq ==
> > - le32_to_cpu(*(ring->trail_fence_cpu_addr)))
> > - break;
> > - udelay(1);
> > - }
> > -
> > - if (i >= adev->usec_timeout) {
> > - r = -EINVAL;
> > - DRM_ERROR("ring %d failed to preempt ib\n", ring->idx);
> > - }
> > -
> > - /* deassert preemption condition */
> > - amdgpu_ring_set_preempt_cond_exec(ring, true);
> > - return r;
> > -}
> > -
> > static void gfx_v11_0_ring_emit_de_meta(struct amdgpu_ring *ring, bool resume)
> > {
> > struct amdgpu_device *adev = ring->adev;
> > @@ -7295,7 +7245,7 @@ static const struct amdgpu_ring_funcs gfx_v11_0_ring_funcs_gfx = {
> > .emit_cntxcntl = gfx_v11_0_ring_emit_cntxcntl,
> > .emit_gfx_shadow = gfx_v11_0_ring_emit_gfx_shadow,
> > .init_cond_exec = gfx_v11_0_ring_emit_init_cond_exec,
> > - .preempt_ib = gfx_v11_0_ring_preempt_ib,
> > + .preempt_ib = amdgpu_gfx_ring_preempt_ib,
> > .emit_frame_cntl = gfx_v11_0_ring_emit_frame_cntl,
> > .emit_wreg = gfx_v11_0_ring_emit_wreg,
> > .emit_reg_wait = gfx_v11_0_ring_emit_reg_wait,
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> > index 65c33823a..6cf244349 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> > @@ -4611,56 +4611,6 @@ static unsigned gfx_v12_0_ring_emit_init_cond_exec(struct amdgpu_ring *ring,
> > return ret;
> > }
> >
> > -static int gfx_v12_0_ring_preempt_ib(struct amdgpu_ring *ring)
> > -{
> > - int i, r = 0;
> > - struct amdgpu_device *adev = ring->adev;
> > - struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
> > - struct amdgpu_ring *kiq_ring = &kiq->ring;
> > - unsigned long flags;
> > -
> > - if (adev->enable_mes)
> > - return -EINVAL;
> > -
> > - if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
> > - return -EINVAL;
> > -
> > - spin_lock_irqsave(&kiq->ring_lock, flags);
> > -
> > - if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size)) {
> > - spin_unlock_irqrestore(&kiq->ring_lock, flags);
> > - return -ENOMEM;
> > - }
> > -
> > - /* assert preemption condition */
> > - amdgpu_ring_set_preempt_cond_exec(ring, false);
> > -
> > - /* assert IB preemption, emit the trailing fence */
> > - kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
> > - ring->trail_fence_gpu_addr,
> > - ++ring->trail_seq);
> > - amdgpu_ring_commit(kiq_ring);
> > -
> > - spin_unlock_irqrestore(&kiq->ring_lock, flags);
> > -
> > - /* poll the trailing fence */
> > - for (i = 0; i < adev->usec_timeout; i++) {
> > - if (ring->trail_seq ==
> > - le32_to_cpu(*(ring->trail_fence_cpu_addr)))
> > - break;
> > - udelay(1);
> > - }
> > -
> > - if (i >= adev->usec_timeout) {
> > - r = -EINVAL;
> > - DRM_ERROR("ring %d failed to preempt ib\n", ring->idx);
> > - }
> > -
> > - /* deassert preemption condition */
> > - amdgpu_ring_set_preempt_cond_exec(ring, true);
> > - return r;
> > -}
> > -
> > static void gfx_v12_0_ring_emit_rreg(struct amdgpu_ring *ring, uint32_t reg,
> > uint32_t reg_val_offs)
> > {
> > @@ -5539,7 +5489,7 @@ static const struct amdgpu_ring_funcs gfx_v12_0_ring_funcs_gfx = {
> > .pad_ib = amdgpu_ring_generic_pad_ib,
> > .emit_cntxcntl = gfx_v12_0_ring_emit_cntxcntl,
> > .init_cond_exec = gfx_v12_0_ring_emit_init_cond_exec,
> > - .preempt_ib = gfx_v12_0_ring_preempt_ib,
> > + .preempt_ib = amdgpu_gfx_ring_preempt_ib,
> > .emit_wreg = gfx_v12_0_ring_emit_wreg,
> > .emit_reg_wait = gfx_v12_0_ring_emit_reg_wait,
> > .emit_reg_write_reg_wait = gfx_v12_0_ring_emit_reg_write_reg_wait,
> > --
> > 2.43.0
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Claude review: Re: [PATCH] drm/amdgpu: deduplicate ring preempt ib function
2026-04-22 6:58 ` Christian König
2026-04-22 12:55 ` Alex Deucher
@ 2026-04-22 21:52 ` Claude Code Review Bot
2026-04-22 21:52 ` Claude Code Review Bot
2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 21:52 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Correctness: Good.** The extracted function in `amdgpu_gfx.c` is an exact copy of the removed functions from both `gfx_v11_0.c` and `gfx_v12_0.c`. The function signature matches the `preempt_ib` callback in `struct amdgpu_ring_funcs` (`int (*preempt_ib)(struct amdgpu_ring *ring)`), so assigning it directly to `.preempt_ib` in the ring_funcs structs is correct.
**Nit — trailing blank line:** The new function in `amdgpu_gfx.c` is followed by two blank lines before the end of the diff:
```c
+ return r;
+}
+
+
```
Kernel coding style uses a single blank line between functions (or at end-of-file). Remove the extra blank line.
**Nit — alignment inconsistency:** In the new shared function, the `kiq_unmap_queues` call arguments use tab+tabs alignment:
```c
+ kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
+ ring->trail_fence_gpu_addr,
+ ++ring->trail_seq);
```
The original code in both `gfx_v11_0.c` and `gfx_v12_0.c` aligned continuation lines to the opening parenthesis using tabs+spaces:
```c
kiq->pmf->kiq_unmap_queues(kiq_ring, ring, PREEMPT_QUEUES_NO_UNMAP,
ring->trail_fence_gpu_addr,
++ring->trail_seq);
```
The original alignment is the more common kernel style for this codebase. Since the goal is to move the code without modification, please preserve the original alignment.
**Missed opportunity — gfx_v10_0:** The `gfx_v10_0_ring_preempt_ib()` function at `gfx_v10_0.c:8855` is functionally identical to the new shared version, with only one difference: it lacks the `adev->enable_mes` early-return check. Since MES is not supported on GFX10 hardware, the `enable_mes` guard would be harmlessly false on v10 and the shared function could be used there as well. This would eliminate a third copy of the same logic. Worth considering for this patch or a follow-up.
**Not in scope but worth noting — gfx_v9_0:** The `gfx_v9_0` version uses a different fence emission mechanism (direct `gfx_v9_0_ring_emit_fence()` calls instead of `kiq_unmap_queues`), so it cannot be consolidated with this shared function. No action needed.
**Overall:** The patch does what it says on the tin and the logic is correct. Fix the trailing blank line and the alignment nit, and consider also converting `gfx_v10_0` to use the shared function since it's the same code.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 5+ messages in thread
* Claude review: Re: [PATCH] drm/amdgpu: deduplicate ring preempt ib function
2026-04-22 6:58 ` Christian König
2026-04-22 12:55 ` Alex Deucher
2026-04-22 21:52 ` Claude review: " Claude Code Review Bot
@ 2026-04-22 21:52 ` Claude Code Review Bot
2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 21:52 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: Re: [PATCH] drm/amdgpu: deduplicate ring preempt ib function
Author: =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>
Patches: 3
Reviewed: 2026-04-23T07:52:56.153629
---
This is a straightforward and correct deduplication patch. The `ring_preempt_ib` function bodies in `gfx_v11_0.c` and `gfx_v12_0.c` are byte-for-byte identical, so extracting them into a shared `amdgpu_gfx_ring_preempt_ib()` in `amdgpu_gfx.c` is the right thing to do. The patch is clean, the logic is preserved faithfully, and the call sites are updated correctly.
There are a few minor style issues and one missed deduplication opportunity worth raising.
**Verdict: Looks good with minor nits.**
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-04-22 21:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-21 20:03 [PATCH] drm/amdgpu: deduplicate ring preempt ib function Leonardo Cesar
2026-04-22 6:58 ` Christian König
2026-04-22 12:55 ` Alex Deucher
2026-04-22 21:52 ` Claude review: " Claude Code Review Bot
2026-04-22 21:52 ` Claude Code Review Bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox