[AMD Official Use Only - Internal Distribution Only]
> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
> Sent: Thursday, April 29, 2021 11:18 AM
> To: Kim, Jonathan <Jonathan.Kim@xxxxxxx>; amd-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Zeng, Oak <Oak.Zeng@xxxxxxx>; Errabolu, Ramesh
> <Ramesh.Errabolu@xxxxxxx>
> 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@xxxxxxx>
> >> Sent: Thursday, April 29, 2021 10:49 AM
> >> To: Kim, Jonathan <Jonathan.Kim@xxxxxxx>; amd-
> >> gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: Zeng, Oak <Oak.Zeng@xxxxxxx>; Errabolu, Ramesh
> >> <Ramesh.Errabolu@xxxxxxx>
> >> 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@xxxxxxx>
>
> Regards,
> Felix
>
>
> >
> > Thanks,
> >
> > Jon
> >
> >> Regards,
> >> Felix
> >>
> >>
> >>> Signed-off-by: Jonathan Kim <jonathan.kim@xxxxxxx>
> >>> Tested-by: Ramesh Errabolu <ramesh.errabolu@xxxxxxx>
> >>> ---
> >>> 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) {
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
