Re: [PATCH v6 7/9] drm/panthor: Don't defer job completion checks

On 11/08/2026 12:23, Boris Brezillon wrote:
> Call group_check_job_completion() directly from
> csg_slot_sync_update_locked() instead of deferring it.
> 
> Signed-off-by: Boris Brezillon <[email protected]>
> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 182 +++++++++++++++-----------------
>  1 file changed, 87 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 794105e239e6..1ef9da55030c 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -697,9 +697,6 @@ struct panthor_group {
>  	 */
>  	struct panthor_kernel_bo *protm_suspend_buf;
>  
> -	/** @sync_upd_work: Work used to check/signal job fences. */
> -	struct work_struct sync_upd_work;
> -
>  	/** @tiler_oom_work: Work used to process tiler OOM events happening on this group. */
>  	struct work_struct tiler_oom_work;
>  
> @@ -1762,6 +1759,92 @@ static void csg_slot_process_idle_event_locked(struct panthor_device *ptdev, u32
>  	sched_queue_delayed_work(sched, tick, 0);
>  }
>  
> +static void update_fdinfo_stats(struct panthor_job *job)
> +{
> +	struct panthor_group *group = job->group;
> +	struct panthor_queue *queue = group->queues[job->queue_idx];
> +	struct panthor_gpu_usage *fdinfo = &group->fdinfo.data;
> +	struct panthor_job_profiling_data *slots = queue->profiling.slots->kmap;
> +	struct panthor_job_profiling_data *data = &slots[job->profiling.slot];
> +
> +	scoped_guard(spinlock_irqsave, &group->fdinfo.lock) {

You've switched to spinlock_irqsave here, but
panthor_fdinfo_gather_group_samples() is still using plain spinlock. I
don't think we need the _irqsave as things stand here (we're in a
threaded IRQ handler).

With that fixed:

Reviewed-by: Steven Price <[email protected]>

Thanks,
Steve

> +		if (job->profiling.mask & PANTHOR_DEVICE_PROFILING_CYCLES)
> +			fdinfo->cycles += data->cycles.after - data->cycles.before;
> +		if (job->profiling.mask & PANTHOR_DEVICE_PROFILING_TIMESTAMP)
> +			fdinfo->time += data->time.after - data->time.before;
> +	}
> +}
> +
> +static bool queue_check_job_completion(struct panthor_queue *queue)
> +{
> +	struct panthor_syncobj_64b *syncobj = NULL;
> +	struct panthor_job *job, *job_tmp;
> +	bool cookie, progress = false;
> +	LIST_HEAD(done_jobs);
> +
> +	cookie = dma_fence_begin_signalling();
> +	scoped_guard(spinlock_irqsave, &queue->fence_ctx.lock) {
> +		list_for_each_entry_safe(job, job_tmp, &queue->fence_ctx.in_flight_jobs, node) {
> +			if (!syncobj) {
> +				struct panthor_group *group = job->group;
> +
> +				syncobj = group->syncobjs->kmap +
> +					  (job->queue_idx * sizeof(*syncobj));
> +			}
> +
> +			if (syncobj->seqno < job->done_fence->seqno)
> +				break;
> +
> +			list_move_tail(&job->node, &done_jobs);
> +			dma_fence_signal_locked(job->done_fence);
> +		}
> +
> +		if (list_empty(&queue->fence_ctx.in_flight_jobs)) {
> +			/* If we have no job left, we cancel the timer, and reset remaining
> +			 * time to its default so it can be restarted next time
> +			 * queue_resume_timeout() is called.
> +			 */
> +			queue_suspend_timeout_locked(queue);
> +
> +			/* If there's no job pending, we consider it progress to avoid a
> +			 * spurious timeout if the timeout handler and the sync update
> +			 * handler raced.
> +			 */
> +			progress = true;
> +		} else if (!list_empty(&done_jobs)) {
> +			queue_reset_timeout_locked(queue);
> +			progress = true;
> +		}
> +	}
> +	dma_fence_end_signalling(cookie);
> +
> +	list_for_each_entry_safe(job, job_tmp, &done_jobs, node) {
> +		if (job->profiling.mask)
> +			update_fdinfo_stats(job);
> +		list_del_init(&job->node);
> +		panthor_job_put(&job->base);
> +	}
> +
> +	return progress;
> +}
> +
> +static void group_check_job_completion(struct panthor_group *group)
> +{
> +	u32 queue_idx;
> +	bool cookie;
> +
> +	cookie = dma_fence_begin_signalling();
> +	for (queue_idx = 0; queue_idx < group->queue_count; queue_idx++) {
> +		struct panthor_queue *queue = group->queues[queue_idx];
> +
> +		if (!queue)
> +			continue;
> +
> +		queue_check_job_completion(queue);
> +	}
> +	dma_fence_end_signalling(cookie);
> +}
> +
>  static void csg_slot_sync_update_locked(struct panthor_device *ptdev,
>  					u32 csg_id)
>  {
> @@ -1771,7 +1854,7 @@ static void csg_slot_sync_update_locked(struct panthor_device *ptdev,
>  	lockdep_assert_held(&ptdev->scheduler->events_lock);
>  
>  	if (group)
> -		group_queue_work(group, sync_upd);
> +		group_check_job_completion(group);
>  
>  	sched_queue_work(ptdev->scheduler, sync_upd);
>  }
> @@ -3043,22 +3126,6 @@ void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed)
>  	}
>  }
>  
> -static void update_fdinfo_stats(struct panthor_job *job)
> -{
> -	struct panthor_group *group = job->group;
> -	struct panthor_queue *queue = group->queues[job->queue_idx];
> -	struct panthor_gpu_usage *fdinfo = &group->fdinfo.data;
> -	struct panthor_job_profiling_data *slots = queue->profiling.slots->kmap;
> -	struct panthor_job_profiling_data *data = &slots[job->profiling.slot];
> -
> -	scoped_guard(spinlock, &group->fdinfo.lock) {
> -		if (job->profiling.mask & PANTHOR_DEVICE_PROFILING_CYCLES)
> -			fdinfo->cycles += data->cycles.after - data->cycles.before;
> -		if (job->profiling.mask & PANTHOR_DEVICE_PROFILING_TIMESTAMP)
> -			fdinfo->time += data->time.after - data->time.before;
> -	}
> -}
> -
>  void panthor_fdinfo_gather_group_samples(struct panthor_file *pfile)
>  {
>  	struct panthor_group_pool *gpool = pfile->groups;
> @@ -3079,80 +3146,6 @@ void panthor_fdinfo_gather_group_samples(struct panthor_file *pfile)
>  	xa_unlock(&gpool->xa);
>  }
>  
> -static bool queue_check_job_completion(struct panthor_queue *queue)
> -{
> -	struct panthor_syncobj_64b *syncobj = NULL;
> -	struct panthor_job *job, *job_tmp;
> -	bool cookie, progress = false;
> -	LIST_HEAD(done_jobs);
> -
> -	cookie = dma_fence_begin_signalling();
> -	scoped_guard(spinlock_irqsave, &queue->fence_ctx.lock) {
> -		list_for_each_entry_safe(job, job_tmp, &queue->fence_ctx.in_flight_jobs, node) {
> -			if (!syncobj) {
> -				struct panthor_group *group = job->group;
> -
> -				syncobj = group->syncobjs->kmap +
> -					  (job->queue_idx * sizeof(*syncobj));
> -			}
> -
> -			if (syncobj->seqno < job->done_fence->seqno)
> -				break;
> -
> -			list_move_tail(&job->node, &done_jobs);
> -			dma_fence_signal_locked(job->done_fence);
> -		}
> -
> -		if (list_empty(&queue->fence_ctx.in_flight_jobs)) {
> -			/* If we have no job left, we cancel the timer, and reset remaining
> -			 * time to its default so it can be restarted next time
> -			 * queue_resume_timeout() is called.
> -			 */
> -			queue_suspend_timeout_locked(queue);
> -
> -			/* If there's no job pending, we consider it progress to avoid a
> -			 * spurious timeout if the timeout handler and the sync update
> -			 * handler raced.
> -			 */
> -			progress = true;
> -		} else if (!list_empty(&done_jobs)) {
> -			queue_reset_timeout_locked(queue);
> -			progress = true;
> -		}
> -	}
> -	dma_fence_end_signalling(cookie);
> -
> -	list_for_each_entry_safe(job, job_tmp, &done_jobs, node) {
> -		if (job->profiling.mask)
> -			update_fdinfo_stats(job);
> -		list_del_init(&job->node);
> -		panthor_job_put(&job->base);
> -	}
> -
> -	return progress;
> -}
> -
> -static void group_sync_upd_work(struct work_struct *work)
> -{
> -	struct panthor_group *group =
> -		container_of(work, struct panthor_group, sync_upd_work);
> -	u32 queue_idx;
> -	bool cookie;
> -
> -	cookie = dma_fence_begin_signalling();
> -	for (queue_idx = 0; queue_idx < group->queue_count; queue_idx++) {
> -		struct panthor_queue *queue = group->queues[queue_idx];
> -
> -		if (!queue)
> -			continue;
> -
> -		queue_check_job_completion(queue);
> -	}
> -	dma_fence_end_signalling(cookie);
> -
> -	group_put(group);
> -}
> -
>  struct panthor_job_ringbuf_instrs {
>  	u64 buffer[MAX_INSTRS_PER_JOB];
>  	u32 count;
> @@ -3718,7 +3711,6 @@ int panthor_group_create(struct panthor_file *pfile,
>  	INIT_LIST_HEAD(&group->wait_node);
>  	INIT_LIST_HEAD(&group->run_node);
>  	INIT_WORK(&group->term_work, group_term_work);
> -	INIT_WORK(&group->sync_upd_work, group_sync_upd_work);
>  	INIT_WORK(&group->tiler_oom_work, group_tiler_oom_work);
>  	INIT_WORK(&group->release_work, group_release_work);
>  
>