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

[John Harrison <john.c.harrison@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.