Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/msm: memory leak of queue in msm_submitqueue_create() when ring is NULL - [High] drm/msm: broken GPU hang recovery and hangcheck for LPAC ring - [Medium] drm/msm/a6xx: mismatched always-on counter for LPAC in a7xx_submit() - [Low] drm/msm/a6xx: unused variable timestamp_iova in a7xx_submit() --
commit b513ca2d3a9d2c681ec6e6cf4dcbe0f4500714aa Author: Anna Maniscalco <[email protected]> drm/msm: Add LPAC submitqueue This commit adds the MSM_SUBMITQUEUE_LPAC UAPI flag and driver support for creating LPAC submitqueues and dispatching submissions to the LPAC ringbuffer. > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index e7bc12de650ca..e82d0c0bc10ff 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c [ ... ] > @@ -493,7 +498,10 @@ static void a7xx_submit(struct msm_gpu *gpu, struct > msm_gem_submit *submit) > struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); > struct msm_ringbuffer *ring = submit->ring; > - u32 rbbm_perfctr_cp0, cp_always_on_context; > + u32 rbbm_perfctr_cp0, cp_always_on_context, > + cp_always_on_counter; > + bool is_lpac = ring == gpu->lpac_rb; > + u64 timestamp_iova; [Severity: Low] Is timestamp_iova needed in a7xx_submit()? It appears to be unused, which triggers an unused-variable warning at build time (and breaks the build when CONFIG_WERROR is enabled). > unsigned int i, ibs = 0; > > adreno_check_and_reenable_stall(adreno_gpu); > @@ -511,19 +519,24 @@ static void a7xx_submit(struct msm_gpu *gpu, struct > msm_gem_submit *submit) > * If preemption is enabled, then set the pseudo register for the save > * sequence > */ > - if (gpu->nr_rings > 1) > + if (gpu->nr_rings > 1 && !is_lpac) > a6xx_emit_set_pseudo_reg(ring, a6xx_gpu, submit->queue); > > if (adreno_is_a8xx(adreno_gpu)) { > rbbm_perfctr_cp0 = REG_A8XX_RBBM_PERFCTR_CP(0); > cp_always_on_context = REG_A8XX_CP_ALWAYS_ON_CONTEXT; > + cp_always_on_counter = REG_A8XX_CP_ALWAYS_ON_COUNTER; > } else { > rbbm_perfctr_cp0 = REG_A7XX_RBBM_PERFCTR_CP(0); > cp_always_on_context = REG_A6XX_CP_ALWAYS_ON_CONTEXT; > + cp_always_on_counter = REG_A6XX_CP_ALWAYS_ON_COUNTER; > } > > get_stats_counter(ring, rbbm_perfctr_cp0, rbmemptr_stats(ring, index, > cpcycles_start)); > - get_stats_counter(ring, cp_always_on_context, rbmemptr_stats(ring, > index, alwayson_start)); > + if (is_lpac) > + get_stats_counter(ring, cp_always_on_counter, > rbmemptr_stats(ring, index, alwayson_start)); > + else > + get_stats_counter(ring, cp_always_on_context, > rbmemptr_stats(ring, index, alwayson_start)); [Severity: Medium] Should the corresponding alwayson_end sample in a7xx_submit() also use cp_always_on_counter when is_lpac is true? After emitting the IB commands, a7xx_submit() still unconditionally reads cp_always_on_context: get_stats_counter(ring, rbbm_perfctr_cp0, rbmemptr_stats(ring, index, cpcycles_end)); get_stats_counter(ring, cp_always_on_context, rbmemptr_stats(ring, index, alwayson_end)); When retire_submit() later computes: elapsed = (stats->alwayson_end - stats->alwayson_start) * 10000; for an LPAC submission, it will subtract CP_ALWAYS_ON_COUNTER from CP_ALWAYS_ON_CONTEXT, corrupting the per-submit elapsed time, ctx->elapsed_ns, and frequency statistics. [ ... ] > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > index aa3d6ff456fda..4244d9324cda7 100644 > --- a/drivers/gpu/drm/msm/msm_gpu.c > +++ b/drivers/gpu/drm/msm/msm_gpu.c > @@ -537,8 +537,8 @@ static void recover_worker(struct kthread_work *work) > * needs to happen after msm_rd_dump_submit() to ensure that the > * bo's referenced by the offending submit are still around. > */ > - for (i = 0; i < gpu->nr_rings; i++) { > - struct msm_ringbuffer *ring = gpu->rb[i]; > + for (i = 0; i < gpu->nr_rings + !!gpu->lpac_rb; i++) { > + struct msm_ringbuffer *ring = i < gpu->nr_rings ? gpu->rb[i] : > gpu->lpac_rb; > > uint32_t fence = ring->memptrs->fence; > > @@ -561,8 +561,8 @@ static void recover_worker(struct kthread_work *work) > * Replay all remaining submits starting with highest priority > * ring > */ > - for (i = 0; i < gpu->nr_rings; i++) { > - struct msm_ringbuffer *ring = gpu->rb[i]; > + for (i = 0; i < gpu->nr_rings + !!gpu->lpac_rb; i++) { > + struct msm_ringbuffer *ring = i < gpu->nr_rings ? gpu->rb[i] : > gpu->lpac_rb; > unsigned long flags; > > spin_lock_irqsave(&ring->submit_lock, flags); [Severity: High] How does recover_worker() handle a hang or fault originating on gpu->lpac_rb? At the start of recover_worker(), cur_ring is looked up via gpu->funcs->active_ring(gpu) (a6xx_active_ring()), which returns a6xx_gpu->cur_ring: struct msm_ringbuffer *cur_ring = gpu->funcs->active_ring(gpu); ... submit = find_submit(cur_ring, cur_ring->memptrs->fence + 1); if (!submit) goto out_unlock; Because a6xx_gpu->cur_ring only tracks the 3D rings (gpu->rb[]) and is never set to gpu->lpac_rb, two things can go wrong when an LPAC submission triggers a6xx_fault_detect_irq() or a7xx_sw_fuse_violation_irq(): 1. If cur_ring is idle, find_submit(cur_ring, ...) returns NULL and recover_worker() bails out via goto out_unlock without running gpu->funcs->recover(gpu), after a6xx_fault_detect_irq() has already written 0 to REG_A6XX_RBBM_INT_0_MASK, leaving GPU interrupts disabled. 2. If cur_ring has an unrelated 3D submit in flight, recover_worker() attributes the fault to that 3D submit, marks its VM unusable, and in the fence update loop: if (ring == cur_ring) ring->memptrs->fence = ++fence; advances cur_ring->memptrs->fence while leaving gpu->lpac_rb->memptrs->fence unadvanced, so the replay loop re-submits the hung LPAC job. Similarly, hangcheck_handler() only checks gpu->funcs->active_ring(gpu), and in a6xx_irq(): if (priv->disable_err_irq) status &= A6XX_RBBM_INT_0_MASK_CP_CACHE_FLUSH_TS; A6XX_RBBM_INT_0_MASK_CP_CACHE_FLUSH_TS_LPAC is masked out when disable_err_irq is set, dropping LPAC completion interrupts. > diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c > b/drivers/gpu/drm/msm/msm_submitqueue.c > index 1a5a77b280163..a0249430bd005 100644 > --- a/drivers/gpu/drm/msm/msm_submitqueue.c > +++ b/drivers/gpu/drm/msm/msm_submitqueue.c [ ... ] > @@ -209,6 +210,16 @@ int msm_submitqueue_create(struct drm_device *drm, > struct msm_context *ctx, > return ret; > > queue = kzalloc_obj(*queue); > + > + if (flags & MSM_SUBMITQUEUE_LPAC) { > + ring_nr = priv->gpu->nr_rings; > + ring = priv->gpu->lpac_rb; > + } else { > + ring = priv->gpu->rb[ring_nr]; > + } > + > + if (!ring) > + return -EINVAL; > } [Severity: High] Does msm_submitqueue_create() leak queue when ring is NULL? In the non-MSM_SUBMITQUEUE_VM_BIND path (reachable from userspace via DRM_IOCTL_MSM_SUBMITQUEUE_NEW -> msm_ioctl_submitqueue_new()), queue is allocated with kzalloc_obj(*queue) before checking whether ring is non-NULL. If userspace passes MSM_SUBMITQUEUE_LPAC on a GPU where priv->gpu->lpac_rb is NULL, returning -EINVAL here leaks queue (and if kzalloc_obj() also returned NULL, checking !ring before !queue returns -EINVAL instead of -ENOMEM). Should the ring lookup and !ring check happen before allocating queue? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261029-descriptive-name-lpac-upstream-v2-0-a21a5c850...@gmail.com?part=10
