> -----Original Message-----
> From: Sider, Graham <Graham.Sider@xxxxxxx>
> Sent: December 20, 2021 1:19 AM
> To: Kim, Jonathan <Jonathan.Kim@xxxxxxx>; Chen, Guchun
> <Guchun.Chen@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher,
> Alexander <Alexander.Deucher@xxxxxxx>; Kuehling, Felix
> <Felix.Kuehling@xxxxxxx>
> Subject: RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd
> device init (v2)
>
> [Public]
>
> > -----Original Message-----
> > From: Kim, Jonathan <Jonathan.Kim@xxxxxxx>
> > Sent: Monday, December 20, 2021 12:44 AM
> > To: Chen, Guchun <Guchun.Chen@xxxxxxx>; amd-
> > gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander
> > <Alexander.Deucher@xxxxxxx>; Sider, Graham
> <Graham.Sider@xxxxxxx>;
> > Kuehling, Felix <Felix.Kuehling@xxxxxxx>
> > Subject: RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd
> > device init (v2)
> >
> > [AMD Official Use Only]
> >
> > > -----Original Message-----
> > > From: Chen, Guchun <Guchun.Chen@xxxxxxx>
> > > Sent: December 19, 2021 10:09 PM
> > > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander
> > > <Alexander.Deucher@xxxxxxx>; Sider, Graham
> > <Graham.Sider@xxxxxxx>;
> > > Kuehling, Felix <Felix.Kuehling@xxxxxxx>; Kim, Jonathan
> > > <Jonathan.Kim@xxxxxxx>
> > > Cc: Chen, Guchun <Guchun.Chen@xxxxxxx>
> > > Subject: [PATCH] drm/amdkfd: correct sdma queue number in kfd
> device
> > > init (v2)
> > >
> > > sdma queue number is not correct like on vega20, this patch promises
> > > the
> >
> > I think you've also fixed Vega12 and Raven (they were being set to 8
> > before rather than 2). No need to mention this in your description,
> > just double checking.
> >
>
> I believe it was only Vega20 that was being set incorrectly. The condition
> was:
>
> sdma_version >= IP_VERSION(4, 0, 0) &&
> sdma_version <= IP_VERSION(4, 2, 0))
>
> which encapsulates Vega12 and Raven setting sdma_queues_per_engine to
> 2, but also accidently encapsulates Vega20.
Ah right. It was a range check before.
Thanks,
Jon
>
> > > setting keeps the same after code refactor.
> > > Additionally, improve code to use switch case to list IP version to
> > > complete kfd device_info structure filling.
> > > This keeps consistency with the IP parse code in amdgpu_discovery.c.
> > >
> > > v2: use dev_warn for the default switch case;
> > > set default sdma queue per engine(8) and IH handler to v9.
> > > (Jonathan)
> > >
> > > Fixes: a9e2c4dc6cc4("drm/amdkfd: add kfd_device_info_init function")
> > > Signed-off-by: Guchun Chen <guchun.chen@xxxxxxx>
> >
> > Other than the missing checks for Raven when setting the interrupt
> > class (see inline comments and reference kgd2kfd_probe in
> > kfd_device.c) and one nit-pick inline, this looks good to me.
> >
> > With those fixed, this patch is
> > Reviewed-by: Jonathan Kim <jonathan.kim@xxxxxxx>
> >
>
> Other than Jon's comments, this patch is also
>
> Reviewed-by: Graham Sider <Graham.Sider@xxxxxxx>
>
> > > ---
> > > drivers/gpu/drm/amd/amdkfd/kfd_device.c | 77
> > > ++++++++++++++++++++++---
> > > 1 file changed, 68 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > > b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > > index facc28f58c1f..36406a261203 100644
> > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > > @@ -59,11 +59,75 @@ static void kfd_gtt_sa_fini(struct kfd_dev
> > > *kfd);
> > >
> > > static int kfd_resume(struct kfd_dev *kfd);
> > >
> > > +static void kfd_device_info_set_sdma_queue_num(struct kfd_dev
> *kfd)
> > {
> > > + uint32_t sdma_version = kfd->adev-
> > > >ip_versions[SDMA0_HWIP][0];
> > > +
> > > + switch (sdma_version) {
> >
> > Please pull in the indentation for all cases to line up with the switch block.
> >
> > > + case IP_VERSION(4, 0, 0):/* VEGA10 */
> > > + case IP_VERSION(4, 0, 1):/* VEGA12 */
> > > + case IP_VERSION(4, 1, 0):/* RAVEN */
> > > + case IP_VERSION(4, 1, 1):/* RAVEN */
> >
> > As mentioned, you've also fixed Vega12 & Raven here I presume since
> > afaik, they're based off Vega10?
> >
> > > + case IP_VERSION(4, 1, 2):/* RENIOR */
> > > + case IP_VERSION(5, 2, 1):/* VANGOGH */
> > > + case IP_VERSION(5, 2, 3):/* YELLOW_CARP */
> > > + kfd->device_info.num_sdma_queues_per_engine =
> > > 2;
> > > + break;
> > > + case IP_VERSION(4, 2, 0):/* VEGA20 */
> > > + case IP_VERSION(4, 2, 2):/* ARCTUTUS */
> > > + case IP_VERSION(4, 4, 0):/* ALDEBARAN */
> > > + case IP_VERSION(5, 0, 0):/* NAVI10 */
> > > + case IP_VERSION(5, 0, 1):/* CYAN_SKILLFISH */
> > > + case IP_VERSION(5, 0, 2):/* NAVI14 */
> > > + case IP_VERSION(5, 0, 5):/* NAVI12 */
> > > + case IP_VERSION(5, 2, 0):/* SIENNA_CICHLID */
> > > + case IP_VERSION(5, 2, 2):/* NAVY_FLOUDER */
> > > + case IP_VERSION(5, 2, 4):/* DIMGREY_CAVEFISH */
> > > + kfd->device_info.num_sdma_queues_per_engine =
> > > 8;
> > > + break;
> > > + default:
> > > + dev_warn(kfd_device,
> > > + "Default sdma queue per engine(8) is
> > > + set due
> > > to "
> > > + "mismatch of sdma ip
> > > block(SDMA_HWIP:0x%x).\n",
> > > + sdma_version);
> > > + kfd->device_info.num_sdma_queues_per_engine =
> > > 8;
> > > + }
> > > +}
> > > +
> > > +static void kfd_device_info_set_event_interrupt_class(struct
> > > +kfd_dev
> > > +*kfd) {
> > > + uint32_t gc_version = KFD_GC_VERSION(kfd);
> > > +
> > > + switch (gc_version) {
> > > + case IP_VERSION(9, 0, 1): /* VEGA10 */
> >
> > Missing check for case IP_VERSION(9, 1, 0): /* RAVEN */
> >
> > > + case IP_VERSION(9, 2, 1): /* VEGA12 */
> >
> > Missing check for case IP_VERSION(9, 2, 2): /* RAVEN */
> >
> > Thanks,
> >
> > Jon
> >
> > > + case IP_VERSION(9, 3, 0): /* RENOIR */
> > > + case IP_VERSION(9, 4, 0): /* VEGA20 */
> > > + case IP_VERSION(9, 4, 1): /* ARCTURUS */
> > > + case IP_VERSION(9, 4, 2): /* ALDEBARAN */
> > > + case IP_VERSION(10, 3, 1): /* VANGOGH */
> > > + case IP_VERSION(10, 3, 3): /* YELLOW_CARP */
> > > + case IP_VERSION(10, 1, 3): /* CYAN_SKILLFISH */
> > > + case IP_VERSION(10, 1, 10): /* NAVI10 */
> > > + case IP_VERSION(10, 1, 2): /* NAVI12 */
> > > + case IP_VERSION(10, 1, 1): /* NAVI14 */
> > > + case IP_VERSION(10, 3, 0): /* SIENNA_CICHLID */
> > > + case IP_VERSION(10, 3, 2): /* NAVY_FLOUNDER */
> > > + case IP_VERSION(10, 3, 4): /* DIMGREY_CAVEFISH */
> > > + case IP_VERSION(10, 3, 5): /* BEIGE_GOBY */
> > > + kfd->device_info.event_interrupt_class =
> > > &event_interrupt_class_v9;
> > > + break;
> > > + default:
> > > + dev_warn(kfd_device, "v9 event interrupt handler is
> > > + set due
> > > to "
> > > + "mismatch of gc ip block(GC_HWIP:0x%x).\n",
> > > gc_version);
> > > + kfd->device_info.event_interrupt_class =
> > > &event_interrupt_class_v9;
> > > + }
> > > +}
> > > +
> > > static void kfd_device_info_init(struct kfd_dev *kfd,
> > > bool vf, uint32_t gfx_target_version) {
> > > uint32_t gc_version = KFD_GC_VERSION(kfd);
> > > - uint32_t sdma_version = kfd->adev-
> > > >ip_versions[SDMA0_HWIP][0];
> > > uint32_t asic_type = kfd->adev->asic_type;
> > >
> > > kfd->device_info.max_pasid_bits = 16; @@ -75,16 +139,11 @@
> > > static void kfd_device_info_init(struct kfd_dev *kfd,
> > > if (KFD_IS_SOC15(kfd)) {
> > > kfd->device_info.doorbell_size = 8;
> > > kfd->device_info.ih_ring_entry_size = 8 * sizeof(uint32_t);
> > > - kfd->device_info.event_interrupt_class =
> > > &event_interrupt_class_v9;
> > > kfd->device_info.supports_cwsr = true;
> > >
> > > - if ((sdma_version >= IP_VERSION(4, 0, 0) &&
> > > - sdma_version <= IP_VERSION(4, 2, 0)) ||
> > > - sdma_version == IP_VERSION(5, 2, 1) ||
> > > - sdma_version == IP_VERSION(5, 2, 3))
> > > - kfd->device_info.num_sdma_queues_per_engine =
> > > 2;
> > > - else
> > > - kfd->device_info.num_sdma_queues_per_engine =
> > > 8;
> > > + kfd_device_info_set_sdma_queue_num(kfd);
> > > +
> > > + kfd_device_info_set_event_interrupt_class(kfd);
> > >
> > > /* Raven */
> > > if (gc_version == IP_VERSION(9, 1, 0) ||
> > > --
> > > 2.17.1
> >
