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

[Zeng, Oak]

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. + at 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 at amd.com> 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 at amd.com> wrote:
    >
    >     Link atomics support over xGMI should be reported independently of PCIe.
    >
    >     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) {
    >     -- 
    >     2.17.1
    >
    >