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 <[email protected]> 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/[email protected]?part=1
