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

[John Harrison]

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 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
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 at intel.com>
>