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