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

[Christian König <christian.koenig@amd.com>]

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
>