dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
@ 2022-04-08 19:05 Richard Gong
  2022-04-08 19:08 ` Alex Deucher
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Richard Gong @ 2022-04-08 19:05 UTC (permalink / raw)
  To: alexander.deucher, christian.koenig, xinhui.pan, airlied, daniel
  Cc: mario.limonciello, richard.gong, dri-devel, amd-gfx, linux-kernel

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
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);
 }
 
+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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-08 19:05 [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems Richard Gong
@ 2022-04-08 19:08 ` Alex Deucher
  2022-04-08 19:12   ` Limonciello, Mario
  2022-04-09  0:19 ` Paul Menzel
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Alex Deucher @ 2022-04-08 19:08 UTC (permalink / raw)
  To: Richard Gong
  Cc: amd-gfx list, Dave Airlie, xinhui pan, LKML,
	Maling list - DRI developers, Limonciello, Mario, Deucher,
	Alexander, Christian Koenig

On Fri, Apr 8, 2022 at 3:05 PM Richard Gong <richard.gong@amd.com> wrote:
>
> 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
> Signed-off-by: Richard Gong <richard.gong@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@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);
>  }
>
> +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
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-08 19:08 ` Alex Deucher
@ 2022-04-08 19:12   ` Limonciello, Mario
  0 siblings, 0 replies; 15+ messages in thread
From: Limonciello, Mario @ 2022-04-08 19:12 UTC (permalink / raw)
  To: Alex Deucher, Gong, Richard
  Cc: Dave Airlie, Pan, Xinhui, LKML, Maling list - DRI developers,
	amd-gfx list, Deucher, Alexander, Koenig, Christian

[Public]



> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Friday, April 8, 2022 14:09
> To: Gong, Richard <Richard.Gong@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; Dave Airlie
> <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Maling list - DRI developers <dri-
> devel@lists.freedesktop.org>; amd-gfx list <amd-gfx@lists.freedesktop.org>;
> LKML <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based
> systems
> 
> On Fri, Apr 8, 2022 at 3:05 PM Richard Gong <richard.gong@amd.com> wrote:
> >
> > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.fr
> eedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F1885&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7
> C440357cd10e74d8c4e1d08da1993344b%7C3dd8961fe4884e608e11a82d994e1
> 83d%7C0%7C0%7C637850417310167943%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 3000&amp;sdata=WXyESh1FGlxgFLH14P7pYJu3tsyp53uKpUP9NyDV5yE%3D&am
> p;reserved=0
> > Signed-off-by: Richard Gong <richard.gong@amd.com>
> 
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Reviewed-by: Mario Limonciello <mario.limonciello@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);
> >  }
> >
> > +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
> >

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-08 19:05 [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems Richard Gong
  2022-04-08 19:08 ` Alex Deucher
@ 2022-04-09  0:19 ` Paul Menzel
  2022-04-11  0:27   ` Gong, Richard
  2022-04-11  0:37   ` Gong, Richard
  2022-04-10 13:54 ` Nils Wallménius
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 15+ messages in thread
From: Paul Menzel @ 2022-04-09  0:19 UTC (permalink / raw)
  To: Richard Gong
  Cc: airlied, dri-devel, xinhui.pan, linux-kernel, amd-gfx,
	mario.limonciello, alexander.deucher, christian.koenig

Dear Richard,


Thank you for your patch.

Am 08.04.22 um 21:05 schrieb Richard Gong:
> 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

Alder Lake

> cards as video/display output, Intel Alder Lake based systems will hang
> during suspend/resume.

Please reflow for 75 characters per line.

Also please mention the exact system you had problems with (also 
firmware versions).

> 
> Add extra check to disable ASPM on Intel AlderLake based systems.

Is that a problem with Intel Alder Lake or the Dell system? Shouldn’t 
ASPM just be disabled for the problematic cards for the Dell system. You 
write newer cards worked fine.

> Fixes: 0064b0ce85bb ("drm/amd/pm: enable ASPM by default")
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1885
> 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);
>   }
>   
> +static bool intel_core_apsm_chk(void)

aspm

> +{
> +#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

Please do the check in C code and not the preprocessor.

> +}
> +
>   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 ||


Kind regards,

Paul

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-08 19:05 [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems Richard Gong
  2022-04-08 19:08 ` Alex Deucher
  2022-04-09  0:19 ` Paul Menzel
@ 2022-04-10 13:54 ` Nils Wallménius
  2022-04-11  0:30   ` Gong, Richard
  2022-04-11  4:15 ` Lazar, Lijo
  2022-04-11 22:57 ` kernel test robot
  4 siblings, 1 reply; 15+ messages in thread
From: Nils Wallménius @ 2022-04-10 13:54 UTC (permalink / raw)
  To: Richard Gong
  Cc: amd-gfx, airlied, xinhui.pan, linux-kernel,
	Maling list - DRI developers, mario.limonciello, Alex Deucher,
	Christian König

[-- Attachment #1: Type: text/plain, Size: 2366 bytes --]

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
> 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?

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
>
>

[-- Attachment #2: Type: text/html, Size: 3752 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-09  0:19 ` Paul Menzel
@ 2022-04-11  0:27   ` Gong, Richard
  2022-04-11  7:41     ` Paul Menzel
  2022-04-11  0:37   ` Gong, Richard
  1 sibling, 1 reply; 15+ messages in thread
From: Gong, Richard @ 2022-04-11  0:27 UTC (permalink / raw)
  To: Paul Menzel
  Cc: airlied, dri-devel, xinhui.pan, linux-kernel, amd-gfx,
	mario.limonciello, alexander.deucher, christian.koenig

Hi Pail.

On 4/8/2022 7:19 PM, Paul Menzel wrote:
> Dear Richard,
>
>
> Thank you for your patch.
>
> Am 08.04.22 um 21:05 schrieb Richard Gong:
>> 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
>
> Alder Lake
will correct in the next version.
>
>> cards as video/display output, Intel Alder Lake based systems will hang
>> during suspend/resume.
>
> Please reflow for 75 characters per line.
>
> Also please mention the exact system you had problems with (also 
> firmware versions).
>
>>
>> Add extra check to disable ASPM on Intel AlderLake based systems.
>
> Is that a problem with Intel Alder Lake or the Dell system? Shouldn’t 
> ASPM just be disabled for the problematic cards for the Dell system. 
> You write newer cards worked fine.

There is a problem with Dell system (Dell Precision DT workstation), 
which is based on Intel Alder Lake.

ASPM works just fine on these GPU's. It's more of an issue with whether 
the underlying platform supports ASPM or not.

>
>> Fixes: 0064b0ce85bb ("drm/amd/pm: enable ASPM by default")
>> Link: 
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1885&amp;data=04%7C01%7Crichard.gong%40amd.com%7C6b94ff2249244c04974e08da19bea71b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637850604066094079%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=%2FiF%2Bnwzm5RlClT%2Fv%2B0RJvmVwsc%2FiwV3jCiFq7PB84wM%3D&amp;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);
>>   }
>>   +static bool intel_core_apsm_chk(void)
>
> aspm
s/apsm/aspm in the next version
>
>> +{
>> +#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
>
> Please do the check in C code and not the preprocessor.
followed the pattern with other upsteram drivers.
>
>> +}
>> +
>>   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 ||
>
>
> Kind regards,
>
> Paul

Regards,

Richard


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-10 13:54 ` Nils Wallménius
@ 2022-04-11  0:30   ` Gong, Richard
  0 siblings, 0 replies; 15+ messages in thread
From: Gong, Richard @ 2022-04-11  0:30 UTC (permalink / raw)
  To: Nils Wallménius
  Cc: amd-gfx, airlied, xinhui.pan, linux-kernel,
	Maling list - DRI developers, mario.limonciello, Alex Deucher,
	Christian König

[-- Attachment #1: Type: text/plain, Size: 3302 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 6698 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-09  0:19 ` Paul Menzel
  2022-04-11  0:27   ` Gong, Richard
@ 2022-04-11  0:37   ` Gong, Richard
  2022-04-11  7:37     ` Paul Menzel
  1 sibling, 1 reply; 15+ messages in thread
From: Gong, Richard @ 2022-04-11  0:37 UTC (permalink / raw)
  To: Paul Menzel
  Cc: airlied, dri-devel, xinhui.pan, linux-kernel, amd-gfx,
	mario.limonciello, alexander.deucher, christian.koenig


On 4/8/2022 7:19 PM, Paul Menzel wrote:
> Dear Richard,
>
>
> Thank you for your patch.
>
> Am 08.04.22 um 21:05 schrieb Richard Gong:
>> 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
>
> Alder Lake
Actually there are 2 formats (one with space, another is w/o space) in 
the upstream sources, so I will keep that unchanged and use the format 
w/o space.
>
>> cards as video/display output, Intel Alder Lake based systems will hang
>> during suspend/resume.
>
> Please reflow for 75 characters per line.
>
> Also please mention the exact system you had problems with (also 
> firmware versions).
>
>>
>> Add extra check to disable ASPM on Intel AlderLake based systems.
>
> Is that a problem with Intel Alder Lake or the Dell system? Shouldn’t 
> ASPM just be disabled for the problematic cards for the Dell system. 
> You write newer cards worked fine.
>
>> Fixes: 0064b0ce85bb ("drm/amd/pm: enable ASPM by default")
>> Link: 
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1885&amp;data=04%7C01%7Crichard.gong%40amd.com%7C6b94ff2249244c04974e08da19bea71b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637850604066094079%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=%2FiF%2Bnwzm5RlClT%2Fv%2B0RJvmVwsc%2FiwV3jCiFq7PB84wM%3D&amp;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);
>>   }
>>   +static bool intel_core_apsm_chk(void)
>
> aspm
>
>> +{
>> +#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
>
> Please do the check in C code and not the preprocessor.
>
>> +}
>> +
>>   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 ||
>
>
> Kind regards,
>
> Paul

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-08 19:05 [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems Richard Gong
                   ` (2 preceding siblings ...)
  2022-04-10 13:54 ` Nils Wallménius
@ 2022-04-11  4:15 ` Lazar, Lijo
  2022-04-11 11:11   ` Gong, Richard
  2022-04-11 22:57 ` kernel test robot
  4 siblings, 1 reply; 15+ messages in thread
From: Lazar, Lijo @ 2022-04-11  4:15 UTC (permalink / raw)
  To: Richard Gong, alexander.deucher, christian.koenig, xinhui.pan,
	airlied, daniel
  Cc: amd-gfx, mario.limonciello, dri-devel, linux-kernel



On 4/9/2022 12:35 AM, Richard Gong wrote:
> 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
> 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);
>   }
>   
> +static bool intel_core_apsm_chk(void)

If this is only for Dell systems, use DMI_SYS_VENDOR/DMI_PRODUCT_NAME to 
identify the platform information from SMBIOS.

Better to rename to aspm_support_quirk_check() or similar, and return 
false on is_alderlake() or is_dell_xyz();

Thanks,
Lijo

> +{
> +#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 ||
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-11  0:37   ` Gong, Richard
@ 2022-04-11  7:37     ` Paul Menzel
  0 siblings, 0 replies; 15+ messages in thread
From: Paul Menzel @ 2022-04-11  7:37 UTC (permalink / raw)
  To: Richard Gong
  Cc: airlied, xinhui.pan, linux-kernel, amd-gfx, dri-devel,
	alexander.deucher, christian.koenig, mario.limonciello

Dear Richard,


Thank you for your response, but please reply to your own reply next time.

Am 11.04.22 um 02:37 schrieb Gong, Richard:
> 
> On 4/8/2022 7:19 PM, Paul Menzel wrote:

>> Thank you for your patch.
>>
>> Am 08.04.22 um 21:05 schrieb Richard Gong:
>>> 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
>>
>> Alder Lake
> Actually there are 2 formats (one with space, another is w/o space) in 
> the upstream sources, so I will keep that unchanged and use the format 
> w/o space.

Do you mean the Linux kernel sources? Anyway, please use the correct 
spelling [1].


Kind regards,

Paul


[1]: 
https://ark.intel.com/content/www/us/en/ark/products/codename/147470/products-formerly-alder-lake.html
[2]: https://en.wikipedia.org/wiki/Alder_Lake

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-11  0:27   ` Gong, Richard
@ 2022-04-11  7:41     ` Paul Menzel
  2022-04-11 11:38       ` Gong, Richard
  0 siblings, 1 reply; 15+ messages in thread
From: Paul Menzel @ 2022-04-11  7:41 UTC (permalink / raw)
  To: Richard Gong
  Cc: airlied, xinhui.pan, linux-kernel, amd-gfx, dri-devel,
	alexander.deucher, Dell.Client.Kernel, christian.koenig,
	mario.limonciello

[Cc: +<Dell.Client.Kernel@dell.com>]

Dear Richard,


Am 11.04.22 um 02:27 schrieb Gong, Richard:

> On 4/8/2022 7:19 PM, Paul Menzel wrote:

>> Am 08.04.22 um 21:05 schrieb Richard Gong:
>>> 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
>>
>> Alder Lake
> will correct in the next version.
>>
>>> cards as video/display output, Intel Alder Lake based systems will hang
>>> during suspend/resume.
>>
>> Please reflow for 75 characters per line.
>>
>> Also please mention the exact system you had problems with (also 
>> firmware versions).
>>
>>>
>>> Add extra check to disable ASPM on Intel AlderLake based systems.
>>
>> Is that a problem with Intel Alder Lake or the Dell system? Shouldn’t 
>> ASPM just be disabled for the problematic cards for the Dell system. 
>> You write newer cards worked fine.
> 
> There is a problem with Dell system (Dell Precision DT workstation), 
> which is based on Intel Alder Lake.
> 
> ASPM works just fine on these GPU's. It's more of an issue with whether 
> the underlying platform supports ASPM or not.

At least you didn’t document what the real issue is, that ASPM does not 
work. With current information (some GPU graphics card with the the Dell 
system and others don’t), it could be the GPU, the Dell system 
(firmware, …), a problem with Alder Lake SOC, or another bug. I hope you 
are in contact with Dell to analyze it, so ASPM can be enabled again.

[…]


Kind regards,

Paul

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-11  4:15 ` Lazar, Lijo
@ 2022-04-11 11:11   ` Gong, Richard
  0 siblings, 0 replies; 15+ messages in thread
From: Gong, Richard @ 2022-04-11 11:11 UTC (permalink / raw)
  To: Lazar, Lijo, alexander.deucher, christian.koenig, xinhui.pan,
	airlied, daniel
  Cc: amd-gfx, mario.limonciello, dri-devel, linux-kernel

Hi Lijo,

On 4/10/2022 11:15 PM, Lazar, Lijo wrote:
>
>
> On 4/9/2022 12:35 AM, Richard Gong wrote:
>> 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
>> 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);
>>   }
>>   +static bool intel_core_apsm_chk(void)
>
> If this is only for Dell systems, use DMI_SYS_VENDOR/DMI_PRODUCT_NAME 
> to identify the platform information from SMBIOS.
Not sure, the report issue was originally for Dell system but may occur 
with others. This is why I just check CPU info.
>
> Better to rename to aspm_support_quirk_check() or similar, and return 
> false on is_alderlake() or is_dell_xyz();
>
Ok, will rename to aspm_support_quick_check in the next versin.

> Thanks,
> Lijo
>
>> +{
>> +#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 ||
>>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-11  7:41     ` Paul Menzel
@ 2022-04-11 11:38       ` Gong, Richard
  2022-04-11 11:45         ` Paul Menzel
  0 siblings, 1 reply; 15+ messages in thread
From: Gong, Richard @ 2022-04-11 11:38 UTC (permalink / raw)
  To: Paul Menzel
  Cc: airlied, xinhui.pan, linux-kernel, amd-gfx, dri-devel,
	alexander.deucher, Dell.Client.Kernel, christian.koenig,
	mario.limonciello

Hi Paul,

On 4/11/2022 2:41 AM, Paul Menzel wrote:
> [Cc: +<Dell.Client.Kernel@dell.com>]
>
> Dear Richard,
>
>
> Am 11.04.22 um 02:27 schrieb Gong, Richard:
>
>> On 4/8/2022 7:19 PM, Paul Menzel wrote:
>
>>> Am 08.04.22 um 21:05 schrieb Richard Gong:
>>>> 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
>>>
>>> Alder Lake
>> will correct in the next version.
>>>
>>>> cards as video/display output, Intel Alder Lake based systems will 
>>>> hang
>>>> during suspend/resume.
>>>
>>> Please reflow for 75 characters per line.
>>>
>>> Also please mention the exact system you had problems with (also 
>>> firmware versions).
>>>
>>>>
>>>> Add extra check to disable ASPM on Intel AlderLake based systems.
>>>
>>> Is that a problem with Intel Alder Lake or the Dell system? 
>>> Shouldn’t ASPM just be disabled for the problematic cards for the 
>>> Dell system. You write newer cards worked fine.
>>
>> There is a problem with Dell system (Dell Precision DT workstation), 
>> which is based on Intel Alder Lake.
>>
>> ASPM works just fine on these GPU's. It's more of an issue with 
>> whether the underlying platform supports ASPM or not.
>
> At least you didn’t document what the real issue is,

You can refer to bug tag from the comment messages.

Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1885

Regards,

Richard

> that ASPM does not work. With current information (some GPU graphics 
> card with the the Dell system and others don’t), it could be the GPU, 
> the Dell system (firmware, …), a problem with Alder Lake SOC, or 
> another bug. I hope you are in contact with Dell to analyze it, so 
> ASPM can be enabled again.
>
> […]
>
>
> Kind regards,
>
> Paul

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-11 11:38       ` Gong, Richard
@ 2022-04-11 11:45         ` Paul Menzel
  0 siblings, 0 replies; 15+ messages in thread
From: Paul Menzel @ 2022-04-11 11:45 UTC (permalink / raw)
  To: Richard Gong
  Cc: airlied, xinhui.pan, linux-kernel, dri-devel, amd-gfx,
	alexander.deucher, Dell.Client.Kernel, christian.koenig,
	mario.limonciello

Dear Richard,


Am 11.04.22 um 13:38 schrieb Gong, Richard:

> On 4/11/2022 2:41 AM, Paul Menzel wrote:
>> [Cc: +<Dell.Client.Kernel@dell.com>]

>> Am 11.04.22 um 02:27 schrieb Gong, Richard:
>>
>>> On 4/8/2022 7:19 PM, Paul Menzel wrote:
>>
>>>> Am 08.04.22 um 21:05 schrieb Richard Gong:
>>>>> 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
>>>>
>>>> Alder Lake
>>> will correct in the next version.
>>>>
>>>>> cards as video/display output, Intel Alder Lake based systems will hang
>>>>> during suspend/resume.
>>>>
>>>> Please reflow for 75 characters per line.
>>>>
>>>> Also please mention the exact system you had problems with (also 
>>>> firmware versions).
>>>>
>>>>>
>>>>> Add extra check to disable ASPM on Intel AlderLake based systems.
>>>>
>>>> Is that a problem with Intel Alder Lake or the Dell system? 
>>>> Shouldn’t ASPM just be disabled for the problematic cards for the 
>>>> Dell system. You write newer cards worked fine.
>>>
>>> There is a problem with Dell system (Dell Precision DT workstation), 
>>> which is based on Intel Alder Lake.
>>>
>>> ASPM works just fine on these GPU's. It's more of an issue with 
>>> whether the underlying platform supports ASPM or not.
>>
>> At least you didn’t document what the real issue is,
> 
> You can refer to bug tag from the comment messages.
> 
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1885

No, the commit message should be self-contained, and reviewers and 
readers of the commit message not required to read comments of bug 
reports. Please add the necessary information to the commit message.


Kind regards,

Paul


>> that ASPM does not work. With current information (some GPU graphics 
>> card with the the Dell system and others don’t), it could be the GPU, 
>> the Dell system (firmware, …), a problem with Alder Lake SOC, or 
>> another bug. I hope you are in contact with Dell to analyze it, so 
>> ASPM can be enabled again.
>>
>> […]
>>
>>
>> Kind regards,
>>
>> Paul

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems
  2022-04-08 19:05 [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems Richard Gong
                   ` (3 preceding siblings ...)
  2022-04-11  4:15 ` Lazar, Lijo
@ 2022-04-11 22:57 ` kernel test robot
  4 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2022-04-11 22:57 UTC (permalink / raw)
  To: Richard Gong, alexander.deucher, christian.koenig, xinhui.pan,
	airlied, daniel
  Cc: kbuild-all, richard.gong, dri-devel, linux-kernel, amd-gfx,
	mario.limonciello

Hi Richard,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v5.18-rc2 next-20220411]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Richard-Gong/drm-amdgpu-disable-ASPM-on-Intel-AlderLake-based-systems/20220409-030656
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: um-allmodconfig (https://download.01.org/0day-ci/archive/20220412/202204120618.4I6dOUw9-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/bb9a037cafa91918c2ece823591d1d04b812ae17
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Richard-Gong/drm-amdgpu-disable-ASPM-on-Intel-AlderLake-based-systems/20220409-030656
        git checkout bb9a037cafa91918c2ece823591d1d04b812ae17
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=um SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/x86/um/asm/processor.h:41,
                    from include/linux/mutex.h:19,
                    from include/linux/kernfs.h:11,
                    from include/linux/sysfs.h:16,
                    from include/linux/kobject.h:20,
                    from include/linux/pci.h:35,
                    from drivers/gpu/drm/amd/amdgpu/vi.c:24:
   drivers/gpu/drm/amd/amdgpu/vi.c: In function 'intel_core_apsm_chk':
   arch/um/include/asm/processor-generic.h:103:19: error: called object is not a function or function pointer
     103 | #define cpu_data (&boot_cpu_data)
         |                  ~^~~~~~~~~~~~~~~
   drivers/gpu/drm/amd/amdgpu/vi.c:1144:34: note: in expansion of macro 'cpu_data'
    1144 |         struct cpuinfo_x86 *c = &cpu_data(0);
         |                                  ^~~~~~~~
>> drivers/gpu/drm/amd/amdgpu/vi.c:1146:18: error: invalid use of undefined type 'struct cpuinfo_x86'
    1146 |         return (c->x86 == 6 && c->x86_model == INTEL_FAM6_ALDERLAKE);
         |                  ^~
   drivers/gpu/drm/amd/amdgpu/vi.c:1146:33: error: invalid use of undefined type 'struct cpuinfo_x86'
    1146 |         return (c->x86 == 6 && c->x86_model == INTEL_FAM6_ALDERLAKE);
         |                                 ^~
   drivers/gpu/drm/amd/amdgpu/vi.c:1150:1: error: control reaches end of non-void function [-Werror=return-type]
    1150 | }
         | ^
   cc1: some warnings being treated as errors


vim +1146 drivers/gpu/drm/amd/amdgpu/vi.c

  1140	
  1141	static bool intel_core_apsm_chk(void)
  1142	{
  1143	#if IS_ENABLED(CONFIG_X86_64)
  1144		struct cpuinfo_x86 *c = &cpu_data(0);
  1145	
> 1146		return (c->x86 == 6 && c->x86_model == INTEL_FAM6_ALDERLAKE);
  1147	#else
  1148		return false;
  1149	#endif
  1150	}
  1151	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-04-11 22:58 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-08 19:05 [PATCHv2] drm/amdgpu: disable ASPM on Intel AlderLake based systems Richard Gong
2022-04-08 19:08 ` Alex Deucher
2022-04-08 19:12   ` Limonciello, Mario
2022-04-09  0:19 ` Paul Menzel
2022-04-11  0:27   ` Gong, Richard
2022-04-11  7:41     ` Paul Menzel
2022-04-11 11:38       ` Gong, Richard
2022-04-11 11:45         ` Paul Menzel
2022-04-11  0:37   ` Gong, Richard
2022-04-11  7:37     ` Paul Menzel
2022-04-10 13:54 ` Nils Wallménius
2022-04-11  0:30   ` Gong, Richard
2022-04-11  4:15 ` Lazar, Lijo
2022-04-11 11:11   ` Gong, Richard
2022-04-11 22:57 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).