[PATCH v3 0/4] Implement Wa_14022698537

[PATCH v3 0/4] Implement Wa_14022698537

[Raag Jadav]

This series implements Wa_14022698537 for DG2 along with its prerequisites
in i915. Now that we have a common pciids.h in place, this can be extended
to xe as well. Detailed description in commit message.

v1: https://patchwork.freedesktop.org/series/139628/

v2: Introduce DG2_WA subplatform for workaround (Jani)
    Fix Wa_ID and include it in subject (Badal)
    Rephrase commit message (Jani)
    Move CPU whitelist to intel_wa_cpu.c

v3: Rework subplatform naming (Jani)
    Move CPU file out of gt directory (Riana)
    Rephrase CPU file description (Jani)
    Add kernel doc, re-order macro (Riana)
    Move workaround to i915_pcode_init() (Badal, Anshuman)

Raag Jadav (4):
  drm/intel/pciids: Refactor DG2 PCI IDs into segment ranges
  drm/i915/dg2: Introduce DG2_D subplatform
  drm/i915: Introduce intel_cpu_info.c for CPU IDs
  drm/i915/dg2: Implement Wa_14022698537

 drivers/gpu/drm/i915/Makefile            |  1 +
 drivers/gpu/drm/i915/i915_driver.c       | 15 +++++++
 drivers/gpu/drm/i915/i915_drv.h          |  2 +
 drivers/gpu/drm/i915/i915_reg.h          |  1 +
 drivers/gpu/drm/i915/intel_cpu_info.c    | 42 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_cpu_info.h    | 13 ++++++
 drivers/gpu/drm/i915/intel_device_info.c |  9 ++++
 drivers/gpu/drm/i915/intel_device_info.h |  5 ++-
 include/drm/intel/pciids.h               | 55 ++++++++++++++++++------
 9 files changed, 129 insertions(+), 14 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_cpu_info.c
 create mode 100644 drivers/gpu/drm/i915/intel_cpu_info.h

-- 
2.34.1

[PATCH v3 1/4] drm/intel/pciids: Refactor DG2 PCI IDs into segment ranges

[Raag Jadav]

Refactor DG2 PCI IDs into D, E and M ranges which will be useful for
segment specific features.

v3: Rework subplatform naming (Jani)

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 include/drm/intel/pciids.h | 55 +++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 13 deletions(-)

diff --git a/include/drm/intel/pciids.h b/include/drm/intel/pciids.h
index 7632507af166..83aac9f17372 100644
--- a/include/drm/intel/pciids.h
+++ b/include/drm/intel/pciids.h
@@ -717,37 +717,66 @@
 	MACRO__(0xA7AB, ## __VA_ARGS__)
 
 /* DG2 */
-#define INTEL_DG2_G10_IDS(MACRO__, ...) \
-	MACRO__(0x5690, ## __VA_ARGS__), \
-	MACRO__(0x5691, ## __VA_ARGS__), \
-	MACRO__(0x5692, ## __VA_ARGS__), \
+#define INTEL_DG2_G10_D_IDS(MACRO__, ...) \
 	MACRO__(0x56A0, ## __VA_ARGS__), \
 	MACRO__(0x56A1, ## __VA_ARGS__), \
-	MACRO__(0x56A2, ## __VA_ARGS__), \
+	MACRO__(0x56A2, ## __VA_ARGS__)
+
+#define INTEL_DG2_G10_E_IDS(MACRO__, ...) \
 	MACRO__(0x56BE, ## __VA_ARGS__), \
 	MACRO__(0x56BF, ## __VA_ARGS__)
 
-#define INTEL_DG2_G11_IDS(MACRO__, ...) \
-	MACRO__(0x5693, ## __VA_ARGS__), \
-	MACRO__(0x5694, ## __VA_ARGS__), \
-	MACRO__(0x5695, ## __VA_ARGS__), \
+#define INTEL_DG2_G10_M_IDS(MACRO__, ...) \
+	MACRO__(0x5690, ## __VA_ARGS__), \
+	MACRO__(0x5691, ## __VA_ARGS__), \
+	MACRO__(0x5692, ## __VA_ARGS__)
+
+#define INTEL_DG2_G10_IDS(MACRO__, ...) \
+	INTEL_DG2_G10_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G10_E_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G10_M_IDS(MACRO__, ## __VA_ARGS__)
+
+#define INTEL_DG2_G11_D_IDS(MACRO__, ...) \
 	MACRO__(0x56A5, ## __VA_ARGS__), \
 	MACRO__(0x56A6, ## __VA_ARGS__), \
 	MACRO__(0x56B0, ## __VA_ARGS__), \
-	MACRO__(0x56B1, ## __VA_ARGS__), \
+	MACRO__(0x56B1, ## __VA_ARGS__)
+
+#define INTEL_DG2_G11_E_IDS(MACRO__, ...) \
 	MACRO__(0x56BA, ## __VA_ARGS__), \
 	MACRO__(0x56BB, ## __VA_ARGS__), \
 	MACRO__(0x56BC, ## __VA_ARGS__), \
 	MACRO__(0x56BD, ## __VA_ARGS__)
 
-#define INTEL_DG2_G12_IDS(MACRO__, ...) \
-	MACRO__(0x5696, ## __VA_ARGS__), \
-	MACRO__(0x5697, ## __VA_ARGS__), \
+#define INTEL_DG2_G11_M_IDS(MACRO__, ...) \
+	MACRO__(0x5693, ## __VA_ARGS__), \
+	MACRO__(0x5694, ## __VA_ARGS__), \
+	MACRO__(0x5695, ## __VA_ARGS__)
+
+#define INTEL_DG2_G11_IDS(MACRO__, ...) \
+	INTEL_DG2_G11_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G11_E_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G11_M_IDS(MACRO__, ## __VA_ARGS__)
+
+#define INTEL_DG2_G12_D_IDS(MACRO__, ...) \
 	MACRO__(0x56A3, ## __VA_ARGS__), \
 	MACRO__(0x56A4, ## __VA_ARGS__), \
 	MACRO__(0x56B2, ## __VA_ARGS__), \
 	MACRO__(0x56B3, ## __VA_ARGS__)
 
+#define INTEL_DG2_G12_M_IDS(MACRO__, ...) \
+	MACRO__(0x5696, ## __VA_ARGS__), \
+	MACRO__(0x5697, ## __VA_ARGS__)
+
+#define INTEL_DG2_G12_IDS(MACRO__, ...) \
+	INTEL_DG2_G12_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G12_M_IDS(MACRO__, ## __VA_ARGS__)
+
+#define INTEL_DG2_D_IDS(MACRO__, ...) \
+	INTEL_DG2_G10_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G11_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G12_D_IDS(MACRO__, ## __VA_ARGS__)
+
 #define INTEL_DG2_IDS(MACRO__, ...) \
 	INTEL_DG2_G10_IDS(MACRO__, ## __VA_ARGS__), \
 	INTEL_DG2_G11_IDS(MACRO__, ## __VA_ARGS__), \
-- 
2.34.1

[PATCH v3 2/4] drm/i915/dg2: Introduce DG2_D subplatform

[Raag Jadav]

Introduce DG2_D subplatform for the devices that span across multiple
DG2 subplatforms but are within same segment and will be useful for
segment specific features.

v3: Rework subplatform naming (Jani)
    Split subplatform check into separate case (Jani)

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          | 2 ++
 drivers/gpu/drm/i915/intel_device_info.c | 9 +++++++++
 drivers/gpu/drm/i915/intel_device_info.h | 5 ++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0f19cbd36829..fdb7158c7c1a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -546,6 +546,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
 	IS_SUBPLATFORM(i915, INTEL_DG2, INTEL_SUBPLATFORM_G11)
 #define IS_DG2_G12(i915) \
 	IS_SUBPLATFORM(i915, INTEL_DG2, INTEL_SUBPLATFORM_G12)
+#define IS_DG2_D(i915) \
+	IS_SUBPLATFORM(i915, INTEL_DG2, INTEL_SUBPLATFORM_D)
 #define IS_RAPTORLAKE_S(i915) \
 	IS_SUBPLATFORM(i915, INTEL_ALDERLAKE_S, INTEL_SUBPLATFORM_RPL)
 #define IS_ALDERLAKE_P_N(i915) \
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index ff9500194d15..89d1549a83ae 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -200,6 +200,10 @@ static const u16 subplatform_g12_ids[] = {
 	INTEL_DG2_G12_IDS(ID),
 };
 
+static const u16 subplatform_dg2_d_ids[] = {
+	INTEL_DG2_D_IDS(ID),
+};
+
 static const u16 subplatform_arl_ids[] = {
 	INTEL_ARL_IDS(ID),
 };
@@ -266,6 +270,11 @@ static void intel_device_info_subplatform_init(struct drm_i915_private *i915)
 		mask = BIT(INTEL_SUBPLATFORM_ARL);
 	}
 
+	/* DG2_D ids span across multiple DG2 subplatforms */
+	if (find_devid(devid, subplatform_dg2_d_ids,
+		       ARRAY_SIZE(subplatform_dg2_d_ids)))
+		mask |= BIT(INTEL_SUBPLATFORM_D);
+
 	GEM_BUG_ON(mask & ~INTEL_SUBPLATFORM_MASK);
 
 	RUNTIME_INFO(i915)->platform_mask[pi] |= mask;
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index 4f4aa4ff9963..7f1f84b1d0e3 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -95,9 +95,11 @@ enum intel_platform {
 /*
  * Subplatform bits share the same namespace per parent platform. In other words
  * it is fine for the same bit to be used on multiple parent platforms.
+ * Devices can belong to multiple subplatforms if needed, so it's possible to set
+ * multiple bits for same device.
  */
 
-#define INTEL_SUBPLATFORM_BITS (3)
+#define INTEL_SUBPLATFORM_BITS (4)
 #define INTEL_SUBPLATFORM_MASK (BIT(INTEL_SUBPLATFORM_BITS) - 1)
 
 /* HSW/BDW/SKL/KBL/CFL */
@@ -114,6 +116,7 @@ enum intel_platform {
 #define INTEL_SUBPLATFORM_G10	0
 #define INTEL_SUBPLATFORM_G11	1
 #define INTEL_SUBPLATFORM_G12	2
+#define INTEL_SUBPLATFORM_D	3
 
 /* ADL */
 #define INTEL_SUBPLATFORM_RPL	0
-- 
2.34.1

[PATCH v3 3/4] drm/i915: Introduce intel_cpu_info.c for CPU IDs

[Raag Jadav]

Having similar naming convention in intel-family.h and intel_device_info.h
results in redefinition of a few platforms. Define CPU IDs in its own file
to avoid this.

v3: Move file out of gt directory, add kernel doc (Riana)
    Rephrase file description (Jani)

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/gpu/drm/i915/Makefile         |  1 +
 drivers/gpu/drm/i915/intel_cpu_info.c | 42 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_cpu_info.h | 13 +++++++++
 3 files changed, 56 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/intel_cpu_info.c
 create mode 100644 drivers/gpu/drm/i915/intel_cpu_info.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 31710d98cad5..208929358b25 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -35,6 +35,7 @@ i915-y += \
 	i915_sysfs.o \
 	i915_utils.o \
 	intel_clock_gating.o \
+	intel_cpu_info.o \
 	intel_device_info.o \
 	intel_memory_region.o \
 	intel_pcode.o \
diff --git a/drivers/gpu/drm/i915/intel_cpu_info.c b/drivers/gpu/drm/i915/intel_cpu_info.c
new file mode 100644
index 000000000000..9a1bfff4be7d
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_cpu_info.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ *
+ * Avoid INTEL_<PLATFORM> name collisions between asm/intel-family.h and
+ * intel_device_info.h by having a separate file.
+ */
+
+#include "intel_cpu_info.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),
+	{}
+};
+
+/**
+ * intel_match_g8_cpu - match current CPU against g8_cpu_ids[]
+ *
+ * This matches current CPU against g8_cpu_ids[], which are applicable
+ * for G8 workaround.
+ *
+ * Returns: %true if matches, %false otherwise.
+ */
+bool intel_match_g8_cpu(void)
+{
+	return x86_match_cpu(g8_cpu_ids);
+}
+#else
+bool intel_match_g8_cpu(void) { return false; }
+#endif
diff --git a/drivers/gpu/drm/i915/intel_cpu_info.h b/drivers/gpu/drm/i915/intel_cpu_info.h
new file mode 100644
index 000000000000..dd0c5e784b97
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_cpu_info.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#ifndef _INTEL_CPU_INFO_H_
+#define _INTEL_CPU_INFO_H_
+
+#include <linux/types.h>
+
+bool intel_match_g8_cpu(void);
+
+#endif
-- 
2.34.1

[PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Raag Jadav]

G8 power state entry is disabled due to a limitation on DG2, so we
enable it from driver with Wa_14022698537. For now we enable it for
all DG2 devices with the exception of a few, for which, we enable
only when paired with whitelisted CPU models. This works with Native
ASMP and reduces idle power consumption.

$ echo powersave > /sys/module/pcie_aspm/parameters/policy
$ lspci -s 0000:03:00.0 -vvv
LnkCtl: ASPM L1 Enabled; RCB 64 bytes, Disabled- CommClk-

v2: Fix Wa_ID and include it in subject (Badal)
    Rephrase commit message (Jani)
v3: Move workaround to i915_pcode_init() (Badal, Anshuman)
    Re-order macro (Riana)

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

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 365329ff8a07..59c6124c9bc2 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -93,12 +93,14 @@
 #include "i915_memcpy.h"
 #include "i915_perf.h"
 #include "i915_query.h"
+#include "i915_reg.h"
 #include "i915_suspend.h"
 #include "i915_switcheroo.h"
 #include "i915_sysfs.h"
 #include "i915_utils.h"
 #include "i915_vgpu.h"
 #include "intel_clock_gating.h"
+#include "intel_cpu_info.h"
 #include "intel_gvt.h"
 #include "intel_memory_region.h"
 #include "intel_pci_config.h"
@@ -415,6 +417,18 @@ static int i915_set_dma_info(struct drm_i915_private *i915)
 	return ret;
 }
 
+/* Wa_14022698537:dg2 */
+static void i915_enable_g8(struct drm_i915_private *i915)
+{
+	if (IS_DG2(i915)) {
+		if (IS_DG2_D(i915) && !intel_match_g8_cpu())
+			return;
+
+		snb_pcode_write_p(&i915->uncore, PCODE_POWER_SETUP,
+				  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
+	}
+}
+
 static int i915_pcode_init(struct drm_i915_private *i915)
 {
 	struct intel_gt *gt;
@@ -428,6 +442,7 @@ static int i915_pcode_init(struct drm_i915_private *i915)
 		}
 	}
 
+	i915_enable_g8(i915);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 89e4381f8baa..d400c77423a5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3617,6 +3617,7 @@
 #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)
+#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6
 #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
 #define   XEHP_PCODE_FREQUENCY_CONFIG		0x6e	/* pvc */
 /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */
-- 
2.34.1

✗ Fi.CI.CHECKPATCH: warning for Implement Wa_14022698537 (rev2)

[Patchwork]

== Series Details ==

Series: Implement Wa_14022698537 (rev2)
URL   : https://patchwork.freedesktop.org/series/139883/
State : warning

== Summary ==

Error: dim checkpatch failed
6b96c7b8885a drm/intel/pciids: Refactor DG2 PCI IDs into segment ranges
-:25: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#25: FILE: include/drm/intel/pciids.h:720:
+#define INTEL_DG2_G10_D_IDS(MACRO__, ...) \
 	MACRO__(0x56A0, ## __VA_ARGS__), \
 	MACRO__(0x56A1, ## __VA_ARGS__), \
+	MACRO__(0x56A2, ## __VA_ARGS__)

-:25: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#25: FILE: include/drm/intel/pciids.h:720:
+#define INTEL_DG2_G10_D_IDS(MACRO__, ...) \
 	MACRO__(0x56A0, ## __VA_ARGS__), \
 	MACRO__(0x56A1, ## __VA_ARGS__), \
+	MACRO__(0x56A2, ## __VA_ARGS__)

-:31: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#31: FILE: include/drm/intel/pciids.h:725:
+#define INTEL_DG2_G10_E_IDS(MACRO__, ...) \
 	MACRO__(0x56BE, ## __VA_ARGS__), \
 	MACRO__(0x56BF, ## __VA_ARGS__)

-:31: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#31: FILE: include/drm/intel/pciids.h:725:
+#define INTEL_DG2_G10_E_IDS(MACRO__, ...) \
 	MACRO__(0x56BE, ## __VA_ARGS__), \
 	MACRO__(0x56BF, ## __VA_ARGS__)

-:39: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#39: FILE: include/drm/intel/pciids.h:729:
+#define INTEL_DG2_G10_M_IDS(MACRO__, ...) \
+	MACRO__(0x5690, ## __VA_ARGS__), \
+	MACRO__(0x5691, ## __VA_ARGS__), \
+	MACRO__(0x5692, ## __VA_ARGS__)

-:39: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#39: FILE: include/drm/intel/pciids.h:729:
+#define INTEL_DG2_G10_M_IDS(MACRO__, ...) \
+	MACRO__(0x5690, ## __VA_ARGS__), \
+	MACRO__(0x5691, ## __VA_ARGS__), \
+	MACRO__(0x5692, ## __VA_ARGS__)

-:44: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#44: FILE: include/drm/intel/pciids.h:734:
+#define INTEL_DG2_G10_IDS(MACRO__, ...) \
+	INTEL_DG2_G10_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G10_E_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G10_M_IDS(MACRO__, ## __VA_ARGS__)

-:44: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#44: FILE: include/drm/intel/pciids.h:734:
+#define INTEL_DG2_G10_IDS(MACRO__, ...) \
+	INTEL_DG2_G10_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G10_E_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G10_M_IDS(MACRO__, ## __VA_ARGS__)

-:49: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#49: FILE: include/drm/intel/pciids.h:739:
+#define INTEL_DG2_G11_D_IDS(MACRO__, ...) \
 	MACRO__(0x56A5, ## __VA_ARGS__), \
 	MACRO__(0x56A6, ## __VA_ARGS__), \
 	MACRO__(0x56B0, ## __VA_ARGS__), \
+	MACRO__(0x56B1, ## __VA_ARGS__)

-:49: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#49: FILE: include/drm/intel/pciids.h:739:
+#define INTEL_DG2_G11_D_IDS(MACRO__, ...) \
 	MACRO__(0x56A5, ## __VA_ARGS__), \
 	MACRO__(0x56A6, ## __VA_ARGS__), \
 	MACRO__(0x56B0, ## __VA_ARGS__), \
+	MACRO__(0x56B1, ## __VA_ARGS__)

-:56: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#56: FILE: include/drm/intel/pciids.h:745:
+#define INTEL_DG2_G11_E_IDS(MACRO__, ...) \
 	MACRO__(0x56BA, ## __VA_ARGS__), \
 	MACRO__(0x56BB, ## __VA_ARGS__), \
 	MACRO__(0x56BC, ## __VA_ARGS__), \
 	MACRO__(0x56BD, ## __VA_ARGS__)

-:56: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#56: FILE: include/drm/intel/pciids.h:745:
+#define INTEL_DG2_G11_E_IDS(MACRO__, ...) \
 	MACRO__(0x56BA, ## __VA_ARGS__), \
 	MACRO__(0x56BB, ## __VA_ARGS__), \
 	MACRO__(0x56BC, ## __VA_ARGS__), \
 	MACRO__(0x56BD, ## __VA_ARGS__)

-:65: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#65: FILE: include/drm/intel/pciids.h:751:
+#define INTEL_DG2_G11_M_IDS(MACRO__, ...) \
+	MACRO__(0x5693, ## __VA_ARGS__), \
+	MACRO__(0x5694, ## __VA_ARGS__), \
+	MACRO__(0x5695, ## __VA_ARGS__)

-:65: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#65: FILE: include/drm/intel/pciids.h:751:
+#define INTEL_DG2_G11_M_IDS(MACRO__, ...) \
+	MACRO__(0x5693, ## __VA_ARGS__), \
+	MACRO__(0x5694, ## __VA_ARGS__), \
+	MACRO__(0x5695, ## __VA_ARGS__)

-:70: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#70: FILE: include/drm/intel/pciids.h:756:
+#define INTEL_DG2_G11_IDS(MACRO__, ...) \
+	INTEL_DG2_G11_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G11_E_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G11_M_IDS(MACRO__, ## __VA_ARGS__)

-:70: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#70: FILE: include/drm/intel/pciids.h:756:
+#define INTEL_DG2_G11_IDS(MACRO__, ...) \
+	INTEL_DG2_G11_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G11_E_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G11_M_IDS(MACRO__, ## __VA_ARGS__)

-:75: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#75: FILE: include/drm/intel/pciids.h:761:
+#define INTEL_DG2_G12_D_IDS(MACRO__, ...) \
 	MACRO__(0x56A3, ## __VA_ARGS__), \
 	MACRO__(0x56A4, ## __VA_ARGS__), \
 	MACRO__(0x56B2, ## __VA_ARGS__), \
 	MACRO__(0x56B3, ## __VA_ARGS__)

-:75: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#75: FILE: include/drm/intel/pciids.h:761:
+#define INTEL_DG2_G12_D_IDS(MACRO__, ...) \
 	MACRO__(0x56A3, ## __VA_ARGS__), \
 	MACRO__(0x56A4, ## __VA_ARGS__), \
 	MACRO__(0x56B2, ## __VA_ARGS__), \
 	MACRO__(0x56B3, ## __VA_ARGS__)

-:81: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#81: FILE: include/drm/intel/pciids.h:767:
+#define INTEL_DG2_G12_M_IDS(MACRO__, ...) \
+	MACRO__(0x5696, ## __VA_ARGS__), \
+	MACRO__(0x5697, ## __VA_ARGS__)

-:81: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#81: FILE: include/drm/intel/pciids.h:767:
+#define INTEL_DG2_G12_M_IDS(MACRO__, ...) \
+	MACRO__(0x5696, ## __VA_ARGS__), \
+	MACRO__(0x5697, ## __VA_ARGS__)

-:85: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#85: FILE: include/drm/intel/pciids.h:771:
+#define INTEL_DG2_G12_IDS(MACRO__, ...) \
+	INTEL_DG2_G12_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G12_M_IDS(MACRO__, ## __VA_ARGS__)

-:85: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#85: FILE: include/drm/intel/pciids.h:771:
+#define INTEL_DG2_G12_IDS(MACRO__, ...) \
+	INTEL_DG2_G12_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G12_M_IDS(MACRO__, ## __VA_ARGS__)

-:89: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#89: FILE: include/drm/intel/pciids.h:775:
+#define INTEL_DG2_D_IDS(MACRO__, ...) \
+	INTEL_DG2_G10_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G11_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G12_D_IDS(MACRO__, ## __VA_ARGS__)

-:89: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'MACRO__' - possible side-effects?
#89: FILE: include/drm/intel/pciids.h:775:
+#define INTEL_DG2_D_IDS(MACRO__, ...) \
+	INTEL_DG2_G10_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G11_D_IDS(MACRO__, ## __VA_ARGS__), \
+	INTEL_DG2_G12_D_IDS(MACRO__, ## __VA_ARGS__)

total: 12 errors, 0 warnings, 12 checks, 79 lines checked
6e91624b80d3 drm/i915/dg2: Introduce DG2_D subplatform
fb34b2b10b7c drm/i915: Introduce intel_cpu_info.c for CPU IDs
-:28: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#28: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 62 lines checked
07a9b38a0475 drm/i915/dg2: Implement Wa_14022698537

✗ Fi.CI.SPARSE: warning for Implement Wa_14022698537 (rev2)

[Patchwork]

== Series Details ==

Series: Implement Wa_14022698537 (rev2)
URL   : https://patchwork.freedesktop.org/series/139883/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

Re: [PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Andi Shyti]

Hi Raag,

...

> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 89e4381f8baa..d400c77423a5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3617,6 +3617,7 @@
>  #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)
> +#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6

0x06 for alignment, please.

Andi

>  #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
>  #define   XEHP_PCODE_FREQUENCY_CONFIG		0x6e	/* pvc */
>  /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */
> -- 
> 2.34.1

✓ Fi.CI.BAT: success for Implement Wa_14022698537 (rev2)

[Patchwork]

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

== Series Details ==

Series: Implement Wa_14022698537 (rev2)
URL   : https://patchwork.freedesktop.org/series/139883/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_15612 -> Patchwork_139883v2
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_139883v2/index.html

Participating hosts (47 -> 44)
------------------------------

  Missing    (3): bat-arls-2 bat-arls-1 fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_139883v2 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@i915_selftest@live:
    - bat-arlh-3:         [DMESG-FAIL][1] ([i915#10341]) -> [PASS][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15612/bat-arlh-3/igt@i915_selftest@live.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_139883v2/bat-arlh-3/igt@i915_selftest@live.html
    - bat-twl-1:          [INCOMPLETE][3] ([i915#12133] / [i915#9413]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15612/bat-twl-1/igt@i915_selftest@live.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_139883v2/bat-twl-1/igt@i915_selftest@live.html
    - bat-arlh-2:         [ABORT][5] ([i915#12133]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15612/bat-arlh-2/igt@i915_selftest@live.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_139883v2/bat-arlh-2/igt@i915_selftest@live.html

  * igt@i915_selftest@live@gt_lrc:
    - bat-twl-1:          [INCOMPLETE][7] ([i915#9413]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15612/bat-twl-1/igt@i915_selftest@live@gt_lrc.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_139883v2/bat-twl-1/igt@i915_selftest@live@gt_lrc.html

  * igt@i915_selftest@live@hangcheck:
    - bat-arlh-2:         [ABORT][9] ([i915#9500]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15612/bat-arlh-2/igt@i915_selftest@live@hangcheck.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_139883v2/bat-arlh-2/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@workarounds:
    - bat-arlh-3:         [DMESG-FAIL][11] ([i915#9500]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15612/bat-arlh-3/igt@i915_selftest@live@workarounds.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_139883v2/bat-arlh-3/igt@i915_selftest@live@workarounds.html

  
  [i915#10341]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10341
  [i915#12133]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12133
  [i915#9413]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9413
  [i915#9500]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9500


Build changes
-------------

  * Linux: CI_DRM_15612 -> Patchwork_139883v2

  CI-20190529: 20190529
  CI_DRM_15612: fb992a15d9b3dc0ed1395b1a30a27f50fb8dba63 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8088: 0030d5bc92b8e4ac991db1c88af1f0ad7593812a @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_139883v2: fb992a15d9b3dc0ed1395b1a30a27f50fb8dba63 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_139883v2/index.html

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

Re: [PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Raag Jadav]

On Wed, Oct 30, 2024 at 04:34:17PM +0100, Andi Shyti wrote:
> Hi Raag,
> 
> ...
> 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 89e4381f8baa..d400c77423a5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3617,6 +3617,7 @@
> >  #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)
> > +#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6
> 
> 0x06 for alignment, please.

This is aligned with POWER_SETUP_SUBCOMMAND macros which are not visible
in the diff :(

Raag

> >  #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
> >  #define   XEHP_PCODE_FREQUENCY_CONFIG		0x6e	/* pvc */
> >  /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */
> > -- 
> > 2.34.1

Re: [PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Jani Nikula]

On Wed, 30 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> G8 power state entry is disabled due to a limitation on DG2, so we
> enable it from driver with Wa_14022698537. For now we enable it for
> all DG2 devices with the exception of a few, for which, we enable
> only when paired with whitelisted CPU models. This works with Native
> ASMP and reduces idle power consumption.
>
> $ echo powersave > /sys/module/pcie_aspm/parameters/policy
> $ lspci -s 0000:03:00.0 -vvv
> LnkCtl: ASPM L1 Enabled; RCB 64 bytes, Disabled- CommClk-
>
> v2: Fix Wa_ID and include it in subject (Badal)
>     Rephrase commit message (Jani)
> v3: Move workaround to i915_pcode_init() (Badal, Anshuman)
>     Re-order macro (Riana)
>
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 15 +++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h    |  1 +
>  2 files changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 365329ff8a07..59c6124c9bc2 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -93,12 +93,14 @@
>  #include "i915_memcpy.h"
>  #include "i915_perf.h"
>  #include "i915_query.h"
> +#include "i915_reg.h"
>  #include "i915_suspend.h"
>  #include "i915_switcheroo.h"
>  #include "i915_sysfs.h"
>  #include "i915_utils.h"
>  #include "i915_vgpu.h"
>  #include "intel_clock_gating.h"
> +#include "intel_cpu_info.h"
>  #include "intel_gvt.h"
>  #include "intel_memory_region.h"
>  #include "intel_pci_config.h"
> @@ -415,6 +417,18 @@ static int i915_set_dma_info(struct drm_i915_private *i915)
>  	return ret;
>  }
>  
> +/* Wa_14022698537:dg2 */
> +static void i915_enable_g8(struct drm_i915_private *i915)
> +{
> +	if (IS_DG2(i915)) {
> +		if (IS_DG2_D(i915) && !intel_match_g8_cpu())
> +			return;

You don't need to check for DG2 twice.

BR,
Jani.

> +
> +		snb_pcode_write_p(&i915->uncore, PCODE_POWER_SETUP,
> +				  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
> +	}
> +}
> +
>  static int i915_pcode_init(struct drm_i915_private *i915)
>  {
>  	struct intel_gt *gt;
> @@ -428,6 +442,7 @@ static int i915_pcode_init(struct drm_i915_private *i915)
>  		}
>  	}
>  
> +	i915_enable_g8(i915);
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 89e4381f8baa..d400c77423a5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3617,6 +3617,7 @@
>  #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)
> +#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6
>  #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
>  #define   XEHP_PCODE_FREQUENCY_CONFIG		0x6e	/* pvc */
>  /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */

-- 
Jani Nikula, Intel

Re: [PATCH v3 0/4] Implement Wa_14022698537

[Jani Nikula]

On Wed, 30 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> This series implements Wa_14022698537 for DG2 along with its prerequisites
> in i915. Now that we have a common pciids.h in place, this can be extended
> to xe as well. Detailed description in commit message.

Okay, so this bumps the requirements during development, but where's the
implementation for xe? What's it going to look like?

BR,
Jani.



>
> v1: https://patchwork.freedesktop.org/series/139628/
>
> v2: Introduce DG2_WA subplatform for workaround (Jani)
>     Fix Wa_ID and include it in subject (Badal)
>     Rephrase commit message (Jani)
>     Move CPU whitelist to intel_wa_cpu.c
>
> v3: Rework subplatform naming (Jani)
>     Move CPU file out of gt directory (Riana)
>     Rephrase CPU file description (Jani)
>     Add kernel doc, re-order macro (Riana)
>     Move workaround to i915_pcode_init() (Badal, Anshuman)
>
> Raag Jadav (4):
>   drm/intel/pciids: Refactor DG2 PCI IDs into segment ranges
>   drm/i915/dg2: Introduce DG2_D subplatform
>   drm/i915: Introduce intel_cpu_info.c for CPU IDs
>   drm/i915/dg2: Implement Wa_14022698537
>
>  drivers/gpu/drm/i915/Makefile            |  1 +
>  drivers/gpu/drm/i915/i915_driver.c       | 15 +++++++
>  drivers/gpu/drm/i915/i915_drv.h          |  2 +
>  drivers/gpu/drm/i915/i915_reg.h          |  1 +
>  drivers/gpu/drm/i915/intel_cpu_info.c    | 42 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_cpu_info.h    | 13 ++++++
>  drivers/gpu/drm/i915/intel_device_info.c |  9 ++++
>  drivers/gpu/drm/i915/intel_device_info.h |  5 ++-
>  include/drm/intel/pciids.h               | 55 ++++++++++++++++++------
>  9 files changed, 129 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_cpu_info.c
>  create mode 100644 drivers/gpu/drm/i915/intel_cpu_info.h

-- 
Jani Nikula, Intel

Re: [PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Raag Jadav]

On Wed, Oct 30, 2024 at 08:39:31PM +0200, Jani Nikula wrote:
> On Wed, 30 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> > G8 power state entry is disabled due to a limitation on DG2, so we
> > enable it from driver with Wa_14022698537. For now we enable it for
> > all DG2 devices with the exception of a few, for which, we enable
> > only when paired with whitelisted CPU models. This works with Native
> > ASMP and reduces idle power consumption.

...

> > +/* Wa_14022698537:dg2 */
> > +static void i915_enable_g8(struct drm_i915_private *i915)
> > +{
> > +	if (IS_DG2(i915)) {
> > +		if (IS_DG2_D(i915) && !intel_match_g8_cpu())
> > +			return;
> 
> You don't need to check for DG2 twice.

My understanding is that DG2_D is a subset of DG2.

> > +		snb_pcode_write_p(&i915->uncore, PCODE_POWER_SETUP,
> > +				  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
> > +	}
> > +}

Raag

Re: [PATCH v3 0/4] Implement Wa_14022698537

[Raag Jadav]

On Wed, Oct 30, 2024 at 08:40:50PM +0200, Jani Nikula wrote:
> On Wed, 30 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> > This series implements Wa_14022698537 for DG2 along with its prerequisites
> > in i915. Now that we have a common pciids.h in place, this can be extended
> > to xe as well. Detailed description in commit message.
> 
> Okay, so this bumps the requirements during development, but where's the
> implementation for xe? What's it going to look like?

The requirement is only for i915. What I mean here is, if the requirement
arises for xe in the future, this *can* be implemented without significant
refactoring now that we have a common pciids.h in place.

I know it's not the best choice of words but I thought you wouldn't mind :)

Raag

Re: [PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Jani Nikula]

On Thu, 31 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> On Wed, Oct 30, 2024 at 08:39:31PM +0200, Jani Nikula wrote:
>> On Wed, 30 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
>> > G8 power state entry is disabled due to a limitation on DG2, so we
>> > enable it from driver with Wa_14022698537. For now we enable it for
>> > all DG2 devices with the exception of a few, for which, we enable
>> > only when paired with whitelisted CPU models. This works with Native
>> > ASMP and reduces idle power consumption.
>
> ...
>
>> > +/* Wa_14022698537:dg2 */
>> > +static void i915_enable_g8(struct drm_i915_private *i915)
>> > +{
>> > +	if (IS_DG2(i915)) {
>> > +		if (IS_DG2_D(i915) && !intel_match_g8_cpu())
>> > +			return;
>> 
>> You don't need to check for DG2 twice.
>
> My understanding is that DG2_D is a subset of DG2.

Sorry, I misread the code. *facepalm*.

BR,
Jani.

>
>> > +		snb_pcode_write_p(&i915->uncore, PCODE_POWER_SETUP,
>> > +				  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
>> > +	}
>> > +}
>
> Raag

-- 
Jani Nikula, Intel

Re: [PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Raag Jadav]

On Thu, Oct 31, 2024 at 11:27:05AM +0200, Jani Nikula wrote:
> On Thu, 31 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> > On Wed, Oct 30, 2024 at 08:39:31PM +0200, Jani Nikula wrote:
> > > On Wed, 30 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> > > > G8 power state entry is disabled due to a limitation on DG2, so we
> > > > enable it from driver with Wa_14022698537. For now we enable it for
> > > > all DG2 devices with the exception of a few, for which, we enable
> > > > only when paired with whitelisted CPU models. This works with Native
> > > > ASMP and reduces idle power consumption.
> > 
> >  ...
> > 
> > > > +/* Wa_14022698537:dg2 */
> > > > +static void i915_enable_g8(struct drm_i915_private *i915)
> > > > +{
> > > > +	if (IS_DG2(i915)) {
> > > > +		if (IS_DG2_D(i915) && !intel_match_g8_cpu())
> > > > +			return;
> > > 
> > > You don't need to check for DG2 twice.
> >
> > My understanding is that DG2_D is a subset of DG2.
> 
> Sorry, I misread the code. *facepalm*.

Yeah, we definitely need the coffee :D

Raag

Re: [PATCH v3 0/4] Implement Wa_14022698537

[Jani Nikula]

On Wed, 30 Oct 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> This series implements Wa_14022698537 for DG2 along with its prerequisites
> in i915. Now that we have a common pciids.h in place, this can be extended
> to xe as well. Detailed description in commit message.

Overall looks good and my earlier comments were addressed, but will
still need detailed review,

Acked-by: Jani Nikula <jani.nikula@intel.com>

>
> v1: https://patchwork.freedesktop.org/series/139628/
>
> v2: Introduce DG2_WA subplatform for workaround (Jani)
>     Fix Wa_ID and include it in subject (Badal)
>     Rephrase commit message (Jani)
>     Move CPU whitelist to intel_wa_cpu.c
>
> v3: Rework subplatform naming (Jani)
>     Move CPU file out of gt directory (Riana)
>     Rephrase CPU file description (Jani)
>     Add kernel doc, re-order macro (Riana)
>     Move workaround to i915_pcode_init() (Badal, Anshuman)
>
> Raag Jadav (4):
>   drm/intel/pciids: Refactor DG2 PCI IDs into segment ranges
>   drm/i915/dg2: Introduce DG2_D subplatform
>   drm/i915: Introduce intel_cpu_info.c for CPU IDs
>   drm/i915/dg2: Implement Wa_14022698537
>
>  drivers/gpu/drm/i915/Makefile            |  1 +
>  drivers/gpu/drm/i915/i915_driver.c       | 15 +++++++
>  drivers/gpu/drm/i915/i915_drv.h          |  2 +
>  drivers/gpu/drm/i915/i915_reg.h          |  1 +
>  drivers/gpu/drm/i915/intel_cpu_info.c    | 42 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_cpu_info.h    | 13 ++++++
>  drivers/gpu/drm/i915/intel_device_info.c |  9 ++++
>  drivers/gpu/drm/i915/intel_device_info.h |  5 ++-
>  include/drm/intel/pciids.h               | 55 ++++++++++++++++++------
>  9 files changed, 129 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_cpu_info.c
>  create mode 100644 drivers/gpu/drm/i915/intel_cpu_info.h

-- 
Jani Nikula, Intel

Re: [PATCH v3 1/4] drm/intel/pciids: Refactor DG2 PCI IDs into segment ranges

[Riana Tauro]

On 10/30/2024 8:04 PM, Raag Jadav wrote:
> Refactor DG2 PCI IDs into D, E and M ranges which will be useful for
> segment specific features.
> 
> v3: Rework subplatform naming (Jani)
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
Looks good to me
Reviewed-by: Riana Tauro <riana.tauro@intel.com>
> ---
>   include/drm/intel/pciids.h | 55 +++++++++++++++++++++++++++++---------
>   1 file changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/include/drm/intel/pciids.h b/include/drm/intel/pciids.h
> index 7632507af166..83aac9f17372 100644
> --- a/include/drm/intel/pciids.h
> +++ b/include/drm/intel/pciids.h
> @@ -717,37 +717,66 @@
>   	MACRO__(0xA7AB, ## __VA_ARGS__)
>   
>   /* DG2 */
> -#define INTEL_DG2_G10_IDS(MACRO__, ...) \
> -	MACRO__(0x5690, ## __VA_ARGS__), \
> -	MACRO__(0x5691, ## __VA_ARGS__), \
> -	MACRO__(0x5692, ## __VA_ARGS__), \
> +#define INTEL_DG2_G10_D_IDS(MACRO__, ...) \
>   	MACRO__(0x56A0, ## __VA_ARGS__), \
>   	MACRO__(0x56A1, ## __VA_ARGS__), \
> -	MACRO__(0x56A2, ## __VA_ARGS__), \
> +	MACRO__(0x56A2, ## __VA_ARGS__)
> +
> +#define INTEL_DG2_G10_E_IDS(MACRO__, ...) \
>   	MACRO__(0x56BE, ## __VA_ARGS__), \
>   	MACRO__(0x56BF, ## __VA_ARGS__)
>   
> -#define INTEL_DG2_G11_IDS(MACRO__, ...) \
> -	MACRO__(0x5693, ## __VA_ARGS__), \
> -	MACRO__(0x5694, ## __VA_ARGS__), \
> -	MACRO__(0x5695, ## __VA_ARGS__), \
> +#define INTEL_DG2_G10_M_IDS(MACRO__, ...) \
> +	MACRO__(0x5690, ## __VA_ARGS__), \
> +	MACRO__(0x5691, ## __VA_ARGS__), \
> +	MACRO__(0x5692, ## __VA_ARGS__)
> +
> +#define INTEL_DG2_G10_IDS(MACRO__, ...) \
> +	INTEL_DG2_G10_D_IDS(MACRO__, ## __VA_ARGS__), \
> +	INTEL_DG2_G10_E_IDS(MACRO__, ## __VA_ARGS__), \
> +	INTEL_DG2_G10_M_IDS(MACRO__, ## __VA_ARGS__)
> +
> +#define INTEL_DG2_G11_D_IDS(MACRO__, ...) \
>   	MACRO__(0x56A5, ## __VA_ARGS__), \
>   	MACRO__(0x56A6, ## __VA_ARGS__), \
>   	MACRO__(0x56B0, ## __VA_ARGS__), \
> -	MACRO__(0x56B1, ## __VA_ARGS__), \
> +	MACRO__(0x56B1, ## __VA_ARGS__)
> +
> +#define INTEL_DG2_G11_E_IDS(MACRO__, ...) \
>   	MACRO__(0x56BA, ## __VA_ARGS__), \
>   	MACRO__(0x56BB, ## __VA_ARGS__), \
>   	MACRO__(0x56BC, ## __VA_ARGS__), \
>   	MACRO__(0x56BD, ## __VA_ARGS__)
>   
> -#define INTEL_DG2_G12_IDS(MACRO__, ...) \
> -	MACRO__(0x5696, ## __VA_ARGS__), \
> -	MACRO__(0x5697, ## __VA_ARGS__), \
> +#define INTEL_DG2_G11_M_IDS(MACRO__, ...) \
> +	MACRO__(0x5693, ## __VA_ARGS__), \
> +	MACRO__(0x5694, ## __VA_ARGS__), \
> +	MACRO__(0x5695, ## __VA_ARGS__)
> +
> +#define INTEL_DG2_G11_IDS(MACRO__, ...) \
> +	INTEL_DG2_G11_D_IDS(MACRO__, ## __VA_ARGS__), \
> +	INTEL_DG2_G11_E_IDS(MACRO__, ## __VA_ARGS__), \
> +	INTEL_DG2_G11_M_IDS(MACRO__, ## __VA_ARGS__)
> +
> +#define INTEL_DG2_G12_D_IDS(MACRO__, ...) \
>   	MACRO__(0x56A3, ## __VA_ARGS__), \
>   	MACRO__(0x56A4, ## __VA_ARGS__), \
>   	MACRO__(0x56B2, ## __VA_ARGS__), \
>   	MACRO__(0x56B3, ## __VA_ARGS__)
>   
> +#define INTEL_DG2_G12_M_IDS(MACRO__, ...) \
> +	MACRO__(0x5696, ## __VA_ARGS__), \
> +	MACRO__(0x5697, ## __VA_ARGS__)
> +
> +#define INTEL_DG2_G12_IDS(MACRO__, ...) \
> +	INTEL_DG2_G12_D_IDS(MACRO__, ## __VA_ARGS__), \
> +	INTEL_DG2_G12_M_IDS(MACRO__, ## __VA_ARGS__)
> +
> +#define INTEL_DG2_D_IDS(MACRO__, ...) \
> +	INTEL_DG2_G10_D_IDS(MACRO__, ## __VA_ARGS__), \
> +	INTEL_DG2_G11_D_IDS(MACRO__, ## __VA_ARGS__), \
> +	INTEL_DG2_G12_D_IDS(MACRO__, ## __VA_ARGS__)
> +
>   #define INTEL_DG2_IDS(MACRO__, ...) \
>   	INTEL_DG2_G10_IDS(MACRO__, ## __VA_ARGS__), \
>   	INTEL_DG2_G11_IDS(MACRO__, ## __VA_ARGS__), \

Re: [PATCH v3 3/4] drm/i915: Introduce intel_cpu_info.c for CPU IDs

[Riana Tauro]

On 10/30/2024 8:04 PM, Raag Jadav wrote:
> Having similar naming convention in intel-family.h and intel_device_info.h
> results in redefinition of a few platforms. Define CPU IDs in its own file
> to avoid this.
> 
> v3: Move file out of gt directory, add kernel doc (Riana)
>      Rephrase file description (Jani)
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>   drivers/gpu/drm/i915/Makefile         |  1 +
>   drivers/gpu/drm/i915/intel_cpu_info.c | 42 +++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_cpu_info.h | 13 +++++++++
>   3 files changed, 56 insertions(+)
>   create mode 100644 drivers/gpu/drm/i915/intel_cpu_info.c
>   create mode 100644 drivers/gpu/drm/i915/intel_cpu_info.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 31710d98cad5..208929358b25 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -35,6 +35,7 @@ i915-y += \
>   	i915_sysfs.o \
>   	i915_utils.o \
>   	intel_clock_gating.o \
> +	intel_cpu_info.o \
>   	intel_device_info.o \
>   	intel_memory_region.o \
>   	intel_pcode.o \
> diff --git a/drivers/gpu/drm/i915/intel_cpu_info.c b/drivers/gpu/drm/i915/intel_cpu_info.c
> new file mode 100644
> index 000000000000..9a1bfff4be7d
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_cpu_info.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + *
> + * Avoid INTEL_<PLATFORM> name collisions between asm/intel-family.h and
> + * intel_device_info.h by having a separate file.
> + */
> +
> +#include "intel_cpu_info.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),
> +	{}
> +};
> +
> +/**
> + * intel_match_g8_cpu - match current CPU against g8_cpu_ids[]
> + *
> + * This matches current CPU against g8_cpu_ids[], which are applicable
> + * for G8 workaround.
nit:

[] is not necessaray

Looks good to me
Reviewed-by: Riana Tauro <riana.tauro@intel.com>
> + *
> + * Returns: %true if matches, %false otherwise.
> + */
> +bool intel_match_g8_cpu(void)
> +{
> +	return x86_match_cpu(g8_cpu_ids);
> +}
> +#else
> +bool intel_match_g8_cpu(void) { return false; }
> +#endif
> diff --git a/drivers/gpu/drm/i915/intel_cpu_info.h b/drivers/gpu/drm/i915/intel_cpu_info.h
> new file mode 100644
> index 000000000000..dd0c5e784b97
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_cpu_info.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef _INTEL_CPU_INFO_H_
> +#define _INTEL_CPU_INFO_H_
> +
> +#include <linux/types.h>
> +
> +bool intel_match_g8_cpu(void);
> +
> +#endif

Re: [PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Riana Tauro]

On 10/30/2024 8:04 PM, Raag Jadav wrote:
> G8 power state entry is disabled due to a limitation on DG2, so we
> enable it from driver with Wa_14022698537. For now we enable it for
> all DG2 devices with the exception of a few, for which, we enable
> only when paired with whitelisted CPU models. This works with Native
> ASMP and reduces idle power consumption.
%s/ASMP/ASPM

With that
Reviewed-by: Riana Tauro <riana.tauro@intel.com>
> 
> $ echo powersave > /sys/module/pcie_aspm/parameters/policy
> $ lspci -s 0000:03:00.0 -vvv
> LnkCtl: ASPM L1 Enabled; RCB 64 bytes, Disabled- CommClk-
> 
> v2: Fix Wa_ID and include it in subject (Badal)
>      Rephrase commit message (Jani)
> v3: Move workaround to i915_pcode_init() (Badal, Anshuman)
>      Re-order macro (Riana)
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_driver.c | 15 +++++++++++++++
>   drivers/gpu/drm/i915/i915_reg.h    |  1 +
>   2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 365329ff8a07..59c6124c9bc2 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -93,12 +93,14 @@
>   #include "i915_memcpy.h"
>   #include "i915_perf.h"
>   #include "i915_query.h"
> +#include "i915_reg.h"
>   #include "i915_suspend.h"
>   #include "i915_switcheroo.h"
>   #include "i915_sysfs.h"
>   #include "i915_utils.h"
>   #include "i915_vgpu.h"
>   #include "intel_clock_gating.h"
> +#include "intel_cpu_info.h"
>   #include "intel_gvt.h"
>   #include "intel_memory_region.h"
>   #include "intel_pci_config.h"
> @@ -415,6 +417,18 @@ static int i915_set_dma_info(struct drm_i915_private *i915)
>   	return ret;
>   }
>   
> +/* Wa_14022698537:dg2 */
> +static void i915_enable_g8(struct drm_i915_private *i915)
> +{
> +	if (IS_DG2(i915)) {
> +		if (IS_DG2_D(i915) && !intel_match_g8_cpu())
> +			return;
> +
> +		snb_pcode_write_p(&i915->uncore, PCODE_POWER_SETUP,
> +				  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
> +	}
> +}
> +
>   static int i915_pcode_init(struct drm_i915_private *i915)
>   {
>   	struct intel_gt *gt;
> @@ -428,6 +442,7 @@ static int i915_pcode_init(struct drm_i915_private *i915)
>   		}
>   	}
>   
> +	i915_enable_g8(i915);
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 89e4381f8baa..d400c77423a5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3617,6 +3617,7 @@
>   #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)
> +#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6
Is the alignment correct?
>   #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
>   #define   XEHP_PCODE_FREQUENCY_CONFIG		0x6e	/* pvc */
>   /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */

Re: [PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Raag Jadav]

On Tue, Dec 10, 2024 at 01:33:29PM +0530, Riana Tauro wrote:
> On 10/30/2024 8:04 PM, Raag Jadav wrote:
> > G8 power state entry is disabled due to a limitation on DG2, so we
> > enable it from driver with Wa_14022698537. For now we enable it for
> > all DG2 devices with the exception of a few, for which, we enable
> > only when paired with whitelisted CPU models. This works with Native
> > ASMP and reduces idle power consumption.
> %s/ASMP/ASPM

Turns out I already have it locally but I don't remember fixing it.
Bit of a supernatural moment.

> With that
> Reviewed-by: Riana Tauro <riana.tauro@intel.com>

Awesome :)

...

> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 89e4381f8baa..d400c77423a5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3617,6 +3617,7 @@
> >   #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)
> > +#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6
> Is the alignment correct?

Yeah, we all fell for the same trick.
https://lore.kernel.org/intel-gfx/ZyJgYML0jLuHxG7G@black.fi.intel.com/

Raag

> >   #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
> >   #define   XEHP_PCODE_FREQUENCY_CONFIG		0x6e	/* pvc */
> >   /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */
> 

Re: [PATCH v3 1/4] drm/intel/pciids: Refactor DG2 PCI IDs into segment ranges

[Andi Shyti]

Hi Raag,

On Wed, Oct 30, 2024 at 08:04:15PM +0530, Raag Jadav wrote:
> Refactor DG2 PCI IDs into D, E and M ranges which will be useful for
> segment specific features.
> 
> v3: Rework subplatform naming (Jani)
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

Re: [PATCH v3 3/4] drm/i915: Introduce intel_cpu_info.c for CPU IDs

[Andi Shyti]

Hi Raag,

On Wed, Oct 30, 2024 at 08:04:17PM +0530, Raag Jadav wrote:
> Having similar naming convention in intel-family.h and intel_device_info.h
> results in redefinition of a few platforms. Define CPU IDs in its own file
> to avoid this.
> 
> v3: Move file out of gt directory, add kernel doc (Riana)
>     Rephrase file description (Jani)
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

Re: [PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Andi Shyti]

Hi Raag,

> +/* Wa_14022698537:dg2 */
> +static void i915_enable_g8(struct drm_i915_private *i915)
> +{
> +	if (IS_DG2(i915)) {
> +		if (IS_DG2_D(i915) && !intel_match_g8_cpu())
> +			return;
> +
> +		snb_pcode_write_p(&i915->uncore, PCODE_POWER_SETUP,
> +				  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
> +	}

In the workaround description there an "else if" which I am not
understanding. I it suggesting to do nothing or is it suggesting
to do the same thing?

> +}
> +
>  static int i915_pcode_init(struct drm_i915_private *i915)
>  {
>  	struct intel_gt *gt;
> @@ -428,6 +442,7 @@ static int i915_pcode_init(struct drm_i915_private *i915)
>  		}
>  	}
>  
> +	i915_enable_g8(i915);
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 89e4381f8baa..d400c77423a5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3617,6 +3617,7 @@
>  #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)
> +#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6

for aesthetics 0x06 would look better, but this should be
changed everywhere in the file because single digit hex values
are used. It's better to keep uniformity in the style.

Andi

>  #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
>  #define   XEHP_PCODE_FREQUENCY_CONFIG		0x6e	/* pvc */
>  /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */
> -- 
> 2.34.1

Re: [PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Raag Jadav]

On Tue, Dec 10, 2024 at 01:52:21PM +0100, Andi Shyti wrote:
> Hi Raag,
> 
> > +/* Wa_14022698537:dg2 */
> > +static void i915_enable_g8(struct drm_i915_private *i915)
> > +{
> > +	if (IS_DG2(i915)) {
> > +		if (IS_DG2_D(i915) && !intel_match_g8_cpu())
> > +			return;
> > +
> > +		snb_pcode_write_p(&i915->uncore, PCODE_POWER_SETUP,
> > +				  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
> > +	}
> 
> In the workaround description there an "else if" which I am not
> understanding. I it suggesting to do nothing or is it suggesting
> to do the same thing?

We do the same thing (apply WA) except when the _D IDs are not paired
with whitelisted CPUs. What I did here is added a return call with
inverted CPU logic and got rid of the else part. I know it looks quirky
but does the job.

> > +}
> > +
> >  static int i915_pcode_init(struct drm_i915_private *i915)
> >  {
> >  	struct intel_gt *gt;
> > @@ -428,6 +442,7 @@ static int i915_pcode_init(struct drm_i915_private *i915)
> >  		}
> >  	}
> >  
> > +	i915_enable_g8(i915);
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 89e4381f8baa..d400c77423a5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3617,6 +3617,7 @@
> >  #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)
> > +#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6
> 
> for aesthetics 0x06 would look better, but this should be
> changed everywhere in the file because single digit hex values
> are used. It's better to keep uniformity in the style.

I agree about uniformity but perhaps a separate filewide patch would
be much cleaner IMHO. Let's keep it as it is for now.

> >  #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
> >  #define   XEHP_PCODE_FREQUENCY_CONFIG		0x6e	/* pvc */
> >  /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */
> > -- 
> > 2.34.1

Re: [PATCH v3 4/4] drm/i915/dg2: Implement Wa_14022698537

[Andi Shyti]

On Wed, Dec 11, 2024 at 07:21:54AM +0200, Raag Jadav wrote:
> On Tue, Dec 10, 2024 at 01:52:21PM +0100, Andi Shyti wrote:
> > Hi Raag,
> > 
> > > +/* Wa_14022698537:dg2 */
> > > +static void i915_enable_g8(struct drm_i915_private *i915)
> > > +{
> > > +	if (IS_DG2(i915)) {
> > > +		if (IS_DG2_D(i915) && !intel_match_g8_cpu())
> > > +			return;
> > > +
> > > +		snb_pcode_write_p(&i915->uncore, PCODE_POWER_SETUP,
> > > +				  POWER_SETUP_SUBCOMMAND_G8_ENABLE, 0, 0);
> > > +	}
> > 
> > In the workaround description there an "else if" which I am not
> > understanding. I it suggesting to do nothing or is it suggesting
> > to do the same thing?
> 
> We do the same thing (apply WA) except when the _D IDs are not paired
> with whitelisted CPUs. What I did here is added a return call with
> inverted CPU logic and got rid of the else part. I know it looks quirky
> but does the job.

Thanks... the document is weirdly fromatted and I wanted to make
sure everything is aligned.

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

> > > +}
> > > +
> > >  static int i915_pcode_init(struct drm_i915_private *i915)
> > >  {
> > >  	struct intel_gt *gt;
> > > @@ -428,6 +442,7 @@ static int i915_pcode_init(struct drm_i915_private *i915)
> > >  		}
> > >  	}
> > >  
> > > +	i915_enable_g8(i915);
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 89e4381f8baa..d400c77423a5 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -3617,6 +3617,7 @@
> > >  #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)
> > > +#define     POWER_SETUP_SUBCOMMAND_G8_ENABLE	0x6
> > 
> > for aesthetics 0x06 would look better, but this should be
> > changed everywhere in the file because single digit hex values
> > are used. It's better to keep uniformity in the style.
> 
> I agree about uniformity but perhaps a separate filewide patch would
> be much cleaner IMHO. Let's keep it as it is for now.

Yes, that was my point. The problem with a separate patch is that
you would screw up git blame because you would touch so many
lines with a trivial change. Therefore, it's better to leave it
as it is.

Thanks,
Andi

> > >  #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
> > >  #define   XEHP_PCODE_FREQUENCY_CONFIG		0x6e	/* pvc */
> > >  /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */
> > > -- 
> > > 2.34.1