diff mbox series

[v1] drm/i915/dg2: enable G8 with a workaround

Message ID 20241007122424.642796-1-raag.jadav@intel.com (mailing list archive)
State New
Headers show
Series [v1] drm/i915/dg2: enable G8 with a workaround | expand

Commit Message

Raag Jadav Oct. 7, 2024, 12:24 p.m. UTC
Host BIOS doesn't enable G8 power mode due to an issue on DG2, so we
enable it from kernel with Wa_14022698589. Currently it is enabled for
all DG2 devices with the exception of a few, for which, it is enabled
only when paired with whitelisted CPU models.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 43 +++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h             |  1 +
 2 files changed, 44 insertions(+)

Comments

Nilawar, Badal Oct. 7, 2024, 1:05 p.m. UTC | #1
On 07-10-2024 17:54, Raag Jadav wrote:
> Host BIOS doesn't enable G8 power mode due to an issue on DG2, so we
> enable it from kernel with Wa_14022698589. Currently it is enabled for
> all DG2 devices with the exception of a few, for which, it is enabled
> only when paired with whitelisted CPU models.
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 43 +++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_reg.h             |  1 +
>   2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index e539a656cfc3..b2db51377488 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -14,11 +14,30 @@
>   #include "intel_gt_mcr.h"
>   #include "intel_gt_print.h"
>   #include "intel_gt_regs.h"
> +#include "intel_pcode.h"
>   #include "intel_ring.h"
>   #include "intel_workarounds.h"
>   
>   #include "display/intel_fbc_regs.h"
>   
> +#ifdef CONFIG_X86
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +static const struct x86_cpu_id g8_cpu_ids[] = {
> +	X86_MATCH_VFM(INTEL_ALDERLAKE,		NULL),
> +	X86_MATCH_VFM(INTEL_ALDERLAKE_L,	NULL),
> +	X86_MATCH_VFM(INTEL_COMETLAKE,		NULL),
> +	X86_MATCH_VFM(INTEL_KABYLAKE,		NULL),
> +	X86_MATCH_VFM(INTEL_KABYLAKE_L,		NULL),
> +	X86_MATCH_VFM(INTEL_RAPTORLAKE,		NULL),
> +	X86_MATCH_VFM(INTEL_RAPTORLAKE_P,	NULL),
> +	X86_MATCH_VFM(INTEL_RAPTORLAKE_S,	NULL),
> +	X86_MATCH_VFM(INTEL_ROCKETLAKE,		NULL),
> +	{}
> +};
> +#endif
> +
>   /**
>    * DOC: Hardware workarounds
>    *
> @@ -1770,9 +1789,33 @@ static void wa_list_apply(const struct i915_wa_list *wal)
>   	intel_gt_mcr_unlock(gt, flags);
>   }
>   
> +#define DG2_G8_WA_RANGE_1		0x56A0 ... 0x56AF
> +#define DG2_G8_WA_RANGE_2		0x56B0 ... 0x56B9
> +
> +/* Wa_14022698589:dg2 */

As per bspecs correct Wa id for this Wa is 14022698537.

Regards,
Badal

> +static void intel_enable_g8(struct intel_uncore *uncore)
> +{
> +	if (IS_DG2(uncore->i915)) {
> +		switch (INTEL_DEVID(uncore->i915)) {
> +		case DG2_G8_WA_RANGE_1:
> +		case DG2_G8_WA_RANGE_2:
> +#ifdef CONFIG_X86
> +			if (!x86_match_cpu(g8_cpu_ids))
> +#endif
> +				return;
> +		}
> +
> +		snb_pcode_write_p(uncore, PCODE_POWER_SETUP,
> +				  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
> +	}
> +}
> +
>   void intel_gt_apply_workarounds(struct intel_gt *gt)
>   {
>   	wa_list_apply(&gt->wa_list);
> +
> +	/* Special case for pcode mailbox which can't be on wa_list */
> +	intel_enable_g8(gt->uncore);
>   }
>   
>   static bool wa_list_verify(struct intel_gt *gt,
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 41f4350a7c6c..e948b194550c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3568,6 +3568,7 @@
>   #define   PCODE_POWER_SETUP			0x7C
>   #define     POWER_SETUP_SUBCOMMAND_READ_I1	0x4
>   #define     POWER_SETUP_SUBCOMMAND_WRITE_I1	0x5
> +#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6
>   #define	    POWER_SETUP_I1_WATTS		REG_BIT(31)
>   #define	    POWER_SETUP_I1_SHIFT		6	/* 10.6 fixed point format */
>   #define	    POWER_SETUP_I1_DATA_MASK		REG_GENMASK(15, 0)
Nilawar, Badal Oct. 7, 2024, 1:17 p.m. UTC | #2
On 07-10-2024 18:35, Nilawar, Badal wrote:
> 
> 
> On 07-10-2024 17:54, Raag Jadav wrote:

Usually Wa_ID is included in commit message title for simplified searching.

~/workspace/drmtip/drm-tip$ git log --oneline | grep "Wa_"
da9a73b7b25e drm/xe/xe2hpg: Add Wa_15016589081
9db969b36b2f drm/xe/xe2hpg: Add Wa_15016589081
54f90b033359 drm/i915/guc: Fix missing enable of Wa_14019159160 on ARL
8776b0234e1d drm/xe/xe2hpg: Add Wa_14021821874
cbc6e98ab11b drm/xe/xe2: Add Wa_15015404425
03a2dc84f5c4 drm/xe/xe2lpm: Extend Wa_16021639441
27cb2b7fec2a drm/xe/bmg: implement Wa_16023588340
1d734a3e5d6b drm/xe: Name and document Wa_14019789679
21ff3a16e92e drm/xe/xe2hpg: Add Wa_14021821874
843f10ce6539 drm/i915/gt: Add Wa_14019789679
104bcfae57d8 drm/i915/arl: Enable Wa_14019159160 for ARL
86c5b70a9c0c drm/xe/xe2: Add Wa_15015404425
74e307680006 drm/xe/xe2lpm: Extend Wa_16021639441
c55f79f317ab drm/i915: disable fbc due to Wa_16023588340
01570b446939 drm/xe/bmg: implement Wa_16023588340
aaa08078e725 drm/xe/bmg: Apply Wa_22019338487
3b1592fb7835 drm/xe/lnl: Apply Wa_22019338487
d35386b3a77b drm/xe/xelpgp: Extend Wa_14019877138 to graphics 12.74
62712be3a4e0 drm/xe/xe2: Add proper check for media in Wa_14020756599
5d7612ae201e drm/xe/xe2lpg: Add Wa_14021490052
24d0d98af1c3 drm/xe/xe2lpm: Fixup Wa_14020756599
131328aa5699 drm/xe/xe2lpm: Add permanent Wa_14020756599

Badal


>> Host BIOS doesn't enable G8 power mode due to an issue on DG2, so we
>> enable it from kernel with Wa_14022698589. Currently it is enabled for
>> all DG2 devices with the exception of a few, for which, it is enabled
>> only when paired with whitelisted CPU models.
>>
>> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 43 +++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_reg.h             |  1 +
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/ 
>> gpu/drm/i915/gt/intel_workarounds.c
>> index e539a656cfc3..b2db51377488 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -14,11 +14,30 @@
>>   #include "intel_gt_mcr.h"
>>   #include "intel_gt_print.h"
>>   #include "intel_gt_regs.h"
>> +#include "intel_pcode.h"
>>   #include "intel_ring.h"
>>   #include "intel_workarounds.h"
>>   #include "display/intel_fbc_regs.h"
>> +#ifdef CONFIG_X86
>> +#include <asm/cpu_device_id.h>
>> +#include <asm/intel-family.h>
>> +
>> +static const struct x86_cpu_id g8_cpu_ids[] = {
>> +    X86_MATCH_VFM(INTEL_ALDERLAKE,        NULL),
>> +    X86_MATCH_VFM(INTEL_ALDERLAKE_L,    NULL),
>> +    X86_MATCH_VFM(INTEL_COMETLAKE,        NULL),
>> +    X86_MATCH_VFM(INTEL_KABYLAKE,        NULL),
>> +    X86_MATCH_VFM(INTEL_KABYLAKE_L,        NULL),
>> +    X86_MATCH_VFM(INTEL_RAPTORLAKE,        NULL),
>> +    X86_MATCH_VFM(INTEL_RAPTORLAKE_P,    NULL),
>> +    X86_MATCH_VFM(INTEL_RAPTORLAKE_S,    NULL),
>> +    X86_MATCH_VFM(INTEL_ROCKETLAKE,        NULL),
>> +    {}
>> +};
>> +#endif
>> +
>>   /**
>>    * DOC: Hardware workarounds
>>    *
>> @@ -1770,9 +1789,33 @@ static void wa_list_apply(const struct 
>> i915_wa_list *wal)
>>       intel_gt_mcr_unlock(gt, flags);
>>   }
>> +#define DG2_G8_WA_RANGE_1        0x56A0 ... 0x56AF
>> +#define DG2_G8_WA_RANGE_2        0x56B0 ... 0x56B9
>> +
>> +/* Wa_14022698589:dg2 */
> 
> As per bspecs correct Wa id for this Wa is 14022698537.
> 
> Regards,
> Badal
> 
>> +static void intel_enable_g8(struct intel_uncore *uncore)
>> +{
>> +    if (IS_DG2(uncore->i915)) {
>> +        switch (INTEL_DEVID(uncore->i915)) {
>> +        case DG2_G8_WA_RANGE_1:
>> +        case DG2_G8_WA_RANGE_2:
>> +#ifdef CONFIG_X86
>> +            if (!x86_match_cpu(g8_cpu_ids))
>> +#endif
>> +                return;
>> +        }
>> +
>> +        snb_pcode_write_p(uncore, PCODE_POWER_SETUP,
>> +                  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
>> +    }
>> +}
>> +
>>   void intel_gt_apply_workarounds(struct intel_gt *gt)
>>   {
>>       wa_list_apply(&gt->wa_list);
>> +
>> +    /* Special case for pcode mailbox which can't be on wa_list */
>> +    intel_enable_g8(gt->uncore);
>>   }
>>   static bool wa_list_verify(struct intel_gt *gt,
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/ 
>> i915_reg.h
>> index 41f4350a7c6c..e948b194550c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3568,6 +3568,7 @@
>>   #define   PCODE_POWER_SETUP            0x7C
>>   #define     POWER_SETUP_SUBCOMMAND_READ_I1    0x4
>>   #define     POWER_SETUP_SUBCOMMAND_WRITE_I1    0x5
>> +#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE    0x6
>>   #define        POWER_SETUP_I1_WATTS        REG_BIT(31)
>>   #define        POWER_SETUP_I1_SHIFT        6    /* 10.6 fixed point 
>> format */
>>   #define        POWER_SETUP_I1_DATA_MASK        REG_GENMASK(15, 0)
>
Jani Nikula Oct. 8, 2024, 5:24 p.m. UTC | #3
On Mon, 07 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> Host BIOS doesn't enable G8 power mode due to an issue on DG2, so we
> enable it from kernel with Wa_14022698589. Currently it is enabled for
> all DG2 devices with the exception of a few, for which, it is enabled
> only when paired with whitelisted CPU models.

In commit messages "currently" and the present tense usually refer to
the status quo before the patch has been merged. Doesn't seem to be the
case here, and it confuses what we have now and what the patch changes.

>
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 43 +++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h             |  1 +
>  2 files changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index e539a656cfc3..b2db51377488 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -14,11 +14,30 @@
>  #include "intel_gt_mcr.h"
>  #include "intel_gt_print.h"
>  #include "intel_gt_regs.h"
> +#include "intel_pcode.h"
>  #include "intel_ring.h"
>  #include "intel_workarounds.h"
>  
>  #include "display/intel_fbc_regs.h"
>  
> +#ifdef CONFIG_X86
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +static const struct x86_cpu_id g8_cpu_ids[] = {
> +	X86_MATCH_VFM(INTEL_ALDERLAKE,		NULL),
> +	X86_MATCH_VFM(INTEL_ALDERLAKE_L,	NULL),
> +	X86_MATCH_VFM(INTEL_COMETLAKE,		NULL),
> +	X86_MATCH_VFM(INTEL_KABYLAKE,		NULL),
> +	X86_MATCH_VFM(INTEL_KABYLAKE_L,		NULL),
> +	X86_MATCH_VFM(INTEL_RAPTORLAKE,		NULL),
> +	X86_MATCH_VFM(INTEL_RAPTORLAKE_P,	NULL),
> +	X86_MATCH_VFM(INTEL_RAPTORLAKE_S,	NULL),
> +	X86_MATCH_VFM(INTEL_ROCKETLAKE,		NULL),
> +	{}
> +};
> +#endif
> +
>  /**
>   * DOC: Hardware workarounds
>   *
> @@ -1770,9 +1789,33 @@ static void wa_list_apply(const struct i915_wa_list *wal)
>  	intel_gt_mcr_unlock(gt, flags);
>  }
>  
> +#define DG2_G8_WA_RANGE_1		0x56A0 ... 0x56AF
> +#define DG2_G8_WA_RANGE_2		0x56B0 ... 0x56B9

Absolutely not.

> +
> +/* Wa_14022698589:dg2 */
> +static void intel_enable_g8(struct intel_uncore *uncore)
> +{
> +	if (IS_DG2(uncore->i915)) {
> +		switch (INTEL_DEVID(uncore->i915)) {

Even using INTEL_DEVID() is a no-go. There are currently four users, and
even some of them are too much.

We try hard to abstract this stuff at a higher level, and there must be
zero direct PCI ID checks in code other than the table driven device
identification. Otherwise it's just impossible to figure out where we do
platform specific stuff for each platform.

I understand those ranges above are a PITA to deal with, because they
span across DG2 subplatforms.

BR,
Jani.



> +		case DG2_G8_WA_RANGE_1:
> +		case DG2_G8_WA_RANGE_2:
> +#ifdef CONFIG_X86
> +			if (!x86_match_cpu(g8_cpu_ids))
> +#endif
> +				return;
> +		}
> +
> +		snb_pcode_write_p(uncore, PCODE_POWER_SETUP,
> +				  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
> +	}
> +}
> +
>  void intel_gt_apply_workarounds(struct intel_gt *gt)
>  {
>  	wa_list_apply(&gt->wa_list);
> +
> +	/* Special case for pcode mailbox which can't be on wa_list */
> +	intel_enable_g8(gt->uncore);
>  }
>  
>  static bool wa_list_verify(struct intel_gt *gt,
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 41f4350a7c6c..e948b194550c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3568,6 +3568,7 @@
>  #define   PCODE_POWER_SETUP			0x7C
>  #define     POWER_SETUP_SUBCOMMAND_READ_I1	0x4
>  #define     POWER_SETUP_SUBCOMMAND_WRITE_I1	0x5
> +#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6
>  #define	    POWER_SETUP_I1_WATTS		REG_BIT(31)
>  #define	    POWER_SETUP_I1_SHIFT		6	/* 10.6 fixed point format */
>  #define	    POWER_SETUP_I1_DATA_MASK		REG_GENMASK(15, 0)
Raag Jadav Oct. 9, 2024, 12:47 p.m. UTC | #4
On Tue, Oct 08, 2024 at 08:24:42PM +0300, Jani Nikula wrote:
> On Mon, 07 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> > Host BIOS doesn't enable G8 power mode due to an issue on DG2, so we
> > enable it from kernel with Wa_14022698589. Currently it is enabled for
> > all DG2 devices with the exception of a few, for which, it is enabled
> > only when paired with whitelisted CPU models.
> 
> In commit messages "currently" and the present tense usually refer to
> the status quo before the patch has been merged. Doesn't seem to be the
> case here, and it confuses what we have now and what the patch changes.

True.

> >
> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 43 +++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_reg.h             |  1 +
> >  2 files changed, 44 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index e539a656cfc3..b2db51377488 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -14,11 +14,30 @@
> >  #include "intel_gt_mcr.h"
> >  #include "intel_gt_print.h"
> >  #include "intel_gt_regs.h"
> > +#include "intel_pcode.h"
> >  #include "intel_ring.h"
> >  #include "intel_workarounds.h"
> >  
> >  #include "display/intel_fbc_regs.h"
> >  
> > +#ifdef CONFIG_X86
> > +#include <asm/cpu_device_id.h>
> > +#include <asm/intel-family.h>
> > +
> > +static const struct x86_cpu_id g8_cpu_ids[] = {
> > +	X86_MATCH_VFM(INTEL_ALDERLAKE,		NULL),
> > +	X86_MATCH_VFM(INTEL_ALDERLAKE_L,	NULL),
> > +	X86_MATCH_VFM(INTEL_COMETLAKE,		NULL),
> > +	X86_MATCH_VFM(INTEL_KABYLAKE,		NULL),
> > +	X86_MATCH_VFM(INTEL_KABYLAKE_L,		NULL),
> > +	X86_MATCH_VFM(INTEL_RAPTORLAKE,		NULL),
> > +	X86_MATCH_VFM(INTEL_RAPTORLAKE_P,	NULL),
> > +	X86_MATCH_VFM(INTEL_RAPTORLAKE_S,	NULL),
> > +	X86_MATCH_VFM(INTEL_ROCKETLAKE,		NULL),
> > +	{}
> > +};
> > +#endif
> > +
> >  /**
> >   * DOC: Hardware workarounds
> >   *
> > @@ -1770,9 +1789,33 @@ static void wa_list_apply(const struct i915_wa_list *wal)
> >  	intel_gt_mcr_unlock(gt, flags);
> >  }
> >  
> > +#define DG2_G8_WA_RANGE_1		0x56A0 ... 0x56AF
> > +#define DG2_G8_WA_RANGE_2		0x56B0 ... 0x56B9
> 
> Absolutely not.

I had an "ugly" self-note which I forgot to add while sending out :D

> > +
> > +/* Wa_14022698589:dg2 */
> > +static void intel_enable_g8(struct intel_uncore *uncore)
> > +{
> > +	if (IS_DG2(uncore->i915)) {
> > +		switch (INTEL_DEVID(uncore->i915)) {
> 
> Even using INTEL_DEVID() is a no-go. There are currently four users, and
> even some of them are too much.
> 
> We try hard to abstract this stuff at a higher level, and there must be
> zero direct PCI ID checks in code other than the table driven device
> identification. Otherwise it's just impossible to figure out where we do
> platform specific stuff for each platform.

Even if we use pci_match_id(), we'd need an explicit list to match against.
Any better way?

Raag
Jani Nikula Oct. 9, 2024, 1:05 p.m. UTC | #5
On Wed, 09 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> On Tue, Oct 08, 2024 at 08:24:42PM +0300, Jani Nikula wrote:
>> On Mon, 07 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
>> > Host BIOS doesn't enable G8 power mode due to an issue on DG2, so we
>> > enable it from kernel with Wa_14022698589. Currently it is enabled for
>> > all DG2 devices with the exception of a few, for which, it is enabled
>> > only when paired with whitelisted CPU models.
>> 
>> In commit messages "currently" and the present tense usually refer to
>> the status quo before the patch has been merged. Doesn't seem to be the
>> case here, and it confuses what we have now and what the patch changes.
>
> True.
>
>> >
>> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 43 +++++++++++++++++++++
>> >  drivers/gpu/drm/i915/i915_reg.h             |  1 +
>> >  2 files changed, 44 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > index e539a656cfc3..b2db51377488 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > @@ -14,11 +14,30 @@
>> >  #include "intel_gt_mcr.h"
>> >  #include "intel_gt_print.h"
>> >  #include "intel_gt_regs.h"
>> > +#include "intel_pcode.h"
>> >  #include "intel_ring.h"
>> >  #include "intel_workarounds.h"
>> >  
>> >  #include "display/intel_fbc_regs.h"
>> >  
>> > +#ifdef CONFIG_X86
>> > +#include <asm/cpu_device_id.h>
>> > +#include <asm/intel-family.h>
>> > +
>> > +static const struct x86_cpu_id g8_cpu_ids[] = {
>> > +	X86_MATCH_VFM(INTEL_ALDERLAKE,		NULL),
>> > +	X86_MATCH_VFM(INTEL_ALDERLAKE_L,	NULL),
>> > +	X86_MATCH_VFM(INTEL_COMETLAKE,		NULL),
>> > +	X86_MATCH_VFM(INTEL_KABYLAKE,		NULL),
>> > +	X86_MATCH_VFM(INTEL_KABYLAKE_L,		NULL),
>> > +	X86_MATCH_VFM(INTEL_RAPTORLAKE,		NULL),
>> > +	X86_MATCH_VFM(INTEL_RAPTORLAKE_P,	NULL),
>> > +	X86_MATCH_VFM(INTEL_RAPTORLAKE_S,	NULL),
>> > +	X86_MATCH_VFM(INTEL_ROCKETLAKE,		NULL),
>> > +	{}
>> > +};
>> > +#endif
>> > +
>> >  /**
>> >   * DOC: Hardware workarounds
>> >   *
>> > @@ -1770,9 +1789,33 @@ static void wa_list_apply(const struct i915_wa_list *wal)
>> >  	intel_gt_mcr_unlock(gt, flags);
>> >  }
>> >  
>> > +#define DG2_G8_WA_RANGE_1		0x56A0 ... 0x56AF
>> > +#define DG2_G8_WA_RANGE_2		0x56B0 ... 0x56B9
>> 
>> Absolutely not.
>
> I had an "ugly" self-note which I forgot to add while sending out :D
>
>> > +
>> > +/* Wa_14022698589:dg2 */
>> > +static void intel_enable_g8(struct intel_uncore *uncore)
>> > +{
>> > +	if (IS_DG2(uncore->i915)) {
>> > +		switch (INTEL_DEVID(uncore->i915)) {
>> 
>> Even using INTEL_DEVID() is a no-go. There are currently four users, and
>> even some of them are too much.
>> 
>> We try hard to abstract this stuff at a higher level, and there must be
>> zero direct PCI ID checks in code other than the table driven device
>> identification. Otherwise it's just impossible to figure out where we do
>> platform specific stuff for each platform.
>
> Even if we use pci_match_id(), we'd need an explicit list to match against.

Well, we don't use that for individual workarounds or hacks either. When
you think of using something like that, see what git grep says.

> Any better way?

You probably need to turn it into another subplatform, and add it in
intel_device_info.c. You're probably going to need to rehash the
INTEL_DG2_*_IDS PCI ID macros too. That's how we tell platforms apart at
the PCI ID granularity.

BR,
Jani.


>
> Raag
Raag Jadav Oct. 9, 2024, 4:42 p.m. UTC | #6
On Wed, Oct 09, 2024 at 04:05:20PM +0300, Jani Nikula wrote:
> On Wed, 09 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> > On Tue, Oct 08, 2024 at 08:24:42PM +0300, Jani Nikula wrote:
> >> On Mon, 07 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> >> > +
> >> > +/* Wa_14022698589:dg2 */
> >> > +static void intel_enable_g8(struct intel_uncore *uncore)
> >> > +{
> >> > +	if (IS_DG2(uncore->i915)) {
> >> > +		switch (INTEL_DEVID(uncore->i915)) {
> >> 
> >> Even using INTEL_DEVID() is a no-go. There are currently four users, and
> >> even some of them are too much.
> >> 
> >> We try hard to abstract this stuff at a higher level, and there must be
> >> zero direct PCI ID checks in code other than the table driven device
> >> identification. Otherwise it's just impossible to figure out where we do
> >> platform specific stuff for each platform.
> >
> > Even if we use pci_match_id(), we'd need an explicit list to match against.
> 
> Well, we don't use that for individual workarounds or hacks either. When
> you think of using something like that, see what git grep says.
> 
> > Any better way?
> 
> You probably need to turn it into another subplatform, and add it in
> intel_device_info.c. You're probably going to need to rehash the
> INTEL_DG2_*_IDS PCI ID macros too. That's how we tell platforms apart at
> the PCI ID granularity.

Which would be controversial since the ids span across existing subplatforms,
which we don't want to break.

Don't we have quirk mask thing like display?

Raag
Jani Nikula Oct. 9, 2024, 4:50 p.m. UTC | #7
On Wed, 09 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> On Wed, Oct 09, 2024 at 04:05:20PM +0300, Jani Nikula wrote:
>> On Wed, 09 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
>> > On Tue, Oct 08, 2024 at 08:24:42PM +0300, Jani Nikula wrote:
>> >> On Mon, 07 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
>> >> > +
>> >> > +/* Wa_14022698589:dg2 */
>> >> > +static void intel_enable_g8(struct intel_uncore *uncore)
>> >> > +{
>> >> > +	if (IS_DG2(uncore->i915)) {
>> >> > +		switch (INTEL_DEVID(uncore->i915)) {
>> >> 
>> >> Even using INTEL_DEVID() is a no-go. There are currently four users, and
>> >> even some of them are too much.
>> >> 
>> >> We try hard to abstract this stuff at a higher level, and there must be
>> >> zero direct PCI ID checks in code other than the table driven device
>> >> identification. Otherwise it's just impossible to figure out where we do
>> >> platform specific stuff for each platform.
>> >
>> > Even if we use pci_match_id(), we'd need an explicit list to match against.
>> 
>> Well, we don't use that for individual workarounds or hacks either. When
>> you think of using something like that, see what git grep says.
>> 
>> > Any better way?
>> 
>> You probably need to turn it into another subplatform, and add it in
>> intel_device_info.c. You're probably going to need to rehash the
>> INTEL_DG2_*_IDS PCI ID macros too. That's how we tell platforms apart at
>> the PCI ID granularity.
>
> Which would be controversial since the ids span across existing subplatforms,
> which we don't want to break.

The i915 subplatform thing supports multiple subplatforms.

> Don't we have quirk mask thing like display?

Please elaborate.

BR,
Jani.
Matt Roper Oct. 9, 2024, 7:05 p.m. UTC | #8
On Wed, Oct 09, 2024 at 07:42:40PM +0300, Raag Jadav wrote:
> On Wed, Oct 09, 2024 at 04:05:20PM +0300, Jani Nikula wrote:
> > On Wed, 09 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> > > On Tue, Oct 08, 2024 at 08:24:42PM +0300, Jani Nikula wrote:
> > >> On Mon, 07 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> > >> > +
> > >> > +/* Wa_14022698589:dg2 */
> > >> > +static void intel_enable_g8(struct intel_uncore *uncore)
> > >> > +{
> > >> > +	if (IS_DG2(uncore->i915)) {
> > >> > +		switch (INTEL_DEVID(uncore->i915)) {
> > >> 
> > >> Even using INTEL_DEVID() is a no-go. There are currently four users, and
> > >> even some of them are too much.
> > >> 
> > >> We try hard to abstract this stuff at a higher level, and there must be
> > >> zero direct PCI ID checks in code other than the table driven device
> > >> identification. Otherwise it's just impossible to figure out where we do
> > >> platform specific stuff for each platform.
> > >
> > > Even if we use pci_match_id(), we'd need an explicit list to match against.
> > 
> > Well, we don't use that for individual workarounds or hacks either. When
> > you think of using something like that, see what git grep says.
> > 
> > > Any better way?
> > 
> > You probably need to turn it into another subplatform, and add it in
> > intel_device_info.c. You're probably going to need to rehash the
> > INTEL_DG2_*_IDS PCI ID macros too. That's how we tell platforms apart at
> > the PCI ID granularity.
> 
> Which would be controversial since the ids span across existing subplatforms,
> which we don't want to break.

A single platform can belong to multiple subplatforms.  So it's fine,
for example, for a specific device ID to belong to both the DG2-G11
subplatform and the "do extra G8 stuff" subplatform.  You'll just need
to make sure you separate out the G8 subplatform into a separate 'if'
statement rather than trying to include it in the current if/else
ladder.


Matt

> 
> Don't we have quirk mask thing like display?
> 
> Raag
Gupta, Anshuman Oct. 14, 2024, 4:31 a.m. UTC | #9
> -----Original Message-----
> From: Jadav, Raag <raag.jadav@intel.com>
> Sent: Wednesday, October 9, 2024 6:18 PM
> To: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: joonas.lahtinen@linux.intel.com; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> Roper, Matthew D <matthew.d.roper@intel.com>; andi.shyti@linux.intel.com;
> intel-gfx@lists.freedesktop.org; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
> Tauro, Riana <riana.tauro@intel.com>
> Subject: Re: [PATCH v1] drm/i915/dg2: enable G8 with a workaround
> 
> On Tue, Oct 08, 2024 at 08:24:42PM +0300, Jani Nikula wrote:
> > On Mon, 07 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> > > Host BIOS doesn't enable G8 power mode due to an issue on DG2, so we
AFAIU it discrete card firmware not HOST bios.
Thanks,
Anshuman.
> > > enable it from kernel with Wa_14022698589. Currently it is enabled
> > > for all DG2 devices with the exception of a few, for which, it is
> > > enabled only when paired with whitelisted CPU models.
> >
> > In commit messages "currently" and the present tense usually refer to
> > the status quo before the patch has been merged. Doesn't seem to be
> > the case here, and it confuses what we have now and what the patch changes.
> 
> True.
> 
> > >
> > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 43
> +++++++++++++++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h             |  1 +
> > >  2 files changed, 44 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index e539a656cfc3..b2db51377488 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -14,11 +14,30 @@
> > >  #include "intel_gt_mcr.h"
> > >  #include "intel_gt_print.h"
> > >  #include "intel_gt_regs.h"
> > > +#include "intel_pcode.h"
> > >  #include "intel_ring.h"
> > >  #include "intel_workarounds.h"
> > >
> > >  #include "display/intel_fbc_regs.h"
> > >
> > > +#ifdef CONFIG_X86
> > > +#include <asm/cpu_device_id.h>
> > > +#include <asm/intel-family.h>
> > > +
> > > +static const struct x86_cpu_id g8_cpu_ids[] = {
> > > +	X86_MATCH_VFM(INTEL_ALDERLAKE,		NULL),
> > > +	X86_MATCH_VFM(INTEL_ALDERLAKE_L,	NULL),
> > > +	X86_MATCH_VFM(INTEL_COMETLAKE,		NULL),
> > > +	X86_MATCH_VFM(INTEL_KABYLAKE,		NULL),
> > > +	X86_MATCH_VFM(INTEL_KABYLAKE_L,		NULL),
> > > +	X86_MATCH_VFM(INTEL_RAPTORLAKE,		NULL),
> > > +	X86_MATCH_VFM(INTEL_RAPTORLAKE_P,	NULL),
> > > +	X86_MATCH_VFM(INTEL_RAPTORLAKE_S,	NULL),
> > > +	X86_MATCH_VFM(INTEL_ROCKETLAKE,		NULL),
> > > +	{}
> > > +};
> > > +#endif
> > > +
> > >  /**
> > >   * DOC: Hardware workarounds
> > >   *
> > > @@ -1770,9 +1789,33 @@ static void wa_list_apply(const struct
> i915_wa_list *wal)
> > >  	intel_gt_mcr_unlock(gt, flags);
> > >  }
> > >
> > > +#define DG2_G8_WA_RANGE_1		0x56A0 ... 0x56AF
> > > +#define DG2_G8_WA_RANGE_2		0x56B0 ... 0x56B9
> >
> > Absolutely not.
> 
> I had an "ugly" self-note which I forgot to add while sending out :D
> 
> > > +
> > > +/* Wa_14022698589:dg2 */
> > > +static void intel_enable_g8(struct intel_uncore *uncore) {
> > > +	if (IS_DG2(uncore->i915)) {
> > > +		switch (INTEL_DEVID(uncore->i915)) {
> >
> > Even using INTEL_DEVID() is a no-go. There are currently four users,
> > and even some of them are too much.
> >
> > We try hard to abstract this stuff at a higher level, and there must
> > be zero direct PCI ID checks in code other than the table driven
> > device identification. Otherwise it's just impossible to figure out
> > where we do platform specific stuff for each platform.
> 
> Even if we use pci_match_id(), we'd need an explicit list to match against.
> Any better way?
> 
> Raag
kernel test robot Oct. 14, 2024, 6:30 a.m. UTC | #10
Hello,

kernel test robot noticed "UBSAN:array-index-out-of-bounds_in_drivers/gpu/drm/i915/i915_drv.h" on:

commit: 3d845c2240623759952e06027546304b127ec1c5 ("[PATCH v1] drm/i915/dg2: enable G8 with a workaround")
url: https://github.com/intel-lab-lkp/linux/commits/Raag-Jadav/drm-i915-dg2-enable-G8-with-a-workaround/20241007-202716
base: git://anongit.freedesktop.org/drm-intel for-linux-next
patch link: https://lore.kernel.org/all/20241007122424.642796-1-raag.jadav@intel.com/
patch subject: [PATCH v1] drm/i915/dg2: enable G8 with a workaround

in testcase: suspend-stress
version: 
with following parameters:

	mode: freeze
	iterations: 10



config: x86_64-rhel-8.3-func
compiler: gcc-12
test machine: 4 threads (Broadwell) with 8G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202410141449.699cfd23-oliver.sang@intel.com


kern  :warn  : [   28.439916] ------------[ cut here ]------------
kern  :err   : [   28.440035] UBSAN: array-index-out-of-bounds in drivers/gpu/drm/i915/i915_drv.h:466:28
kern  :err   : [   28.440211] index 58 is out of range for type 'u32 [2]'
kern  :warn  : [   28.440322] CPU: 1 UID: 0 PID: 202 Comm: systemd-udevd Not tainted 6.12.0-rc1-00198-g3d845c224062 #1
kern  :warn  : [   28.440489] Hardware name:  /NUC5i3RYB, BIOS RYBDWi35.86A.0363.2017.0316.1028 03/16/2017
kern  :warn  : [   28.440636] Call Trace:
kern  :warn  : [   28.440703]  <TASK>
kern :warn : [   28.440764] dump_stack_lvl (lib/dump_stack.c:123 (discriminator 1)) 
kern :warn : [   28.440862] __ubsan_handle_out_of_bounds (lib/ubsan.c:232 lib/ubsan.c:429) 
kern :warn : [   28.440977] gt_init_workarounds (drivers/gpu/drm/i915/i915_drv.h:466 drivers/gpu/drm/i915/gt/intel_workarounds.c:1689) i915
kern :warn : [   28.441968] ? __pfx_gt_init_workarounds (drivers/gpu/drm/i915/gt/intel_workarounds.c:1665) i915
kern :warn : [   28.442781] ? fwtable_write32 (drivers/gpu/drm/i915/intel_uncore.c:2012 (discriminator 11)) i915
kern :warn : [   28.443526] intel_gt_init_workarounds (drivers/gpu/drm/i915/gt/intel_workarounds.c:1725) i915
kern :warn : [   28.444297] intel_gt_init (drivers/gpu/drm/i915/gt/intel_gt.c:715) i915
kern :warn : [   28.445032] i915_gem_init (drivers/gpu/drm/i915/i915_gem.c:1191) i915
kern :warn : [   28.445793] i915_driver_probe (drivers/gpu/drm/i915/i915_driver.c:799) i915
kern :warn : [   28.446517] ? __pfx_i915_driver_probe (drivers/gpu/drm/i915/i915_driver.c:751) i915
kern :warn : [   28.447243] ? drm_privacy_screen_get (drivers/gpu/drm/drm_privacy_screen.c:168) drm
kern :warn : [   28.447499] ? intel_display_driver_probe_defer (drivers/gpu/drm/i915/display/intel_display_driver.c:81) i915
kern :warn : [   28.448300] ? i915_pci_probe (drivers/gpu/drm/i915/i915_pci.c:996) i915
kern :warn : [   28.449003] ? __pfx_i915_pci_probe (drivers/gpu/drm/i915/i915_pci.c:957) i915
kern :warn : [   28.449712] local_pci_probe (drivers/pci/pci-driver.c:324) 
kern :warn : [   28.449811] pci_call_probe (drivers/pci/pci-driver.c:392) 
kern :warn : [   28.449905] ? _raw_spin_lock (arch/x86/include/asm/atomic.h:107 include/linux/atomic/atomic-arch-fallback.h:2170 include/linux/atomic/atomic-instrumented.h:1302 include/asm-generic/qspinlock.h:111 include/linux/spinlock.h:187 include/linux/spinlock_api_smp.h:134 kernel/locking/spinlock.c:154) 
kern :warn : [   28.449998] ? __pfx_pci_call_probe (drivers/pci/pci-driver.c:352) 
kern :warn : [   28.450095] ? kernfs_add_one (fs/kernfs/dir.c:819) 
kern :warn : [   28.450190] ? pci_assign_irq (drivers/pci/irq.c:149) 
kern :warn : [   28.450282] ? pci_match_device (drivers/pci/pci-driver.c:159 (discriminator 1)) 
kern :warn : [   28.450379] ? kernfs_put (arch/x86/include/asm/atomic.h:67 (discriminator 1) include/linux/atomic/atomic-arch-fallback.h:2278 (discriminator 1) include/linux/atomic/atomic-instrumented.h:1384 (discriminator 1) fs/kernfs/dir.c:557 (discriminator 1)) 
kern :warn : [   28.450465] pci_device_probe (drivers/pci/pci-driver.c:452) 
kern :warn : [   28.450557] ? pci_dma_configure (drivers/pci/pci-driver.c:1656) 
kern :warn : [   28.450653] really_probe (drivers/base/dd.c:579 drivers/base/dd.c:658) 
kern :warn : [   28.450741] __driver_probe_device (drivers/base/dd.c:738 drivers/base/dd.c:798) 
kern :warn : [   28.450841] driver_probe_device (drivers/base/dd.c:830) 
kern :warn : [   28.450937] __driver_attach (drivers/base/dd.c:1217) 
kern :warn : [   28.451028] ? __pfx___driver_attach (drivers/base/dd.c:1157) 
kern :warn : [   28.451124] bus_for_each_dev (drivers/base/bus.c:369) 
kern :warn : [   28.451217] ? __pfx_bus_for_each_dev (drivers/base/bus.c:358) 
kern :warn : [   28.451318] ? klist_add_tail (include/linux/list.h:150 include/linux/list.h:183 lib/klist.c:104 lib/klist.c:137) 
kern :warn : [   28.451410] bus_add_driver (drivers/base/bus.c:675) 
kern :warn : [   28.451500] driver_register (drivers/base/driver.c:246) 
kern :warn : [   28.451594] i915_init (drivers/gpu/drm/i915/i915_driver.c:1395) i915
kern :warn : [   28.452466] ? __pfx_i915_init (drivers/gpu/drm/i915/i915_config.c:13) i915
kern :warn : [   28.453207] do_one_initcall (init/main.c:1269) 
kern :warn : [   28.453304] ? __pfx_do_one_initcall (init/main.c:1260) 
kern :warn : [   28.453408] ? kasan_unpoison (mm/kasan/shadow.c:157 mm/kasan/shadow.c:151 mm/kasan/shadow.c:182) 
kern :warn : [   28.453499] ? kasan_unpoison (mm/kasan/shadow.c:156 mm/kasan/shadow.c:182) 
kern :warn : [   28.453592] do_init_module (kernel/module/main.c:2543) 
kern :warn : [   28.453687] init_module_from_file (kernel/module/main.c:3198) 
kern :warn : [   28.453787] ? __pfx_init_module_from_file (kernel/module/main.c:3174) 
kern :warn : [   28.453895] ? __pfx_userfaultfd_unmap_complete (fs/userfaultfd.c:810) 
kern :warn : [   28.454012] ? __pfx__raw_spin_lock (kernel/locking/spinlock.c:153) 
kern :warn : [   28.454112] idempotent_init_module (kernel/module/main.c:3210) 
kern :warn : [   28.454214] ? __pfx_idempotent_init_module (kernel/module/main.c:3202) 
kern :warn : [   28.454321] ? __pfx___seccomp_filter (kernel/seccomp.c:1218) 
kern :warn : [   28.454424] ? fdget (include/linux/atomic/atomic-arch-fallback.h:479 include/linux/atomic/atomic-instrumented.h:50 fs/file.c:1141 fs/file.c:1155) 
kern :warn : [   28.454504] ? security_capable (security/security.c:1142) 
kern :warn : [   28.454599] __x64_sys_finit_module (include/linux/file.h:68 kernel/module/main.c:3238 kernel/module/main.c:3220 kernel/module/main.c:3220) 
kern :warn : [   28.454698] do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83) 
kern :warn : [   28.454790] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) 
kern  :warn  : [   28.454900] RIP: 0033:0x7efbffb92229
kern :warn : [ 28.454984] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 3f 4c 2b 00 f7 d8 64 89 01 48
All code
========
   0:	00 f3                	add    %dh,%bl
   2:	c3                   	retq   
   3:	66 2e 0f 1f 84 00 00 	nopw   %cs:0x0(%rax,%rax,1)
   a:	00 00 00 
   d:	0f 1f 40 00          	nopl   0x0(%rax)
  11:	48 89 f8             	mov    %rdi,%rax
  14:	48 89 f7             	mov    %rsi,%rdi
  17:	48 89 d6             	mov    %rdx,%rsi
  1a:	48 89 ca             	mov    %rcx,%rdx
  1d:	4d 89 c2             	mov    %r8,%r10
  20:	4d 89 c8             	mov    %r9,%r8
  23:	4c 8b 4c 24 08       	mov    0x8(%rsp),%r9
  28:	0f 05                	syscall 
  2a:*	48 3d 01 f0 ff ff    	cmp    $0xfffffffffffff001,%rax		<-- trapping instruction
  30:	73 01                	jae    0x33
  32:	c3                   	retq   
  33:	48 8b 0d 3f 4c 2b 00 	mov    0x2b4c3f(%rip),%rcx        # 0x2b4c79
  3a:	f7 d8                	neg    %eax
  3c:	64 89 01             	mov    %eax,%fs:(%rcx)
  3f:	48                   	rex.W

Code starting with the faulting instruction
===========================================
   0:	48 3d 01 f0 ff ff    	cmp    $0xfffffffffffff001,%rax
   6:	73 01                	jae    0x9
   8:	c3                   	retq   
   9:	48 8b 0d 3f 4c 2b 00 	mov    0x2b4c3f(%rip),%rcx        # 0x2b4c4f
  10:	f7 d8                	neg    %eax
  12:	64 89 01             	mov    %eax,%fs:(%rcx)
  15:	48                   	rex.W
kern  :warn  : [   28.455283] RSP: 002b:00007ffe0d7f9428 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
kern  :warn  : [   28.455429] RAX: ffffffffffffffda RBX: 000055b5a7168b10 RCX: 00007efbffb92229
kern  :warn  : [   28.455562] RDX: 0000000000000000 RSI: 00007efc004ab265 RDI: 0000000000000014
kern  :warn  : [   28.455695] RBP: 00007efc004ab265 R08: 0000000000000000 R09: 00007ffe0d7f99a0
kern  :warn  : [   28.455826] R10: 0000000000000014 R11: 0000000000000246 R12: 0000000000000000
kern  :warn  : [   28.455959] R13: 000055b5a7170fe0 R14: 0000000000020000 R15: 000055b575213cbc
kern  :warn  : [   28.456095]  </TASK>
kern  :warn  : [   28.456422] ---[ end trace ]---


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20241014/202410141449.699cfd23-oliver.sang@intel.com
Raag Jadav Oct. 15, 2024, 8:37 a.m. UTC | #11
On Mon, Oct 14, 2024 at 10:01:50AM +0530, Gupta, Anshuman wrote:
> > On Tue, Oct 08, 2024 at 08:24:42PM +0300, Jani Nikula wrote:
> > > On Mon, 07 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> > > > Host BIOS doesn't enable G8 power mode due to an issue on DG2, so we
> AFAIU it discrete card firmware not HOST bios.

Yes, although firmware relies on native ASPM for G8 entry, which is setup
by host BIOS.

https://www.intel.com/content/www/us/en/support/articles/000092564/graphics.html

Raag
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index e539a656cfc3..b2db51377488 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -14,11 +14,30 @@ 
 #include "intel_gt_mcr.h"
 #include "intel_gt_print.h"
 #include "intel_gt_regs.h"
+#include "intel_pcode.h"
 #include "intel_ring.h"
 #include "intel_workarounds.h"
 
 #include "display/intel_fbc_regs.h"
 
+#ifdef CONFIG_X86
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+static const struct x86_cpu_id g8_cpu_ids[] = {
+	X86_MATCH_VFM(INTEL_ALDERLAKE,		NULL),
+	X86_MATCH_VFM(INTEL_ALDERLAKE_L,	NULL),
+	X86_MATCH_VFM(INTEL_COMETLAKE,		NULL),
+	X86_MATCH_VFM(INTEL_KABYLAKE,		NULL),
+	X86_MATCH_VFM(INTEL_KABYLAKE_L,		NULL),
+	X86_MATCH_VFM(INTEL_RAPTORLAKE,		NULL),
+	X86_MATCH_VFM(INTEL_RAPTORLAKE_P,	NULL),
+	X86_MATCH_VFM(INTEL_RAPTORLAKE_S,	NULL),
+	X86_MATCH_VFM(INTEL_ROCKETLAKE,		NULL),
+	{}
+};
+#endif
+
 /**
  * DOC: Hardware workarounds
  *
@@ -1770,9 +1789,33 @@  static void wa_list_apply(const struct i915_wa_list *wal)
 	intel_gt_mcr_unlock(gt, flags);
 }
 
+#define DG2_G8_WA_RANGE_1		0x56A0 ... 0x56AF
+#define DG2_G8_WA_RANGE_2		0x56B0 ... 0x56B9
+
+/* Wa_14022698589:dg2 */
+static void intel_enable_g8(struct intel_uncore *uncore)
+{
+	if (IS_DG2(uncore->i915)) {
+		switch (INTEL_DEVID(uncore->i915)) {
+		case DG2_G8_WA_RANGE_1:
+		case DG2_G8_WA_RANGE_2:
+#ifdef CONFIG_X86
+			if (!x86_match_cpu(g8_cpu_ids))
+#endif
+				return;
+		}
+
+		snb_pcode_write_p(uncore, PCODE_POWER_SETUP,
+				  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
+	}
+}
+
 void intel_gt_apply_workarounds(struct intel_gt *gt)
 {
 	wa_list_apply(&gt->wa_list);
+
+	/* Special case for pcode mailbox which can't be on wa_list */
+	intel_enable_g8(gt->uncore);
 }
 
 static bool wa_list_verify(struct intel_gt *gt,
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 41f4350a7c6c..e948b194550c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3568,6 +3568,7 @@ 
 #define   PCODE_POWER_SETUP			0x7C
 #define     POWER_SETUP_SUBCOMMAND_READ_I1	0x4
 #define     POWER_SETUP_SUBCOMMAND_WRITE_I1	0x5
+#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6
 #define	    POWER_SETUP_I1_WATTS		REG_BIT(31)
 #define	    POWER_SETUP_I1_SHIFT		6	/* 10.6 fixed point format */
 #define	    POWER_SETUP_I1_DATA_MASK		REG_GENMASK(15, 0)