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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 17 Aug 2026 16:29:42 +0100
Steven Price <steven.price@arm.com> wrote:

> 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 <boris.brezillon@collabora.com>
> > ---
> >  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).

Oops, that's a leftover from the non-irqsafe -> irqsafe transition that
was done in the previous revisions of this patchset. Will fix that.

> 
> With that fixed:
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> 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);
> >  
> >   
>