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

[Alex Deucher <alexdeucher@gmail.com>]

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