Re: [Intel-gfx] [PATCH 4/5] drm/i915/guc: Capture list clean up - 3

[John Harrison <john.c.harrison@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2182 bytes --]

On 4/25/2023 12:05, Teres Alexis, Alan Previn wrote:
> On Thu, 2023-04-06 at 15:26 -0700,John.C.Harrison@Intel.com  wrote:
>> From: John Harrison<John.C.Harrison@Intel.com>
>>
>> Fix Xe_LP name.
>>
>> Signed-off-by: John Harrison<John.C.Harrison@Intel.com>
> alan:snip
>
>
>> -/* GEN9/XE_LPD - Render / Compute Per-Engine-Instance */
>> +/* GEN8+ Render / Compute Per-Engine-Instance */
> alan: two comments on this:
> 1. shouldnt this go with the earlier patch?
See comment in cover letter:

    NB: The changes are being sent as multiple patches to make code review
    simpler. However, before merging it may be better to squash into a
    single patch, especially if it going to be sent with a 'fixes' tag.



> 2. i agree with renaming the names of the register to reflect the when
>     all of those registers got first introduced... however, considering
>     we only support GuC on Gen12 and beyond (we do have select CI-tests
>     that enable GuC on Gen9 but not on Gen8 and before), should we also
>     change the comments? I think the comment should reflect the usage
>     not just follow the same name of the registe #define - else why even
>     add the comments. (please apply this same comment for gen8_vd_inst_regs,
>     gen8_vec_inst_regs and gen8_blt_inst_regs).
> alternatively, we could keep those GEN8+ comments above the list but maybe
> add just one comment in the default list - see below.
Because at some point we might want to start supporting other platforms. 
My view is that the comment should be accurate. These registers exist on 
Gen8+. So if you are building a register list for a Gen8 or later 
device, they can/should be included.

> alan: snip
>
>> @@ -366,7 +364,7 @@ guc_capture_get_device_reglist(struct intel_guc *guc)
>>   	const struct __guc_mmio_reg_descr_group *lists;
>>   
>>   	if (GRAPHICS_VER(i915) >= 12)
>> -		lists = xe_lpd_lists;
>> +		lists = xe_lp_lists;
>>   	else
>>   		lists = default_lists;
> alan: perhaps add a comment that we really don't support any of this
> on anything below Gen9?
It wasn't me that named it 'default_' rather than gen9_. I could add yet 
another rename of s/default_/gen9_/g...

John.

>
>>   

[-- Attachment #2: Type: text/html, Size: 3873 bytes --]