Hi Tvrtko,
On 06/04/26 11:51, Tvrtko Ursulin wrote:
On 02/04/2026 19:36, Maíra Canal wrote:
Introduce vc4_submit.c with the job submission path rewritten to
integrate with the DRM GPU scheduler. Most of this code is adapted from
vc4_gem.c, with key changes concentrated in the job creation and
lifecycle management. This implementation follows the same design as the
v3d driver.
This code coexists with the legacy path until the switchover commit.
Signed-off-by: Maíra Canal <[email protected]>
---
Ack to all previous comments.
+
+ exec->bin_u = bin;
+
+ exec->shader_rec_v = exec_bo->vaddr + shader_rec_offset;
+ exec->shader_rec_p = exec_bo->dma_addr + shader_rec_offset;
+ exec->shader_rec_size = args->shader_rec_size;
+
+ exec->uniforms_v = exec_bo->vaddr + uniforms_offset;
+ exec->uniforms_p = exec_bo->dma_addr + uniforms_offset;
+ exec->uniforms_size = args->uniforms_size;
+
+ ret = vc4_validate_bin_cl(dev, exec_bo->vaddr, bin, exec);
+ if (ret)
+ goto fail;
+
+ ret = vc4_validate_shader_recs(dev, exec);
+ if (ret)
+ goto fail;
+
+ if (exec->found_tile_binning_mode_config_packet) {
+ ret = vc4_v3d_bin_bo_get(vc4, &bin_job->bin_bo_used);
+ if (ret)
+ goto fail;
+ }
+
+fail:
+ kvfree(bin);
A bit odd exec->bin_u is left pointing to freed memory so I guess it is
not dereferenced by anyone later and it is just a convenience to store
it in struct vc4_exec_info.
I can add a exec->bin_u = NULL here.
+ return ret;
+}
+
+static void
+vc4_job_free(struct kref *ref)
+{
+ struct vc4_job *job = container_of(ref, struct vc4_job, refcount);
+
+ dma_fence_put(job->irq_fence);
Ah I was looking for this while reading the previous patch, all good then.
+ dma_fence_put(job->done_fence);
+
+ if (job->perfmon)
+ vc4_perfmon_put(job->perfmon);
Could the pointer to this job->perfmon be left in vc4->active_perfmon if
no new job was queued to reset that pointer?
Yeah... That's why we have that vc4_perfmon_stop() when destroying the
perfmons from a given file. Again, I believe perfmon lifetime in v3d/vc4
needs to be reviewed. At the moment, it's error prone.
+
+ vc4_v3d_pm_put(job->vc4);
+ kfree(job);
+}
+
[...]
+
+ if (out_sync > 0) {
+ /* Update the return sync object for the job */
+ sync_out = drm_syncobj_find(file_priv, out_sync);
+ if (sync_out) {
+ drm_syncobj_replace_fence(sync_out, done_fence);
+ drm_syncobj_put(sync_out);
+ }
Is it okay to succeed if sync_out passed in by userspace was not found?
Ack, I just noticed that the current code fails the job submission if
the sync_out is not found.
[...]
+ ret = vc4_job_alloc(vc4, file_priv, (void *)&render,
sizeof(*render),
+ vc4_render_job_free, args->in_sync, VC4_RENDER);
On the topic of void * games, I wonder if it would be cleaner for
vc4_job_alloc to take struct vc4_job **? Or it could even return ERR_PTR.
Ack.
[...]
+
+ mutex_lock(&vc4->sched_lock);
+ if (bin) {
+ vc4_push_job(&bin->base);
+
+ ret = drm_sched_job_add_dependency(&render->base.base,
+ dma_fence_get(bin->base.done_fence));
+ if (ret)
+ goto fail_dependency;
+ }
+
+ vc4_push_job(&render->base);
+ mutex_unlock(&vc4->sched_lock);
Is it significant that vc4->sched_lock is held across both render and
bin, or it would work the same if it was held over individual arm-to-
push sections?
I believe it'd work just fine if it was held over individual arm-to-push
sections. I'll think a bit more about it.
+
+
Two blank lines.
+ vc4_attach_fences(file_priv, render, args->out_sync, render-
>base.done_fence);
+ drm_exec_fini(&exec_ctx);
+
+ vc4_job_put((void *)bin);
+ vc4_job_put((void *)render);
Could vc4_job_put not work on &bin->base and so not need the void * casts?
Sure.
+
+ return 0;
+
+fail_dependency:
+ mutex_unlock(&vc4->sched_lock);
+ drm_exec_fini(&exec_ctx);
+
+ /* BIN job has already been pushed; the scheduler owns it. Just drop
+ * our ref. As the RENDER job has not been armed, normal cleanup
still
+ * applies.
+ */
What is the consequence if this path would trigger? GPU hang?
The BIN job can complete just fine without the RENDER job. Now, what
will happen to the user's application? It depends on how user-space will
handle the IOCTL failure. If the user-space treats the errno like a
failure to job submission (that is, understands that no job was
submitted at all), we should be fine.
Thanks for your review!
Best regards,
- Maíra
Regards,
Tvrtko
