On Fri, Mar 14, 2025 at 2:35 PM Antonino Maniscalco <[email protected]> wrote: > > On 3/14/25 10:08 PM, Rob Clark wrote: > > On Fri, Mar 14, 2025 at 1:07 PM Akhil P Oommen <[email protected]> > > wrote: > >> > >> On 3/15/2025 12:04 AM, Rob Clark wrote: > >>> From: Rob Clark <[email protected]> > >>> > >>> IB_SIZE is only b0..b19. Starting with a6xx gen3, additional fields > >>> were added above the IB_SIZE. Accidentially setting them can cause > >>> badness. Fix this by properly defining the CP_INDIRECT_BUFFER packet > >>> and using the generated builder macro to ensure unintended bits are not > >>> set. > > I wonder if we should be returning -EINVAL from the ioctl when a size > larger than max is submitted. I say this because we do a 0 check when > submitting which this bug allows to bypass therefore putting a 0 sized > CP_INDIRECT_BUFFER pkt in the ring. > Fw is inconsistent as to how this is treated (should be a NOP but will > sometimes hang) and that is very confusing.
tbh, I'm not sure I remember why we check for zero.. at least sqe fw explicitly handles this case on newer devices. Maybe it made older devices grumpy? But I wanted to avoid hard-coding (potentially) device specific limits in the frontend. And decided that userspace is allowed to shoot it's own foot if it really wants to. BR, -R > >>> > >>> v2: add missing type attribute for IB_BASE > >>> > >>> Reported-by: Connor Abbott <[email protected]> > >>> Fixes: a83366ef19ea ("drm/msm/a6xx: add A640/A650 to gpulist") > >>> Signed-off-by: Rob Clark <[email protected]> > >>> --- > >>> Backport notes, prior to commit ae22a94997b8 ("drm/msm: import A2xx-A4xx > >>> XML display registers database"), just open code, ie: > >>> > >>> OUT_RING(ring, submit->cmd[i].size & 0xfffff); > >>> > >>> Prior to commit af66706accdf ("drm/msm/a6xx: Add skeleton A7xx > >>> support"), a7xx_submit() did not exist so that hunk can be dropped. > >>> > >>> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 8 ++++---- > >>> drivers/gpu/drm/msm/registers/adreno/adreno_pm4.xml | 7 +++++++ > >>> 2 files changed, 11 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >>> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >>> index d3978cfa3f20..ea52b7d0b212 100644 > >>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >>> @@ -245,10 +245,10 @@ static void a6xx_submit(struct msm_gpu *gpu, struct > >>> msm_gem_submit *submit) > >>> break; > >>> fallthrough; > >>> case MSM_SUBMIT_CMD_BUF: > >>> - OUT_PKT7(ring, CP_INDIRECT_BUFFER_PFE, 3); > >>> + OUT_PKT7(ring, CP_INDIRECT_BUFFER, 3); > >>> OUT_RING(ring, lower_32_bits(submit->cmd[i].iova)); > >>> OUT_RING(ring, upper_32_bits(submit->cmd[i].iova)); > >>> - OUT_RING(ring, submit->cmd[i].size); > >>> + OUT_RING(ring, > >>> A5XX_CP_INDIRECT_BUFFER_3_IB_SIZE(submit->cmd[i].size)); > >>> ibs++; > >>> break; > >>> } > >>> @@ -382,10 +382,10 @@ static void a7xx_submit(struct msm_gpu *gpu, struct > >>> msm_gem_submit *submit) > >>> break; > >>> fallthrough; > >>> case MSM_SUBMIT_CMD_BUF: > >>> - OUT_PKT7(ring, CP_INDIRECT_BUFFER_PFE, 3); > >>> + OUT_PKT7(ring, CP_INDIRECT_BUFFER, 3); > >>> OUT_RING(ring, lower_32_bits(submit->cmd[i].iova)); > >>> OUT_RING(ring, upper_32_bits(submit->cmd[i].iova)); > >>> - OUT_RING(ring, submit->cmd[i].size); > >>> + OUT_RING(ring, > >>> A5XX_CP_INDIRECT_BUFFER_3_IB_SIZE(submit->cmd[i].size)); > >>> ibs++; > >>> break; > >>> } > >>> diff --git a/drivers/gpu/drm/msm/registers/adreno/adreno_pm4.xml > >>> b/drivers/gpu/drm/msm/registers/adreno/adreno_pm4.xml > >>> index 55a35182858c..a71bc6f16cbf 100644 > >>> --- a/drivers/gpu/drm/msm/registers/adreno/adreno_pm4.xml > >>> +++ b/drivers/gpu/drm/msm/registers/adreno/adreno_pm4.xml > >>> @@ -2259,5 +2259,12 @@ opcode: CP_LOAD_STATE4 (30) (4 dwords) > >>> </reg32> > >>> </domain> > >>> > >>> +<domain name="CP_INDIRECT_BUFFER" width="32" varset="chip" prefix="chip" > >>> variants="A5XX-"> > >>> + <reg64 offset="0" name="IB_BASE" type="address"/> > >>> + <reg32 offset="3" name="3"> > >> > >> Why is the offset 3 here? It looks to me that it doesn't match the code > >> above. > > > > oh, bad copy/pasta.. it should be 2 (dword offset) > > > > BR, > > -R > > > >> -Akhil. > >> > >>> + <bitfield name="IB_SIZE" low="0" high="19"/> > >>> + </reg32> > >>> +</domain> > >>> + > >>> </database> > >>> > >> > > Best regards, > -- > Antonino Maniscalco <[email protected]>
