On 06/03/2026 13:01, Maíra Canal wrote:
Hi Tvrtko,
On 05/03/26 09:19, Tvrtko Ursulin wrote:
On 05/02/2026 21:31, Maíra Canal wrote:
[...]
Ack to all previous comments, thanks a lot!
Okay but beware, I later realised at least one of those was incorrect. :)
+ spin_unlock_irqrestore(&vc4->job_lock, irqflags);
+
+ vc4_flush_caches(dev);
+
+ fence = vc4_fence_create(vc4, VC4_BIN);
+ if (IS_ERR(fence))
+ return NULL;
Exits with vc4->bin_job set, safe or accidental?
Accidental 😅
+
+ if (job->base.irq_fence)
+ dma_fence_put(job->base.irq_fence);
When can this happen, on re-submit?
Exactly.
Comment would probably be good.
+ job->base.irq_fence = dma_fence_get(fence);
+
+ trace_vc4_submit_cl(dev, false, to_vc4_fence(fence)->seqno,
+ job->ct0ca, job->ct0ea);
+
+ vc4_switch_perfmon(vc4, &job->base);
+
+ V3D_WRITE(V3D_CTNCA(0), job->ct0ca);
+ V3D_WRITE(V3D_CTNEA(0), job->ct0ea);
+
+ return fence;
+}
+
+static struct dma_fence *vc4_render_job_run(struct drm_sched_job
*sched_job)
+{
+ struct vc4_render_job *job = to_render_job(sched_job);
+ struct vc4_dev *vc4 = job->base.vc4;
+ struct drm_device *dev = &vc4->base;
+ struct dma_fence *fence;
+
+ if (unlikely(job->base.base.s_fence->finished.error)) {
+ vc4->render_job = NULL;
No spinlock for render, only bin? By design?
The RENDER job is only changed in two places: when the job is starting
(here) and when the job is finishing. As the two situations are mutually
exclusive, it's fine not to have the lock. The BIN job is also used in
the memory overflow worker and that's why we need the lock.
Ack.
One meta comment, I have a feeling a bunch of places do not need the
irqsave spin lock variant, or maybe not even irq, although for later I
am not 100% sure. But for stuff running in scheduler callbacks irqsave
is not needed since we know the context is never irq.
Regards,
Tvrtko
