Re: [PATCH RFC 09/13] temp: add LPAC regs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing 'variants' attribute on CP_LPAC_PROTECT_CNTL makes an A7XX-only LPAC register available to older hardware.
- [High] UCHE_DEBUG_CNTL_1 overlaps with existing UCHE_UNKNOWN_0E12 and UCHE_GBIF_GX_CONFIG registers at offset 0x0E12.
- [High] CP_LPAC_RB_* registers are applied to A8XX where they conflict with CP_SLICE_MEM_POOL_DBG_*_PIPE registers.
- [Medium] The patch is explicitly marked as a temporary commit ('temp:') and directly modifies a file that should be synchronized from Mesa.
- [Low] Registers are inserted significantly out of order regarding their offsets.
- [Low] Redundant inline bitfield definitions instead of reusing the existing bitset type.
--

commit 6a32a45a3775682b98c44924ebd725197283df72
Author: Anna Maniscalco <anna.maniscalco2000@gmail.com>

temp: add LPAC regs

This patch adds LPAC register definitions to the adreno a6xx XML database.
It notes that the definitions are pulled from a Mesa merge request.

Link: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/39416

> temp: add LPAC regs
> 
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/39416
> 
> should we merged and we should sync to mesa instead

[Severity: Medium]
This isn't a bug, but since this patch is explicitly marked as a temporary
commit ("temp:") and notes that we should sync from Mesa instead, should
these register definitions be finalized and synchronized through the Mesa
rnndb workflow before merging upstream to avoid divergence?

> diff --git a/drivers/gpu/drm/msm/registers/adreno/a6xx.xml b/drivers/gpu/drm/msm/registers/adreno/a6xx.xml
> index 3349c01646e12..bb49f6a61a9af 100644
> --- a/drivers/gpu/drm/msm/registers/adreno/a6xx.xml
> +++ b/drivers/gpu/drm/msm/registers/adreno/a6xx.xml
> @@ -276,6 +276,11 @@ by a particular renderpass/blit.
>  		<bitfield pos="1" name="ACCESS_FAULT_ON_VIOL_EN" type="boolean"/>
>  		<bitfield pos="0" name="ACCESS_PROT_EN" type="boolean"/>
>  	</bitset>
> +	<reg32 offset="0x0B09" name="CP_LPAC_PROTECT_CNTL">
> +		<bitfield pos="3" name="LAST_SPAN_INF_RANGE" type="boolean"/>
> +		<bitfield pos="1" name="ACCESS_FAULT_ON_VIOL_EN" type="boolean"/>
> +		<bitfield pos="0" name="ACCESS_PROT_EN" type="boolean"/>
> +	</reg32>
>  
>  	<reg32 offset="0x084f" name="CP_PROTECT_CNTL" type="a6xx_cp_protect_cntl" variants="A6XX-A7XX"/>

[Severity: High]
Does CP_LPAC_PROTECT_CNTL need a variants="A7XX-" attribute here? By omitting
the variants attribute, this A7XX-only LPAC register will be generated and
applied for A6XX hardware, which could write to an undefined register at
0x0B09 during early hardware initialization and cause an access fault.

[Severity: Low]
Instead of manually defining these bitfields inline, could this register
reuse the existing a6xx_cp_protect_cntl bitset by specifying
type="a6xx_cp_protect_cntl" to prevent duplication?

[Severity: Low]
Also, this register at offset 0x0B09 is inserted before 0x084f. Should these
be kept ordered by offset to prevent overlapping definitions and XML parser
warnings?

[ ... ]

> @@ -333,6 +338,11 @@ by a particular renderpass/blit.
>  	<reg32 offset="0x0845" name="CP_CRASH_DUMP_STATUS" variants="A8XX-"/>
>  	<reg32 offset="0x0908" name="CP_SQE_STAT_ADDR" variants="A6XX-A7XX"/>
>  	<reg32 offset="0x0909" name="CP_SQE_STAT_DATA" variants="A6XX-A7XX"/>
> +	<reg64 offset="0x0b00" name="CP_LPAC_RB_BASE" variants="A7XX-"/>
> +	<reg32 offset="0x0b02" name="CP_LPAC_RB_CNTL" variants="A7XX-"/>
> +	<reg64 offset="0x0b04" name="CP_LPAC_RB_RPTR_ADDR" variants="A7XX-"/>
> +	<reg32 offset="0x0b06" name="CP_LPAC_RB_RPTR" variants="A7XX-"/>
> +	<reg32 offset="0x0b07" name="CP_LPAC_RB_WPTR" variants="A7XX-"/>
>  	<reg32 offset="0x090a" name="CP_DRAW_STATE_ADDR" variants="A6XX-A7XX"/>
>  	<reg32 offset="0x090b" name="CP_DRAW_STATE_DATA" variants="A6XX-A7XX"/>

[Severity: High]
Do these offsets conflict with existing A8XX definitions? Using
variants="A7XX-" applies them to A8XX, but A8XX already defines
CP_SLICE_MEM_POOL_DBG_ADDR_PIPE at 0x0b00, and the A8XX LPAC ring buffer
base is generally at 0x080b. Should this be restricted to variants="A7XX"
to avoid an XML offset collision during the build?

[Severity: Low]
Similar to the earlier register, the 0x0b00-0x0b07 block is placed between
0x0909 and 0x090a. Would it be better to place it further down to keep the
XML offsets ordered?

[ ... ]

> @@ -1207,6 +1218,7 @@ by a particular renderpass/blit.
>  	<reg64 offset="0x0E08" name="UCHE_TRAP_BASE" variants="A8XX-"/>
>  	<reg64 offset="0x0E0B" name="UCHE_GMEM_RANGE_MIN" variants="A6XX-A7XX"/>
>  	<reg64 offset="0x0E0D" name="UCHE_GMEM_RANGE_MAX" variants="A6XX-A7XX"/>
> +	<reg32 offset="0x0E12" name="UCHE_DEBUG_CNTL_1" usage="cmd"/>
>  	<reg32 offset="0x0e17" name="UCHE_CACHE_WAYS" variants="A6XX-A7XX" usage="init"/>
>  	<reg32 offset="0x0e04" name="UCHE_CACHE_WAYS" variants="A8XX-"/>

[Severity: High]
Will adding UCHE_DEBUG_CNTL_1 at offset 0x0E12 without a variants attribute
cause a build failure? 0x0e12 is already defined as UCHE_UNKNOWN_0E12 for
A6XX-A7XX and as UCHE_GBIF_GX_CONFIG for A8XX. Does this new definition need
a variant constraint, or should the old conflicting registers be removed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-descriptive-name-lpac-upstream-v1-0-01d50c3e0c99@gmail.com?part=9