[PATCH 1/2] drm/amdgpu: cache gpu pcie link width
Alex Deucher
alexdeucher at gmail.com
Tue Jan 14 14:23:51 UTC 2025
On Mon, Jan 13, 2025 at 9:20 PM Wang, Yang(Kevin)
<KevinYang.Wang at amd.com> wrote:
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Alex Deucher
> Sent: Tuesday, January 14, 2025 01:08
> To: Deucher, Alexander <Alexander.Deucher at amd.com>
> Cc: amd-gfx at lists.freedesktop.org
> 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 at amd.com> 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 at amd.com>
> > ---
> > 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 at amd.com>
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
> >
More information about the amd-gfx
mailing list