[PATCH] drm/amdkfd: fix incorrect condition check for pci atomics gpu flags

[PATCH] drm/amdkfd: fix incorrect condition check for pci atomics gpu flags

[Jonathan Kim]

As pci_atomics_device_to_root check is a host to device support check,
the device xgmi hive setup status is irrelevant to setting the NO_ATOMICS
gpu flags so move it under the correct host-device check condition.

Signed-off-by: Jonathan Kim <jonathan.kim@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 30430aefcfc7..c84db6441c4e 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -1206,23 +1206,21 @@ static void kfd_fill_iolink_non_crat_info(struct kfd_topology_device *dev)
 
 	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 (!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) {
 		link->flags = flag;
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdkfd: fix incorrect condition check for pci atomics gpu flags

[Felix Kuehling]

Am 2021-04-30 um 4:51 p.m. schrieb Jonathan Kim:
> As pci_atomics_device_to_root check is a host to device support check,
> the device xgmi hive setup status is irrelevant to setting the NO_ATOMICS
> gpu flags so move it under the correct host-device check condition.

I think that's still not quite correct. The flag is applied to all
direct outgoing links from the GPU. That includes the link to the CPU
and the links to other GPUs in the XGMI hive. Technically we may want
different flags for different types of links.

I think this whole function is just completely broken for systems with
XGMI because it doesn't make that distinction.

Maybe instead of looking at adev->gmc.xgmi.connected_to_cpu, look at
link->iolink_type inside the loop. If it's CRAT_IOLINK_TYPE_PCIEXPRESS,
you apply the NO_ATOMIC flags as appropriate. If it's
CRAT_IOLINK_TYPE_XGMI, you can skip it, because XGMI is assumed to
always support atomics. This would fix A+A as well as normal XGMI
between GPUs at once.

Regards,
  Felix


>
> Signed-off-by: Jonathan Kim <jonathan.kim@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index 30430aefcfc7..c84db6441c4e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -1206,23 +1206,21 @@ static void kfd_fill_iolink_non_crat_info(struct kfd_topology_device *dev)
>  
>  	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 (!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) {
>  		link->flags = flag;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx