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

[Jani Nikula <jani.nikula@linux.intel.com>]

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)

-- 
Jani Nikula, Intel