Sorry I should be clearer.
dev->gpu->pci_atomic_requested is set to the value of adev->have_atomics_support - see function amdgpu_amdkfd_have_atomics_support. adev->have_atomics_support is set through function pci_enable_atomic_ops_to_root currently in amdgpu_device_init - I don't think this logic is correct for xgmi connected devices. For xgmi connected devices, adev->have_atomics_support should always set to true. +@Cornwall, Jay to comment as the original author of this codes.
The codes Jon put below refers dev->gpu->pci_atomic_requested to set the io link properties. I think we should fix adev->have_atomics_support which is the source of dev->gpu->pci_atomic_requested. Once adev->have_atomics_support is fixed, part of the code in kfd_topology.c doesn't need to be changed. Currently kfd_topology.c is the only consumer of adev->have_atomics_support and seems we only use such information for gpu-gpu io-link not for gpu-cpu iolink properties. But I still think we fix it from the source is better because in the future there might be code refers to adev->have_atomics_support. The code I put below is wrong though, it should be:
If (!adev->gmc.xgmi.num_physical_nodes)
r = pci_enable_atomic_ops_to_root(adev->pdev,
PCI_EXP_DEVCAP2_ATOMIC_COMP32 |
PCI_EXP_DEVCAP2_ATOMIC_COMP64);
Regards,
Oak
On 2021-04-29, 10:45 PM, "Kuehling, Felix" <Felix.Kuehling@xxxxxxx> wrote:
Am 2021-04-29 um 9:12 p.m. schrieb Zeng, Oak:
> I think part of this can be done more clean in amdgpu_device_init:
>
> r = 0;
> If (!adev->gmc.xgmi.connected_to_cpu)
> /* enable PCIE atomic ops */
> r = pci_enable_atomic_ops_to_root(adev->pdev,
> PCI_EXP_DEVCAP2_ATOMIC_COMP32 |
> PCI_EXP_DEVCAP2_ATOMIC_COMP64);
> if (r) {
> adev->have_atomics_support = false;
> DRM_INFO("PCIE atomic ops is not supported\n");
> } else {
> adev->have_atomics_support = true;
> }
This code is already in amdgpu_device_init. I'm not sure what's your
point. Are you suggesting that the topology flags should be updated in
amdgpu_device_init? That's not really possible because the KFD topology
data structures don't exist at that time (they do only after the call to
amdgpu_device_ip_init) and the data structures are not known in
amdgpu_device.c. It's cleaner to keep this compartmentalized in
kfd_topology.c.
Regards,
Felix
> Regards,
> Oak
>
>
>
> On 2021-04-29, 5:36 AM, "Kim, Jonathan" <Jonathan.Kim@xxxxxxx> wrote:
>
> Link atomics support over xGMI should be reported independently of PCIe.
>
> 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) {
> --
> 2.17.1
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
