Re: [PATCH v16 1/7] drm/xe/guc: Prepare GuC register list and update ADS size for error capture

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

On 2024-08-27 4:30 p.m., Teres Alexis, Alan Previn wrote:
> So i only have a few nits since the difference between v15 vs v16 of Patch#1
> is limited to the inclusion of the additional variables in the register list
> (to aid with for print-formatting of output), as well as the separation of
> list for lpg vs hpg.
> 
> However, lets ensure we circle back with confirmation that the register
> list ordering is not impacting the debug tool. For now I'm providing the
> conditional-rb pending that ordering - just need a reply on this.
> thanks again for the perseverence.
> 
> 
> Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> 
> 
> On Mon, 2024-08-19 at 19:11 -0700, Zhanjun Dong wrote:
>> Add referenced registers defines and list of registers.
>> Update GuC ADS size allocation to include space for
>> the lists of error state capture register descriptors.
>>
>>
> alan:snip
> 
>> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
>> new file mode 100644
>> index 000000000000..0e75d1553730
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
>> @@ -0,0 +1,508 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>>
> alan:snip
>> +
>> +/*
>> + * Define all device tables of GuC error capture register lists
>> + * NOTE:
>> + *     For engine-registers, GuC only needs the register offsets
>> + *     from the engine-mmio-base
>> + *
>> + *     64 bit registers need 2 entries for low 32 bit register and high 32 bit
>> + *     register, for example:
>> + *       Register           data_type       flags   mask    Register name
>> + *     { XXX_REG_LO(0),  REG_64BIT_LOW_DW,    0,      0,      NULL},
>> + *     { XXX_REG_HI(0),  REG_64BIT_HI_DW,,    0,      0,      "XXX_REG"},
>> + *     1. data_type: Indicate is hi/low 32 bit for a 64 bit register
>> + *     2. Register name: null for incompleted define
>> + *     A 64 bit register define requires 2 consecutive entries.
> alan:nit: this sentence above should be combined as part of "1."
Sure, will do
>> + */
>> +#define COMMON_XELP_BASE_GLOBAL \
>> +       { FORCEWAKE_GT,                 REG_32BIT,      0,      0,      "FORCEWAKE_GT"}
>> +
>> +#define COMMON_BASE_ENGINE_INSTANCE \
> alan: please ensure you have 3d-umd folks confirm that the change in
> ordering of the register printout is not an issue - since we are now
> using the same list for both exec-list and guc... and the ordering is
> will be aligned for obht the guc-ads-reg-list registration as well as
> the dev-core-dump-register-dump printout (which is what we prefer to
> ensure no dulicated lists for exec-list vs guc). If you already confirmed
> with them offline, please reply on this. I would expect UMD's scripts
> are flexible since they have to handle different gen products with slightly
> different lists - but best to be thorough. Thanks in advance.
Sure, I will contact UMD guys and get back to you once I got reply.

> 
>> +       { RING_HWSTAM(0),               REG_32BIT,      0,      0,      "HWSTAM"}, \
>> +       { RING_HWS_PGA(0),              REG_32BIT,      0,      0,      "RING_HWS_PGA"}, \
>> +       { RING_HEAD(0),                 REG_32BIT,      0,      0,      "RING_HEAD"}, \
>> +       { RING_TAIL(0),                 REG_32BIT,      0,      0,      "RING_TAIL"}, \
>> +       { RING_CTL(0),                  REG_32BIT,      0,      0,      "RING_CTL"}, \
>> +       { RING_MI_MODE(0),              REG_32BIT,      0,      0,      "RING_MI_MODE"}, \
>> +       { RING_MODE(0),                 REG_32BIT,      0,      0,      "RING_MODE"}, \
>> +       { RING_ESR(0),                  REG_32BIT,      0,      0,      "RING_ESR"}, \
>> +       { RING_EMR(0),                  REG_32BIT,      0,      0,      "RING_EMR"}, \
>> +       { RING_EIR(0),                  REG_32BIT,      0,      0,      "RING_EIR"}, \
>> +       { RING_IMR(0),                  REG_32BIT,      0,      0,      "RING_IMR"}, \
>> +       { RING_IPEHR(0),                REG_32BIT,      0,      0,      "IPEHR"}, \
> alan:snip
> 
>> +/* Render / Compute Per-Engine-Instance */
> alan:nit: if i understand correctly, this comment should say "Engine-Class", not "Per-Engine-Instance"
Thanks, to be corrected in next rev.
>> +static const struct __guc_mmio_reg_descr xe_rc_class_regs[] = {
>> +       COMMON_XELP_RC_CLASS,
>> +       COMMON_XELP_RC_CLASS_INSTDONE,
>> +};
>> +
>> +/* Render / Compute Per-Engine-Instance for xehpg */
> alan:nit: same as last, comment should say this is for class, not per-instance.
Thanks, to be corrected in next rev.
>> +static const struct __guc_mmio_reg_descr xe_hpg_rc_class_regs[] = {
>> +       COMMON_XELP_RC_CLASS,
>> +};
> alan:snip
> 
> 
>