Re: [PATCH v8 6/6] drm/panthor: Create per queue priority workqueues

[sashiko-bot@kernel.org]

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 <tvrtko.ursulin@igalia.com>

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/64d59ed5538c6af257058533c880740e@kernel.org
[2] https://lore.kernel.org/r/64d59ed5538c6af257058533c880740e@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104095225.32654-1-tvrtko.ursulin@igalia.com?part=6