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 <[email protected]>
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-01d50c3e0...@gmail.com?part=9
