Re: [PATCH 7/8] rust: drm: Add JobQueue

[Philipp Stanner <phasta@mailbox.org>]

On Sat, 2026-11-07 at 13:55 +0000, Younes Akhouayri wrote:
> On Fri, 6 Nov 2026 15:07:52 +0100, Philipp Stanner wrote:
> > +
> > +            drop(jq); // Release lock.
> > +            T::run_job(work.fctx, runnable_job);
> > +            jq = work.inner.lock(); // Retake lock.
> > +
> > +            // run_job() above needs to be called without lock protection. Thus,
> > +            // it is possible that the driver instructed completion right before
> > +            // we could re-take the lock.
> > +            if jq.last_completed >= seqno {
> > +                *jq.as_mut().project().capacity += u64::from(job.cost);
> > +                fence.signal(Ok(()));
> > +
> > +                // Drop here directly if possible. Otherwise, let the DropWorker
> > +                // take care of it.
> > +                if core::mem::needs_drop::<T::JobDataType>() {
> > +                    jq.drop_list.push_back(job);
> > +                }
> > +            } else {
> > +                jq.running_jobs.push_back(job);
> > +            }
> 
> When the IRQ wins the race, `complete_jobs_up_to_seqno()` kicks the
> DropWorker before the job lands on `drop_list`, and this branch
> pushes it there without kicking again. Am I reading it right that the
> job then sits on `drop_list` until the next completion or until the
> queue drops? If so, is that acceptable, or does this path need a
> `check_start_drop_worker()` too?

I tend to think it's acceptable (and I was aware of this). Though we
could kick it here nevertheless, it's no big deal.

@Daniel, you mentioned drop delays recently. Opinion?

> 
> 
> > +        self.with_inner(|inner| {
> > +            let mut inner = inner.lock();
> > +            let drop_list = core::mem::take(&mut inner.drop_list);
> > +            let waiting_jobs = core::mem::take(&mut inner.waiting_jobs);
> > +            let mut running_jobs = core::mem::take(&mut inner.running_jobs);
> > +
> > +            while let Some(job) = running_jobs.pop_front() {
> > +                let fence = job.fence.as_ref().expect("fence not present");
> > +                fence.signal(Err(self.err));
> > +            }
> > +
> > +            drop(inner);
> > +            drop(drop_list);
> > +            drop(waiting_jobs);
> > +            drop(running_jobs);
> > +        })
> 
> In the `while let Some(job) = running_jobs.pop_front()` loop, `job`
> goes out of scope at the end of each iteration, while `inner` is
> still locked. If `JobDataType` has a `Drop` impl, that ends up in
> `DriverFence::drop` -> `synchronize_rcu()`; so is sleeping there with
> the irq spinlock held intended, or should those jobs be collected and
> dropped after `drop(inner)` like `drop_list` and `waiting_jobs` are?

Nope, that code is wrong! Actually, not holding the lock is even the
reason why the mem::take is done.

I added the loop later. What lured me into it is, I think, that
compared to C you cannot clearly see the unlock(). Not sure if that's
idiomatic and intended, but I like to call such variables
`inner_locked` or add a comment so that it's clearly visible that an
unlock takes place.

Will fix, thank you.

P.