On Mon, Jan 13, 2025 at 9:20 PM Wang, Yang(Kevin) <KevinYang.Wang@xxxxxxx> wrote: > > [AMD Official Use Only - AMD Internal Distribution Only] > > -----Original Message----- > From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Alex Deucher > Sent: Tuesday, January 14, 2025 01:08 > To: Deucher, Alexander <Alexander.Deucher@xxxxxxx> > Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 1/2] drm/amdgpu: cache gpu pcie link width > > Ping on this series? > > Alex > > On Tue, Jan 7, 2025 at 11:17 AM Alex Deucher <alexander.deucher@xxxxxxx> wrote: > > > > Get the PCIe link with of the device itself (or it's integrated > > upstream bridge) and cache that. > > > > v2: fix typo > > > > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3820 > > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 152 ++++++++++++++++----- > > drivers/gpu/drm/amd/include/amd_pcie.h | 18 +++ > > 2 files changed, 138 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > index 90eb92c4c2800..72aff70464ed7 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > @@ -6162,6 +6162,44 @@ static void amdgpu_device_partner_bandwidth(struct amdgpu_device *adev, > > } > > } > > > > +/** > > + * amdgpu_device_gpu_bandwidth - find the bandwidth of the GPU > > + * > > + * @adev: amdgpu_device pointer > > + * @speed: pointer to the speed of the link > > + * @width: pointer to the width of the link > > + * > > + * Evaluate the hierarchy to find the speed and bandwidth > > +capabilities of the > > + * AMD dGPU which may be a virtual upstream bridge. > > + */ > > +static void amdgpu_device_gpu_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; > > + > > + parent = pci_upstream_bridge(parent); > > + if (parent && 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); > > + 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(parent); > > + *width = pcie_get_width_cap(parent); > > + } > > +} > > + > > /** > > * amdgpu_device_get_pcie_info - fence pcie info about the PCIE slot > > * > > @@ -6173,9 +6211,8 @@ static void amdgpu_device_partner_bandwidth(struct amdgpu_device *adev, > > */ > > static void amdgpu_device_get_pcie_info(struct amdgpu_device *adev) > > { > > - struct pci_dev *pdev; > > enum pci_bus_speed speed_cap, platform_speed_cap; > > - enum pcie_link_width platform_link_width; > > + enum pcie_link_width platform_link_width, link_width; > > > > if (amdgpu_pcie_gen_cap) > > adev->pm.pcie_gen_mask = amdgpu_pcie_gen_cap; @@ > > -6197,11 +6234,10 @@ static void amdgpu_device_get_pcie_info(struct > > amdgpu_device *adev) > > > > amdgpu_device_partner_bandwidth(adev, &platform_speed_cap, > > &platform_link_width); > > + amdgpu_device_gpu_bandwidth(adev, &speed_cap, &link_width); > > > > if (adev->pm.pcie_gen_mask == 0) { > > /* asic caps */ > > - pdev = adev->pdev; > > - speed_cap = pcie_get_speed_cap(pdev); > > if (speed_cap == PCI_SPEED_UNKNOWN) { > > adev->pm.pcie_gen_mask |= (CAIL_ASIC_PCIE_LINK_SPEED_SUPPORT_GEN1 | > > > > CAIL_ASIC_PCIE_LINK_SPEED_SUPPORT_GEN2 | @@ -6257,51 +6293,103 @@ static void amdgpu_device_get_pcie_info(struct amdgpu_device *adev) > > } > > } > > if (adev->pm.pcie_mlw_mask == 0) { > > + /* asic caps */ > > + if (link_width == PCIE_LNK_WIDTH_UNKNOWN) { > > + adev->pm.pcie_mlw_mask |= AMDGPU_DEFAULT_ASIC_PCIE_MLW_MASK; > > In this condition, we already know this variable (adev->pm.pcie_mlw_mask) is 0, so it seems no reason to use "|=" in this case. > So, it better to change it from "|=" to "=" , right? Sure. I did it for consistency with the pcie gen code and to avoid someone accidently adjusting the code later and missing the |= and overwriting the previous values with =. If you feel strongly about it I can change it. > > The rest of the parts look fine, Series is > > Reviewed-by: Yang Wang <kevinyang.wang@xxxxxxx> Thanks! Alex > > Best Regards, > Kevin > > + } else { > > + switch (link_width) { > > + case PCIE_LNK_X32: > > + adev->pm.pcie_mlw_mask |= (CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X32 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X16 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X12 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X8 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X4 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X1); > > + break; > > + case PCIE_LNK_X16: > > + adev->pm.pcie_mlw_mask |= (CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X16 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X12 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X8 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X4 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X1); > > + break; > > + case PCIE_LNK_X12: > > + adev->pm.pcie_mlw_mask |= (CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X12 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X8 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X4 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X1); > > + break; > > + case PCIE_LNK_X8: > > + adev->pm.pcie_mlw_mask |= (CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X8 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X4 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X1); > > + break; > > + case PCIE_LNK_X4: > > + adev->pm.pcie_mlw_mask |= (CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X4 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X1); > > + break; > > + case PCIE_LNK_X2: > > + adev->pm.pcie_mlw_mask |= (CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X1); > > + break; > > + case PCIE_LNK_X1: > > + adev->pm.pcie_mlw_mask |= CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X1; > > + break; > > + default: > > + break; > > + } > > + } > > + /* platform caps */ > > if (platform_link_width == PCIE_LNK_WIDTH_UNKNOWN) { > > adev->pm.pcie_mlw_mask |= AMDGPU_DEFAULT_PCIE_MLW_MASK; > > } else { > > switch (platform_link_width) { > > case PCIE_LNK_X32: > > - adev->pm.pcie_mlw_mask = (CAIL_PCIE_LINK_WIDTH_SUPPORT_X32 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X16 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X12 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X8 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X4 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > + adev->pm.pcie_mlw_mask |= (CAIL_PCIE_LINK_WIDTH_SUPPORT_X32 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X16 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X12 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X8 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X4 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > break; > > case PCIE_LNK_X16: > > - adev->pm.pcie_mlw_mask = (CAIL_PCIE_LINK_WIDTH_SUPPORT_X16 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X12 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X8 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X4 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > + adev->pm.pcie_mlw_mask |= (CAIL_PCIE_LINK_WIDTH_SUPPORT_X16 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X12 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X8 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X4 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > break; > > case PCIE_LNK_X12: > > - adev->pm.pcie_mlw_mask = (CAIL_PCIE_LINK_WIDTH_SUPPORT_X12 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X8 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X4 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > + adev->pm.pcie_mlw_mask |= (CAIL_PCIE_LINK_WIDTH_SUPPORT_X12 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X8 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X4 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > break; > > case PCIE_LNK_X8: > > - adev->pm.pcie_mlw_mask = (CAIL_PCIE_LINK_WIDTH_SUPPORT_X8 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X4 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > + adev->pm.pcie_mlw_mask |= (CAIL_PCIE_LINK_WIDTH_SUPPORT_X8 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X4 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > break; > > case PCIE_LNK_X4: > > - adev->pm.pcie_mlw_mask = (CAIL_PCIE_LINK_WIDTH_SUPPORT_X4 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > + adev->pm.pcie_mlw_mask |= (CAIL_PCIE_LINK_WIDTH_SUPPORT_X4 | > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > break; > > case PCIE_LNK_X2: > > - adev->pm.pcie_mlw_mask = (CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > - CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > + adev->pm.pcie_mlw_mask |= (CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 | > > + > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X1); > > break; > > case PCIE_LNK_X1: > > - adev->pm.pcie_mlw_mask = CAIL_PCIE_LINK_WIDTH_SUPPORT_X1; > > + adev->pm.pcie_mlw_mask |= > > + CAIL_PCIE_LINK_WIDTH_SUPPORT_X1; > > break; > > default: > > break; diff --git > > a/drivers/gpu/drm/amd/include/amd_pcie.h > > b/drivers/gpu/drm/amd/include/amd_pcie.h > > index a1ece3eecdf5e..a08611cb80411 100644 > > --- a/drivers/gpu/drm/amd/include/amd_pcie.h > > +++ b/drivers/gpu/drm/amd/include/amd_pcie.h > > @@ -49,6 +49,17 @@ > > | > > CAIL_ASIC_PCIE_LINK_SPEED_SUPPORT_GEN3) > > > > /* Following flags shows PCIe lane width switch supported in driver > > which are decided by chipset and ASIC */ > > + > > +#define CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X1 0x00000001 > > +#define CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X2 0x00000002 > > +#define CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X4 0x00000004 > > +#define CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X8 0x00000008 > > +#define CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X12 0x00000010 > > +#define CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X16 0x00000020 > > +#define CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X32 0x00000040 > > +#define CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_MASK 0x0000FFFF > > +#define CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_SHIFT 0 > > + > > #define CAIL_PCIE_LINK_WIDTH_SUPPORT_X1 0x00010000 > > #define CAIL_PCIE_LINK_WIDTH_SUPPORT_X2 0x00020000 > > #define CAIL_PCIE_LINK_WIDTH_SUPPORT_X4 0x00040000 > > @@ -56,6 +67,7 @@ > > #define CAIL_PCIE_LINK_WIDTH_SUPPORT_X12 0x00100000 > > #define CAIL_PCIE_LINK_WIDTH_SUPPORT_X16 0x00200000 > > #define CAIL_PCIE_LINK_WIDTH_SUPPORT_X32 0x00400000 > > +#define CAIL_PCIE_LINK_WIDTH_SUPPORT_MASK 0xFFFF0000 > > #define CAIL_PCIE_LINK_WIDTH_SUPPORT_SHIFT 16 > > > > /* 1/2/4/8/16 lanes */ > > @@ -65,4 +77,10 @@ > > | CAIL_PCIE_LINK_WIDTH_SUPPORT_X8 \ > > | > > CAIL_PCIE_LINK_WIDTH_SUPPORT_X16) > > > > +#define AMDGPU_DEFAULT_ASIC_PCIE_MLW_MASK (CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X1 \ > > + | CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X2 \ > > + | CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X4 \ > > + | CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X8 \ > > + | > > +CAIL_ASIC_PCIE_LINK_WIDTH_SUPPORT_X16) > > + > > #endif > > -- > > 2.47.1 > >
