[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 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.

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