Re: [PATCH RFC v2 10/14] drm/msm: Add LPAC submitqueue

[sashiko-bot@kernel.org]

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 <anna.maniscalco2000@gmail.com>

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-a21a5c850961@gmail.com?part=10