RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init (v2)
> -Original Message- > From: Sider, Graham > Sent: December 20, 2021 1:19 AM > To: Kim, Jonathan ; Chen, Guchun > ; amd-gfx@lists.freedesktop.org; Deucher, > Alexander ; Kuehling, Felix > > Subject: RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd > device init (v2) > > [Public] > > > -Original Message- > > From: Kim, Jonathan > > Sent: Monday, December 20, 2021 12:44 AM > > To: Chen, Guchun ; amd- > > g...@lists.freedesktop.org; Deucher, Alexander > > ; Sider, Graham > ; > > Kuehling, Felix > > Subject: RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd > > device init (v2) > > > > [AMD Official Use Only] > > > > > -Original Message- > > > From: Chen, Guchun > > > Sent: December 19, 2021 10:09 PM > > > To: amd-gfx@lists.freedesktop.org; Deucher, Alexander > > > ; Sider, Graham > > ; > > > Kuehling, Felix ; Kim, Jonathan > > > > > > Cc: Chen, Guchun > > > 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 > > > > 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 > > > > Other than Jon's comments, this patch is also > > Reviewed-by: Graham Sider > > > > --- > > > 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):/* AR
RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init (v2)
[Public] Email crossed:). Graham, I have sent v3 to review, and will add you as another reviewed-by when pushing this patch. Thanks for the review from you and Jonathan. Merry Xmas! Regards, Guchun -Original Message- From: Sider, Graham Sent: Monday, December 20, 2021 2:19 PM To: Kim, Jonathan ; Chen, Guchun ; amd-gfx@lists.freedesktop.org; Deucher, Alexander ; Kuehling, Felix Subject: RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init (v2) [Public] > -Original Message- > From: Kim, Jonathan > Sent: Monday, December 20, 2021 12:44 AM > To: Chen, Guchun ; amd- > g...@lists.freedesktop.org; Deucher, Alexander > ; Sider, Graham ; > Kuehling, Felix > Subject: RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd > device init (v2) > > [AMD Official Use Only] > > > -Original Message- > > From: Chen, Guchun > > Sent: December 19, 2021 10:09 PM > > To: amd-gfx@lists.freedesktop.org; Deucher, Alexander > > ; Sider, Graham > ; > > Kuehling, Felix ; Kim, Jonathan > > > > Cc: Chen, Guchun > > 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. > > 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 > > 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 > Other than Jon's comments, this patch is also Reviewed-by: Graham Sider > > --- > > 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 */ > > +
RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init (v2)
[Public] > 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. No, sdma queue number on vega12 and Raven is correct, it's 2. Anyway, I have dropped it in the commit message of V3. Regards, Guchun -Original Message- From: Kim, Jonathan Sent: Monday, December 20, 2021 1:44 PM To: Chen, Guchun ; amd-gfx@lists.freedesktop.org; Deucher, Alexander ; Sider, Graham ; Kuehling, Felix Subject: RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init (v2) [AMD Official Use Only] > -Original Message- > From: Chen, Guchun > Sent: December 19, 2021 10:09 PM > To: amd-gfx@lists.freedesktop.org; Deucher, Alexander > ; Sider, Graham ; > Kuehling, Felix ; Kim, Jonathan > > Cc: Chen, Guchun > 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. > 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 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 > --- > 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 */ > +
RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init (v2)
[Public] > -Original Message- > From: Kim, Jonathan > Sent: Monday, December 20, 2021 12:44 AM > To: Chen, Guchun ; amd- > g...@lists.freedesktop.org; Deucher, Alexander > ; Sider, Graham > ; Kuehling, Felix > Subject: RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device > init (v2) > > [AMD Official Use Only] > > > -Original Message- > > From: Chen, Guchun > > Sent: December 19, 2021 10:09 PM > > To: amd-gfx@lists.freedesktop.org; Deucher, Alexander > > ; Sider, Graham > ; > > Kuehling, Felix ; Kim, Jonathan > > > > Cc: Chen, Guchun > > 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. > > 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 > > 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 > Other than Jon's comments, this patch is also Reviewed-by: Graham Sider > > --- > > 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 o
RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init (v2)
[AMD Official Use Only] > -Original Message- > From: Chen, Guchun > Sent: December 19, 2021 10:09 PM > To: amd-gfx@lists.freedesktop.org; Deucher, Alexander > ; Sider, Graham > ; Kuehling, Felix ; > Kim, Jonathan > Cc: Chen, Guchun > 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. > 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 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 > --- > 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
RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init
[Public] Hi Graham, My general thought is, from what I observed, IP version does not change in a linear variation manner, so moving to switch case may be easier for user to decode this. Also, I want to get the code aligned with the IP parse code in amdgpu_discovery.c. Please correct me if I am wrong. Regards, Guchun -Original Message- From: Kim, Jonathan Sent: Friday, December 17, 2021 11:35 PM To: Sider, Graham ; Chen, Guchun ; amd-gfx@lists.freedesktop.org; Deucher, Alexander ; Kuehling, Felix Subject: RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init > -Original Message- > From: Sider, Graham > Sent: December 17, 2021 10:06 AM > To: Chen, Guchun ; amd- > g...@lists.freedesktop.org; Deucher, Alexander > ; Kuehling, Felix ; > Kim, Jonathan > Subject: RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd > device init > > [Public] > > > -Original Message- > > From: Chen, Guchun > > Sent: Friday, December 17, 2021 9:31 AM > > To: amd-gfx@lists.freedesktop.org; Deucher, Alexander > > ; Sider, Graham > ; > > Kuehling, Felix ; Kim, Jonathan > > > > Cc: Chen, Guchun > > Subject: [PATCH] drm/amdkfd: correct sdma queue number in kfd device > > init > > > > sdma queue number is not correct like on vega20, this patch promises > > the 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. > > > > Fixes: a9e2c4dc6cc4("drm/amdkfd: add kfd_device_info_init function") > > Signed-off-by: Guchun Chen > > --- > > drivers/gpu/drm/amd/amdkfd/kfd_device.c | 74 > > ++--- > > 1 file changed, 65 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..e50bf992f298 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c > > @@ -59,11 +59,72 @@ 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) { > > + 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 */ > > + 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 */ > > Thanks for spotting this Guchun. My previous patch should have used a "<" > instead of a "<=" on IP_VERSION(4, 2, 0). > > > + 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_err(kfd_device, > > + "Failed to find sdma ip > > blocks(SDMA_HWIP:0x%x) in %s\n", > > +sdma_version, __func__); > > + } > > +} > > + > > +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 */ > > + case IP_VERSION(9, 2, 1): /* VEGA12 */ > > + 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,
RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init
> -Original Message- > From: Sider, Graham > Sent: December 17, 2021 10:06 AM > To: Chen, Guchun ; amd- > g...@lists.freedesktop.org; Deucher, Alexander > ; Kuehling, Felix > ; Kim, Jonathan > Subject: RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd > device init > > [Public] > > > -Original Message- > > From: Chen, Guchun > > Sent: Friday, December 17, 2021 9:31 AM > > To: amd-gfx@lists.freedesktop.org; Deucher, Alexander > > ; Sider, Graham > ; > > Kuehling, Felix ; Kim, Jonathan > > > > Cc: Chen, Guchun > > Subject: [PATCH] drm/amdkfd: correct sdma queue number in kfd device > > init > > > > sdma queue number is not correct like on vega20, this patch promises > > the 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. > > > > Fixes: a9e2c4dc6cc4("drm/amdkfd: add kfd_device_info_init function") > > Signed-off-by: Guchun Chen > > --- > > drivers/gpu/drm/amd/amdkfd/kfd_device.c | 74 > > ++--- > > 1 file changed, 65 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..e50bf992f298 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c > > @@ -59,11 +59,72 @@ 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) { > > + 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 */ > > + 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 */ > > Thanks for spotting this Guchun. My previous patch should have used a "<" > instead of a "<=" on IP_VERSION(4, 2, 0). > > > + 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_err(kfd_device, > > + "Failed to find sdma ip > > blocks(SDMA_HWIP:0x%x) in %s\n", > > +sdma_version, __func__); > > + } > > +} > > + > > +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 */ > > + case IP_VERSION(9, 2, 1): /* VEGA12 */ > > + 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_inf
RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init
[AMD Official Use Only] Are safeguards required for KFD interrupt initialization to fail gracefully in the event of a non-assignment? Same would apply when KGD forwards interrupts to the KFD (although the KFD device reference might not exist at this point if the above comment is handled well so a check may not apply in this case). Also should the dev_errs mention what it's failing to do rather than just reporting that it could not find the HW IP block? In the case of non-assignment of sdma queues per engine, it still seems like the KFD could move forward but the user wouldn't know what the context of the dev_err was. Thanks, Jon > -Original Message- > From: Chen, Guchun > Sent: December 17, 2021 9:31 AM > To: amd-gfx@lists.freedesktop.org; Deucher, Alexander > ; Sider, Graham > ; Kuehling, Felix ; > Kim, Jonathan > Cc: Chen, Guchun > Subject: [PATCH] drm/amdkfd: correct sdma queue number in kfd device > init > > sdma queue number is not correct like on vega20, this patch promises the > 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. > > Fixes: a9e2c4dc6cc4("drm/amdkfd: add kfd_device_info_init function") > Signed-off-by: Guchun Chen > --- > drivers/gpu/drm/amd/amdkfd/kfd_device.c | 74 > ++--- > 1 file changed, 65 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..e50bf992f298 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c > @@ -59,11 +59,72 @@ 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) { > + 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 */ > + 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_err(kfd_device, > + "Failed to find sdma ip > blocks(SDMA_HWIP:0x%x) in %s\n", > +sdma_version, __func__); > + } > +} > + > +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 */ > + case IP_VERSION(9, 2, 1): /* VEGA12 */ > + 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_err(kfd_device, "Failed to find gc ip > blocks(GC_HWIP:0x%x) in %s\n", > + gc_version, __func__); > + } > +} > + > 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
RE: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init
[Public] > -Original Message- > From: Chen, Guchun > Sent: Friday, December 17, 2021 9:31 AM > To: amd-gfx@lists.freedesktop.org; Deucher, Alexander > ; Sider, Graham > ; Kuehling, Felix ; > Kim, Jonathan > Cc: Chen, Guchun > Subject: [PATCH] drm/amdkfd: correct sdma queue number in kfd device init > > sdma queue number is not correct like on vega20, this patch promises the > 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. > > Fixes: a9e2c4dc6cc4("drm/amdkfd: add kfd_device_info_init function") > Signed-off-by: Guchun Chen > --- > drivers/gpu/drm/amd/amdkfd/kfd_device.c | 74 > ++--- > 1 file changed, 65 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..e50bf992f298 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c > @@ -59,11 +59,72 @@ 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) { > + 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 */ > + 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 */ Thanks for spotting this Guchun. My previous patch should have used a "<" instead of a "<=" on IP_VERSION(4, 2, 0). > + 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_err(kfd_device, > + "Failed to find sdma ip > blocks(SDMA_HWIP:0x%x) in %s\n", > +sdma_version, __func__); > + } > +} > + > +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 */ > + case IP_VERSION(9, 2, 1): /* VEGA12 */ > + 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_err(kfd_device, "Failed to find gc ip > blocks(GC_HWIP:0x%x) in %s\n", > + gc_version, __func__); > + } > +} I understand the appeal of moving to a switch for the SDMA queue num above since its setting isn't very linear w.r.t. the SDMA versioning. That said I don't know if I understand moving to a switch for the event interrupt class here. To clarify, originally when I set all SOC15 to event_interrupt_class_v9 it was to follow what was in the device_info structs in drm-staging-next at that time--that said if the plan is in a following patch to change this such that gfx10 are set to to event_interrupt_class_v10, what's the reasoning not to format it along the lines of: if (gc_version >= IP_VERSION(10, 1, 1) kfd->device_info.event_interrupt_class = &event_interrupt_class_v10; else kfd->device_info.event_interrupt_class = &event_interrupt_class_v9; (Assuming this is done