Re: [PATCH v3 7/8] drm/vc4: Switch to DRM GPU scheduler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Missing NULL check for `bin_job` and `render_job` in IRQ handler causes kernel panic on spurious interrupts.
- [High] `vc4_v3d_get_bin_slot` prematurely returns `-ENOMEM` when bin slots are exhausted but the hardware is momentarily idle.
- [High] Unbalanced `vc4_bo_dec_usecnt()` in error path during BO lookup causes usecnt underflow.
- [Medium] `vc4_save_hang_state()` fails to capture BOs for the active bin job, capturing only the render job's BOs.
- [Medium] Unconditional `vc4_perfmon_stop` in IRQ handlers breaks performance monitoring for pipelined or concurrent jobs.

Pre-existing issues:
- [High] An off-by-one error in shader record validation allows userspace to trigger an out-of-bounds read past the end of the `render->bo` array.
--

--- Patch [7]: [PATCH v3 7/8] drm/vc4: Switch to DRM GPU scheduler ---
commit 873ba817e912575f3a2ceffbc30604b80b1e73ac
Author: Maíra Canal <mcanal@igalia.com>

drm/vc4: Switch to DRM GPU scheduler

Wire up the DRM GPU scheduler to the VC4 submission path, replacing the
legacy hand-rolled job queue implementation. The scheduler now handles
job ordering, dependency tracking, and timeout detection.

> diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
> --- a/drivers/gpu/drm/vc4/vc4_irq.c
> +++ b/drivers/gpu/drm/vc4/vc4_irq.c
> @@ -225,16 +136,42 @@ vc4_irq(int irq, void *arg)
>  	if (intctl & V3D_INT_FLDONE) {
> +		struct vc4_bin_job *bin_job;
> +		struct vc4_fence *fence;
> +
>  		spin_lock(&vc4->job_lock);
> -		vc4_irq_finish_bin_job(dev);
> +		bin_job = vc4->bin_job;
> +		vc4->bin_job = NULL;
>  		spin_unlock(&vc4->job_lock);
> +
> +		fence = to_vc4_fence(bin_job->base.irq_fence);

[Severity: High]
Could this cause a NULL pointer dereference on spurious interrupts?

If the hardware generates a spurious V3D_INT_FLDONE interrupt, or if the
interrupt fires after the job state has been reset (e.g., due to a timeout
or GPU reset), vc4->bin_job will be NULL. Unconditionally dereferencing
bin_job->base.irq_fence here would lead to an immediate kernel panic in the
interrupt handler.

> +
> +		vc4_perfmon_stop(vc4, bin_job->base.perfmon, true);

[Severity: Medium]
Is it safe to unconditionally stop the perfmon here?

Under the legacy implementation, the driver left the perfmon running across
the gap between a job's bin and render phases, only stopping it if the next
job didn't use the same perfmon. By calling vc4_perfmon_stop() at the end
of every bin job, the hardware counters might be prematurely disabled while
the corresponding render job is still actively executing, which would corrupt
the gathered statistics.

> +
> +		trace_vc4_bcl_end_irq(dev, fence->seqno);
> +
> +		dma_fence_signal(&fence->base);
> +
>  		status = IRQ_HANDLED;
>  	}
>  
>  	if (intctl & V3D_INT_FRDONE) {
> +		struct vc4_render_job *render_job;
> +		struct vc4_fence *fence;
> +
>  		spin_lock(&vc4->job_lock);
> -		vc4_irq_finish_render_job(dev);
> +		render_job = vc4->render_job;
> +		vc4->render_job = NULL;
>  		spin_unlock(&vc4->job_lock);
> +
> +		fence = to_vc4_fence(render_job->base.irq_fence);

[Severity: High]
Similar to the FLDONE case above, is it possible for render_job to be NULL
here if a spurious V3D_INT_FRDONE interrupt occurs, leading to a panic?

> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -194,21 +193,20 @@ int vc4_v3d_get_bin_slot(struct vc4_dev *vc4)
>  		return slot;
>  	}
>  
> -	/* Couldn't find an open slot.  Wait for render to complete
> +	/* Couldn't find an open slot. Wait for render to complete
>  	 * and try again.
>  	 */
> -	exec = vc4_last_render_job(vc4);
> -	if (exec)
> -		seqno = exec->seqno;
> +	fence = NULL;
> +	if (vc4->render_job)
> +		fence = dma_fence_get(vc4->render_job->base.done_fence);
>  	spin_unlock_irqrestore(&vc4->job_lock, irqflags);

[Severity: High]
Will this prematurely return -ENOMEM when the hardware is momentarily idle?

Bin slots are allocated during vc4_submit_cl_ioctl before jobs are queued
in the DRM scheduler. If the hardware is momentarily idle while jobs are
waiting in the DRM scheduler queue holding bin slots, vc4->render_job
will be NULL. This bypasses the wait and immediately returns -ENOMEM,
which could cause spurious ioctl failures under load.

> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> --- a/drivers/gpu/drm/vc4/vc4_gem.c
> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> @@ -168,21 +150,16 @@ vc4_save_hang_state(struct drm_device *dev)
>  	spin_lock_irqsave(&vc4->job_lock, irqflags);
> -	exec[0] = vc4_first_bin_job(vc4);
> -	exec[1] = vc4_first_render_job(vc4);
> -	if (!exec[0] && !exec[1])
> +	bin_job = vc4->bin_job;
> +	render_job = vc4->render_job;
> +	if (!bin_job && !render_job)
>  		goto err_free_state;
>  
> -	/* Get the bos from both binner and renderer into hang state. */
> +	/* Get the BOs from the render job into hang state. */
>  	state->bo_count = 0;
> -	for (i = 0; i < 2; i++) {
> -		if (!exec[i])
> -			continue;
> -
> -		unref_list_count = 0;
> -		list_for_each_entry(bo, &exec[i]->unref_list, unref_head)
> -			unref_list_count++;
> -		state->bo_count += exec[i]->bo_count + unref_list_count;
> +	if (render_job) {
> +		unref_list_count = list_count_nodes(&render_job->unref_list);
> +		state->bo_count += render_job->bo_count + unref_list_count;
>  	}

[Severity: Medium]
Does this unintentionally skip capturing BOs for the active bin job?

Because the new job structures moved BO tracking exclusively into
vc4_render_job, this only iterates over the bo_count and unref_list of
vc4->render_job. If a hang happens during a binning operation while the
render queue is idle, it seems no BOs will be captured at all.

> diff --git a/drivers/gpu/drm/vc4/vc4_validate.c b/drivers/gpu/drm/vc4/vc4_validate.c
> --- a/drivers/gpu/drm/vc4/vc4_validate.c
> +++ b/drivers/gpu/drm/vc4/vc4_validate.c
> @@ -798,12 +801,12 @@ validate_gl_shader_rec(struct drm_device *dev,
>  	exec->shader_rec_size -= packet_size;
>  
>  	for (i = 0; i < shader_reloc_count; i++) {
> -		if (src_handles[i] > exec->bo_count) {
> +		if (src_handles[i] > render->bo_count) {
>  			DRM_DEBUG("Shader handle %d too big\n", src_handles[i]);
>  			return -EINVAL;
>  		}
>  
> -		bo[i] = to_drm_gem_dma_obj(exec->bo[src_handles[i]]);
> +		bo[i] = to_drm_gem_dma_obj(render->bo[src_handles[i]]);

[Severity: High]
This is a pre-existing issue, but does this bounds check contain an
off-by-one error?

Since the render->bo array has bo_count elements, valid indices are from
0 to bo_count - 1. If userspace provides an index exactly equal to
bo_count, the check incorrectly succeeds. The kernel then dereferences
render->bo[bo_count], resulting in an out-of-bounds read past the end
of the array.

> diff --git a/drivers/gpu/drm/vc4/vc4_submit.c b/drivers/gpu/drm/vc4/vc4_submit.c
> --- /dev/null
> +++ b/drivers/gpu/drm/vc4/vc4_submit.c
[ ... ]
> +fail_dec_usecnt:
> +	/* Decrease usecnt on acquired objects */
> +	for (i--; i >= 0; i--)
> +		vc4_bo_dec_usecnt(to_vc4_bo(job->bo[i]));

[Severity: High]
Are the reference counts getting underflowed during the error path?

If an increment fails, the error path loops backwards decrementing the
usecnt. However, because the BO array remains fully populated in the job
struct, the subsequent failure cleanup in vc4_render_job_free() iterates
over the entire array again:

vc4_submit.c:vc4_render_job_free() {
    if (job->bo) {
        for (int i = 0; i < job->bo_count; i++) {
            struct vc4_bo *bo = to_vc4_bo(job->bo[i]);
            vc4_bo_dec_usecnt(bo);
            ...
        }
    }
}

This would double-decrement the BOs that were unwound here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-vc4-drm-scheduler-v3-0-ed6dc4defa53@igalia.com?part=7