[PATCH] drm/amdkfd: report atomics support in io_links over xgmi

[Kim, Jonathan]

[AMD Official Use Only - Internal Distribution Only]

> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling at amd.com>
> Sent: Thursday, April 29, 2021 11:18 AM
> To: Kim, Jonathan <Jonathan.Kim at amd.com>; amd-
> gfx at lists.freedesktop.org
> Cc: Zeng, Oak <Oak.Zeng at amd.com>; Errabolu, Ramesh
> <Ramesh.Errabolu at amd.com>
> Subject: Re: [PATCH] drm/amdkfd: report atomics support in io_links over
> xgmi
>
> Am 2021-04-29 um 11:04 a.m. schrieb Kim, Jonathan:
> > [AMD Official Use Only - Internal Distribution Only]
> >
> >> -----Original Message-----
> >> From: Kuehling, Felix <Felix.Kuehling at amd.com>
> >> Sent: Thursday, April 29, 2021 10:49 AM
> >> To: Kim, Jonathan <Jonathan.Kim at amd.com>; amd-
> >> gfx at lists.freedesktop.org
> >> Cc: Zeng, Oak <Oak.Zeng at amd.com>; Errabolu, Ramesh
> >> <Ramesh.Errabolu at amd.com>
> >> Subject: Re: [PATCH] drm/amdkfd: report atomics support in io_links
> >> over xgmi
> >>
> >> Am 2021-04-29 um 5:36 a.m. schrieb Jonathan Kim:
> >>> Link atomics support over xGMI should be reported independently of
> PCIe.
> >> I don't understand this change. I don't see any code that gets
> >> executed if (adev->gmc.xgmi.connected_to_cpu). Where is the code that
> >> reports atomics support for this case?
> > The atomics aren't set but rather NO_ATOMICS are set if non-xgmi and non
> PCIe supported:
> > cpu_flag |= CRAT_IOLINK_FLAGS_NO_ATOMICS_32_BIT |
> > CRAT_IOLINK_FLAGS_NO_ATOMICS_64_BIT;
> > adev->gmc.xgmi.connected_to_cpu == true would bypass this flag
> NO_ATOMICS setting.
> >
> >> Also, the PCIe code doesn't clear any atomic flags. It only sets
> >> flags that would be set for XGMI anyway. So I don't see why you need
> >> to make that code conditional.
> > As mentioned above they set NO_ATOMICS if not PCIe supported.
> > This has been observed on Aldebaran with AMD systems.
>
> OK. I missed that these flags are negative logic. Thanks, the change makes
> sense now. But the patch description is a bit misleading compared to the
> code. A different wording would make it clearer, e.g.: "Don't set
> NO_ATOMICS flags on XGMI links between CPU and GPU."

Thanks for the review Felix.  Will do.

Jon

>
> With that fixed, the patch is
>
> Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>
>
> Regards,
>   Felix
>
>
> >
> > Thanks,
> >
> > Jon
> >
> >> Regards,
> >>   Felix
> >>
> >>
> >>> Signed-off-by: Jonathan Kim <jonathan.kim at amd.com>
> >>> Tested-by: Ramesh Errabolu <ramesh.errabolu at amd.com>
> >>> ---
> >>>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 29
> >>> ++++++++++++++---------
> >>>  1 file changed, 18 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> >>> b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> >>> index 083ac9babfa8..30430aefcfc7 100644
> >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> >>> @@ -1196,6 +1196,7 @@ static void
> >>> kfd_fill_iolink_non_crat_info(struct
> >>> kfd_topology_device *dev)  {
> >>>  struct kfd_iolink_properties *link, *cpu_link;  struct
> >>> kfd_topology_device *cpu_dev;
> >>> +struct amdgpu_device *adev;
> >>>  uint32_t cap;
> >>>  uint32_t cpu_flag = CRAT_IOLINK_FLAGS_ENABLED;  uint32_t flag =
> >>> CRAT_IOLINK_FLAGS_ENABLED; @@ -1203,18
> >> +1204,24 @@
> >>> static void kfd_fill_iolink_non_crat_info(struct kfd_topology_device
> >>> *dev)  if (!dev || !dev->gpu)  return;
> >>>
> >>> -pcie_capability_read_dword(dev->gpu->pdev,
> >>> -PCI_EXP_DEVCAP2, &cap);
> >>> -
> >>> -if (!(cap & (PCI_EXP_DEVCAP2_ATOMIC_COMP32 |
> >>> -     PCI_EXP_DEVCAP2_ATOMIC_COMP64)))
> >>> -cpu_flag |= CRAT_IOLINK_FLAGS_NO_ATOMICS_32_BIT |
> >>> -CRAT_IOLINK_FLAGS_NO_ATOMICS_64_BIT;
> >>> +adev = (struct amdgpu_device *)(dev->gpu->kgd); if
> >>> +(!adev->gmc.xgmi.connected_to_cpu) {
> >>> +pcie_capability_read_dword(dev->gpu->pdev,
> >>> +PCI_EXP_DEVCAP2, &cap);
> >>> +
> >>> +if (!(cap & (PCI_EXP_DEVCAP2_ATOMIC_COMP32 |
> >>> +     PCI_EXP_DEVCAP2_ATOMIC_COMP64))) cpu_flag |=
> >> CRAT_IOLINK_FLAGS_NO_ATOMICS_32_BIT |
> >>> +CRAT_IOLINK_FLAGS_NO_ATOMICS_64_BIT;
> >>> +}
> >>>
> >>> -if (!dev->gpu->pci_atomic_requested ||
> >>> -    dev->gpu->device_info->asic_family == CHIP_HAWAII)
> >>> -flag |= CRAT_IOLINK_FLAGS_NO_ATOMICS_32_BIT |
> >>> -CRAT_IOLINK_FLAGS_NO_ATOMICS_64_BIT;
> >>> +if (!adev->gmc.xgmi.num_physical_nodes) { if
> >>> +(!dev->gpu->pci_atomic_requested ||
> >>> +dev->gpu->device_info->asic_family ==
> >>> +CHIP_HAWAII)
> >>> +flag |= CRAT_IOLINK_FLAGS_NO_ATOMICS_32_BIT |
> >>> +CRAT_IOLINK_FLAGS_NO_ATOMICS_64_BIT;
> >>> +}
> >>>
> >>>  /* GPU only creates direct links so apply flags setting to all */
> >>> list_for_each_entry(link, &dev->io_link_props, list) {