Re: [Intel-gfx] [PATCH] drm/i915/guc: Don't capture Gen8 regs on Gen12 devices
On Wed, Apr 05, 2023 at 02:13:31PM -0700, John Harrison wrote:
> On 4/3/2023 17:34, Matt Roper wrote:
> > On Mon, Apr 03, 2023 at 02:33:34PM -0700, john.c.harri...@intel.com wrote:
> > > From: John Harrison
> > >
> > > A pair of pre-Gen12 registers were being included in the Gen12 capture
> > > list. GuC was rejecting those as being invalid and logging errors
> > > about them. So, stop doing it.
> > Looks like these registers existed from gen8-gen11. With this change,
> > it looks like they also won't be included in the GuC error capture for
> > gen11 (ICL and EHL/JSL) since those platforms return xe_lpd_lists [1]
> > rather than default_lists; do we care about that? I assume not (since
> > those platforms don't use GuC submission unless you force it with the
> > enable_guc modparam and taint your kernel), but I figured I should point
> > it out.
> Yeah, I think the code is treating Gen11 as Gen12 rather than Gen9 or it's
> own thing. I hadn't spotted that before. It certainly seems incorrect.
>
> >
> > Reviewed-by: Matt Roper
> >
> >
> > [1] Why is the main list we use called xe_lpd (i.e., the name of ADL-P's
> > display IP)? It doesn't seem like we're doing anything with display
> > registers here so using display IP naming seems really confusing.
> I think because no-one has a clue what name refers to what hardware any more
> :(.
>
> What are the official names for IP_VER 9, 11, 12.00, 12.50 and 12.55?
Yeah, the naming is a real mess. :-( For graphics IP, the official
terms are supposed to be:
12.00 = Xe_LP
12.10 = Xe_LP+ (basically the same as Xe_LP except for interrupts)
12.50 = Xe_HP
12.55 = Xe_HPG (it's nearly identical to Xe_HP)
12.7x = Xe_LPG
There are separate names for media, although we didn't really start
using them anywhere in the i915 until the separation of IPs started
becoming more important with MTL:
12.00 = Xe_M (or Xe_M+ for DG1, but we treat it the same in the KMD)
12.50 = Xe_XPM
12.55 = Xe_HPM
12.60 = Xe_XPM+
13.00 = Xe_LPM+
and display:
12.00 = Xe_D
13.00 = Xe_LPD (ADL-P) or Xe_HPD (DG2)
14.00 = Xe_LPD+
The pre-12 stuff predates the fancy new marketing-mandated names. Even
though we're not using "gen" terminology going forward, those old ones
are grandfathered in, so it's still okay to refer to them as gen9,
gen11, etc.
Matt
>
> John.
>
> >
> >
> > Matt
> >
> > > Signed-off-by: John Harrison
> > > Fixes: dce2bd542337 ("drm/i915/guc: Add Gen9 registers for GuC error
> > > state capture.")
> > > Cc: Alan Previn
> > > Cc: Umesh Nerlige Ramappa
> > > Cc: Lucas De Marchi
> > > Cc: John Harrison
> > > Cc: Jani Nikula
> > > Cc: Matt Roper
> > > Cc: Balasubramani Vivekanandan
> > > Cc: Daniele Ceraolo Spurio
> > > ---
> > > drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c | 7 +--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > > b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > > index cf49188db6a6e..e0e793167d61b 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > > @@ -31,12 +31,14 @@
> > > { FORCEWAKE_MT, 0, 0, "FORCEWAKE" }
> > > #define COMMON_GEN9BASE_GLOBAL \
> > > - { GEN8_FAULT_TLB_DATA0, 0, 0, "GEN8_FAULT_TLB_DATA0" }, \
> > > - { GEN8_FAULT_TLB_DATA1, 0, 0, "GEN8_FAULT_TLB_DATA1" }, \
> > > { ERROR_GEN6, 0, 0, "ERROR_GEN6" }, \
> > > { DONE_REG, 0, 0, "DONE_REG" }, \
> > > { HSW_GTT_CACHE_EN, 0, 0, "HSW_GTT_CACHE_EN" }
> > > +#define GEN9_GLOBAL \
> > > + { GEN8_FAULT_TLB_DATA0, 0, 0, "GEN8_FAULT_TLB_DATA0" }, \
> > > + { GEN8_FAULT_TLB_DATA1, 0, 0, "GEN8_FAULT_TLB_DATA1" }
> > > +
> > > #define COMMON_GEN12BASE_GLOBAL \
> > > { GEN12_FAULT_TLB_DATA0,0, 0, "GEN12_FAULT_TLB_DATA0"
> > > }, \
> > > { GEN12_FAULT_TLB_DATA1,0, 0, "GEN12_FAULT_TLB_DATA1"
> > > }, \
> > > @@ -142,6 +144,7 @@ static const struct __guc_mmio_reg_descr
> > > xe_lpd_gsc_inst_regs[] = {
> > > static const struct __guc_mmio_reg_descr default_global_regs[] = {
> > > COMMON_BASE_GLOBAL,
> > > COMMON_GEN9BASE_GLOBAL,
> > > + GEN9_GLOBAL,
> > > };
> > > static const struct __guc_mmio_reg_descr default_rc_class_regs[] = {
> > > --
> > > 2.39.1
> > >
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
Re: [Intel-gfx] [PATCH] drm/i915/guc: Don't capture Gen8 regs on Gen12 devices
On 4/3/2023 17:34, Matt Roper wrote:
On Mon, Apr 03, 2023 at 02:33:34PM -0700, john.c.harri...@intel.com wrote:
From: John Harrison
A pair of pre-Gen12 registers were being included in the Gen12 capture
list. GuC was rejecting those as being invalid and logging errors
about them. So, stop doing it.
Looks like these registers existed from gen8-gen11. With this change,
it looks like they also won't be included in the GuC error capture for
gen11 (ICL and EHL/JSL) since those platforms return xe_lpd_lists [1]
rather than default_lists; do we care about that? I assume not (since
those platforms don't use GuC submission unless you force it with the
enable_guc modparam and taint your kernel), but I figured I should point
it out.
Yeah, I think the code is treating Gen11 as Gen12 rather than Gen9 or
it's own thing. I hadn't spotted that before. It certainly seems incorrect.
Reviewed-by: Matt Roper
[1] Why is the main list we use called xe_lpd (i.e., the name of ADL-P's
display IP)? It doesn't seem like we're doing anything with display
registers here so using display IP naming seems really confusing.
I think because no-one has a clue what name refers to what hardware any
more :(.
What are the official names for IP_VER 9, 11, 12.00, 12.50 and 12.55?
John.
Matt
Signed-off-by: John Harrison
Fixes: dce2bd542337 ("drm/i915/guc: Add Gen9 registers for GuC error state
capture.")
Cc: Alan Previn
Cc: Umesh Nerlige Ramappa
Cc: Lucas De Marchi
Cc: John Harrison
Cc: Jani Nikula
Cc: Matt Roper
Cc: Balasubramani Vivekanandan
Cc: Daniele Ceraolo Spurio
---
drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
index cf49188db6a6e..e0e793167d61b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -31,12 +31,14 @@
{ FORCEWAKE_MT, 0, 0, "FORCEWAKE" }
#define COMMON_GEN9BASE_GLOBAL \
- { GEN8_FAULT_TLB_DATA0, 0, 0, "GEN8_FAULT_TLB_DATA0" }, \
- { GEN8_FAULT_TLB_DATA1, 0, 0, "GEN8_FAULT_TLB_DATA1" }, \
{ ERROR_GEN6, 0, 0, "ERROR_GEN6" }, \
{ DONE_REG, 0, 0, "DONE_REG" }, \
{ HSW_GTT_CACHE_EN, 0, 0, "HSW_GTT_CACHE_EN" }
+#define GEN9_GLOBAL \
+ { GEN8_FAULT_TLB_DATA0, 0, 0, "GEN8_FAULT_TLB_DATA0" }, \
+ { GEN8_FAULT_TLB_DATA1, 0, 0, "GEN8_FAULT_TLB_DATA1" }
+
#define COMMON_GEN12BASE_GLOBAL \
{ GEN12_FAULT_TLB_DATA0,0, 0, "GEN12_FAULT_TLB_DATA0" }, \
{ GEN12_FAULT_TLB_DATA1,0, 0, "GEN12_FAULT_TLB_DATA1" }, \
@@ -142,6 +144,7 @@ static const struct __guc_mmio_reg_descr
xe_lpd_gsc_inst_regs[] = {
static const struct __guc_mmio_reg_descr default_global_regs[] = {
COMMON_BASE_GLOBAL,
COMMON_GEN9BASE_GLOBAL,
+ GEN9_GLOBAL,
};
static const struct __guc_mmio_reg_descr default_rc_class_regs[] = {
--
2.39.1
Re: [Intel-gfx] [PATCH] drm/i915/guc: Don't capture Gen8 regs on Gen12 devices
On Mon, Apr 03, 2023 at 02:33:34PM -0700, john.c.harri...@intel.com wrote:
> From: John Harrison
>
> A pair of pre-Gen12 registers were being included in the Gen12 capture
> list. GuC was rejecting those as being invalid and logging errors
> about them. So, stop doing it.
Looks like these registers existed from gen8-gen11. With this change,
it looks like they also won't be included in the GuC error capture for
gen11 (ICL and EHL/JSL) since those platforms return xe_lpd_lists [1]
rather than default_lists; do we care about that? I assume not (since
those platforms don't use GuC submission unless you force it with the
enable_guc modparam and taint your kernel), but I figured I should point
it out.
Reviewed-by: Matt Roper
[1] Why is the main list we use called xe_lpd (i.e., the name of ADL-P's
display IP)? It doesn't seem like we're doing anything with display
registers here so using display IP naming seems really confusing.
Matt
>
> Signed-off-by: John Harrison
> Fixes: dce2bd542337 ("drm/i915/guc: Add Gen9 registers for GuC error state
> capture.")
> Cc: Alan Previn
> Cc: Umesh Nerlige Ramappa
> Cc: Lucas De Marchi
> Cc: John Harrison
> Cc: Jani Nikula
> Cc: Matt Roper
> Cc: Balasubramani Vivekanandan
> Cc: Daniele Ceraolo Spurio
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> index cf49188db6a6e..e0e793167d61b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> @@ -31,12 +31,14 @@
> { FORCEWAKE_MT, 0, 0, "FORCEWAKE" }
>
> #define COMMON_GEN9BASE_GLOBAL \
> - { GEN8_FAULT_TLB_DATA0, 0, 0, "GEN8_FAULT_TLB_DATA0" }, \
> - { GEN8_FAULT_TLB_DATA1, 0, 0, "GEN8_FAULT_TLB_DATA1" }, \
> { ERROR_GEN6, 0, 0, "ERROR_GEN6" }, \
> { DONE_REG, 0, 0, "DONE_REG" }, \
> { HSW_GTT_CACHE_EN, 0, 0, "HSW_GTT_CACHE_EN" }
>
> +#define GEN9_GLOBAL \
> + { GEN8_FAULT_TLB_DATA0, 0, 0, "GEN8_FAULT_TLB_DATA0" }, \
> + { GEN8_FAULT_TLB_DATA1, 0, 0, "GEN8_FAULT_TLB_DATA1" }
> +
> #define COMMON_GEN12BASE_GLOBAL \
> { GEN12_FAULT_TLB_DATA0,0, 0, "GEN12_FAULT_TLB_DATA0" }, \
> { GEN12_FAULT_TLB_DATA1,0, 0, "GEN12_FAULT_TLB_DATA1" }, \
> @@ -142,6 +144,7 @@ static const struct __guc_mmio_reg_descr
> xe_lpd_gsc_inst_regs[] = {
> static const struct __guc_mmio_reg_descr default_global_regs[] = {
> COMMON_BASE_GLOBAL,
> COMMON_GEN9BASE_GLOBAL,
> + GEN9_GLOBAL,
> };
>
> static const struct __guc_mmio_reg_descr default_rc_class_regs[] = {
> --
> 2.39.1
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
[Intel-gfx] [PATCH] drm/i915/guc: Don't capture Gen8 regs on Gen12 devices
From: John Harrison
A pair of pre-Gen12 registers were being included in the Gen12 capture
list. GuC was rejecting those as being invalid and logging errors
about them. So, stop doing it.
Signed-off-by: John Harrison
Fixes: dce2bd542337 ("drm/i915/guc: Add Gen9 registers for GuC error state
capture.")
Cc: Alan Previn
Cc: Umesh Nerlige Ramappa
Cc: Lucas De Marchi
Cc: John Harrison
Cc: Jani Nikula
Cc: Matt Roper
Cc: Balasubramani Vivekanandan
Cc: Daniele Ceraolo Spurio
---
drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
index cf49188db6a6e..e0e793167d61b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -31,12 +31,14 @@
{ FORCEWAKE_MT, 0, 0, "FORCEWAKE" }
#define COMMON_GEN9BASE_GLOBAL \
- { GEN8_FAULT_TLB_DATA0, 0, 0, "GEN8_FAULT_TLB_DATA0" }, \
- { GEN8_FAULT_TLB_DATA1, 0, 0, "GEN8_FAULT_TLB_DATA1" }, \
{ ERROR_GEN6, 0, 0, "ERROR_GEN6" }, \
{ DONE_REG, 0, 0, "DONE_REG" }, \
{ HSW_GTT_CACHE_EN, 0, 0, "HSW_GTT_CACHE_EN" }
+#define GEN9_GLOBAL \
+ { GEN8_FAULT_TLB_DATA0, 0, 0, "GEN8_FAULT_TLB_DATA0" }, \
+ { GEN8_FAULT_TLB_DATA1, 0, 0, "GEN8_FAULT_TLB_DATA1" }
+
#define COMMON_GEN12BASE_GLOBAL \
{ GEN12_FAULT_TLB_DATA0,0, 0, "GEN12_FAULT_TLB_DATA0" }, \
{ GEN12_FAULT_TLB_DATA1,0, 0, "GEN12_FAULT_TLB_DATA1" }, \
@@ -142,6 +144,7 @@ static const struct __guc_mmio_reg_descr
xe_lpd_gsc_inst_regs[] = {
static const struct __guc_mmio_reg_descr default_global_regs[] = {
COMMON_BASE_GLOBAL,
COMMON_GEN9BASE_GLOBAL,
+ GEN9_GLOBAL,
};
static const struct __guc_mmio_reg_descr default_rc_class_regs[] = {
--
2.39.1
