[PATCH] drm/amdgpu: deduplicate ring preempt ib function

[PATCH] drm/amdgpu: deduplicate ring preempt ib function

[Leonardo Cesar]

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

Re: [PATCH] drm/amdgpu: deduplicate ring preempt ib function

[Christian König]

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
> 

Re: [PATCH] drm/amdgpu: deduplicate ring preempt ib function

[Alex Deucher]

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

Claude review: Re: [PATCH] drm/amdgpu: deduplicate ring preempt ib function

[Claude Code Review Bot]

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

Claude review: Re: [PATCH] drm/amdgpu: deduplicate ring preempt ib function

[Claude Code Review Bot]

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