Re: [PATCH v3 1/3] drm/v3d: Associate BOs with every job that accesses them

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] V3D_CACHE_CLEAN jobs execute after their associated buffer objects (BOs) are freed, causing the GPU L2 cache to write dirty lines back to unmapped or reassigned physical memory.

Pre-existing issues:
- [High] The BIN job explicitly skips acquiring implicit dependencies, which violates the dma-buf implicit sync contract for imported buffers and allows the BIN job to execute concurrently with writers.
--

commit 9c4142c6fc75015db16c542de5fec66fe98c34f8
Author: Maíra Canal <mcanal@igalia.com>

drm/v3d: Associate BOs with every job that accesses them

This commit addresses an implicit synchronization issue by calling
v3d_lookup_bos() for each job that references the submission's BOs. This
gives earlier jobs in the submission chain their own implicit dependencies
during reservation locking.

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index ee2ac2540ed50..9d23935b05dd0 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c

[ ... ]

> @@ -446,7 +441,8 @@ v3d_setup_csd_jobs_and_bos(struct v3d_submit *submit,
>  	if (IS_ERR(clean_job))
>  		return PTR_ERR(clean_job);
>  
> -	return v3d_lookup_bos(submit, args->bo_handles, args->bo_handle_count);
> +	return v3d_lookup_bos(submit->file_priv, &job->base,
> +			      args->bo_handles, args->bo_handle_count);
>  }

[Severity: Critical]
Does attaching the BOs exclusively to the CSD job leave the appended
CACHE_CLEAN job without references to keep the BOs alive?

If userspace submits a job with the flush cache flag and immediately closes
the BO handles, the CSD job's completion might drop the final reference to
the BOs. 

If the BOs are freed and unmapped before the CACHE_CLEAN job executes
v3d_clean_caches(), could the synchronous flush write dirty lines back to
reallocated physical memory?

[ ... ]

> @@ -1085,6 +1081,17 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		goto fail;
>  
> +	/*
> +	 * We don't associate the BOs with the BIN job. Fences are only
> +	 * attached to the last job in the submission chain, and BIN jobs
> +	 * don't need implicit dependencies since depending on results from
> +	 * another context is not a realistic scenario for binning.
> +	 */
> +	ret = v3d_lookup_bos(submit.file_priv, &render->base,
> +			     args->bo_handles, args->bo_handle_count);

[Severity: High]
This is a pre-existing issue, but does skipping the implicit dependencies
for the BIN job violate the dma-buf implicit sync contract for imported
buffers?

If userspace imports a dma-buf that is currently being written by another
context (like a compute shader or video decoder) and uses it for a BIN job,
the DRM scheduler will execute the BIN job immediately because it was given
zero implicit dependencies.

Could this cause the hardware to read from the buffer while the other context
is still writing to it, resulting in data corruption?

> +	if (ret)
> +		goto fail;
> +
>  	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
>  		clean_job = v3d_submit_add_job(&submit, V3D_CACHE_CLEAN);
>  		if (IS_ERR(clean_job)) {

[Severity: Critical]
Similar to the CSD job in v3d_setup_csd_jobs_and_bos(), does associating
the BOs exclusively with the RENDER job leave the cache clean job without
references to the BOs?

If the BOs are freed after the RENDER job completes, could the CACHE_CLEAN job
flush the L2 cache back to freed physical addresses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-v3d-cpu-job-fixes-v3-0-bc51b1f3eeb5@igalia.com?part=1