On 2026-06-10 23:17, Iago Toral wrote:
> 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?
Yeah, you should be working toward dropping implicit sync entirely.
tu's VM_BIND is really nice and where you want to be eventually, but
even if not, then you at least want something like
MSM_SUBMIT_NO_IMPLICIT.
I probably just didn't think about this implicit sync case in the
initial implementation, because a bin job dependency on results from
another context is just not a thing that's going to happen in practice.
The implicit sync was mostly about render targets / texturing to be
ordered correctly with X11, and about lifetime management of the BO
(which only needed the last use).
> 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;
>> }
>>
