Re: [PATCH 2/9] drm/amdgpu/gfx7: Refactor MQD initialization and finalization

On Wed, Jul 15, 2026 at 5:30 PM Timur Kristóf <[email protected]> wrote:
>
> On 2026. július 15., szerda 23:10:02 közép-európai nyári idő Alex Deucher
> wrote:
> > On Wed, Jul 15, 2026 at 4:32 PM Timur Kristóf <[email protected]>
> wrote:
> > > On 2026. július 15., szerda 21:50:53 közép-európai nyári idő Alex Deucher
> > >
> > > wrote:
> > > > On Wed, Jul 15, 2026 at 11:54 AM Timur Kristóf <[email protected]>
> > >
> > > wrote:
> > > > > On 2026. július 14., kedd 20:47:14 közép-európai nyári idő Tvrtko
> > > > > Ursulin
> > > > >
> > > > > wrote:
> > > > > > On 13/07/2026 13:58, Timur Kristóf wrote:
> > > > > > > Call amdgpu_gfx_mqd_sw_init()/_fini() on GFX7 to initialize and
> > > > > > > finalize the MQD, just like GFX8 and newer; instead of doing
> > > > > > > an ad-hoc BO allocation. This introduces the possibility of
> > > > > > > doing an MQD backup instead of trying to reinitialize the
> > > > > > > MQD every time.
> > > > > > >
> > > > > > > This solves an issue with GFX IP block soft reset where
> > > > > > > all compute rings would hang after the reset.
> > > > > > >
> > > > > > > Signed-off-by: Timur Kristóf <[email protected]>
> > > > > > > ---
> > > > > > >
> > > > > > >   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 106
> > > > > > >   +++++++++++++-------------
> > > > > > >   1 file changed, 51 insertions(+), 55 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > > > > > b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c index
> > > > > > > 65b8497ad5f0..9c4b3ac27e1f
> > > > > > > 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > > > > > @@ -2698,25 +2698,6 @@ static int
> > > > > > > gfx_v7_0_cp_compute_load_microcode(struct amdgpu_device *adev)>
> > > > > > >
> > > > > > >     return 0;
> > > > > > >
> > > > > > >   }
> > > > > > >
> > > > > > > -/**
> > > > > > > - * gfx_v7_0_cp_compute_fini - stop the compute queues
> > > > > > > - *
> > > > > > > - * @adev: amdgpu_device pointer
> > > > > > > - *
> > > > > > > - * Stop the compute queues and tear down the driver queue
> > > > > > > - * info.
> > > > > > > - */
> > > > > > > -static void gfx_v7_0_cp_compute_fini(struct amdgpu_device *adev)
> > > > > > > -{
> > > > > > > -   int i;
> > > > > > > -
> > > > > > > -   for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> > > > > > > -           struct amdgpu_ring *ring = &adev->gfx.compute_ring[i];
> > > > > > > -
> > > > > > > -           amdgpu_bo_free_kernel(&ring->mqd_obj, NULL, NULL);
> > > > > > > -   }
> > > > > > > -}
> > > > > > > -
> > > > > > >
> > > > > > >   static void gfx_v7_0_mec_fini(struct amdgpu_device *adev)
> > > > > > >   {
> > > > > > >
> > > > > > >     amdgpu_bo_free_kernel(&adev->gfx.mec.hpd_eop_obj, NULL, NULL);
> > > > > > >
> > > > > > > @@ -2788,28 +2769,29 @@ static void
> > > > > > > gfx_v7_0_compute_pipe_init(struct
> > > > > > > amdgpu_device *adev,>
> > > > > > >
> > > > > > >     mutex_unlock(&adev->srbm_mutex);
> > > > > > >
> > > > > > >   }
> > > > > > >
> > > > > > > -static int gfx_v7_0_mqd_deactivate(struct amdgpu_device *adev)
> > > > > > > +static int gfx_v7_0_mqd_deactivate(struct amdgpu_device *adev,
> > > > > > > u32
> > > > > > > req)
> > > > > > >
> > > > > > >   {
> > > > > > >
> > > > > > > -   int i;
> > > > > > > +   int i, r = 0;
> > > > > > >
> > > > > > >     /* disable the queue if it's active */
> > > > > > >
> > > > > > > -   if (RREG32(mmCP_HQD_ACTIVE) & 1) {
> > > > > > > -           WREG32(mmCP_HQD_DEQUEUE_REQUEST, 1);
> > > > > > > +   if (RREG32(mmCP_HQD_ACTIVE) & CP_HQD_ACTIVE__ACTIVE_MASK) {
> > > > > > > +           WREG32_FIELD(CP_HQD_DEQUEUE_REQUEST, DEQUEUE_REQ,
> > > > > > > req);
> > > > > > >
> > > > > > >             for (i = 0; i < adev->usec_timeout; i++) {
> > > > > > >
> > > > > > > -                   if (!(RREG32(mmCP_HQD_ACTIVE) & 1))
> > > > > > > +                   if (!(RREG32(mmCP_HQD_ACTIVE) &
> > > > >
> > > > > CP_HQD_ACTIVE__ACTIVE_MASK))
> > > > >
> > > > > > >                             break;
> > > > > > >
> > > > > > >                     udelay(1);
> > > > > > >
> > > > > > >             }
> > > > > > >
> > > > > > >             if (i == adev->usec_timeout)
> > > > > > >
> > > > > > > -                   return -ETIMEDOUT;
> > > > > > > +                   r = -ETIMEDOUT;
> > > > > > >
> > > > > > > -           WREG32(mmCP_HQD_DEQUEUE_REQUEST, 0);
> > > > > > > -           WREG32(mmCP_HQD_PQ_RPTR, 0);
> > > > > > > -           WREG32(mmCP_HQD_PQ_WPTR, 0);
> > > > > > >
> > > > > > >     }
> > > > > > >
> > > > > > > -   return 0;
> > > > > > > +   WREG32(mmCP_HQD_DEQUEUE_REQUEST, 0);
> > > > > > > +   WREG32(mmCP_HQD_PQ_RPTR, 0);
> > > > > > > +   WREG32(mmCP_HQD_PQ_WPTR, 0);
> > > > > > > +
> > > > > > > +   return r;
> > > > > >
> > > > > > I can see this matches gfx_v8_0_deactivate_hqd. If I am not missing
> > > > > > anything only to replace the hardcoded 1 with
> > > > > > CP_HQD_ACTIVE__ACTIVE_MASK?
> > > > >
> > > > > There are two changes here:
> > > > > - Replacing the hardcoded "1" with the define from the register
> > > > > definition
> > > > > - When it times out, still write the CP_HQD_ registers afterwards like
> > > > > gfx8
> > > > >
> > > > > > Is it okay to call the function mqd if the registers are hqd and is
> > > > > > v7
> > > > > > or v8 (which calls it hqd) more correct? Not saying either way, just
> > > > > > observing a curiosity.
> > > > >
> > > > > My best guess is that it's just that they used a different naming
> > > > > convention and forgot to update the older code.
> > > >
> > > > Technically, the MQD (Memory Queue Descriptor) and HQD (Hardware Queue
> > > > Descriptor) are separate things.  The MQD is a structure stored in
> > > > memory which stores the state of a queue.  The HQD is the hardware
> > > > state which stores the queue when it's mapped to hardware.  The way
> > > > it's supposed to work is that the driver sets up MQDs in memory and
> > > > then asks KIQ to map those queues to an HQD or put them on a runlist
> > > > (for user queues) if there are more MQDs than HQDs.  For gfx8 and
> > > > newer the driver sets up the MQD and then uses the KIQ to map the
> > > > queues.  gfx7 could work that way as well, but we never got around to
> > > > porting the changes back to gfx7.  Instead the driver maps the queues
> > > > to the HQD directly via MMIO rather than having KIQ do it on gfx7.
> > > >
> > > > Alex
> > >
> > > As far as I understood, Tvrtko's point was why the function is called
> > > gfx_v7_0_mqd_deactivate() when what it does is the same as
> > > gfx_v8_0_deactivate_hqd(), ie. why does one have MQD in the name while the
> > > other has HQD in the name.
> >
> > I don't remember why it's named like that, but deactivate_hqd() is
> > better aligned with what it actually does.  This is effectively the
> > MMIO equivalent of submitting an unmap queue packet to the KIQ.
> >
> > Alex
>
> Would it be OK if I renamed it?
> gfx_v7_0_mqd_deactivate() --> gfx_v7_0_deactivate_hqd()
> to better align it with gfx8 and ease the confusion.

yeah, that would be better.

Alex

>
>
> >
> > > > > > >   }
> > > > > > >
> > > > > > >   static void gfx_v7_0_mqd_init(struct amdgpu_device *adev,
> > > > > > >
> > > > > > > @@ -2964,31 +2946,42 @@ static int gfx_v7_0_mqd_commit(struct
> > > > > > > amdgpu_device *adev, struct cik_mqd *mqd)>
> > > > > > >
> > > > > > >   static int gfx_v7_0_compute_queue_init(struct amdgpu_device
> > > > > > >   *adev,
> > > > > > >   int
> > > > > > >   ring_id) {
> > > > > > >
> > > > > > > -   int r;
> > > > > > > -   u64 mqd_gpu_addr;
> > > > > > > -   struct cik_mqd *mqd;
> > > > > > >
> > > > > > >     struct amdgpu_ring *ring = &adev->gfx.compute_ring[ring_id];
> > > > > > >
> > > > > > > -
> > > > > > > -   r = amdgpu_bo_create_reserved(adev, sizeof(struct cik_mqd),
> > > > >
> > > > > PAGE_SIZE,
> > > > >
> > > > > > > -                                 AMDGPU_GEM_DOMAIN_GTT,
> > > > >
> > > > > &ring->mqd_obj,
> > > > >
> > > > > > > -                                 &mqd_gpu_addr, (void
> > > > >
> > > > > **)&mqd);
> > > > >
> > > > > > > -   if (r) {
> > > > > > > -           dev_warn(adev->dev, "(%d) create MQD bo failed\n", r);
> > > > > > > -           return r;
> > > > > > > +   struct cik_mqd *mqd = ring->mqd_ptr;
> > > > > > > +   int mqd_idx = ring - &adev->gfx.compute_ring[0];
> > > > > > > +
> > > > > > > +   if (!amdgpu_in_reset(adev) && !adev->in_suspend) {
> > > > > > > +           memset((void *)mqd, 0, ring->mqd_size);
> > > > > > > +           mutex_lock(&adev->srbm_mutex);
> > > > > > > +           cik_srbm_select(adev, ring->me, ring->pipe, ring-
> > > > > >
> > > > > >queue, 0);
> > > > > >
> > > > > > > +           gfx_v7_0_mqd_init(adev, mqd, ring->mqd_gpu_addr,
> > > > > > > ring);
> > > > > > > +           gfx_v7_0_mqd_deactivate(adev, 1);
> > > > > > > +           gfx_v7_0_mqd_commit(adev, mqd);
> > > > > > > +           cik_srbm_select(adev, 0, 0, 0, 0);
> > > > > > > +           mutex_unlock(&adev->srbm_mutex);
> > > > > > > +
> > > > > > > +           if (adev->gfx.mec.mqd_backup[mqd_idx])
> > > > > > > +                   memcpy(adev->gfx.mec.mqd_backup[mqd_idx],
> > > > >
> > > > > mqd, ring->mqd_size);
> > > > >
> > > > > > > +   } else {
> > > > > > > +           /* restore MQD to a clean status */
> > > > > > > +           if (adev->gfx.mec.mqd_backup[mqd_idx])
> > > > > > > +                   memcpy(mqd, adev-
> > > > > >
> > > > > >gfx.mec.mqd_backup[mqd_idx], ring->mqd_size);
> > > > > >
> > > > > > > +
> > > > > > > +           /* Re-commit the restored backup */
> > > > > > > +           mutex_lock(&adev->srbm_mutex);
> > > > > > > +           cik_srbm_select(adev, ring->me, ring->pipe, ring-
> > > > > >
> > > > > >queue, 0);
> > > > > >
> > > > > > > +           gfx_v7_0_mqd_deactivate(adev, 2);
> > > > > > > +           gfx_v7_0_mqd_commit(adev, mqd);
> > > > > > > +           cik_srbm_select(adev, 0, 0, 0, 0);
> > > > > > > +           mutex_unlock(&adev->srbm_mutex);
> > > > > > > +
> > > > > > > +           /* reset ring buffer */
> > > > > > > +           ring->wptr = 0;
> > > > > > > +           atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
> > > > > > > +           atomic64_set((atomic64_t *)ring->rptr_cpu_addr, 0);
> > > > > > > +           amdgpu_ring_clear_ring(ring);
> > > > > > >
> > > > > > >     }
> > > > > > >
> > > > > > > -   mutex_lock(&adev->srbm_mutex);
> > > > > > > -   cik_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
> > > > > > > -
> > > > > > > -   gfx_v7_0_mqd_init(adev, mqd, mqd_gpu_addr, ring);
> > > > > > > -   gfx_v7_0_mqd_deactivate(adev);
> > > > > > > -   gfx_v7_0_mqd_commit(adev, mqd);
> > > > > > > -
> > > > > > > -   cik_srbm_select(adev, 0, 0, 0, 0);
> > > > > > > -   mutex_unlock(&adev->srbm_mutex);
> > > > > > > -
> > > > > > > -   amdgpu_bo_kunmap(ring->mqd_obj);
> > > > > > > -   amdgpu_bo_unreserve(ring->mqd_obj);
> > > > > > >
> > > > > > >     return 0;
> > > > > > >
> > > > > > >   }
> > > > > >
> > > > > > I think I can follow this - only the wptr and rptr reset is a bit
> > > > > > different than what v8 does it. Any specific reason? Gfx9 then
> > > > > > reverts
> > > > > > back to a single ring->wptr = 0. I guess v8 is somehow special?
> > > > > >
> > > > > > > @@ -3020,10 +3013,8 @@ static int
> > > > > > > gfx_v7_0_cp_compute_resume(struct
> > > > > > > amdgpu_device *adev)>
> > > > > > >
> > > > > > >     /* init the queues */
> > > > > > >     for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> > > > > > >
> > > > > > >             r = gfx_v7_0_compute_queue_init(adev, i);
> > > > > > >
> > > > > > > -           if (r) {
> > > > > > > -                   gfx_v7_0_cp_compute_fini(adev);
> > > > > > > +           if (r)
> > > > > > >
> > > > > > >                     return r;
> > > > > > >
> > > > > > > -           }
> > > > > > >
> > > > > > >     }
> > > > > > >
> > > > > > >     gfx_v7_0_cp_compute_enable(adev, true);
> > > > > > >
> > > > > > > @@ -4430,6 +4421,11 @@ static int gfx_v7_0_sw_init(struct
> > > > > > > amdgpu_ip_block
> > > > > > > *ip_block)>
> > > > > > >
> > > > > > >             }
> > > > > > >
> > > > > > >     }
> > > > > > >
> > > > > > > +   /* create MQD for all compute queues */
> > > > > > > +   r = amdgpu_gfx_mqd_sw_init(adev, sizeof(struct cik_mqd), 0);
> > > > > > > +   if (r)
> > > > > > > +           return r;
> > > > > > > +
> > > > > > >
> > > > > > >     adev->gfx.ce_ram_size = 0x8000;
> > > > > > >
> > > > > > >     gfx_v7_0_gpu_early_init(adev);
> > > > > > >
> > > > > > > @@ -4452,7 +4448,7 @@ static int gfx_v7_0_sw_fini(struct
> > > > > > > amdgpu_ip_block
> > > > > > > *ip_block)>
> > > > > > >
> > > > > > >     for (i = 0; i < adev->gfx.num_compute_rings; i++)
> > > > > > >
> > > > > > >             amdgpu_ring_fini(&adev->gfx.compute_ring[i]);
> > > > > > >
> > > > > > > -   gfx_v7_0_cp_compute_fini(adev);
> > > > > > > +   amdgpu_gfx_mqd_sw_fini(adev, 0);
> > > > > > >
> > > > > > >     amdgpu_gfx_rlc_fini(adev);
> > > > > > >     gfx_v7_0_mec_fini(adev);
> > > > > > >     amdgpu_bo_free_kernel(&adev->gfx.rlc.clear_state_obj,
> > > > > >
> > > > > > I am assuming all this applies only to compute because gfx is single
> > > > > > instance on v7?
> > > > >
> > > > > It applies only to compute because only compute has HQD/MQD
> > > > > on these hardware generations.
> > > > >
> > > > > > Anyway, it looks plausible to me so assuming you were able to
> > > > > > exercise
> > > > > > both paths
> > > > >
> > > > > What do you mean by "both paths"?
> > > > >
> > > > > Thanks,
> > > > > Timur
>
>
>
>