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: 2413 bytes --]

On 4/26/2023 10:29, John Harrison wrote:
> 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.
Although looking at the lists, there is nothing gen9 specific anywhere. 
So gen8_ would be the more accurate name.

John.

>
>>>   
>

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