Re: [PATCH 1/5] drm/i915/guc: Don't capture Gen8 regs on Xe devices
On 4/25/2023 10:55, Teres Alexis, Alan Previn wrote:
On Thu, 2023-04-06 at 15:26 -0700, Harrison, John C wrote:
From: John Harrison
A pair of pre-Xe registers were being included in the Xe capture list.
GuC was rejecting those as being invalid and logging errors about
them. So, stop doing it.
alan:snip
#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,
};
alan: splitting out a couple registers from COMMON_GEN9BASE_GLOBAL into
GEN9_GLOBAL
doesn't seem to communicate the intent of fix for this patch. This is more of a
naming,
thing and i am not sure what counter-proposal will work well in terms of
readibility.
One idea: perhaps we rename "COMMON_GEN9BASE_GLOBAL" to
"COMMON_GEN9PLUS_BASE_GLOBAL"
and rename GEN9_GLOBAL to COMMON_GEN9LEGACY_GLOBAL. so we would have two
gen9-global
with a clear distinction in naming where one is "GEN9PLUS" and the other is
"GEN9LEGACY".
But since this is a list-naming thing, i am okay either above change... OR...
keeping the same but with the condition of adding a comment under
COMMON_GEN9BASE_GLOBAL and GEN9_GLOBAL names that explain the differences where
one
is gen9-legacy and the other is gen9-and-future that carries over to beyond
Gen9.
(side note: coding style wise, is it possible to add the comment right under
the #define
line as opposed to under the entire list?)
(conditional) Reviewed-by: Alan Previn
I'm not entirely sure what you are arguing here.
My reading of the original code is that COMMON_GENX_ means the registers
were introduced on the named device but a are common to later devices.
Whereas GENX_ means the registers are specific to that device alone.
That seems a pretty straight forward and simple naming scheme to me.
John.
Re: [PATCH 1/5] drm/i915/guc: Don't capture Gen8 regs on Xe devices
On Thu, 2023-04-06 at 15:26 -0700, Harrison, John C wrote:
> From: John Harrison
>
> A pair of pre-Xe registers were being included in the Xe capture list.
> GuC was rejecting those as being invalid and logging errors about
> them. So, stop doing it.
>
alan:snip
> #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,
> };
>
alan: splitting out a couple registers from COMMON_GEN9BASE_GLOBAL into
GEN9_GLOBAL
doesn't seem to communicate the intent of fix for this patch. This is more of a
naming,
thing and i am not sure what counter-proposal will work well in terms of
readibility.
One idea: perhaps we rename "COMMON_GEN9BASE_GLOBAL" to
"COMMON_GEN9PLUS_BASE_GLOBAL"
and rename GEN9_GLOBAL to COMMON_GEN9LEGACY_GLOBAL. so we would have two
gen9-global
with a clear distinction in naming where one is "GEN9PLUS" and the other is
"GEN9LEGACY".
But since this is a list-naming thing, i am okay either above change... OR...
keeping the same but with the condition of adding a comment under
COMMON_GEN9BASE_GLOBAL and GEN9_GLOBAL names that explain the differences where
one
is gen9-legacy and the other is gen9-and-future that carries over to beyond
Gen9.
(side note: coding style wise, is it possible to add the comment right under
the #define
line as opposed to under the entire list?)
(conditional) Reviewed-by: Alan Previn
[PATCH 1/5] drm/i915/guc: Don't capture Gen8 regs on Xe devices
From: John Harrison
A pair of pre-Xe registers were being included in the Xe 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
