Re: [PATCH 1/2] drm/amd: Add dedicated helper for amdgpu_device_find_parent()

[Lazar, Lijo]

On 20-May-26 9:16 PM, Mario Limonciello wrote:
There are a few cases that code walks up the topology to find the
link partner of the integrated switch in a dGPU.  Split this out
to a helper and call in all places.

This does have a functional change that amdgpu_device_gpu_bandwidth()
doesn't cache the internal link but only the parent.

Signed-off-by: Mario Limonciello <mario.limoncie...@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 44 +++++++++++-----------
  1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 073f632f295a3..b6a24faaf78dd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1913,6 +1913,19 @@ static void amdgpu_uid_fini(struct amdgpu_device *adev)
        adev->uid_info = NULL;
  }
  
+static struct pci_dev *amdgpu_device_find_parent(struct amdgpu_device *adev)
+{
+       struct pci_dev *parent = adev->pdev;
+
+       /* skip upstream/downstream switches internal to dGPU */
+       while ((parent = pci_upstream_bridge(parent))) {
+               if (parent->vendor == PCI_VENDOR_ID_ATI)
+                       continue;
+       }
+
+       return parent;
+}
+
  /**
   * amdgpu_device_ip_early_init - run early init for hardware IPs
   *
@@ -5913,8 +5926,6 @@ static void amdgpu_device_partner_bandwidth(struct 
amdgpu_device *adev,
                                            enum pci_bus_speed *speed,
                                            enum pcie_link_width *width)
  {
-       struct pci_dev *parent = adev->pdev;
-
        if (!speed || !width)
                return;
  
@@ -5922,13 +5933,11 @@ static void amdgpu_device_partner_bandwidth(struct amdgpu_device *adev,
        *width = PCIE_LNK_WIDTH_UNKNOWN;
  
  	if (amdgpu_device_pcie_dynamic_switching_supported(adev)) {
-               while ((parent = pci_upstream_bridge(parent))) {
-                       /* skip upstream/downstream switches internal to dGPU*/
-                       if (parent->vendor == PCI_VENDOR_ID_ATI)
-                               continue;
+               struct pci_dev *parent = amdgpu_device_find_parent(adev);
+
+               if (parent) {
                        *speed = pcie_get_speed_cap(parent);
                        *width = pcie_get_width_cap(parent);
-                       break;
                }
        } else {
                /* use the current speeds rather than max if switching is not 
supported */
@@ -5955,22 +5964,15 @@ static void amdgpu_device_gpu_bandwidth(struct 
amdgpu_device *adev,
        if (!speed || !width)
                return;
  
-	parent = pci_upstream_bridge(parent);
-       if (parent && parent->vendor == PCI_VENDOR_ID_ATI) {
-               /* use the upstream/downstream switches internal to dGPU */
+       /* use the device itself */
+       *speed = pcie_get_speed_cap(adev->pdev);
+       *width = pcie_get_width_cap(adev->pdev);
+
+       /* use the link outside the device */
+       parent = amdgpu_device_find_parent(adev);

This is for GPU device's bandwidth and not the parent device. Suggest to 
cache both pdevs separately as link_dev (device which is connected to 
actual physical link) and link_partner (the other end of link) one-time 
during init. That will avoid confusion.

Thanks,
Lijo

+       if (parent) {
                *speed = pcie_get_speed_cap(parent);
                *width = pcie_get_width_cap(parent);
-               while ((parent = pci_upstream_bridge(parent))) {
-                       if (parent->vendor == PCI_VENDOR_ID_ATI) {
-                               /* use the upstream/downstream switches 
internal to dGPU */
-                               *speed = pcie_get_speed_cap(parent);
-                               *width = pcie_get_width_cap(parent);
-                       }
-               }
-       } else {
-               /* use the device itself */
-               *speed = pcie_get_speed_cap(adev->pdev);
-               *width = pcie_get_width_cap(adev->pdev);
        }
  }