| Message ID | 20230406222617.790484-2-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improvements to GuC error capture list processing | expand |
On Thu, 2023-04-06 at 15:26 -0700, Harrison, John C wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > 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 <alan.previn.teres.alexis@intel.com>
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 <John.C.Harrison@Intel.com> >> >> 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 <alan.previn.teres.alexis@intel.com> > 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.
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[] = {