On 4/26/2023 14:14, Teres Alexis, Alan Previn wrote:
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@xxxxxxxxx> 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@xxxxxxxxx>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
PREXE_GEN9_... just sounds confused to me. What is Gen9 if it is not PreXe?The point is to ensure that platform specific register lists are constructed from the sublists that apply to that platform. Therefore the sublists are named for the platform they apply to. Thus the gen8 list should only contain gen8 sub-lists. The Xe list can contain Xe sublists together with gen8 sublists that are still applicable. Those sublists have a COMMON_ prefix if they are expected to be multi-platform and don't if they are specific to their one named platform.
Note that you can't get hung on 'the end result looks odd' when only looking at the first step of a whole series of cleanups. This patch is specifically about moving the pre-Xe registers out of the COMMON_ define so that they are not used on Xe. It is not trying to fix up all the naming and other issues.
John.
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@xxxxxxxxx>
