Re: [PATCH] drm/amdkfd: map sdma queues onto extended engines for navi2x

[Felix Kuehling <felix.kuehling@xxxxxxx>]

On 2022-02-09 19:18, Kim, Jonathan wrote:
> [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.

You can already get that from the struct packet_manager *pm. You can get 
the dqm with container_of and get the device with dqm->dev.

Or add a flag for SDMA extended mode in the pm structure itself and set 
it in pm_init.

Regards,
  Felix


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