Hi Maíra,
I have a couple of thoughts here:
1. The original code seems to very specifica about targeting this only
for the last job, which makes it look like a very intentional decision,
so I wonder if there is something we are missing here. I am adding Emma
to the CC in case she has any thoughts about it.
2. I am not aware of any issues despite not having implicit sync for
all the jobs in the chanin. Like you mention, this is because user-
space is already trying to handle job depedencies, so I wonder if we
should instead work on the opposite direction and try to drop implicit
sync in the kernel entirely. Would that make sense? What are other
drivers doing in this regard?
As for the patch itself, I have a comment below:
El mié, 10-06-2026 a las 19:50 -0300, Maíra Canal escribió:
> A submission can expand into a chain of jobs (e.g. bin + render +
> cache
> clean), but v3d_lookup_bos() only looked up the user's BO list onto
> the
> *last* job of the submission. Every earlier job was left with
> bo_count == 0 and an empty bo[] array.
>
> As a consequence, when implicit synchronization happens in
> v3d_submit_lock_reservations(), earlier jobs get no implicit
> dependencies at all, as the loop is gated on job->bo_count and
> earlier
> jobs don't have any BO attached to them. With that, the BIN job reads
> the
> same buffers as the RENDER job, yet nothing made it wait for a prior
> writer to finish. The BIN job could therefore be dispatched to the
> hardware and read a BO while another context was still writing it,
> leading to data corruption that was only avoided as the userspace
> adds
> explicit syncobjs.
>
> Fix this by calling v3d_lookup_bos() for each job that references the
> submission's BOs, so every job carries its own bo[]/bo_count and
> picks
> up the correct implicit dependencies during reservation locking.
>
> Fixes: dffa9b7a78c4 ("drm/v3d: Add missing implicit
> synchronization.")
> Signed-off-by: Maíra Canal <[email protected]>
> ---
> drivers/gpu/drm/v3d/v3d_submit.c | 41 +++++++++++++++++++++---------
> ----------
> 1 file changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c
> b/drivers/gpu/drm/v3d/v3d_submit.c
> index ee2ac2540ed5..3d6582dfb1bf 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -68,7 +68,6 @@ v3d_submit_unlock_reservations(struct v3d_submit
> *submit)
> /**
> * v3d_lookup_bos() - Sets up job->bo[] with the GEM objects
> * referenced by the job.
> - * @dev: DRM device
> * @file_priv: DRM file for this fd
> * @job: V3D job being set up
> * @bo_handles: GEM handles
> @@ -82,23 +81,19 @@ v3d_submit_unlock_reservations(struct v3d_submit
> *submit)
> * failure, because that will happen at `v3d_job_free()`.
> */
> static int
> -v3d_lookup_bos(struct v3d_submit *submit, u64 bo_handles, u32
> bo_count)
> +v3d_lookup_bos(struct drm_file *file_priv, struct v3d_job *job,
> + u64 bo_handles, u32 bo_count)
> {
Wouldn't it make more sense to have this function take the submit like
it did originally and loop through all the jobs in a chain instead of
calling this multiple times for each job in a chain?
> - struct v3d_job *last_job = submit->jobs[submit->job_count -
> 1];
> -
> - last_job->bo_count = bo_count;
> -
> - if (!last_job->bo_count) {
> - /* See comment on bo_index for why we have to check
> - * this.
> - */
> - drm_warn(&submit->v3d->drm, "Rendering requires
> BOs\n");
> + if (!bo_count) {
> + drm_warn(&job->v3d->drm, "Rendering requires
> BOs\n");
> return -EINVAL;
> }
>
> - return drm_gem_objects_lookup(submit->file_priv,
> + job->bo_count = bo_count;
> +
> + return drm_gem_objects_lookup(file_priv,
> (void __user
> *)(uintptr_t)bo_handles,
> - last_job->bo_count, &last_job-
> >bo);
> + job->bo_count, &job->bo);
> }
>
> static void
> @@ -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);
> }
>
> static void
> @@ -1066,6 +1062,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
> void *data,
> &se);
> if (ret)
> goto fail;
> +
> + ret = v3d_lookup_bos(submit.file_priv, &bin->base,
> + args->bo_handles, args-
> >bo_handle_count);
> + if (ret)
> + goto fail;
> }
>
> render = (struct v3d_render_job
> *)v3d_submit_add_job(&submit, V3D_RENDER);
> @@ -1085,6 +1086,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
> void *data,
> if (ret)
> goto fail;
>
> + ret = v3d_lookup_bos(submit.file_priv, &render->base,
> + args->bo_handles, args-
> >bo_handle_count);
> + 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)) {
> @@ -1097,10 +1103,6 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
> void *data,
> if (ret)
> goto fail;
>
> - ret = v3d_lookup_bos(&submit, args->bo_handles, args-
> >bo_handle_count);
> - if (ret)
> - goto fail;
> -
> ret = v3d_submit_lock_reservations(&submit);
> if (ret)
> goto fail;
> @@ -1359,7 +1361,8 @@ v3d_submit_cpu_ioctl(struct drm_device *dev,
> void *data,
> * the CSD and clean jobs in the case of indirect CSD job.
> */
> if (args->bo_handle_count) {
> - ret = v3d_lookup_bos(&submit, args->bo_handles,
> args->bo_handle_count);
> + ret = v3d_lookup_bos(submit.file_priv, &cpu_job-
> >base,
> + args->bo_handles, args-
> >bo_handle_count);
> if (ret)
> goto fail;
> }
>
