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

[Felix Kuehling]

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

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

With that fixed, the patch is

Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>

Regards,
  Felix


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