Re: [PATCH 5/6] drm/xe/guc: add TDL gfx registers to capture list

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Typographical error in register name 'ROW_INSDONE_TDL_GFX' (missing 'T' in 'INSTDONE').
- [Low] Improper indentation using spaces instead of tabs.
--

--- Patch [5]: [PATCH 5/6] drm/xe/guc: add TDL gfx registers to capture list ---
commit 3c0be61d6ad550502777320cdeb9da502f084893
Author: Nareshkumar Gollakoti <[email protected]>

drm/xe/guc: add TDL gfx registers to capture list

Add ROW_INSDONE_TDL_GFX and SS_LSC_HDC_IDLE_TDL_GFX register
definitions and include them in the XeHPG extended GuC capture register
list.

> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index 25014bccd3a69..8c55d766a5a63 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -572,6 +572,9 @@
>  #define EU_SYSTOLIC_LIC_THROTTLE_CTL_WITH_LOCK	XE_REG_MCR(0xe534)
>  #define   EU_SYSTOLIC_LIC_THROTTLE_CTL_LOCK_BIT	REG_BIT(31)
>  
> +#define ROW_INSDONE_TDL_GFX                    XE_REG_MCR(0xe580)
          ^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
This isn't a bug, but there seems to be a typo in the register name. 
Should this be ROW_INSTDONE_TDL_GFX (missing 'T' in INSTDONE)? If userspace
telemetry and GPU hang analysis tools rely on substring matching for
'INSTDONE', they will silently miss this register.

> +#define SS_LSC_HDC_IDLE_TDL_GFX                        XE_REG_MCR(0xe584)

[Severity: Low]
This isn't a bug, but it looks like these definitions use spaces instead of
tabs for alignment, unlike the surrounding macros.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index 368769857c9b8..a6052da717878 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
> @@ -376,7 +376,9 @@ static const struct __ext_steer_reg xehpg_extregs[] = {
>  	{"SC_INSTDONE",			XEHPG_SC_INSTDONE},
>  	{"SC_INSTDONE_EXTRA",		XEHPG_SC_INSTDONE_EXTRA},
>  	{"SC_INSTDONE_EXTRA2",		XEHPG_SC_INSTDONE_EXTRA2},
> -	{"INSTDONE_GEOM_SVGUNIT",	XEHPG_INSTDONE_GEOM_SVGUNIT}
> +	{"INSTDONE_GEOM_SVGUNIT",	XEHPG_INSTDONE_GEOM_SVGUNIT},
> +	{"ROW_INSDONE_TDL_GFX",		ROW_INSDONE_TDL_GFX},
         ^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Since this string name is exported to userspace and GuC capture tools, will 
the misspelled "INSDONE" break tooling expectations that look for "INSTDONE"?

> +	{"SS_LSC_HDC_IDLE_TDL_GFX",	SS_LSC_HDC_IDLE_TDL_GFX}
>  };
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5