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

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

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."
> + */
> +#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.

> +       { 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"
> +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.
> +static const struct __guc_mmio_reg_descr xe_hpg_rc_class_regs[] = {
> +       COMMON_XELP_RC_CLASS,
> +};
alan:snip