Hi Nils,

On 4/10/2022 8:54 AM, Nils Wallménius wrote:
> Hi Richard, see inline comment.
>
> Den fre 8 apr. 2022 21:05Richard Gong <richard.gong@amd.com> skrev:
>
>     Active State Power Management (ASPM) feature is enabled since
>     kernel 5.14.
>     There are some AMD GFX cards (such as WX3200 and RX640) that cannot be
>     used with Intel AlderLake based systems to enable ASPM. Using
>     these GFX
>     cards as video/display output, Intel Alder Lake based systems will
>     hang
>     during suspend/resume.
>
>     Add extra check to disable ASPM on Intel AlderLake based systems.
>
>     Fixes: 0064b0ce85bb ("drm/amd/pm: enable ASPM by default")
>     Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1885
>     <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1885&data=04%7C01%7Crichard.gong%40amd.com%7Cd46b7fb14c6d44a277d408da1af99f63%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637851956929855129%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=B3SXfB65dqx3EBQzUZh6xCmtpO9ynPj3PvwrWe9Q%2BpU%3D&reserved=0>
>     Signed-off-by: Richard Gong <richard.gong@amd.com>
>     ---
>     v2: correct commit description
>         move the check from chip family to problematic platform
>     ---
>      drivers/gpu/drm/amd/amdgpu/vi.c | 17 ++++++++++++++++-
>      1 file changed, 16 insertions(+), 1 deletion(-)
>
>     diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
>     b/drivers/gpu/drm/amd/amdgpu/vi.c
>     index 039b90cdc3bc..8b4eaf54b23e 100644
>     --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>     +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>     @@ -81,6 +81,10 @@
>      #include "mxgpu_vi.h"
>      #include "amdgpu_dm.h"
>
>     +#if IS_ENABLED(CONFIG_X86_64)
>     +#include <asm/intel-family.h>
>     +#endif
>     +
>      #define ixPCIE_LC_L1_PM_SUBSTATE       0x100100C6
>      #define PCIE_LC_L1_PM_SUBSTATE__LC_L1_SUBSTATES_OVERRIDE_EN_MASK
>          0x00000001L
>      #define PCIE_LC_L1_PM_SUBSTATE__LC_PCI_PM_L1_2_OVERRIDE_MASK
>      0x00000002L
>     @@ -1134,13 +1138,24 @@ static void vi_enable_aspm(struct
>     amdgpu_device *adev)
>                     WREG32_PCIE(ixPCIE_LC_CNTL, data);
>      }
>
>
> There's a typo in the new function name apsm/aspm. Btw might be worth 
> a comment why this check is done?
>
Thanks for review,

s/intel_core_apsm_chk/intel_core_aspm_chk in the next view.

Regards,

Richard

> Regards
> Nils
>
>
>     +static bool intel_core_apsm_chk(void)
>     +{
>     +#if IS_ENABLED(CONFIG_X86_64)
>     +       struct cpuinfo_x86 *c = &cpu_data(0);
>     +
>     +       return (c->x86 == 6 && c->x86_model == INTEL_FAM6_ALDERLAKE);
>     +#else
>     +       return false;
>     +#endif
>     +}
>     +
>      static void vi_program_aspm(struct amdgpu_device *adev)
>      {
>             u32 data, data1, orig;
>             bool bL1SS = false;
>             bool bClkReqSupport = true;
>
>     -       if (!amdgpu_device_should_use_aspm(adev))
>     +       if (!amdgpu_device_should_use_aspm(adev) ||
>     intel_core_apsm_chk())
>                     return;
>
>             if (adev->flags & AMD_IS_APU ||
>     -- 
>     2.25.1
>