Hi Iago,
On 11/06/26 03: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.
I have the impression that this is a consequence of several design
changes that the driver passed through the years. Initially, the v3d
driver only had a CL submission and attaching BOs to the last job meant
attaching to the render job. This was fine, only considering the caveat
mentioned in commit dffa9b7a78c4 "drm/v3d: Add missing implicit
synchronization": "we currently only implicitly sync for the rendering
pass, not binning -- if you texture-from-pixmap in the binning vertex
shader you'll miss out on synchronization."
However, with time, we added the CACHE_CLEAN flag to the CL submission
(so the last job could be a CACHE_CLEAN job) and we added CSD and TFU
submissions. Then, attaching the BOs to the last job wasn't the correct
approach anymore.
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?
Maybe some one who has been around for longer could bring a historical
overview about this topic. From my understand, I don't think we can drop
implicit sync because it's part of the dma-buf synchronization contract.
If I'm not mistaken, v3d_submit_attach_object_fences() publishes our
fence so a consumer (e.g. compositor, KMS) can wait on us, and
drm_sched_job_add_implicit_dependencies() makes us wait on other actors'
fences already on the buffer. Mesa only syncs within its own context,
but it doesn't sync shared buffers cross-processes.
We can think about creating a EXPLICIT_SYNC flag for the BOs in the
future. We can talk more about this idea privately.
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?
We don't want to attach the BOs to all the jobs in the chain. For
example, I don't believe we would want to attach BOs to a CLEAN_CACHE
job.
Best regards,
- Maíra
