Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] Unconditional execution of LPAC initialization on all A6xx/A7xx GPUs will crash hardware without LPAC support. - [High] The error check for the LPAC ringbuffer allocation evaluates the wrong variable, ignoring allocation failures and reading out-of-bounds. - [Medium] The GPU hangcheck timer does not monitor the LPAC ring, causing LPAC-only hangs to go undetected. - [Medium] Missing cleanup of `gpu->lpac_rb` in the error path of `msm_gpu_init()`. - [Low] Unconditional allocation of the LPAC ringbuffer wastes resources for all MSM GPUs. --
commit 35c6bd5d02bb9ed8858bdd2395719b06651d77e8 Author: Anna Maniscalco <[email protected]> drm/msm: initialize LPAC ring Program all the register needed to start up LPAC and send initialization sequence. Additionally, now that the LPAC ring is live, make the code aware of it. > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c [ ... ] > @@ -1481,6 +1567,10 @@ static int hw_init(struct msm_gpu *gpu) > if (ret) > goto out; > > + ret = lpac_cp_init(gpu); > + if (ret) > + goto out; > + [Severity: Critical] Will this unconditional execution of LPAC initialization cause a regression on hardware without LPAC support? During hw_init(), the driver unconditionally writes to LPAC-specific registers and calls lpac_cp_init(). This function sends a CP_ME_INIT packet to the LPAC ring and waits for it to idle using a6xx_idle(). On older GPUs lacking LPAC hardware (e.g., A630), the CP engine ignores this ring, causing the idle wait to time out and hw_init() to return -EINVAL. > /* > * Try to load a zap shader into the secure world. If successful [ ... ] > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > --- a/drivers/gpu/drm/msm/msm_gpu.c > +++ b/drivers/gpu/drm/msm/msm_gpu.c [ ... ] > @@ -1006,6 +1009,15 @@ int msm_gpu_init(struct drm_device *drm, struct > platform_device *pdev, > memptrs_iova += sizeof(struct msm_rbmemptrs); > } > > + gpu->lpac_rb = msm_ringbuffer_new(gpu, nr_rings, memptrs, memptrs_iova); > + [Severity: Low] Does this unconditional allocation of the LPAC ringbuffer waste resources for legacy non-LPAC GPUs? A GEM object, DRM scheduler kthread, and fence context are allocated for every MSM GPU even if they do not support LPAC. > + if (IS_ERR(gpu->rb[i])) { > + ret = PTR_ERR(gpu->rb[i]); > + DRM_DEV_ERROR(drm->dev, > + "could not create lpac ringbuffer > %d\n", ret); > + goto fail; > + } > + [Severity: High] Does this check the wrong variable for the LPAC allocation failure? The error check evaluates gpu->rb[i] instead of gpu->lpac_rb. If the LPAC allocation fails, gpu->lpac_rb holds an error pointer but the initialization succeeds. This later causes a kernel panic when the error pointer is dereferenced during device setup. Additionally, because the loop counter i equals nr_rings here, checking gpu->rb[i] results in an out-of-bounds array read if nr_rings equals MSM_GPU_MAX_RINGS. [Severity: Medium] If GPU initialization fails and we take this goto fail path, is there a memory leak for the LPAC ringbuffer? The fail path in msm_gpu_init() iterates over gpu->rb and destroys those ringbuffers, but it lacks a call to msm_ringbuffer_destroy(gpu->lpac_rb). > gpu->nr_rings = nr_rings; > > refcount_set(&gpu->sysprof_active, 1); [ ... ] > diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h > --- a/drivers/gpu/drm/msm/msm_gpu.h > +++ b/drivers/gpu/drm/msm/msm_gpu.h [ ... ] > @@ -181,6 +181,8 @@ struct msm_gpu { > struct msm_ringbuffer *rb[MSM_GPU_MAX_RINGS]; > int nr_rings; > > + struct msm_ringbuffer *lpac_rb; > + [Severity: Medium] Is the GPU hangcheck timer missing monitoring for the LPAC ring? In msm_gpu.c:hangcheck_handler(), the code only monitors the primary active ring: struct msm_ringbuffer *ring = gpu->funcs->active_ring(gpu); ... if (fence_after(ring->fctx->last_fence, ring->hangcheck_fence)) hangcheck_timer_reset(gpu); If a task submitted to the LPAC ring hangs and the primary ring is idle, fence_after() evaluates to false and the timer is not reset. The stall goes undetected, leaving the task hanging indefinitely without GPU recovery. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260705-descriptive-name-lpac-upstream-v1-0-01d50c3e0...@gmail.com?part=10
