[Intel-gfx] [PATCH 1/5] drm/i915/guc: Don't capture Gen8 regs on Xe devices

[Teres Alexis, Alan Previn]

On Wed, 2023-04-26 at 10:23 -0700, Harrison, John C wrote:
> 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 at 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 at 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.
> 
alan: you said "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".
That wasnt the original intent when authored. The original intent
was list names and its comments to corresponded to what platforms we
supported (thats why the first patch was all of Gen12 registers in a
single list that included Gen8/9 register definitions and Gen9 sublists
got abstracted out later).

I'm okay with changing the intent of the list naming - but your code still
looks a bit off considering you have at least one list with two sublists
that carry the term "GEN9":
  static const struct __guc_mmio_reg_descr default_global_regs[] = {
	COMMON_BASE_GLOBAL,
	COMMON_GEN9BASE_GLOBAL,
	GEN9_GLOBAL,

Again, i feel the name of these two sublists ("COMMON_GEN9BASE_GLOBAL" vs
"GEN9_GLOBAL") dont easily portray differences and why they were separated
out. If they both reflect "when the named register got introduced", then
why not just have them in the same list? Since this patch is not
about "when a register got introduced" but "pre-Xe registers are not
recognized by GuC on Xe", then perhaps we can change the names to:
	COMMON_GEN9BASE_GLOBAL -> [same]
	GEN9_GLOBAL -> PREXE_GEN9_GLOBAL

Either way, i rather not argue about variable names - so here is the Rb
(since patch comment describes the issue and functionality seems correct):
Reviewed-by: Alan Previn <alan.previn.teres.alexis at intel.com>