Re: [PATCH 01/11] drm/panthor: Always use the IRQ-safe variant when acquiring the fence lock

[Liviu Dudau]

On Thu, Jun 25, 2026 at 02:40:27PM +0200, Boris Brezillon wrote:
> Since dma_fence objects can be shared with other subsystems, they may be
> accessed from hardirq context in those drivers, and we have to take
> that into account by also using the IRQ-safe variant when acquiring
> the lock.
> 
> While at it, switch to the guard model.
> 
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> Reported-by: sashiko-bot@xxxxxxxxxx
> Closes: https://sashiko.dev/#/patchset/20260625-panthor-signal-from-irq-v5-0-8836a74e0ef9@xxxxxxxxxxxxx?part=11
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>

Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 83 ++++++++++++++++-----------------
>  1 file changed, 39 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 5b34032deff8..e97f29469d28 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1151,15 +1151,14 @@ queue_suspend_timeout_locked(struct panthor_queue *queue)
>  static void
>  queue_suspend_timeout(struct panthor_queue *queue)
>  {
> -	spin_lock(&queue->fence_ctx.lock);
> +	guard(spinlock_irqsave)(&queue->fence_ctx.lock);
>  	queue_suspend_timeout_locked(queue);
> -	spin_unlock(&queue->fence_ctx.lock);
>  }
>  
>  static void
>  queue_resume_timeout(struct panthor_queue *queue)
>  {
> -	spin_lock(&queue->fence_ctx.lock);
> +	guard(spinlock_irqsave)(&queue->fence_ctx.lock);
>  
>  	if (queue_timeout_is_suspended(queue)) {
>  		mod_delayed_work(queue->scheduler.timeout_wq,
> @@ -1168,8 +1167,6 @@ queue_resume_timeout(struct panthor_queue *queue)
>  
>  		queue->timeout.remaining = MAX_SCHEDULE_TIMEOUT;
>  	}
> -
> -	spin_unlock(&queue->fence_ctx.lock);
>  }
>  
>  /**
> @@ -1542,7 +1539,7 @@ cs_slot_process_fault_event_locked(struct panthor_device *ptdev,
>  		u64 cs_extract = queue->iface.output->extract;
>  		struct panthor_job *job;
>  
> -		spin_lock(&queue->fence_ctx.lock);
> +		guard(spinlock_irqsave)(&queue->fence_ctx.lock);
>  		list_for_each_entry(job, &queue->fence_ctx.in_flight_jobs, node) {
>  			if (cs_extract >= job->ringbuf.end)
>  				continue;
> @@ -1552,7 +1549,6 @@ cs_slot_process_fault_event_locked(struct panthor_device *ptdev,
>  
>  			dma_fence_set_error(job->done_fence, -EINVAL);
>  		}
> -		spin_unlock(&queue->fence_ctx.lock);
>  	}
>  
>  	if (group) {
> @@ -2183,13 +2179,13 @@ group_term_post_processing(struct panthor_group *group)
>  		if (!queue)
>  			continue;
>  
> -		spin_lock(&queue->fence_ctx.lock);
> -		list_for_each_entry_safe(job, tmp, &queue->fence_ctx.in_flight_jobs, node) {
> -			list_move_tail(&job->node, &faulty_jobs);
> -			dma_fence_set_error(job->done_fence, err);
> -			dma_fence_signal_locked(job->done_fence);
> +		scoped_guard(spinlock_irqsave, &queue->fence_ctx.lock) {
> +			list_for_each_entry_safe(job, tmp, &queue->fence_ctx.in_flight_jobs, node) {
> +				list_move_tail(&job->node, &faulty_jobs);
> +				dma_fence_set_error(job->done_fence, err);
> +				dma_fence_signal_locked(job->done_fence);
> +			}
>  		}
> -		spin_unlock(&queue->fence_ctx.lock);
>  
>  		/* Manually update the syncobj seqno to unblock waiters. */
>  		syncobj = group->syncobjs->kmap + (i * sizeof(*syncobj));
> @@ -3049,39 +3045,39 @@ static bool queue_check_job_completion(struct panthor_queue *queue)
>  	LIST_HEAD(done_jobs);
>  
>  	cookie = dma_fence_begin_signalling();
> -	spin_lock(&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;
> +	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));
> +				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 (syncobj->seqno < job->done_fence->seqno)
> -			break;
> +		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);
>  
> -		list_move_tail(&job->node, &done_jobs);
> -		dma_fence_signal_locked(job->done_fence);
> +			/* 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;
> +		}
>  	}
> -
> -	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;
> -	}
> -	spin_unlock(&queue->fence_ctx.lock);
>  	dma_fence_end_signalling(cookie);
>  
>  	list_for_each_entry_safe(job, job_tmp, &done_jobs, node) {
> @@ -3346,9 +3342,8 @@ queue_run_job(struct drm_sched_job *sched_job)
>  	job->ringbuf.end = job->ringbuf.start + (instrs.count * sizeof(u64));
>  
>  	panthor_job_get(&job->base);
> -	spin_lock(&queue->fence_ctx.lock);
> -	list_add_tail(&job->node, &queue->fence_ctx.in_flight_jobs);
> -	spin_unlock(&queue->fence_ctx.lock);
> +	scoped_guard(spinlock_irqsave, &queue->fence_ctx.lock)
> +		list_add_tail(&job->node, &queue->fence_ctx.in_flight_jobs);
>  
>  	/* Make sure the ring buffer is updated before the INSERT
>  	 * register.
> 
> -- 
> 2.54.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯