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

Reply via email to