Re: [PATCH] drm/amdgpu/gfx11: fix mes mqd settings and map_queue packet

[Alex Deucher <alexdeucher@gmail.com>]

On Tue, May 10, 2022 at 3:30 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Alex, dear Jack,
>
>
> Thank you for the patch.
>
> Am 09.05.22 um 21:06 schrieb Alex Deucher:
> > From: Jack Xiao <Jack.Xiao@amd.com>
> >
> > a. use correct mes mqd settings
>
> Can you please elaborate? What is wrong with the old ones, and what are
> the correct ones?

Use the gfx11 mqd structures rather than the gfx10 structures.

>
> > b. fix me field in map_queue packet
>
> Can you please add some background? The new value is 2. What does it do?

The ME is the microengine.  You need to select the right engine based
on the queue type.

>
> It’d be great, if you could make it two patches.
>
> > Signed-off-by: Jack Xiao <Jack.Xiao@amd.com>
> > Reviewed-by: Hawking Zhang <Hawking.Zhang@amd.com>
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c |  7 +++++--
> >   drivers/gpu/drm/amd/amdgpu/mes_v11_0.c | 20 ++++++++++----------
> >   2 files changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> > index 7614f38ff381..8a1bec70c719 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> > @@ -145,16 +145,19 @@ static void gfx11_kiq_map_queues(struct amdgpu_ring *kiq_ring,
> >   {
> >       uint64_t mqd_addr = amdgpu_bo_gpu_offset(ring->mqd_obj);
> >       uint64_t wptr_addr = ring->wptr_gpu_addr;
> > -     uint32_t eng_sel = 0;
> > +     uint32_t me = 0, eng_sel = 0;
> >
> >       switch (ring->funcs->type) {
> >       case AMDGPU_RING_TYPE_COMPUTE:
> > +             me = 1;
> >               eng_sel = 0;
> >               break;
> >       case AMDGPU_RING_TYPE_GFX:
> > +             me = 0;
> >               eng_sel = 4;
> >               break;
> >       case AMDGPU_RING_TYPE_MES:
> > +             me = 2;
> >               eng_sel = 5;
> >               break;
> >       default:
> > @@ -168,7 +171,7 @@ static void gfx11_kiq_map_queues(struct amdgpu_ring *kiq_ring,
> >                         PACKET3_MAP_QUEUES_VMID(0) | /* VMID */
> >                         PACKET3_MAP_QUEUES_QUEUE(ring->queue) |
> >                         PACKET3_MAP_QUEUES_PIPE(ring->pipe) |
> > -                       PACKET3_MAP_QUEUES_ME((ring->me == 1 ? 0 : 1)) |
> > +                       PACKET3_MAP_QUEUES_ME((me)) |
> >                         PACKET3_MAP_QUEUES_QUEUE_TYPE(0) | /*queue_type: normal compute queue */
> >                         PACKET3_MAP_QUEUES_ALLOC_FORMAT(0) | /* alloc format: all_on_one_pipe */
> >                         PACKET3_MAP_QUEUES_ENGINE_SEL(eng_sel) |
> > diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> > index 5d4d54cabf70..fcf51947bb18 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> > @@ -29,7 +29,7 @@
> >   #include "gc/gc_11_0_0_offset.h"
> >   #include "gc/gc_11_0_0_sh_mask.h"
> >   #include "gc/gc_11_0_0_default.h"
> > -#include "v10_structs.h"
> > +#include "v11_structs.h"
> >   #include "mes_v11_api_def.h"
> >
> >   MODULE_FIRMWARE("amdgpu/gc_11_0_0_mes.bin");
> > @@ -637,7 +637,7 @@ static int mes_v11_0_allocate_eop_buf(struct amdgpu_device *adev,
> >
> >   static int mes_v11_0_mqd_init(struct amdgpu_ring *ring)
> >   {
> > -     struct v10_compute_mqd *mqd = ring->mqd_ptr;
> > +     struct v11_compute_mqd *mqd = ring->mqd_ptr;
> >       uint64_t hqd_gpu_addr, wb_gpu_addr, eop_base_addr;
> >       uint32_t tmp;
> >
> > @@ -724,22 +724,22 @@ static int mes_v11_0_mqd_init(struct amdgpu_ring *ring)
> >       mqd->cp_hqd_vmid = 0;
> >       /* activate the queue */
> >       mqd->cp_hqd_active = 1;
> > -     mqd->cp_hqd_persistent_state = regCP_HQD_PERSISTENT_STATE_DEFAULT;
> > +
> > +     tmp = regCP_HQD_PERSISTENT_STATE_DEFAULT;
> > +     tmp = REG_SET_FIELD(tmp, CP_HQD_PERSISTENT_STATE,
> > +                         PRELOAD_SIZE, 0x55);
> > +     mqd->cp_hqd_persistent_state = tmp;
> > +
> >       mqd->cp_hqd_ib_control = regCP_HQD_IB_CONTROL_DEFAULT;
> >       mqd->cp_hqd_iq_timer = regCP_HQD_IQ_TIMER_DEFAULT;
> >       mqd->cp_hqd_quantum = regCP_HQD_QUANTUM_DEFAULT;
> >
> > -     tmp = regCP_HQD_GFX_CONTROL_DEFAULT;
> > -     tmp = REG_SET_FIELD(tmp, CP_HQD_GFX_CONTROL, DB_UPDATED_MSG_EN, 1);
> > -     /* offset: 184 - this is used for CP_HQD_GFX_CONTROL */
> > -     mqd->cp_hqd_suspend_cntl_stack_offset = tmp;
> > -
>
> What was wrong with this?

It doesn't exist in the gfx11 structure.

Alex

>
>
> Kind regards,
>
> Paul
>
>
> >       return 0;
> >   }
> >
> >   static void mes_v11_0_queue_init_register(struct amdgpu_ring *ring)
> >   {
> > -     struct v10_compute_mqd *mqd = ring->mqd_ptr;
> > +     struct v11_compute_mqd *mqd = ring->mqd_ptr;
> >       struct amdgpu_device *adev = ring->adev;
> >       uint32_t data = 0;
> >
> > @@ -910,7 +910,7 @@ static int mes_v11_0_kiq_ring_init(struct amdgpu_device *adev)
> >   static int mes_v11_0_mqd_sw_init(struct amdgpu_device *adev,
> >                                enum admgpu_mes_pipe pipe)
> >   {
> > -     int r, mqd_size = sizeof(struct v10_compute_mqd);
> > +     int r, mqd_size = sizeof(struct v11_compute_mqd);
> >       struct amdgpu_ring *ring;
> >
> >       if (pipe == AMDGPU_MES_KIQ_PIPE)