Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/panthor: per-priority unbound submit_wq in panthor_sched_init() stalls on sched->lock and preempts tick_work() - [Low] drm/panthor: dead submit_wq init and redundant check in group_create_queue() --
commit f453c56b36202882aec84caf56004a890024791a Author: Tvrtko Ursulin <[email protected]> drm/panthor: Create per queue priority workqueues This commit splits the single workqueue shared between the Panthor driver's internal scheduler logic and the DRM scheduler into separate per-priority DRM scheduler submit workqueues. > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c > b/drivers/gpu/drm/panthor/panthor_sched.c > index a343c5180e01e..cbff37e5e1d1c 100644 > --- a/drivers/gpu/drm/panthor/panthor_sched.c > +++ b/drivers/gpu/drm/panthor/panthor_sched.c > @@ -153,11 +153,18 @@ struct panthor_scheduler { > * > * Used for the scheduler tick, group update or other kind of FW > * event processing that can't be handled in the threaded interrupt > - * path. Also passed to the drm_gpu_scheduler instances embedded > - * in panthor_queue. > + * path. > */ > struct workqueue_struct *wq; > > + /** > + * @submit_wq: Per priority workqueues for the DRM scheduler > + * > + * Passed to the drm_gpu_scheduler instances embedded > + * in panthor_queue based on the queue priority. > + */ > + struct workqueue_struct *submit_wq[PANTHOR_CSG_PRIORITY_COUNT]; > + > /** > * @heap_alloc_wq: Workqueue used to schedule tiler_oom works. > * > @@ -3572,8 +3579,14 @@ group_create_queue(struct panthor_group *group, > goto err_free_queue; > } > > - sched_args.name = queue->name; > + if (group->priority >= ARRAY_SIZE(group->ptdev->scheduler->submit_wq) || > + !group->ptdev->scheduler->submit_wq[group->priority]) { > + ret = -EINVAL; > + goto err_free_queue; > + } > > + sched_args.name = queue->name; > + sched_args.submit_wq = > group->ptdev->scheduler->submit_wq[group->priority]; > ret = drm_sched_init(&queue->scheduler, &sched_args); > if (ret) > goto err_free_queue; [Severity: Low] As reported by Tejun Heo on RFC v6 [1], this isn't a bug, but group_create_queue() still initializes sched_args.submit_wq at the top of the function before overwriting it here: drivers/gpu/drm/panthor/panthor_sched.c:group_create_queue() { struct drm_sched_init_args sched_args = { .ops = &panthor_queue_sched_ops, .submit_wq = group->ptdev->scheduler->wq, ... }; } Similarly, the kernel-doc summary line for panthor_scheduler::wq still says it is used by drm_gpu_scheduler. In addition, can the bounds and NULL check on group->priority and group->ptdev->scheduler->submit_wq[group->priority] in group_create_queue() ever trigger? Its caller panthor_group_create() already rejects group_args->priority >= PANTHOR_CSG_PRIORITY_COUNT, and panthor_sched_init() populates all PANTHOR_CSG_PRIORITY_COUNT entries or fails initialization. Finally, the commit message mentions WQ_RTPRI ("high is server by a WQ_HIGHPRI instance, while realtime GPU priority is using the newly added WQ_RTPRI flag"), whereas the flag used in panthor_sched_init() is WQ_RT. Should those be cleaned up? [ ... ] > @@ -4172,7 +4194,14 @@ int panthor_sched_init(struct panthor_device *ptdev) > */ > sched->heap_alloc_wq = alloc_workqueue("panthor-heap-alloc", > WQ_UNBOUND, 0); > sched->wq = alloc_workqueue("panthor-csf-sched", WQ_MEM_RECLAIM | > WQ_UNBOUND, 0); > - if (!sched->wq || !sched->heap_alloc_wq) { > + sched->submit_wq[PANTHOR_CSG_PRIORITY_MEDIUM] = > alloc_workqueue("panthor-drm", WQ_MEM_RECLAIM | WQ_UNBOUND, 2); > + sched->submit_wq[PANTHOR_CSG_PRIORITY_LOW] = > sched->submit_wq[PANTHOR_CSG_PRIORITY_MEDIUM]; > + sched->submit_wq[PANTHOR_CSG_PRIORITY_HIGH] = > alloc_workqueue("panthor-drm-high", WQ_HIGHPRI | WQ_MEM_RECLAIM | WQ_UNBOUND, > 2); > + sched->submit_wq[PANTHOR_CSG_PRIORITY_RT] = > alloc_workqueue("panthor-drm-rt", WQ_RT | WQ_MEM_RECLAIM | WQ_UNBOUND, 2); > + if (!sched->wq || !sched->heap_alloc_wq || > + !sched->submit_wq[PANTHOR_CSG_PRIORITY_MEDIUM] || > + !sched->submit_wq[PANTHOR_CSG_PRIORITY_HIGH] || > + !sched->submit_wq[PANTHOR_CSG_PRIORITY_RT]) { > panthor_sched_fini(&ptdev->base, sched); > drm_err(&ptdev->base, "Failed to allocate the workqueues"); > return -ENOMEM; [Severity: Medium] As reported by Tejun Heo on RFC v6 [2], for a WQ_UNBOUND workqueue, max_active applies across the entire workqueue rather than per CPU. Since panthor_sched_init() allocates each submit_wq with WQ_UNBOUND and max_active = 2 (whereas sched->wq previously used max_active = 0), and group_create_queue() shares that submit_wq across all queues of a given priority on the device for both drm_gpu_scheduler::work_run_job and drm_gpu_scheduler::work_free_job, could two in-flight queue_run_job() calls stalling on sched->lock block all other queues at that priority? Specifically, in queue_run_job(): drivers/gpu/drm/panthor/panthor_sched.c:queue_run_job() { ... ret = panthor_device_resume_and_get(ptdev); if (drm_WARN_ON(&ptdev->base, ret)) return ERR_PTR(ret); mutex_lock(&sched->lock); ... } tick_work() holds sched->lock across firmware round-trip waits in csgs_upd_ctx_apply_locked(). When two queue_run_job() work items block on sched->lock (or runtime PM resume), no other work_run_job (including sync-only jobs with !job->call_info.size that do not take sched->lock) or work_free_job (queue_free_job()) on that submit_wq can run. Because submit_wq[PANTHOR_CSG_PRIORITY_LOW] aliases submit_wq[PANTHOR_CSG_PRIORITY_MEDIUM], two blocked LOW-priority jobs will also stall MEDIUM-priority job submissions and job completions. Furthermore, because submit_wq[PANTHOR_CSG_PRIORITY_RT] is allocated with WQ_RT while panthor_scheduler::wq (which runs tick_work and sync_upd_work) remains a regular-priority workqueue, can a busy RT submission context preempt tick_work() on sched->wq and prevent time-slicing and fairness rotation between RT groups? [1] https://lore.kernel.org/r/[email protected] [2] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
