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

[Emma Anholt <anholt@igalia.com>]

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 <mcanal@igalia.com>
>> ---
>>  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;
>>  	}
>>