[PATCH v5 0/2] drm/panthor: Scheduler fixes for termination failure and timeouts

[PATCH v5 0/2] drm/panthor: Scheduler fixes for termination failure and timeouts

[Ashley Smith]

This patch series includes previously attempted patches to fix panthor
scheduler issues with spurious timeouts and issues when a termination
failed which would lead to a race condition.

---
Changes in v5:
 - Swiched to a patch series to make sure the patch which addresses the
   bug is added as a requirement on the scheduler patch.
Changes in v4:
 - Moved code related to a timeout bug to a separate patch as this
   was not relevant to this change.
Changes in v3:
 - Moved to a patch series to make sure this bug fix happens before the
   changes to the scheduler
Changes in v2:
 - Fixed syntax error

Signed-off-by: Ashley Smith <ashley.smith@collabora.com>
---
Ashley Smith (2):
  drm/panthor: Reset queue slots if termination fails
  drm/panthor: Make the timeout per-queue instead of per-job

 drivers/gpu/drm/panthor/panthor_sched.c | 244 +++++++++++++++++-------
 1 file changed, 177 insertions(+), 67 deletions(-)


base-commit: 9528e54198f29548b18b0a5b343a31724e83c68b
-- 
2.43.0

[PATCH v5 1/2] drm/panthor: Reset queue slots if termination fails

[Ashley Smith]

This fixes a bug where if we timeout after a suspend and the termination
fails, due to waiting on a fence that will never be signalled for
example, we do not resume the group correctly. The fix forces a reset
for groups that are not terminated correctly.

Signed-off-by: Ashley Smith <ashley.smith@collabora.com>
Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
---
 drivers/gpu/drm/panthor/panthor_sched.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 43ee57728de5..65d8ae3dcac1 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2727,8 +2727,17 @@ void panthor_sched_suspend(struct panthor_device *ptdev)
 			 * automatically terminate all active groups, so let's
 			 * force the state to halted here.
 			 */
-			if (csg_slot->group->state != PANTHOR_CS_GROUP_TERMINATED)
+			if (csg_slot->group->state != PANTHOR_CS_GROUP_TERMINATED) {
 				csg_slot->group->state = PANTHOR_CS_GROUP_TERMINATED;
+
+				/* Reset the queue slots manually if the termination
+				 * request failed.
+				 */
+				for (i = 0; i < group->queue_count; i++) {
+					if (group->queues[i])
+						cs_slot_reset_locked(ptdev, csg_id, i);
+				}
+			}
 			slot_mask &= ~BIT(csg_id);
 		}
 	}
-- 
2.43.0

[PATCH v5 2/2] drm/panthor: Make the timeout per-queue instead of per-job

[Ashley Smith]

The timeout logic provided by drm_sched leads to races when we try
to suspend it while the drm_sched workqueue queues more jobs. Let's
overhaul the timeout handling in panthor to have our own delayed work
that's resumed/suspended when a group is resumed/suspended. When an
actual timeout occurs, we call drm_sched_fault() to report it
through drm_sched, still. But otherwise, the drm_sched timeout is
disabled (set to MAX_SCHEDULE_TIMEOUT), which leaves us in control of
how we protect modifications on the timer.

One issue seems to be when we call drm_sched_suspend_timeout() from
both queue_run_job() and tick_work() which could lead to races due to
drm_sched_suspend_timeout() not having a lock. Another issue seems to
be in queue_run_job() if the group is not scheduled, we suspend the
timeout again which undoes what drm_sched_job_begin() did when calling
drm_sched_start_timeout(). So the timeout does not reset when a job
is finished.

Co-developed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
---
Signed-off-by: Ashley Smith <ashley.smith@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 233 +++++++++++++++++-------
 1 file changed, 167 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 65d8ae3dcac1..fb5a66ca5384 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -360,17 +360,20 @@ struct panthor_queue {
 	/** @entity: DRM scheduling entity used for this queue. */
 	struct drm_sched_entity entity;
 
-	/**
-	 * @remaining_time: Time remaining before the job timeout expires.
-	 *
-	 * The job timeout is suspended when the queue is not scheduled by the
-	 * FW. Every time we suspend the timer, we need to save the remaining
-	 * time so we can restore it later on.
-	 */
-	unsigned long remaining_time;
+	/** @timeout: Queue timeout related fields. */
+	struct {
+		/** @timeout.work: Work executed when a queue timeout occurs. */
+		struct delayed_work work;
 
-	/** @timeout_suspended: True if the job timeout was suspended. */
-	bool timeout_suspended;
+		/**
+		 * @timeout.remaining: Time remaining before a queue timeout.
+		 *
+		 * When the timer is running, this value is set to MAX_SCHEDULE_TIMEOUT.
+		 * When the timer is suspended, it's set to the time remaining when the
+		 * timer was suspended.
+		 */
+		unsigned long remaining;
+	} timeout;
 
 	/**
 	 * @doorbell_id: Doorbell assigned to this queue.
@@ -1031,6 +1034,82 @@ group_unbind_locked(struct panthor_group *group)
 	return 0;
 }
 
+static bool
+group_is_idle(struct panthor_group *group)
+{
+	struct panthor_device *ptdev = group->ptdev;
+	u32 inactive_queues;
+
+	if (group->csg_id >= 0)
+		return ptdev->scheduler->csg_slots[group->csg_id].idle;
+
+	inactive_queues = group->idle_queues | group->blocked_queues;
+	return hweight32(inactive_queues) == group->queue_count;
+}
+
+static void
+queue_suspend_timeout(struct panthor_queue *queue)
+{
+	unsigned long qtimeout, now;
+	struct panthor_group *group;
+	struct panthor_job *job;
+	bool timer_was_active;
+
+	spin_lock(&queue->fence_ctx.lock);
+
+	/* Already suspended, nothing to do. */
+	if (queue->timeout.remaining != MAX_SCHEDULE_TIMEOUT)
+		goto out_unlock;
+
+	job = list_first_entry_or_null(&queue->fence_ctx.in_flight_jobs,
+				       struct panthor_job, node);
+	group = job ? job->group : NULL;
+
+	/* If the queue is blocked and the group is idle, we want the timer to
+	 * keep running because the group can't be unblocked by other queues,
+	 * so it has to come from an external source, and we want to timebox
+	 * this external signalling.
+	 */
+	if (group && (group->blocked_queues & BIT(job->queue_idx)) &&
+	    group_is_idle(group))
+		goto out_unlock;
+
+	now = jiffies;
+	qtimeout = queue->timeout.work.timer.expires;
+
+	/* Cancel the timer. */
+	timer_was_active = cancel_delayed_work(&queue->timeout.work);
+	if (!timer_was_active || !job)
+		queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
+	else if (time_after(qtimeout, now))
+		queue->timeout.remaining = qtimeout - now;
+	else
+		queue->timeout.remaining = 0;
+
+	if (WARN_ON_ONCE(queue->timeout.remaining > msecs_to_jiffies(JOB_TIMEOUT_MS)))
+		queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
+
+out_unlock:
+	spin_unlock(&queue->fence_ctx.lock);
+}
+
+static void
+queue_resume_timeout(struct panthor_queue *queue)
+{
+	spin_lock(&queue->fence_ctx.lock);
+
+	/* When running, the remaining time is set to MAX_SCHEDULE_TIMEOUT. */
+	if (queue->timeout.remaining != MAX_SCHEDULE_TIMEOUT) {
+		mod_delayed_work(queue->scheduler.timeout_wq,
+				 &queue->timeout.work,
+				 queue->timeout.remaining);
+
+		queue->timeout.remaining = MAX_SCHEDULE_TIMEOUT;
+	}
+
+	spin_unlock(&queue->fence_ctx.lock);
+}
+
 /**
  * cs_slot_prog_locked() - Program a queue slot
  * @ptdev: Device.
@@ -1069,10 +1148,8 @@ cs_slot_prog_locked(struct panthor_device *ptdev, u32 csg_id, u32 cs_id)
 			       CS_IDLE_EMPTY |
 			       CS_STATE_MASK |
 			       CS_EXTRACT_EVENT);
-	if (queue->iface.input->insert != queue->iface.input->extract && queue->timeout_suspended) {
-		drm_sched_resume_timeout(&queue->scheduler, queue->remaining_time);
-		queue->timeout_suspended = false;
-	}
+	if (queue->iface.input->insert != queue->iface.input->extract)
+		queue_resume_timeout(queue);
 }
 
 /**
@@ -1099,14 +1176,7 @@ cs_slot_reset_locked(struct panthor_device *ptdev, u32 csg_id, u32 cs_id)
 			       CS_STATE_STOP,
 			       CS_STATE_MASK);
 
-	/* If the queue is blocked, we want to keep the timeout running, so
-	 * we can detect unbounded waits and kill the group when that happens.
-	 */
-	if (!(group->blocked_queues & BIT(cs_id)) && !queue->timeout_suspended) {
-		queue->remaining_time = drm_sched_suspend_timeout(&queue->scheduler);
-		queue->timeout_suspended = true;
-		WARN_ON(queue->remaining_time > msecs_to_jiffies(JOB_TIMEOUT_MS));
-	}
+	queue_suspend_timeout(queue);
 
 	return 0;
 }
@@ -1888,19 +1958,6 @@ tick_ctx_is_full(const struct panthor_scheduler *sched,
 	return ctx->group_count == sched->csg_slot_count;
 }
 
-static bool
-group_is_idle(struct panthor_group *group)
-{
-	struct panthor_device *ptdev = group->ptdev;
-	u32 inactive_queues;
-
-	if (group->csg_id >= 0)
-		return ptdev->scheduler->csg_slots[group->csg_id].idle;
-
-	inactive_queues = group->idle_queues | group->blocked_queues;
-	return hweight32(inactive_queues) == group->queue_count;
-}
-
 static bool
 group_can_run(struct panthor_group *group)
 {
@@ -2897,35 +2954,50 @@ void panthor_fdinfo_gather_group_samples(struct panthor_file *pfile)
 	xa_unlock(&gpool->xa);
 }
 
-static void group_sync_upd_work(struct work_struct *work)
+static bool queue_check_job_completion(struct panthor_queue *queue)
 {
-	struct panthor_group *group =
-		container_of(work, struct panthor_group, sync_upd_work);
+	struct panthor_syncobj_64b *syncobj = NULL;
 	struct panthor_job *job, *job_tmp;
+	bool cookie, progress = false;
 	LIST_HEAD(done_jobs);
-	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];
-		struct panthor_syncobj_64b *syncobj;
+	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;
 
-		if (!queue)
-			continue;
+			syncobj = group->syncobjs->kmap +
+				  (job->queue_idx * sizeof(*syncobj));
+		}
 
-		syncobj = group->syncobjs->kmap + (queue_idx * sizeof(*syncobj));
+		if (syncobj->seqno < job->done_fence->seqno)
+			break;
 
-		spin_lock(&queue->fence_ctx.lock);
-		list_for_each_entry_safe(job, job_tmp, &queue->fence_ctx.in_flight_jobs, node) {
-			if (syncobj->seqno < job->done_fence->seqno)
-				break;
+		list_move_tail(&job->node, &done_jobs);
+		dma_fence_signal_locked(job->done_fence);
+	}
 
-			list_move_tail(&job->node, &done_jobs);
-			dma_fence_signal_locked(job->done_fence);
-		}
-		spin_unlock(&queue->fence_ctx.lock);
+	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.
+		 */
+		cancel_delayed_work(&queue->timeout.work);
+		queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
+
+		/* 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)) {
+		mod_delayed_work(queue->scheduler.timeout_wq,
+				 &queue->timeout.work,
+				 msecs_to_jiffies(JOB_TIMEOUT_MS));
+		progress = true;
 	}
+	spin_unlock(&queue->fence_ctx.lock);
 	dma_fence_end_signalling(cookie);
 
 	list_for_each_entry_safe(job, job_tmp, &done_jobs, node) {
@@ -2935,6 +3007,27 @@ static void group_sync_upd_work(struct work_struct *work)
 		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);
 }
 
@@ -3182,17 +3275,6 @@ queue_run_job(struct drm_sched_job *sched_job)
 	queue->iface.input->insert = job->ringbuf.end;
 
 	if (group->csg_id < 0) {
-		/* If the queue is blocked, we want to keep the timeout running, so we
-		 * can detect unbounded waits and kill the group when that happens.
-		 * Otherwise, we suspend the timeout so the time we spend waiting for
-		 * a CSG slot is not counted.
-		 */
-		if (!(group->blocked_queues & BIT(job->queue_idx)) &&
-		    !queue->timeout_suspended) {
-			queue->remaining_time = drm_sched_suspend_timeout(&queue->scheduler);
-			queue->timeout_suspended = true;
-		}
-
 		group_schedule_locked(group, BIT(job->queue_idx));
 	} else {
 		gpu_write(ptdev, CSF_DOORBELL(queue->doorbell_id), 1);
@@ -3201,6 +3283,7 @@ queue_run_job(struct drm_sched_job *sched_job)
 			pm_runtime_get(ptdev->base.dev);
 			sched->pm.has_ref = true;
 		}
+		queue_resume_timeout(queue);
 		panthor_devfreq_record_busy(sched->ptdev);
 	}
 
@@ -3250,6 +3333,11 @@ queue_timedout_job(struct drm_sched_job *sched_job)
 
 	queue_start(queue);
 
+	/* We already flagged the queue as faulty, make sure we don't get
+	 * called again.
+	 */
+	queue->scheduler.timeout = MAX_SCHEDULE_TIMEOUT;
+
 	return DRM_GPU_SCHED_STAT_NOMINAL;
 }
 
@@ -3292,6 +3380,17 @@ static u32 calc_profiling_ringbuf_num_slots(struct panthor_device *ptdev,
 	return DIV_ROUND_UP(cs_ringbuf_size, min_profiled_job_instrs * sizeof(u64));
 }
 
+static void queue_timeout_work(struct work_struct *work)
+{
+	struct panthor_queue *queue = container_of(work, struct panthor_queue,
+						   timeout.work.work);
+	bool progress;
+
+	progress = queue_check_job_completion(queue);
+	if (!progress)
+		drm_sched_fault(&queue->scheduler);
+}
+
 static struct panthor_queue *
 group_create_queue(struct panthor_group *group,
 		   const struct drm_panthor_queue_create *args)
@@ -3307,7 +3406,7 @@ group_create_queue(struct panthor_group *group,
 		 * their profiling status.
 		 */
 		.credit_limit = args->ringbuf_size / sizeof(u64),
-		.timeout = msecs_to_jiffies(JOB_TIMEOUT_MS),
+		.timeout = MAX_SCHEDULE_TIMEOUT,
 		.timeout_wq = group->ptdev->reset.wq,
 		.name = "panthor-queue",
 		.dev = group->ptdev->base.dev,
@@ -3330,6 +3429,8 @@ group_create_queue(struct panthor_group *group,
 	if (!queue)
 		return ERR_PTR(-ENOMEM);
 
+	queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
+	INIT_DELAYED_WORK(&queue->timeout.work, queue_timeout_work);
 	queue->fence_ctx.id = dma_fence_context_alloc(1);
 	spin_lock_init(&queue->fence_ctx.lock);
 	INIT_LIST_HEAD(&queue->fence_ctx.in_flight_jobs);
-- 
2.43.0

Re: [PATCH v5 1/2] drm/panthor: Reset queue slots if termination fails

[Liviu Dudau]

On Tue, Jun 03, 2025 at 10:49:31AM +0100, Ashley Smith wrote:
> This fixes a bug where if we timeout after a suspend and the termination
> fails, due to waiting on a fence that will never be signalled for
> example, we do not resume the group correctly. The fix forces a reset
> for groups that are not terminated correctly.

I have a question on the commit message: you're describing a situation where
a fence will *never* be signalled. Is that a real example? I thought this is
not supposed to ever happen! Or are you trying to say that the fence signalling
happens after the timeout?

> 
> Signed-off-by: Ashley Smith <ashley.smith@collabora.com>
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 43ee57728de5..65d8ae3dcac1 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2727,8 +2727,17 @@ void panthor_sched_suspend(struct panthor_device *ptdev)
>  			 * automatically terminate all active groups, so let's
>  			 * force the state to halted here.
>  			 */
> -			if (csg_slot->group->state != PANTHOR_CS_GROUP_TERMINATED)
> +			if (csg_slot->group->state != PANTHOR_CS_GROUP_TERMINATED) {
>  				csg_slot->group->state = PANTHOR_CS_GROUP_TERMINATED;
> +
> +				/* Reset the queue slots manually if the termination
> +				 * request failed.
> +				 */
> +				for (i = 0; i < group->queue_count; i++) {
> +					if (group->queues[i])
> +						cs_slot_reset_locked(ptdev, csg_id, i);
> +				}
> +			}
>  			slot_mask &= ~BIT(csg_id);
>  		}
>  	}
> -- 
> 2.43.0
> 

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

Best regards,
Liviu

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

Re: [PATCH v5 1/2] drm/panthor: Reset queue slots if termination fails

[Ashley Smith]

On Tue, 03 Jun 2025 12:09:44 +0100 Liviu Dudau <liviu.dudau@arm.com> wrote:
 > On Tue, Jun 03, 2025 at 10:49:31AM +0100, Ashley Smith wrote: 
 > > This fixes a bug where if we timeout after a suspend and the termination 
 > > fails, due to waiting on a fence that will never be signalled for 
 > > example, we do not resume the group correctly. The fix forces a reset 
 > > for groups that are not terminated correctly. 
 >  
 > I have a question on the commit message: you're describing a situation where 
 > a fence will *never* be signalled. Is that a real example? I thought this is 
 > not supposed to ever happen! Or are you trying to say that the fence signalling 
 > happens after the timeout?

This covers cases where a fence is never signalled. It shouldn't happen, but we have found this in some situations with a FW hang. Since queue_suspend_timeout() is only called on state update, if a suspend/terminate fails due to a FW hang for example this will leave delayed work, possibly leading to an incorrect queue_timeout_work(). Maybe I should not have used the word bug, it's more choosing a failsafe path.

Re: [PATCH v5 1/2] drm/panthor: Reset queue slots if termination fails

[Boris Brezillon]

On Wed, 04 Jun 2025 11:01:27 +0100
Ashley Smith <ashley.smith@collabora.com> wrote:

> On Tue, 03 Jun 2025 12:09:44 +0100 Liviu Dudau <liviu.dudau@arm.com>
> wrote:
>  > On Tue, Jun 03, 2025 at 10:49:31AM +0100, Ashley Smith wrote:   
>  > > This fixes a bug where if we timeout after a suspend and the
>  > > termination fails, due to waiting on a fence that will never be
>  > > signalled for example, we do not resume the group correctly. The
>  > > fix forces a reset for groups that are not terminated correctly.
>  > >   
>  >  
>  > I have a question on the commit message: you're describing a
>  > situation where a fence will *never* be signalled. Is that a real
>  > example? I thought this is not supposed to ever happen! Or are you
>  > trying to say that the fence signalling happens after the timeout?
>  >  
> 
> This covers cases where a fence is never signalled.

Fence not being signaled in time is covered by the job timeout. What
you're fixing here is the FW not responding to a CSG SUSPEND/TERMINATE
request, which is different.

> It shouldn't
> happen, but we have found this in some situations with a FW hang.

This ^.

> Since queue_suspend_timeout() is only called on state update, if a
> suspend/terminate fails due to a FW hang for example this will leave
> delayed work, possibly leading to an incorrect queue_timeout_work().

Nah, that's not true until the second patch in this series is applied,
because drm_sched_stop(), which is called in the
panthor_sched_pre_reset() path, takes care of suspending the drm_sched
timer. What's still problematic if we don't call cs_slot_reset_locked()
here is that we don't re-initialize the FW CS slots, and since
cs_slot_prog_locked() is only called on active queues, we might end up
with an unused CS slot that still has values from the previous user of
this slot. Not sure how harmful this is in practice, but it's certainly
not great.

The true reason we have a Fixes tag is because the second patch has
a Fixes tag too, and it relies on the new driver timer being stopped
even if the FW hangs on a TERMINATE request (queue_suspend_timeout() is
called in cs_slot_reset_locked()). So, either we merge the two patches
back, like was done in v2, or we have to flag both as Fixes.

Re: [PATCH v5 2/2] drm/panthor: Make the timeout per-queue instead of per-job

[Liviu Dudau]

On Tue, Jun 03, 2025 at 10:49:32AM +0100, Ashley Smith wrote:
> The timeout logic provided by drm_sched leads to races when we try
> to suspend it while the drm_sched workqueue queues more jobs. Let's
> overhaul the timeout handling in panthor to have our own delayed work
> that's resumed/suspended when a group is resumed/suspended. When an
> actual timeout occurs, we call drm_sched_fault() to report it
> through drm_sched, still. But otherwise, the drm_sched timeout is
> disabled (set to MAX_SCHEDULE_TIMEOUT), which leaves us in control of
> how we protect modifications on the timer.
> 
> One issue seems to be when we call drm_sched_suspend_timeout() from
> both queue_run_job() and tick_work() which could lead to races due to
> drm_sched_suspend_timeout() not having a lock. Another issue seems to
> be in queue_run_job() if the group is not scheduled, we suspend the
> timeout again which undoes what drm_sched_job_begin() did when calling
> drm_sched_start_timeout(). So the timeout does not reset when a job
> is finished.
> 
> Co-developed-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> ---
Don't know why you have the dashes in line above in the patch, it will make git to skip your S-o-b.


> Signed-off-by: Ashley Smith <ashley.smith@collabora.com>

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

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 233 +++++++++++++++++-------
>  1 file changed, 167 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 65d8ae3dcac1..fb5a66ca5384 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -360,17 +360,20 @@ struct panthor_queue {
>  	/** @entity: DRM scheduling entity used for this queue. */
>  	struct drm_sched_entity entity;
>  
> -	/**
> -	 * @remaining_time: Time remaining before the job timeout expires.
> -	 *
> -	 * The job timeout is suspended when the queue is not scheduled by the
> -	 * FW. Every time we suspend the timer, we need to save the remaining
> -	 * time so we can restore it later on.
> -	 */
> -	unsigned long remaining_time;
> +	/** @timeout: Queue timeout related fields. */
> +	struct {
> +		/** @timeout.work: Work executed when a queue timeout occurs. */
> +		struct delayed_work work;
>  
> -	/** @timeout_suspended: True if the job timeout was suspended. */
> -	bool timeout_suspended;
> +		/**
> +		 * @timeout.remaining: Time remaining before a queue timeout.
> +		 *
> +		 * When the timer is running, this value is set to MAX_SCHEDULE_TIMEOUT.
> +		 * When the timer is suspended, it's set to the time remaining when the
> +		 * timer was suspended.
> +		 */
> +		unsigned long remaining;
> +	} timeout;
>  
>  	/**
>  	 * @doorbell_id: Doorbell assigned to this queue.
> @@ -1031,6 +1034,82 @@ group_unbind_locked(struct panthor_group *group)
>  	return 0;
>  }
>  
> +static bool
> +group_is_idle(struct panthor_group *group)
> +{
> +	struct panthor_device *ptdev = group->ptdev;
> +	u32 inactive_queues;
> +
> +	if (group->csg_id >= 0)
> +		return ptdev->scheduler->csg_slots[group->csg_id].idle;
> +
> +	inactive_queues = group->idle_queues | group->blocked_queues;
> +	return hweight32(inactive_queues) == group->queue_count;
> +}
> +
> +static void
> +queue_suspend_timeout(struct panthor_queue *queue)
> +{
> +	unsigned long qtimeout, now;
> +	struct panthor_group *group;
> +	struct panthor_job *job;
> +	bool timer_was_active;
> +
> +	spin_lock(&queue->fence_ctx.lock);
> +
> +	/* Already suspended, nothing to do. */
> +	if (queue->timeout.remaining != MAX_SCHEDULE_TIMEOUT)
> +		goto out_unlock;
> +
> +	job = list_first_entry_or_null(&queue->fence_ctx.in_flight_jobs,
> +				       struct panthor_job, node);
> +	group = job ? job->group : NULL;
> +
> +	/* If the queue is blocked and the group is idle, we want the timer to
> +	 * keep running because the group can't be unblocked by other queues,
> +	 * so it has to come from an external source, and we want to timebox
> +	 * this external signalling.
> +	 */
> +	if (group && (group->blocked_queues & BIT(job->queue_idx)) &&
> +	    group_is_idle(group))
> +		goto out_unlock;
> +
> +	now = jiffies;
> +	qtimeout = queue->timeout.work.timer.expires;
> +
> +	/* Cancel the timer. */
> +	timer_was_active = cancel_delayed_work(&queue->timeout.work);
> +	if (!timer_was_active || !job)
> +		queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
> +	else if (time_after(qtimeout, now))
> +		queue->timeout.remaining = qtimeout - now;
> +	else
> +		queue->timeout.remaining = 0;
> +
> +	if (WARN_ON_ONCE(queue->timeout.remaining > msecs_to_jiffies(JOB_TIMEOUT_MS)))
> +		queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
> +
> +out_unlock:
> +	spin_unlock(&queue->fence_ctx.lock);
> +}
> +
> +static void
> +queue_resume_timeout(struct panthor_queue *queue)
> +{
> +	spin_lock(&queue->fence_ctx.lock);
> +
> +	/* When running, the remaining time is set to MAX_SCHEDULE_TIMEOUT. */
> +	if (queue->timeout.remaining != MAX_SCHEDULE_TIMEOUT) {
> +		mod_delayed_work(queue->scheduler.timeout_wq,
> +				 &queue->timeout.work,
> +				 queue->timeout.remaining);
> +
> +		queue->timeout.remaining = MAX_SCHEDULE_TIMEOUT;
> +	}
> +
> +	spin_unlock(&queue->fence_ctx.lock);
> +}
> +
>  /**
>   * cs_slot_prog_locked() - Program a queue slot
>   * @ptdev: Device.
> @@ -1069,10 +1148,8 @@ cs_slot_prog_locked(struct panthor_device *ptdev, u32 csg_id, u32 cs_id)
>  			       CS_IDLE_EMPTY |
>  			       CS_STATE_MASK |
>  			       CS_EXTRACT_EVENT);
> -	if (queue->iface.input->insert != queue->iface.input->extract && queue->timeout_suspended) {
> -		drm_sched_resume_timeout(&queue->scheduler, queue->remaining_time);
> -		queue->timeout_suspended = false;
> -	}
> +	if (queue->iface.input->insert != queue->iface.input->extract)
> +		queue_resume_timeout(queue);
>  }
>  
>  /**
> @@ -1099,14 +1176,7 @@ cs_slot_reset_locked(struct panthor_device *ptdev, u32 csg_id, u32 cs_id)
>  			       CS_STATE_STOP,
>  			       CS_STATE_MASK);
>  
> -	/* If the queue is blocked, we want to keep the timeout running, so
> -	 * we can detect unbounded waits and kill the group when that happens.
> -	 */
> -	if (!(group->blocked_queues & BIT(cs_id)) && !queue->timeout_suspended) {
> -		queue->remaining_time = drm_sched_suspend_timeout(&queue->scheduler);
> -		queue->timeout_suspended = true;
> -		WARN_ON(queue->remaining_time > msecs_to_jiffies(JOB_TIMEOUT_MS));
> -	}
> +	queue_suspend_timeout(queue);
>  
>  	return 0;
>  }
> @@ -1888,19 +1958,6 @@ tick_ctx_is_full(const struct panthor_scheduler *sched,
>  	return ctx->group_count == sched->csg_slot_count;
>  }
>  
> -static bool
> -group_is_idle(struct panthor_group *group)
> -{
> -	struct panthor_device *ptdev = group->ptdev;
> -	u32 inactive_queues;
> -
> -	if (group->csg_id >= 0)
> -		return ptdev->scheduler->csg_slots[group->csg_id].idle;
> -
> -	inactive_queues = group->idle_queues | group->blocked_queues;
> -	return hweight32(inactive_queues) == group->queue_count;
> -}
> -
>  static bool
>  group_can_run(struct panthor_group *group)
>  {
> @@ -2897,35 +2954,50 @@ void panthor_fdinfo_gather_group_samples(struct panthor_file *pfile)
>  	xa_unlock(&gpool->xa);
>  }
>  
> -static void group_sync_upd_work(struct work_struct *work)
> +static bool queue_check_job_completion(struct panthor_queue *queue)
>  {
> -	struct panthor_group *group =
> -		container_of(work, struct panthor_group, sync_upd_work);
> +	struct panthor_syncobj_64b *syncobj = NULL;
>  	struct panthor_job *job, *job_tmp;
> +	bool cookie, progress = false;
>  	LIST_HEAD(done_jobs);
> -	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];
> -		struct panthor_syncobj_64b *syncobj;
> +	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;
>  
> -		if (!queue)
> -			continue;
> +			syncobj = group->syncobjs->kmap +
> +				  (job->queue_idx * sizeof(*syncobj));
> +		}
>  
> -		syncobj = group->syncobjs->kmap + (queue_idx * sizeof(*syncobj));
> +		if (syncobj->seqno < job->done_fence->seqno)
> +			break;
>  
> -		spin_lock(&queue->fence_ctx.lock);
> -		list_for_each_entry_safe(job, job_tmp, &queue->fence_ctx.in_flight_jobs, node) {
> -			if (syncobj->seqno < job->done_fence->seqno)
> -				break;
> +		list_move_tail(&job->node, &done_jobs);
> +		dma_fence_signal_locked(job->done_fence);
> +	}
>  
> -			list_move_tail(&job->node, &done_jobs);
> -			dma_fence_signal_locked(job->done_fence);
> -		}
> -		spin_unlock(&queue->fence_ctx.lock);
> +	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.
> +		 */
> +		cancel_delayed_work(&queue->timeout.work);
> +		queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
> +
> +		/* 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)) {
> +		mod_delayed_work(queue->scheduler.timeout_wq,
> +				 &queue->timeout.work,
> +				 msecs_to_jiffies(JOB_TIMEOUT_MS));
> +		progress = true;
>  	}
> +	spin_unlock(&queue->fence_ctx.lock);
>  	dma_fence_end_signalling(cookie);
>  
>  	list_for_each_entry_safe(job, job_tmp, &done_jobs, node) {
> @@ -2935,6 +3007,27 @@ static void group_sync_upd_work(struct work_struct *work)
>  		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);
>  }
>  
> @@ -3182,17 +3275,6 @@ queue_run_job(struct drm_sched_job *sched_job)
>  	queue->iface.input->insert = job->ringbuf.end;
>  
>  	if (group->csg_id < 0) {
> -		/* If the queue is blocked, we want to keep the timeout running, so we
> -		 * can detect unbounded waits and kill the group when that happens.
> -		 * Otherwise, we suspend the timeout so the time we spend waiting for
> -		 * a CSG slot is not counted.
> -		 */
> -		if (!(group->blocked_queues & BIT(job->queue_idx)) &&
> -		    !queue->timeout_suspended) {
> -			queue->remaining_time = drm_sched_suspend_timeout(&queue->scheduler);
> -			queue->timeout_suspended = true;
> -		}
> -
>  		group_schedule_locked(group, BIT(job->queue_idx));
>  	} else {
>  		gpu_write(ptdev, CSF_DOORBELL(queue->doorbell_id), 1);
> @@ -3201,6 +3283,7 @@ queue_run_job(struct drm_sched_job *sched_job)
>  			pm_runtime_get(ptdev->base.dev);
>  			sched->pm.has_ref = true;
>  		}
> +		queue_resume_timeout(queue);
>  		panthor_devfreq_record_busy(sched->ptdev);
>  	}
>  
> @@ -3250,6 +3333,11 @@ queue_timedout_job(struct drm_sched_job *sched_job)
>  
>  	queue_start(queue);
>  
> +	/* We already flagged the queue as faulty, make sure we don't get
> +	 * called again.
> +	 */
> +	queue->scheduler.timeout = MAX_SCHEDULE_TIMEOUT;
> +
>  	return DRM_GPU_SCHED_STAT_NOMINAL;
>  }
>  
> @@ -3292,6 +3380,17 @@ static u32 calc_profiling_ringbuf_num_slots(struct panthor_device *ptdev,
>  	return DIV_ROUND_UP(cs_ringbuf_size, min_profiled_job_instrs * sizeof(u64));
>  }
>  
> +static void queue_timeout_work(struct work_struct *work)
> +{
> +	struct panthor_queue *queue = container_of(work, struct panthor_queue,
> +						   timeout.work.work);
> +	bool progress;
> +
> +	progress = queue_check_job_completion(queue);
> +	if (!progress)
> +		drm_sched_fault(&queue->scheduler);
> +}
> +
>  static struct panthor_queue *
>  group_create_queue(struct panthor_group *group,
>  		   const struct drm_panthor_queue_create *args)
> @@ -3307,7 +3406,7 @@ group_create_queue(struct panthor_group *group,
>  		 * their profiling status.
>  		 */
>  		.credit_limit = args->ringbuf_size / sizeof(u64),
> -		.timeout = msecs_to_jiffies(JOB_TIMEOUT_MS),
> +		.timeout = MAX_SCHEDULE_TIMEOUT,
>  		.timeout_wq = group->ptdev->reset.wq,
>  		.name = "panthor-queue",
>  		.dev = group->ptdev->base.dev,
> @@ -3330,6 +3429,8 @@ group_create_queue(struct panthor_group *group,
>  	if (!queue)
>  		return ERR_PTR(-ENOMEM);
>  
> +	queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
> +	INIT_DELAYED_WORK(&queue->timeout.work, queue_timeout_work);
>  	queue->fence_ctx.id = dma_fence_context_alloc(1);
>  	spin_lock_init(&queue->fence_ctx.lock);
>  	INIT_LIST_HEAD(&queue->fence_ctx.in_flight_jobs);
> -- 
> 2.43.0
> 

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

Re: [PATCH v5 1/2] drm/panthor: Reset queue slots if termination fails

[Steven Price]

On 03/06/2025 10:49, Ashley Smith wrote:
> This fixes a bug where if we timeout after a suspend and the termination
> fails, due to waiting on a fence that will never be signalled for
> example, we do not resume the group correctly. The fix forces a reset
> for groups that are not terminated correctly.
> 
> Signed-off-by: Ashley Smith <ashley.smith@collabora.com>
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 43ee57728de5..65d8ae3dcac1 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2727,8 +2727,17 @@ void panthor_sched_suspend(struct panthor_device *ptdev)
>  			 * automatically terminate all active groups, so let's
>  			 * force the state to halted here.
>  			 */
> -			if (csg_slot->group->state != PANTHOR_CS_GROUP_TERMINATED)
> +			if (csg_slot->group->state != PANTHOR_CS_GROUP_TERMINATED) {
>  				csg_slot->group->state = PANTHOR_CS_GROUP_TERMINATED;
> +
> +				/* Reset the queue slots manually if the termination
> +				 * request failed.
> +				 */
> +				for (i = 0; i < group->queue_count; i++) {
> +					if (group->queues[i])
> +						cs_slot_reset_locked(ptdev, csg_id, i);
> +				}
> +			}
>  			slot_mask &= ~BIT(csg_id);
>  		}
>  	}

Re: [PATCH v5 2/2] drm/panthor: Make the timeout per-queue instead of per-job

[Steven Price]

On 03/06/2025 10:49, Ashley Smith wrote:
> The timeout logic provided by drm_sched leads to races when we try
> to suspend it while the drm_sched workqueue queues more jobs. Let's
> overhaul the timeout handling in panthor to have our own delayed work
> that's resumed/suspended when a group is resumed/suspended. When an
> actual timeout occurs, we call drm_sched_fault() to report it
> through drm_sched, still. But otherwise, the drm_sched timeout is
> disabled (set to MAX_SCHEDULE_TIMEOUT), which leaves us in control of
> how we protect modifications on the timer.
> 
> One issue seems to be when we call drm_sched_suspend_timeout() from
> both queue_run_job() and tick_work() which could lead to races due to
> drm_sched_suspend_timeout() not having a lock. Another issue seems to
> be in queue_run_job() if the group is not scheduled, we suspend the
> timeout again which undoes what drm_sched_job_begin() did when calling
> drm_sched_start_timeout(). So the timeout does not reset when a job
> is finished.
> 
> Co-developed-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> ---
> Signed-off-by: Ashley Smith <ashley.smith@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 233 +++++++++++++++++-------
>  1 file changed, 167 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 65d8ae3dcac1..fb5a66ca5384 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -360,17 +360,20 @@ struct panthor_queue {
>  	/** @entity: DRM scheduling entity used for this queue. */
>  	struct drm_sched_entity entity;
>  
> -	/**
> -	 * @remaining_time: Time remaining before the job timeout expires.
> -	 *
> -	 * The job timeout is suspended when the queue is not scheduled by the
> -	 * FW. Every time we suspend the timer, we need to save the remaining
> -	 * time so we can restore it later on.
> -	 */
> -	unsigned long remaining_time;
> +	/** @timeout: Queue timeout related fields. */
> +	struct {
> +		/** @timeout.work: Work executed when a queue timeout occurs. */
> +		struct delayed_work work;
>  
> -	/** @timeout_suspended: True if the job timeout was suspended. */
> -	bool timeout_suspended;
> +		/**
> +		 * @timeout.remaining: Time remaining before a queue timeout.
> +		 *
> +		 * When the timer is running, this value is set to MAX_SCHEDULE_TIMEOUT.
> +		 * When the timer is suspended, it's set to the time remaining when the
> +		 * timer was suspended.
> +		 */
> +		unsigned long remaining;
> +	} timeout;
>  
>  	/**
>  	 * @doorbell_id: Doorbell assigned to this queue.
> @@ -1031,6 +1034,82 @@ group_unbind_locked(struct panthor_group *group)
>  	return 0;
>  }
>  
> +static bool
> +group_is_idle(struct panthor_group *group)
> +{
> +	struct panthor_device *ptdev = group->ptdev;
> +	u32 inactive_queues;
> +
> +	if (group->csg_id >= 0)
> +		return ptdev->scheduler->csg_slots[group->csg_id].idle;
> +
> +	inactive_queues = group->idle_queues | group->blocked_queues;
> +	return hweight32(inactive_queues) == group->queue_count;
> +}
> +
> +static void
> +queue_suspend_timeout(struct panthor_queue *queue)
> +{
> +	unsigned long qtimeout, now;
> +	struct panthor_group *group;
> +	struct panthor_job *job;
> +	bool timer_was_active;
> +
> +	spin_lock(&queue->fence_ctx.lock);
> +
> +	/* Already suspended, nothing to do. */
> +	if (queue->timeout.remaining != MAX_SCHEDULE_TIMEOUT)
> +		goto out_unlock;
> +
> +	job = list_first_entry_or_null(&queue->fence_ctx.in_flight_jobs,
> +				       struct panthor_job, node);
> +	group = job ? job->group : NULL;
> +
> +	/* If the queue is blocked and the group is idle, we want the timer to
> +	 * keep running because the group can't be unblocked by other queues,
> +	 * so it has to come from an external source, and we want to timebox
> +	 * this external signalling.
> +	 */
> +	if (group && (group->blocked_queues & BIT(job->queue_idx)) &&
> +	    group_is_idle(group))
> +		goto out_unlock;
> +
> +	now = jiffies;
> +	qtimeout = queue->timeout.work.timer.expires;
> +
> +	/* Cancel the timer. */
> +	timer_was_active = cancel_delayed_work(&queue->timeout.work);
> +	if (!timer_was_active || !job)
> +		queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
> +	else if (time_after(qtimeout, now))
> +		queue->timeout.remaining = qtimeout - now;
> +	else
> +		queue->timeout.remaining = 0;
> +
> +	if (WARN_ON_ONCE(queue->timeout.remaining > msecs_to_jiffies(JOB_TIMEOUT_MS)))
> +		queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
> +
> +out_unlock:
> +	spin_unlock(&queue->fence_ctx.lock);
> +}
> +
> +static void
> +queue_resume_timeout(struct panthor_queue *queue)
> +{
> +	spin_lock(&queue->fence_ctx.lock);
> +
> +	/* When running, the remaining time is set to MAX_SCHEDULE_TIMEOUT. */
> +	if (queue->timeout.remaining != MAX_SCHEDULE_TIMEOUT) {
> +		mod_delayed_work(queue->scheduler.timeout_wq,
> +				 &queue->timeout.work,
> +				 queue->timeout.remaining);
> +
> +		queue->timeout.remaining = MAX_SCHEDULE_TIMEOUT;
> +	}
> +
> +	spin_unlock(&queue->fence_ctx.lock);
> +}
> +
>  /**
>   * cs_slot_prog_locked() - Program a queue slot
>   * @ptdev: Device.
> @@ -1069,10 +1148,8 @@ cs_slot_prog_locked(struct panthor_device *ptdev, u32 csg_id, u32 cs_id)
>  			       CS_IDLE_EMPTY |
>  			       CS_STATE_MASK |
>  			       CS_EXTRACT_EVENT);
> -	if (queue->iface.input->insert != queue->iface.input->extract && queue->timeout_suspended) {
> -		drm_sched_resume_timeout(&queue->scheduler, queue->remaining_time);
> -		queue->timeout_suspended = false;
> -	}
> +	if (queue->iface.input->insert != queue->iface.input->extract)
> +		queue_resume_timeout(queue);
>  }
>  
>  /**
> @@ -1099,14 +1176,7 @@ cs_slot_reset_locked(struct panthor_device *ptdev, u32 csg_id, u32 cs_id)
>  			       CS_STATE_STOP,
>  			       CS_STATE_MASK);
>  
> -	/* If the queue is blocked, we want to keep the timeout running, so
> -	 * we can detect unbounded waits and kill the group when that happens.
> -	 */
> -	if (!(group->blocked_queues & BIT(cs_id)) && !queue->timeout_suspended) {
> -		queue->remaining_time = drm_sched_suspend_timeout(&queue->scheduler);
> -		queue->timeout_suspended = true;
> -		WARN_ON(queue->remaining_time > msecs_to_jiffies(JOB_TIMEOUT_MS));
> -	}
> +	queue_suspend_timeout(queue);
>  
>  	return 0;
>  }
> @@ -1888,19 +1958,6 @@ tick_ctx_is_full(const struct panthor_scheduler *sched,
>  	return ctx->group_count == sched->csg_slot_count;
>  }
>  
> -static bool
> -group_is_idle(struct panthor_group *group)
> -{
> -	struct panthor_device *ptdev = group->ptdev;
> -	u32 inactive_queues;
> -
> -	if (group->csg_id >= 0)
> -		return ptdev->scheduler->csg_slots[group->csg_id].idle;
> -
> -	inactive_queues = group->idle_queues | group->blocked_queues;
> -	return hweight32(inactive_queues) == group->queue_count;
> -}
> -
>  static bool
>  group_can_run(struct panthor_group *group)
>  {
> @@ -2897,35 +2954,50 @@ void panthor_fdinfo_gather_group_samples(struct panthor_file *pfile)
>  	xa_unlock(&gpool->xa);
>  }
>  
> -static void group_sync_upd_work(struct work_struct *work)
> +static bool queue_check_job_completion(struct panthor_queue *queue)
>  {
> -	struct panthor_group *group =
> -		container_of(work, struct panthor_group, sync_upd_work);
> +	struct panthor_syncobj_64b *syncobj = NULL;
>  	struct panthor_job *job, *job_tmp;
> +	bool cookie, progress = false;
>  	LIST_HEAD(done_jobs);
> -	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];
> -		struct panthor_syncobj_64b *syncobj;
> +	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;
>  
> -		if (!queue)
> -			continue;
> +			syncobj = group->syncobjs->kmap +
> +				  (job->queue_idx * sizeof(*syncobj));
> +		}
>  
> -		syncobj = group->syncobjs->kmap + (queue_idx * sizeof(*syncobj));
> +		if (syncobj->seqno < job->done_fence->seqno)
> +			break;
>  
> -		spin_lock(&queue->fence_ctx.lock);
> -		list_for_each_entry_safe(job, job_tmp, &queue->fence_ctx.in_flight_jobs, node) {
> -			if (syncobj->seqno < job->done_fence->seqno)
> -				break;
> +		list_move_tail(&job->node, &done_jobs);
> +		dma_fence_signal_locked(job->done_fence);
> +	}
>  
> -			list_move_tail(&job->node, &done_jobs);
> -			dma_fence_signal_locked(job->done_fence);
> -		}
> -		spin_unlock(&queue->fence_ctx.lock);
> +	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.
> +		 */
> +		cancel_delayed_work(&queue->timeout.work);
> +		queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
> +
> +		/* 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)) {
> +		mod_delayed_work(queue->scheduler.timeout_wq,
> +				 &queue->timeout.work,
> +				 msecs_to_jiffies(JOB_TIMEOUT_MS));
> +		progress = true;
>  	}
> +	spin_unlock(&queue->fence_ctx.lock);
>  	dma_fence_end_signalling(cookie);
>  
>  	list_for_each_entry_safe(job, job_tmp, &done_jobs, node) {
> @@ -2935,6 +3007,27 @@ static void group_sync_upd_work(struct work_struct *work)
>  		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);
>  }
>  
> @@ -3182,17 +3275,6 @@ queue_run_job(struct drm_sched_job *sched_job)
>  	queue->iface.input->insert = job->ringbuf.end;
>  
>  	if (group->csg_id < 0) {
> -		/* If the queue is blocked, we want to keep the timeout running, so we
> -		 * can detect unbounded waits and kill the group when that happens.
> -		 * Otherwise, we suspend the timeout so the time we spend waiting for
> -		 * a CSG slot is not counted.
> -		 */
> -		if (!(group->blocked_queues & BIT(job->queue_idx)) &&
> -		    !queue->timeout_suspended) {
> -			queue->remaining_time = drm_sched_suspend_timeout(&queue->scheduler);
> -			queue->timeout_suspended = true;
> -		}
> -
>  		group_schedule_locked(group, BIT(job->queue_idx));
>  	} else {
>  		gpu_write(ptdev, CSF_DOORBELL(queue->doorbell_id), 1);
> @@ -3201,6 +3283,7 @@ queue_run_job(struct drm_sched_job *sched_job)
>  			pm_runtime_get(ptdev->base.dev);
>  			sched->pm.has_ref = true;
>  		}
> +		queue_resume_timeout(queue);
>  		panthor_devfreq_record_busy(sched->ptdev);
>  	}
>  
> @@ -3250,6 +3333,11 @@ queue_timedout_job(struct drm_sched_job *sched_job)
>  
>  	queue_start(queue);
>  
> +	/* We already flagged the queue as faulty, make sure we don't get
> +	 * called again.
> +	 */
> +	queue->scheduler.timeout = MAX_SCHEDULE_TIMEOUT;
> +
>  	return DRM_GPU_SCHED_STAT_NOMINAL;
>  }
>  
> @@ -3292,6 +3380,17 @@ static u32 calc_profiling_ringbuf_num_slots(struct panthor_device *ptdev,
>  	return DIV_ROUND_UP(cs_ringbuf_size, min_profiled_job_instrs * sizeof(u64));
>  }
>  
> +static void queue_timeout_work(struct work_struct *work)
> +{
> +	struct panthor_queue *queue = container_of(work, struct panthor_queue,
> +						   timeout.work.work);
> +	bool progress;
> +
> +	progress = queue_check_job_completion(queue);
> +	if (!progress)
> +		drm_sched_fault(&queue->scheduler);
> +}
> +
>  static struct panthor_queue *
>  group_create_queue(struct panthor_group *group,
>  		   const struct drm_panthor_queue_create *args)
> @@ -3307,7 +3406,7 @@ group_create_queue(struct panthor_group *group,
>  		 * their profiling status.
>  		 */
>  		.credit_limit = args->ringbuf_size / sizeof(u64),
> -		.timeout = msecs_to_jiffies(JOB_TIMEOUT_MS),
> +		.timeout = MAX_SCHEDULE_TIMEOUT,
>  		.timeout_wq = group->ptdev->reset.wq,
>  		.name = "panthor-queue",
>  		.dev = group->ptdev->base.dev,
> @@ -3330,6 +3429,8 @@ group_create_queue(struct panthor_group *group,
>  	if (!queue)
>  		return ERR_PTR(-ENOMEM);
>  
> +	queue->timeout.remaining = msecs_to_jiffies(JOB_TIMEOUT_MS);
> +	INIT_DELAYED_WORK(&queue->timeout.work, queue_timeout_work);
>  	queue->fence_ctx.id = dma_fence_context_alloc(1);
>  	spin_lock_init(&queue->fence_ctx.lock);
>  	INIT_LIST_HEAD(&queue->fence_ctx.in_flight_jobs);

Re: [PATCH v5 1/2] drm/panthor: Reset queue slots if termination fails

[Boris Brezillon]

On Tue,  3 Jun 2025 10:49:31 +0100
Ashley Smith <ashley.smith@collabora.com> wrote:

> This fixes a bug where if we timeout after a suspend and the termination
> fails, due to waiting on a fence that will never be signalled for
> example, we do not resume the group correctly. The fix forces a reset
> for groups that are not terminated correctly.
> 
> Signed-off-by: Ashley Smith <ashley.smith@collabora.com>
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 43ee57728de5..65d8ae3dcac1 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2727,8 +2727,17 @@ void panthor_sched_suspend(struct panthor_device *ptdev)
>  			 * automatically terminate all active groups, so let's
>  			 * force the state to halted here.
>  			 */
> -			if (csg_slot->group->state != PANTHOR_CS_GROUP_TERMINATED)
> +			if (csg_slot->group->state != PANTHOR_CS_GROUP_TERMINATED) {
>  				csg_slot->group->state = PANTHOR_CS_GROUP_TERMINATED;
> +
> +				/* Reset the queue slots manually if the termination
> +				 * request failed.
> +				 */
> +				for (i = 0; i < group->queue_count; i++) {

group is used uninitialized which leads to a random (most likely NULL)
pointer deref. Either we go:

				for (i = 0; i < csg_slot->group->queue_count; i++) {

and we move the group variable to the last for loop, so we're not
tempted to use it again in places where it's not initialized, or
we use the group variable consistently accross this function by having

		group = csg_slot->group;

assignments where csg_slot->group is currently used.

We might also want to consider splitting this huge function in
sub-functions, but probably not in a patch that's flagged for
backporting.


> +					if (group->queues[i])
> +						cs_slot_reset_locked(ptdev, csg_id, i);
> +				}
> +			}
>  			slot_mask &= ~BIT(csg_id);
>  		}
>  	}