[AMD Official Use Only]
> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
> Sent: February 9, 2022 4:26 PM
> To: Kim, Jonathan <Jonathan.Kim@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drm/amdkfd: map sdma queues onto extended engines for
> navi2x
>
>
> On 2022-02-09 11:11, Jonathan Kim wrote:
> > The hardware scheduler requires that all SDMA 5.2.x queues are put on
> > the RUN_LIST through the extended engines.
> >
> > Make extended engine unmap available as well.
> >
> > Signed-off-by: Jonathan Kim <jonathan.kim@xxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 2 +-
> > drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c | 5 +++--
> > drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c | 8 +++++---
> > drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_vi.c | 3 ++-
> > drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 5 +++--
> > 5 files changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > index 7f6f1a842b0b..f12e32335eb3 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > @@ -1555,7 +1555,7 @@ static int unmap_queues_cpsch(struct
> device_queue_manager *dqm,
> > return retval;
> >
> > retval = pm_send_unmap_queue(&dqm->packet_mgr,
> KFD_QUEUE_TYPE_COMPUTE,
> > - filter, filter_param, reset, 0);
> > + filter, filter_param, reset, 0, false);
>
> Does this still work correctly? We currently rely on HWS unmapping SDMA
> queues when we request unmapping of compute queues. Is that still the case
> with extended queue selection in map_queues?
I wasn't aware of the implicit sdma unmap ...
That makes much more sense.
I followed up on the FW spec and apparently as long as extended_engine_select=0x1 (sdma0_sdma7),
a single call to unmap all queues or all dynamic queues will unmap both compute
queues mapped in legacy mode and sdma queues mapped in extended engine mode.
>
> How would the caller know to set this to "true"? For mapping, this detail is
> hidden in the packet-manager implementation. But for unmapping the caller
> needs to know? That doesn't make sense. But we could probably remove the
> SDMA filtering functionality from pm_send_unmap_queue completely. I don't
> see any calls where we try to unmap specific SDMA queues. Since we always
> have to replace the entire runlist anyway, there is not use case for it.
Agreed.
Aside from removing SDMA checks, maybe also pass the device itself through to pm_send_unmap_queue then?
Or could it be the SDMA ip version?
That way we can hide the check to toggle between extended_engine_select = 0x0 or 0x1 from the caller.
Thanks,
Jon
>
> Regards,
> Felix
>
>
> > if (retval)
> > return retval;
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
> > index 1439420925a0..8694cfcd57d1 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
> > @@ -371,7 +371,8 @@ int pm_send_query_status(struct packet_manager
> *pm, uint64_t fence_address,
> > int pm_send_unmap_queue(struct packet_manager *pm, enum
> kfd_queue_type type,
> > enum kfd_unmap_queues_filter filter,
> > uint32_t filter_param, bool reset,
> > - unsigned int sdma_engine)
> > + unsigned int sdma_engine,
> > + bool is_sdma_ext)
> > {
> > uint32_t *buffer, size;
> > int retval = 0;
> > @@ -387,7 +388,7 @@ int pm_send_unmap_queue(struct packet_manager
> *pm, enum kfd_queue_type type,
> > }
> >
> > retval = pm->pmf->unmap_queues(pm, buffer, type, filter, filter_param,
> > - reset, sdma_engine);
> > + reset, sdma_engine, is_sdma_ext);
> > if (!retval)
> > kq_submit_packet(pm->priv_queue);
> > else
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> > index 7ea3f671b325..08f736080b7e 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> > @@ -183,6 +183,7 @@ static int pm_map_queues_v9(struct packet_manager
> *pm, uint32_t *buffer,
> > {
> > struct pm4_mes_map_queues *packet;
> > bool use_static = is_static;
> > + bool is_sdma_ext = q->device->adev->ip_versions[SDMA0_HWIP][0] >=
> > +IP_VERSION(5, 2, 0);
> >
> > packet = (struct pm4_mes_map_queues *)buffer;
> > memset(buffer, 0, sizeof(struct pm4_mes_map_queues)); @@ -214,7
> > +215,7 @@ static int pm_map_queues_v9(struct packet_manager *pm,
> uint32_t *buffer,
> > case KFD_QUEUE_TYPE_SDMA:
> > case KFD_QUEUE_TYPE_SDMA_XGMI:
> > use_static = false; /* no static queues under SDMA */
> > - if (q->properties.sdma_engine_id < 2)
> > + if (q->properties.sdma_engine_id < 2 && !is_sdma_ext)
> > packet->bitfields2.engine_sel = q-
> >properties.sdma_engine_id +
> > engine_sel__mes_map_queues__sdma0_vi;
> > else {
> > @@ -249,7 +250,8 @@ static int pm_unmap_queues_v9(struct
> packet_manager *pm, uint32_t *buffer,
> > enum kfd_queue_type type,
> > enum kfd_unmap_queues_filter filter,
> > uint32_t filter_param, bool reset,
> > - unsigned int sdma_engine)
> > + unsigned int sdma_engine,
> > + bool is_sdma_ext)
> > {
> > struct pm4_mes_unmap_queues *packet;
> >
> > @@ -268,7 +270,7 @@ static int pm_unmap_queues_v9(struct
> packet_manager *pm, uint32_t *buffer,
> > break;
> > case KFD_QUEUE_TYPE_SDMA:
> > case KFD_QUEUE_TYPE_SDMA_XGMI:
> > - if (sdma_engine < 2) {
> > + if (sdma_engine < 2 && !is_sdma_ext) {
> > packet->bitfields2.extended_engine_sel =
> >
> extended_engine_sel__mes_unmap_queues__legacy_engine_sel;
> > packet->bitfields2.engine_sel =
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_vi.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_vi.c
> > index 3c0658e32e93..a83aa94972e7 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_vi.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_vi.c
> > @@ -200,7 +200,8 @@ static int pm_unmap_queues_vi(struct
> packet_manager *pm, uint32_t *buffer,
> > enum kfd_queue_type type,
> > enum kfd_unmap_queues_filter filter,
> > uint32_t filter_param, bool reset,
> > - unsigned int sdma_engine)
> > + unsigned int sdma_engine,
> > + bool is_sdma_ext)
> > {
> > struct pm4_mes_unmap_queues *packet;
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > index b6790a637f5c..b157ba0216f0 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > @@ -1249,7 +1249,8 @@ struct packet_manager_funcs {
> > enum kfd_queue_type type,
> > enum kfd_unmap_queues_filter mode,
> > uint32_t filter_param, bool reset,
> > - unsigned int sdma_engine);
> > + unsigned int sdma_engine,
> > + bool is_sdma_ext);
> > int (*query_status)(struct packet_manager *pm, uint32_t *buffer,
> > uint64_t fence_address, uint64_t fence_value);
> > int (*release_mem)(uint64_t gpu_addr, uint32_t *buffer); @@ -1279,7
> > +1280,7 @@ int pm_send_query_status(struct packet_manager *pm, uint64_t
> fence_address,
> > int pm_send_unmap_queue(struct packet_manager *pm, enum
> kfd_queue_type type,
> > enum kfd_unmap_queues_filter mode,
> > uint32_t filter_param, bool reset,
> > - unsigned int sdma_engine);
> > + unsigned int sdma_engine, bool is_sdma_ext);
> >
> > void pm_release_ib(struct packet_manager *pm);
> >
