Re: [PATCH v3 4/8] drm/vc4: Add new job submission implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Reference count double-decrement and underflow in job submission error path
- [High] ABI Breakage: `seqno` is not returned to userspace upon successful job submission
- [High] Uninitialized `exec->bo` and `exec->rcl_write_bo_count` fields break job validation and dependency tracking
--

--- Patch [4]: [PATCH v3 4/8] drm/vc4: Add new job submission implementation ---
commit 394b85fa6f0403b5520bcd58bae0ec277c6cabc7
Author: Maíra Canal <mcanal@igalia.com>

drm/vc4: Add new job submission implementation

Introduce vc4_submit.c with the job submission path rewritten to
integrate with the DRM GPU scheduler. Most of this code is adapted from
vc4_gem.c, with key changes concentrated in the job creation and
lifecycle management.

> --- /dev/null
> +++ b/drivers/gpu/drm/vc4/vc4_submit.c

[ ... ]

> +static int
> +vc4_add_implicit_dependencies(struct vc4_exec_info *exec)
> +{
> +	struct vc4_render_job *render = exec->render;

[ ... ]

> +	/*
> +	 * The renderer writes the RCL targets (color/Z/MSAA buffers), so it
> +	 * additionally has to wait for prior readers of those.
> +	 */
> +	for (int i = 0; i < render->rcl_write_bo_count; i++) {
> +		ret = drm_sched_job_add_implicit_dependencies(&render->base.base,
> +							      &render->rcl_write_bo[i]->base,
> +							      true);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Does this loop skip adding implicit dependencies?

The validation code in vc4_get_rcl() appears to populate
exec->rcl_write_bo_count, but this loop iterates over
render->rcl_write_bo_count, which remains 0.

[ ... ]

> +static int
> +vc4_lookup_bos(struct drm_device *dev, struct drm_file *file_priv,
> +	       struct vc4_render_job *job, u64 bo_handles, u32 bo_count)
> +{

[ ... ]

> +	for (i = 0; i < job->bo_count; i++) {
> +		ret = vc4_bo_inc_usecnt(to_vc4_bo(job->bo[i]));
> +		if (ret)
> +			goto fail_dec_usecnt;
> +	}
> +
> +	return 0;
> +
> +fail_dec_usecnt:
> +	/* Decrease usecnt on acquired objects */
> +	for (i--; i >= 0; i--)
> +		vc4_bo_dec_usecnt(to_vc4_bo(job->bo[i]));
> +	return ret;
> +}

[ ... ]

> +static void
> +vc4_render_job_free(struct kref *ref)
> +{
> +	struct vc4_render_job *job = container_of(ref, struct vc4_render_job,
> +						  base.refcount);
> +	struct vc4_dev *vc4 = job->base.vc4;
> +	struct vc4_bo *bo, *tmp;
> +	unsigned long irqflags;
> +
> +	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);

[Severity: High]
If vc4_bo_inc_usecnt() fails in vc4_lookup_bos(), the error path rolls back
the increments but leaves job->bo fully populated.

When the error propagates up and triggers vc4_render_job_free(), the cleanup
code iterates over job->bo and unconditionally decrements the usecnt again.

Could this cause a double-decrement and underflow for the BOs?

[ ... ]

> +int
> +vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
> +		    struct drm_file *file_priv)
> +{

[ ... ]

> +	exec.render = render;
> +	INIT_LIST_HEAD(&render->unref_list);
> +
> +	ret = vc4_lookup_bos(dev, file_priv, render, args->bo_handles,
> +			     args->bo_handle_count);
> +	if (ret)
> +		goto fail;

[Severity: High]
This call populates render->bo, but does it leave exec.bo uninitialized and
exec.bo_count as 0?

If validation functions like vc4_get_bcl() and vc4_get_rcl() still rely on
the legacy exec->bo array via vc4_use_bo(), this could cause shader
validation to fail with -EINVAL.

[ ... ]

> +	scoped_guard(mutex, &vc4->sched_lock) {
> +		ret = vc4_push_jobs(&exec);
> +	}
> +
> +	if (!ret) {
> +		vc4_attach_fences(render, sync_out, render->base.done_fence);
> +	} else if (sync_out) {
> +		/* The jobs were never submitted, so release the unpublished syncobj */
> +		drm_syncobj_put(sync_out);
> +	}

[Severity: High]
Are we missing the sequence number assignment to userspace here?

The drm_vc4_submit_cl structure expects the kernel to return the sequence
number of the submitted job in the seqno field.

Since args->seqno is not populated before returning success, could this
prevent userspace from properly waiting on the submitted job?

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