[PATCH v1 0/8] drm/panthor: Misc scheduler fixes

[PATCH v1 0/8] drm/panthor: Misc scheduler fixes

[Boris Brezillon]

Hello,

This series is a collection of fixes that seem to address the problem
reported here [1]. In order to validate those changes, I added a few
IGT tests [2], but I'd like to extend the test coverage before claiming
this is working properly. Until I get to it, I thought I'd post what
I have for preliminary review/testing.

Regards,

Boris

[1]https://gitlab.freedesktop.org/panfrost/linux/-/issues/48
[2]https://gitlab.freedesktop.org/bbrezillon/igt-gpu-tools/-/tree/panthor-sched?ref_type=heads

Boris Brezillon (8):
  drm/panthor: Simplify group idleness tracking
  drm/panthor: Don't try to enable extract events
  drm/panthor: Fix the group priority rotation logic
  drm/panthor: Fix the full_tick check
  drm/panthor: Fix immediate ticking on a disabled tick
  drm/panthor: Fix the logic that decides when to stop ticking
  drm/panthor: Make sure we resume the tick when new jobs are submitted
  drm/panthor: Kill panthor_sched_immediate_tick()

 drivers/gpu/drm/panthor/panthor_sched.c | 186 ++++++++++++------------
 1 file changed, 95 insertions(+), 91 deletions(-)

-- 
2.51.1

[PATCH v1 1/8] drm/panthor: Simplify group idleness tracking

[Boris Brezillon]

csg_slot_sync_queues_state_locked() queries the queues state which can
then be used to determine if a group is idle or not. Let's base our
idleness detection logic solely on the {idle,blocked}_queues masks to
avoid inconsistencies between the group state and the state of its
subqueues.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 31 ++-----------------------
 1 file changed, 2 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index e74ca071159d..9931f4a6cd96 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -108,15 +108,6 @@ struct panthor_csg_slot {
 
 	/** @priority: Group priority. */
 	u8 priority;
-
-	/**
-	 * @idle: True if the group bound to this slot is idle.
-	 *
-	 * A group is idle when it has nothing waiting for execution on
-	 * all its queues, or when queues are blocked waiting for something
-	 * to happen (synchronization object).
-	 */
-	bool idle;
 };
 
 /**
@@ -1607,17 +1598,6 @@ static bool cs_slot_process_irq_locked(struct panthor_device *ptdev,
 	return (events & (CS_FAULT | CS_TILER_OOM)) != 0;
 }
 
-static void csg_slot_sync_idle_state_locked(struct panthor_device *ptdev, u32 csg_id)
-{
-	struct panthor_csg_slot *csg_slot = &ptdev->scheduler->csg_slots[csg_id];
-	struct panthor_fw_csg_iface *csg_iface;
-
-	lockdep_assert_held(&ptdev->scheduler->lock);
-
-	csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
-	csg_slot->idle = csg_iface->output->status_state & CSG_STATUS_STATE_IS_IDLE;
-}
-
 static void csg_slot_process_idle_event_locked(struct panthor_device *ptdev, u32 csg_id)
 {
 	struct panthor_scheduler *sched = ptdev->scheduler;
@@ -1879,10 +1859,8 @@ static int csgs_upd_ctx_apply_locked(struct panthor_device *ptdev,
 		if (acked & CSG_STATE_MASK)
 			csg_slot_sync_state_locked(ptdev, csg_id);
 
-		if (acked & CSG_STATUS_UPDATE) {
+		if (acked & CSG_STATUS_UPDATE)
 			csg_slot_sync_queues_state_locked(ptdev, csg_id);
-			csg_slot_sync_idle_state_locked(ptdev, csg_id);
-		}
 
 		if (ret && acked != req_mask &&
 		    ((csg_iface->input->req ^ csg_iface->output->ack) & req_mask) != 0) {
@@ -1919,13 +1897,8 @@ tick_ctx_is_full(const struct panthor_scheduler *sched,
 static bool
 group_is_idle(struct panthor_group *group)
 {
-	struct panthor_device *ptdev = group->ptdev;
-	u32 inactive_queues;
+	u32 inactive_queues = group->idle_queues | group->blocked_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;
 }
 
-- 
2.51.1

Re: [PATCH v1 1/8] drm/panthor: Simplify group idleness tracking

[Steven Price]

On 06/11/2025 14:46, Boris Brezillon wrote:
> csg_slot_sync_queues_state_locked() queries the queues state which can
> then be used to determine if a group is idle or not. Let's base our
> idleness detection logic solely on the {idle,blocked}_queues masks to
> avoid inconsistencies between the group state and the state of its
> subqueues.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Seems reasonable to me.

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

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 31 ++-----------------------
>  1 file changed, 2 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index e74ca071159d..9931f4a6cd96 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -108,15 +108,6 @@ struct panthor_csg_slot {
>  
>  	/** @priority: Group priority. */
>  	u8 priority;
> -
> -	/**
> -	 * @idle: True if the group bound to this slot is idle.
> -	 *
> -	 * A group is idle when it has nothing waiting for execution on
> -	 * all its queues, or when queues are blocked waiting for something
> -	 * to happen (synchronization object).
> -	 */
> -	bool idle;
>  };
>  
>  /**
> @@ -1607,17 +1598,6 @@ static bool cs_slot_process_irq_locked(struct panthor_device *ptdev,
>  	return (events & (CS_FAULT | CS_TILER_OOM)) != 0;
>  }
>  
> -static void csg_slot_sync_idle_state_locked(struct panthor_device *ptdev, u32 csg_id)
> -{
> -	struct panthor_csg_slot *csg_slot = &ptdev->scheduler->csg_slots[csg_id];
> -	struct panthor_fw_csg_iface *csg_iface;
> -
> -	lockdep_assert_held(&ptdev->scheduler->lock);
> -
> -	csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> -	csg_slot->idle = csg_iface->output->status_state & CSG_STATUS_STATE_IS_IDLE;
> -}
> -
>  static void csg_slot_process_idle_event_locked(struct panthor_device *ptdev, u32 csg_id)
>  {
>  	struct panthor_scheduler *sched = ptdev->scheduler;
> @@ -1879,10 +1859,8 @@ static int csgs_upd_ctx_apply_locked(struct panthor_device *ptdev,
>  		if (acked & CSG_STATE_MASK)
>  			csg_slot_sync_state_locked(ptdev, csg_id);
>  
> -		if (acked & CSG_STATUS_UPDATE) {
> +		if (acked & CSG_STATUS_UPDATE)
>  			csg_slot_sync_queues_state_locked(ptdev, csg_id);
> -			csg_slot_sync_idle_state_locked(ptdev, csg_id);
> -		}
>  
>  		if (ret && acked != req_mask &&
>  		    ((csg_iface->input->req ^ csg_iface->output->ack) & req_mask) != 0) {
> @@ -1919,13 +1897,8 @@ tick_ctx_is_full(const struct panthor_scheduler *sched,
>  static bool
>  group_is_idle(struct panthor_group *group)
>  {
> -	struct panthor_device *ptdev = group->ptdev;
> -	u32 inactive_queues;
> +	u32 inactive_queues = group->idle_queues | group->blocked_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;
>  }
>  

[PATCH v1 2/8] drm/panthor: Don't try to enable extract events

[Boris Brezillon]

Not only this only works once, because of how extract events work
(event is enabled if the req and ack bit differ, and it's signalled
by the FW by setting identical req and ack, to re-enable the event,
we need to toggle the bit, which we never do). But more importantly,
we never do anything with this event, so we're better off dropping it
when programming the CS slot.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 9931f4a6cd96..94514d464e64 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -1069,12 +1069,10 @@ cs_slot_prog_locked(struct panthor_device *ptdev, u32 csg_id, u32 cs_id)
 	panthor_fw_update_reqs(cs_iface, req,
 			       CS_IDLE_SYNC_WAIT |
 			       CS_IDLE_EMPTY |
-			       CS_STATE_START |
-			       CS_EXTRACT_EVENT,
+			       CS_STATE_START,
 			       CS_IDLE_SYNC_WAIT |
 			       CS_IDLE_EMPTY |
-			       CS_STATE_MASK |
-			       CS_EXTRACT_EVENT);
+			       CS_STATE_MASK);
 	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;
-- 
2.51.1

Re: [PATCH v1 2/8] drm/panthor: Don't try to enable extract events

[Steven Price]

On 06/11/2025 14:46, Boris Brezillon wrote:
> Not only this only works once, because of how extract events work
> (event is enabled if the req and ack bit differ, and it's signalled
> by the FW by setting identical req and ack, to re-enable the event,
> we need to toggle the bit, which we never do). But more importantly,
> we never do anything with this event, so we're better off dropping it
> when programming the CS slot.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

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

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 9931f4a6cd96..94514d464e64 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1069,12 +1069,10 @@ cs_slot_prog_locked(struct panthor_device *ptdev, u32 csg_id, u32 cs_id)
>  	panthor_fw_update_reqs(cs_iface, req,
>  			       CS_IDLE_SYNC_WAIT |
>  			       CS_IDLE_EMPTY |
> -			       CS_STATE_START |
> -			       CS_EXTRACT_EVENT,
> +			       CS_STATE_START,
>  			       CS_IDLE_SYNC_WAIT |
>  			       CS_IDLE_EMPTY |
> -			       CS_STATE_MASK |
> -			       CS_EXTRACT_EVENT);
> +			       CS_STATE_MASK);
>  	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;

[PATCH v1 3/8] drm/panthor: Fix the group priority rotation logic

[Boris Brezillon]

It's the group item that's supposed to be inserted before other_group,
not the other way around.

Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 46 +++++++++++++++----------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 94514d464e64..69cc1b4c23f2 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -1960,31 +1960,22 @@ tick_ctx_pick_groups_from_list(const struct panthor_scheduler *sched,
 static void
 tick_ctx_insert_old_group(struct panthor_scheduler *sched,
 			  struct panthor_sched_tick_ctx *ctx,
-			  struct panthor_group *group,
-			  bool full_tick)
+			  struct panthor_group *group)
 {
 	struct panthor_csg_slot *csg_slot = &sched->csg_slots[group->csg_id];
 	struct panthor_group *other_group;
 
-	if (!full_tick) {
-		list_add_tail(&group->run_node, &ctx->old_groups[group->priority]);
-		return;
-	}
-
-	/* Rotate to make sure groups with lower CSG slot
-	 * priorities have a chance to get a higher CSG slot
-	 * priority next time they get picked. This priority
-	 * has an impact on resource request ordering, so it's
-	 * important to make sure we don't let one group starve
-	 * all other groups with the same group priority.
-	 */
+	/* Class groups in descending priority order so we can easily rotate. */
 	list_for_each_entry(other_group,
 			    &ctx->old_groups[csg_slot->group->priority],
 			    run_node) {
 		struct panthor_csg_slot *other_csg_slot = &sched->csg_slots[other_group->csg_id];
 
-		if (other_csg_slot->priority > csg_slot->priority) {
-			list_add_tail(&csg_slot->group->run_node, &other_group->run_node);
+		/* Our group has a higher prio than the one we're testing against,
+		 * place it just before.
+		 */
+		if (csg_slot->priority > other_csg_slot->priority) {
+			list_add_tail(&group->run_node, &other_group->run_node);
 			return;
 		}
 	}
@@ -2033,7 +2024,7 @@ tick_ctx_init(struct panthor_scheduler *sched,
 				group->fatal_queues |= GENMASK(group->queue_count - 1, 0);
 		}
 
-		tick_ctx_insert_old_group(sched, ctx, group, full_tick);
+		tick_ctx_insert_old_group(sched, ctx, group);
 		csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, i,
 					csg_iface->output->ack ^ CSG_STATUS_UPDATE,
 					CSG_STATUS_UPDATE);
@@ -2399,9 +2390,28 @@ static void tick_work(struct work_struct *work)
 	for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1;
 	     prio >= 0 && !tick_ctx_is_full(sched, &ctx);
 	     prio--) {
+		struct panthor_group *old_highest_prio_group =
+			list_first_entry_or_null(&ctx.old_groups[prio],
+						 struct panthor_group, run_node);
+
+		/* Pull out the group with the highest prio for rotation. */
+		if (old_highest_prio_group)
+			list_del(&old_highest_prio_group->run_node);
+
+		/* Re-insert old active groups so they get a chance to run with higher prio. */
+		tick_ctx_pick_groups_from_list(sched, &ctx, &ctx.old_groups[prio], true, true);
+
+		/* Fill the remaining slots with runnable groups. */
 		tick_ctx_pick_groups_from_list(sched, &ctx, &sched->groups.runnable[prio],
 					       true, false);
-		tick_ctx_pick_groups_from_list(sched, &ctx, &ctx.old_groups[prio], true, true);
+
+		/* Re-insert the old group with the highest prio, and give it a chance to be
+		 * scheduled again (but with a lower prio) if there's room left.
+		 */
+		if (old_highest_prio_group) {
+			list_add_tail(&old_highest_prio_group->run_node, &ctx.old_groups[prio]);
+			tick_ctx_pick_groups_from_list(sched, &ctx, &ctx.old_groups[prio], true, true);
+		}
 	}
 
 	/* If we have free CSG slots left, pick idle groups */
-- 
2.51.1

Re: [PATCH v1 3/8] drm/panthor: Fix the group priority rotation logic

[Steven Price]

On 06/11/2025 14:46, Boris Brezillon wrote:
> It's the group item that's supposed to be inserted before other_group,
> not the other way around.
> 
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Looks correct to me (but then I think I thought that about the old
code... ;) ).

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

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 46 +++++++++++++++----------
>  1 file changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 94514d464e64..69cc1b4c23f2 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1960,31 +1960,22 @@ tick_ctx_pick_groups_from_list(const struct panthor_scheduler *sched,
>  static void
>  tick_ctx_insert_old_group(struct panthor_scheduler *sched,
>  			  struct panthor_sched_tick_ctx *ctx,
> -			  struct panthor_group *group,
> -			  bool full_tick)
> +			  struct panthor_group *group)
>  {
>  	struct panthor_csg_slot *csg_slot = &sched->csg_slots[group->csg_id];
>  	struct panthor_group *other_group;
>  
> -	if (!full_tick) {
> -		list_add_tail(&group->run_node, &ctx->old_groups[group->priority]);
> -		return;
> -	}
> -
> -	/* Rotate to make sure groups with lower CSG slot
> -	 * priorities have a chance to get a higher CSG slot
> -	 * priority next time they get picked. This priority
> -	 * has an impact on resource request ordering, so it's
> -	 * important to make sure we don't let one group starve
> -	 * all other groups with the same group priority.
> -	 */
> +	/* Class groups in descending priority order so we can easily rotate. */
>  	list_for_each_entry(other_group,
>  			    &ctx->old_groups[csg_slot->group->priority],
>  			    run_node) {
>  		struct panthor_csg_slot *other_csg_slot = &sched->csg_slots[other_group->csg_id];
>  
> -		if (other_csg_slot->priority > csg_slot->priority) {
> -			list_add_tail(&csg_slot->group->run_node, &other_group->run_node);
> +		/* Our group has a higher prio than the one we're testing against,
> +		 * place it just before.
> +		 */
> +		if (csg_slot->priority > other_csg_slot->priority) {
> +			list_add_tail(&group->run_node, &other_group->run_node);
>  			return;
>  		}
>  	}
> @@ -2033,7 +2024,7 @@ tick_ctx_init(struct panthor_scheduler *sched,
>  				group->fatal_queues |= GENMASK(group->queue_count - 1, 0);
>  		}
>  
> -		tick_ctx_insert_old_group(sched, ctx, group, full_tick);
> +		tick_ctx_insert_old_group(sched, ctx, group);
>  		csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, i,
>  					csg_iface->output->ack ^ CSG_STATUS_UPDATE,
>  					CSG_STATUS_UPDATE);
> @@ -2399,9 +2390,28 @@ static void tick_work(struct work_struct *work)
>  	for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1;
>  	     prio >= 0 && !tick_ctx_is_full(sched, &ctx);
>  	     prio--) {
> +		struct panthor_group *old_highest_prio_group =
> +			list_first_entry_or_null(&ctx.old_groups[prio],
> +						 struct panthor_group, run_node);
> +
> +		/* Pull out the group with the highest prio for rotation. */
> +		if (old_highest_prio_group)
> +			list_del(&old_highest_prio_group->run_node);
> +
> +		/* Re-insert old active groups so they get a chance to run with higher prio. */
> +		tick_ctx_pick_groups_from_list(sched, &ctx, &ctx.old_groups[prio], true, true);
> +
> +		/* Fill the remaining slots with runnable groups. */
>  		tick_ctx_pick_groups_from_list(sched, &ctx, &sched->groups.runnable[prio],
>  					       true, false);
> -		tick_ctx_pick_groups_from_list(sched, &ctx, &ctx.old_groups[prio], true, true);
> +
> +		/* Re-insert the old group with the highest prio, and give it a chance to be
> +		 * scheduled again (but with a lower prio) if there's room left.
> +		 */
> +		if (old_highest_prio_group) {
> +			list_add_tail(&old_highest_prio_group->run_node, &ctx.old_groups[prio]);
> +			tick_ctx_pick_groups_from_list(sched, &ctx, &ctx.old_groups[prio], true, true);
> +		}
>  	}
>  
>  	/* If we have free CSG slots left, pick idle groups */

Re: [PATCH v1 3/8] drm/panthor: Fix the group priority rotation logic

[Boris Brezillon]

On Thu, 6 Nov 2025 16:18:13 +0000
Steven Price <steven.price@arm.com> wrote:

> On 06/11/2025 14:46, Boris Brezillon wrote:
> > It's the group item that's supposed to be inserted before other_group,
> > not the other way around.
> > 
> > Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> Looks correct to me (but then I think I thought that about the old
> code... ;) ).

Actually, I realize I forgot to update the commit message, because
there's way more than the insertion order in the list that's
problematic.

> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> > ---
> >  drivers/gpu/drm/panthor/panthor_sched.c | 46 +++++++++++++++----------
> >  1 file changed, 28 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> > index 94514d464e64..69cc1b4c23f2 100644
> > --- a/drivers/gpu/drm/panthor/panthor_sched.c
> > +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> > @@ -1960,31 +1960,22 @@ tick_ctx_pick_groups_from_list(const struct panthor_scheduler *sched,
> >  static void
> >  tick_ctx_insert_old_group(struct panthor_scheduler *sched,
> >  			  struct panthor_sched_tick_ctx *ctx,
> > -			  struct panthor_group *group,
> > -			  bool full_tick)
> > +			  struct panthor_group *group)
> >  {
> >  	struct panthor_csg_slot *csg_slot = &sched->csg_slots[group->csg_id];
> >  	struct panthor_group *other_group;
> >  
> > -	if (!full_tick) {
> > -		list_add_tail(&group->run_node, &ctx->old_groups[group->priority]);
> > -		return;
> > -	}
> > -
> > -	/* Rotate to make sure groups with lower CSG slot
> > -	 * priorities have a chance to get a higher CSG slot
> > -	 * priority next time they get picked. This priority
> > -	 * has an impact on resource request ordering, so it's
> > -	 * important to make sure we don't let one group starve
> > -	 * all other groups with the same group priority.
> > -	 */
> > +	/* Class groups in descending priority order so we can easily rotate. */
> >  	list_for_each_entry(other_group,
> >  			    &ctx->old_groups[csg_slot->group->priority],
> >  			    run_node) {
> >  		struct panthor_csg_slot *other_csg_slot = &sched->csg_slots[other_group->csg_id];
> >  
> > -		if (other_csg_slot->priority > csg_slot->priority) {
> > -			list_add_tail(&csg_slot->group->run_node, &other_group->run_node);
> > +		/* Our group has a higher prio than the one we're testing against,
> > +		 * place it just before.
> > +		 */
> > +		if (csg_slot->priority > other_csg_slot->priority) {
> > +			list_add_tail(&group->run_node, &other_group->run_node);
> >  			return;
> >  		}
> >  	}
> > @@ -2033,7 +2024,7 @@ tick_ctx_init(struct panthor_scheduler *sched,
> >  				group->fatal_queues |= GENMASK(group->queue_count - 1, 0);
> >  		}
> >  
> > -		tick_ctx_insert_old_group(sched, ctx, group, full_tick);
> > +		tick_ctx_insert_old_group(sched, ctx, group);
> >  		csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, i,
> >  					csg_iface->output->ack ^ CSG_STATUS_UPDATE,
> >  					CSG_STATUS_UPDATE);
> > @@ -2399,9 +2390,28 @@ static void tick_work(struct work_struct *work)
> >  	for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1;
> >  	     prio >= 0 && !tick_ctx_is_full(sched, &ctx);
> >  	     prio--) {
> > +		struct panthor_group *old_highest_prio_group =
> > +			list_first_entry_or_null(&ctx.old_groups[prio],
> > +						 struct panthor_group, run_node);
> > +
> > +		/* Pull out the group with the highest prio for rotation. */
> > +		if (old_highest_prio_group)
> > +			list_del(&old_highest_prio_group->run_node);
> > +
> > +		/* Re-insert old active groups so they get a chance to run with higher prio. */
> > +		tick_ctx_pick_groups_from_list(sched, &ctx, &ctx.old_groups[prio], true, true);
> > +
> > +		/* Fill the remaining slots with runnable groups. */
> >  		tick_ctx_pick_groups_from_list(sched, &ctx, &sched->groups.runnable[prio],
> >  					       true, false);
> > -		tick_ctx_pick_groups_from_list(sched, &ctx, &ctx.old_groups[prio], true, true);
> > +
> > +		/* Re-insert the old group with the highest prio, and give it a chance to be
> > +		 * scheduled again (but with a lower prio) if there's room left.
> > +		 */
> > +		if (old_highest_prio_group) {
> > +			list_add_tail(&old_highest_prio_group->run_node, &ctx.old_groups[prio]);
> > +			tick_ctx_pick_groups_from_list(sched, &ctx, &ctx.old_groups[prio], true, true);
> > +		}
> >  	}
> >  
> >  	/* If we have free CSG slots left, pick idle groups */  
> 

[PATCH v1 4/8] drm/panthor: Fix the full_tick check

[Boris Brezillon]

We have a full tick when the remaining time to the next tick is zero,
not the other way around. Declare a full_tick variable so we don't get
that test wrong in other places.

Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 69cc1b4c23f2..b6489e9ba1f0 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2349,6 +2349,7 @@ static void tick_work(struct work_struct *work)
 	u64 remaining_jiffies = 0, resched_delay;
 	u64 now = get_jiffies_64();
 	int prio, ret, cookie;
+	bool full_tick;
 
 	if (!drm_dev_enter(&ptdev->base, &cookie))
 		return;
@@ -2360,15 +2361,17 @@ static void tick_work(struct work_struct *work)
 	if (time_before64(now, sched->resched_target))
 		remaining_jiffies = sched->resched_target - now;
 
+	full_tick = remaining_jiffies == 0;
+
 	mutex_lock(&sched->lock);
 	if (panthor_device_reset_is_pending(sched->ptdev))
 		goto out_unlock;
 
-	tick_ctx_init(sched, &ctx, remaining_jiffies != 0);
+	tick_ctx_init(sched, &ctx, full_tick);
 	if (ctx.csg_upd_failed_mask)
 		goto out_cleanup_ctx;
 
-	if (remaining_jiffies) {
+	if (!full_tick) {
 		/* Scheduling forced in the middle of a tick. Only RT groups
 		 * can preempt non-RT ones. Currently running RT groups can't be
 		 * preempted.
-- 
2.51.1

Re: [PATCH v1 4/8] drm/panthor: Fix the full_tick check

[Steven Price]

On 06/11/2025 14:46, Boris Brezillon wrote:
> We have a full tick when the remaining time to the next tick is zero,
> not the other way around. Declare a full_tick variable so we don't get
> that test wrong in other places.
> 
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

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

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 69cc1b4c23f2..b6489e9ba1f0 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2349,6 +2349,7 @@ static void tick_work(struct work_struct *work)
>  	u64 remaining_jiffies = 0, resched_delay;
>  	u64 now = get_jiffies_64();
>  	int prio, ret, cookie;
> +	bool full_tick;
>  
>  	if (!drm_dev_enter(&ptdev->base, &cookie))
>  		return;
> @@ -2360,15 +2361,17 @@ static void tick_work(struct work_struct *work)
>  	if (time_before64(now, sched->resched_target))
>  		remaining_jiffies = sched->resched_target - now;
>  
> +	full_tick = remaining_jiffies == 0;
> +
>  	mutex_lock(&sched->lock);
>  	if (panthor_device_reset_is_pending(sched->ptdev))
>  		goto out_unlock;
>  
> -	tick_ctx_init(sched, &ctx, remaining_jiffies != 0);
> +	tick_ctx_init(sched, &ctx, full_tick);
>  	if (ctx.csg_upd_failed_mask)
>  		goto out_cleanup_ctx;
>  
> -	if (remaining_jiffies) {
> +	if (!full_tick) {
>  		/* Scheduling forced in the middle of a tick. Only RT groups
>  		 * can preempt non-RT ones. Currently running RT groups can't be
>  		 * preempted.

[PATCH v1 5/8] drm/panthor: Fix immediate ticking on a disabled tick

[Boris Brezillon]

We have a few path where we schedule the tick work immediately without
changing the resched_target. If the tick was stopped, this would lead
to a remaining_jiffies that's always > 0, and it wouldn't force a full
tick in that case. Add extra checks to cover that case properly.

Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index b6489e9ba1f0..1eba56e7360d 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2358,8 +2358,12 @@ static void tick_work(struct work_struct *work)
 	if (drm_WARN_ON(&ptdev->base, ret))
 		goto out_dev_exit;
 
-	if (time_before64(now, sched->resched_target))
+	if (sched->resched_target != U64_MAX &&
+	    time_before64(now, sched->resched_target))
 		remaining_jiffies = sched->resched_target - now;
+	else if (sched->resched_target == U64_MAX &&
+		 time_before64(now, sched->last_tick + sched->tick_period))
+		remaining_jiffies = sched->last_tick + sched->tick_period - now;
 
 	full_tick = remaining_jiffies == 0;
 
-- 
2.51.1

Re: [PATCH v1 5/8] drm/panthor: Fix immediate ticking on a disabled tick

[Steven Price]

On 06/11/2025 14:46, Boris Brezillon wrote:
> We have a few path where we schedule the tick work immediately without
NIT: s/path/paths/> changing the resched_target. If the tick was
stopped, this would lead
> to a remaining_jiffies that's always > 0, and it wouldn't force a full
> tick in that case. Add extra checks to cover that case properly.
> 
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index b6489e9ba1f0..1eba56e7360d 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2358,8 +2358,12 @@ static void tick_work(struct work_struct *work)
>  	if (drm_WARN_ON(&ptdev->base, ret))
>  		goto out_dev_exit;
>  
> -	if (time_before64(now, sched->resched_target))
> +	if (sched->resched_target != U64_MAX &&
> +	    time_before64(now, sched->resched_target))
>  		remaining_jiffies = sched->resched_target - now;
> +	else if (sched->resched_target == U64_MAX &&
> +		 time_before64(now, sched->last_tick + sched->tick_period))
> +		remaining_jiffies = sched->last_tick + sched->tick_period - now;

I'm wondering if this would be cleaner with an extra variable (and a
comment):

   u64 resched_target = sched->resched_target;

   /* If the tick is stopped, calculate when the next tick would be */
   if (resched_target == U64_MAX)
	   resched_target = sched->last_tick + sched->tick_period;

   if (time_before64(now, resched_target)
	   remaining_jiffies = resched_target - now;

It at least avoids some repetition.

Thanks,
Steve

>  
>  	full_tick = remaining_jiffies == 0;
>  

Re: [PATCH v1 5/8] drm/panthor: Fix immediate ticking on a disabled tick

[Boris Brezillon]

On Thu, 6 Nov 2025 16:23:24 +0000
Steven Price <steven.price@arm.com> wrote:

> On 06/11/2025 14:46, Boris Brezillon wrote:
> > We have a few path where we schedule the tick work immediately without  
> NIT: s/path/paths/> changing the resched_target. If the tick was
> stopped, this would lead
> > to a remaining_jiffies that's always > 0, and it wouldn't force a full
> > tick in that case. Add extra checks to cover that case properly.
> > 
> > Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_sched.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> > index b6489e9ba1f0..1eba56e7360d 100644
> > --- a/drivers/gpu/drm/panthor/panthor_sched.c
> > +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> > @@ -2358,8 +2358,12 @@ static void tick_work(struct work_struct *work)
> >  	if (drm_WARN_ON(&ptdev->base, ret))
> >  		goto out_dev_exit;
> >  
> > -	if (time_before64(now, sched->resched_target))
> > +	if (sched->resched_target != U64_MAX &&
> > +	    time_before64(now, sched->resched_target))
> >  		remaining_jiffies = sched->resched_target - now;
> > +	else if (sched->resched_target == U64_MAX &&
> > +		 time_before64(now, sched->last_tick + sched->tick_period))
> > +		remaining_jiffies = sched->last_tick + sched->tick_period - now;  
> 
> I'm wondering if this would be cleaner with an extra variable (and a
> comment):
> 
>    u64 resched_target = sched->resched_target;
> 
>    /* If the tick is stopped, calculate when the next tick would be */
>    if (resched_target == U64_MAX)
> 	   resched_target = sched->last_tick + sched->tick_period;
> 
>    if (time_before64(now, resched_target)
> 	   remaining_jiffies = resched_target - now;
> 
> It at least avoids some repetition.

Yep that looks better expressed like that. I'll go for your suggestion
in the next version.

[PATCH v1 6/8] drm/panthor: Fix the logic that decides when to stop ticking

[Boris Brezillon]

When we have multiple active groups with the same priority, we need to
keep ticking for the priority rotation to take place. If we don't do
that, we might starve slots with lower priorities.

It's annoying to deal with that in tick_ctx_update_resched_target(),
so let's add a ::stop_tick field to the tick context which is
initialized to true, and downgraded to false as soon as we detect
something that requires to tick to happen. This way we can complement
the current logic with extra conditions if needed.

Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 36 ++++++++++++-------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 1eba56e7360d..7b164228af7b 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -1882,6 +1882,7 @@ struct panthor_sched_tick_ctx {
 	struct panthor_vm *vms[MAX_CS_PER_CSG];
 	u32 as_count;
 	bool immediate_tick;
+	bool stop_tick;
 	u32 csg_upd_failed_mask;
 };
 
@@ -1941,10 +1942,17 @@ tick_ctx_pick_groups_from_list(const struct panthor_scheduler *sched,
 		if (!owned_by_tick_ctx)
 			group_get(group);
 
-		list_move_tail(&group->run_node, &ctx->groups[group->priority]);
 		ctx->group_count++;
+
+		/* If we have more than one active group with the same priority,
+		 * we need to keep ticking to rotate the CSG priority.
+		 */
 		if (group_is_idle(group))
 			ctx->idle_group_count++;
+		else if (!list_empty(&ctx->groups[group->priority]))
+			ctx->stop_tick = false;
+
+		list_move_tail(&group->run_node, &ctx->groups[group->priority]);
 
 		if (i == ctx->as_count)
 			ctx->vms[ctx->as_count++] = group->vm;
@@ -1996,6 +2004,7 @@ tick_ctx_init(struct panthor_scheduler *sched,
 	memset(ctx, 0, sizeof(*ctx));
 	csgs_upd_ctx_init(&upd_ctx);
 
+	ctx->stop_tick = true;
 	ctx->min_priority = PANTHOR_CSG_PRIORITY_COUNT;
 	for (i = 0; i < ARRAY_SIZE(ctx->groups); i++) {
 		INIT_LIST_HEAD(&ctx->groups[i]);
@@ -2308,32 +2317,21 @@ static u64
 tick_ctx_update_resched_target(struct panthor_scheduler *sched,
 			       const struct panthor_sched_tick_ctx *ctx)
 {
-	/* We had space left, no need to reschedule until some external event happens. */
-	if (!tick_ctx_is_full(sched, ctx))
-		goto no_tick;
+	u64 resched_target;
 
-	/* If idle groups were scheduled, no need to wake up until some external
-	 * event happens (group unblocked, new job submitted, ...).
-	 */
-	if (ctx->idle_group_count)
+	if (ctx->stop_tick)
 		goto no_tick;
 
 	if (drm_WARN_ON(&sched->ptdev->base, ctx->min_priority >= PANTHOR_CSG_PRIORITY_COUNT))
 		goto no_tick;
 
-	/* If there are groups of the same priority waiting, we need to
-	 * keep the scheduler ticking, otherwise, we'll just wait for
-	 * new groups with higher priority to be queued.
-	 */
-	if (!list_empty(&sched->groups.runnable[ctx->min_priority])) {
-		u64 resched_target = sched->last_tick + sched->tick_period;
+	resched_target = sched->last_tick + sched->tick_period;
 
-		if (time_before64(sched->resched_target, sched->last_tick) ||
-		    time_before64(resched_target, sched->resched_target))
-			sched->resched_target = resched_target;
+	if (time_before64(sched->resched_target, sched->last_tick) ||
+	    time_before64(resched_target, sched->resched_target))
+		sched->resched_target = resched_target;
 
-		return sched->resched_target - sched->last_tick;
-	}
+	return sched->resched_target - sched->last_tick;
 
 no_tick:
 	sched->resched_target = U64_MAX;
-- 
2.51.1

Re: [PATCH v1 6/8] drm/panthor: Fix the logic that decides when to stop ticking

[Steven Price]

On 06/11/2025 14:46, Boris Brezillon wrote:
> When we have multiple active groups with the same priority, we need to
> keep ticking for the priority rotation to take place. If we don't do
> that, we might starve slots with lower priorities.
> 
> It's annoying to deal with that in tick_ctx_update_resched_target(),
> so let's add a ::stop_tick field to the tick context which is
> initialized to true, and downgraded to false as soon as we detect
> something that requires to tick to happen. This way we can complement
> the current logic with extra conditions if needed.
> 
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

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

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 36 ++++++++++++-------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 1eba56e7360d..7b164228af7b 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1882,6 +1882,7 @@ struct panthor_sched_tick_ctx {
>  	struct panthor_vm *vms[MAX_CS_PER_CSG];
>  	u32 as_count;
>  	bool immediate_tick;
> +	bool stop_tick;
>  	u32 csg_upd_failed_mask;
>  };
>  
> @@ -1941,10 +1942,17 @@ tick_ctx_pick_groups_from_list(const struct panthor_scheduler *sched,
>  		if (!owned_by_tick_ctx)
>  			group_get(group);
>  
> -		list_move_tail(&group->run_node, &ctx->groups[group->priority]);
>  		ctx->group_count++;
> +
> +		/* If we have more than one active group with the same priority,
> +		 * we need to keep ticking to rotate the CSG priority.
> +		 */
>  		if (group_is_idle(group))
>  			ctx->idle_group_count++;
> +		else if (!list_empty(&ctx->groups[group->priority]))
> +			ctx->stop_tick = false;
> +
> +		list_move_tail(&group->run_node, &ctx->groups[group->priority]);
>  
>  		if (i == ctx->as_count)
>  			ctx->vms[ctx->as_count++] = group->vm;
> @@ -1996,6 +2004,7 @@ tick_ctx_init(struct panthor_scheduler *sched,
>  	memset(ctx, 0, sizeof(*ctx));
>  	csgs_upd_ctx_init(&upd_ctx);
>  
> +	ctx->stop_tick = true;
>  	ctx->min_priority = PANTHOR_CSG_PRIORITY_COUNT;
>  	for (i = 0; i < ARRAY_SIZE(ctx->groups); i++) {
>  		INIT_LIST_HEAD(&ctx->groups[i]);
> @@ -2308,32 +2317,21 @@ static u64
>  tick_ctx_update_resched_target(struct panthor_scheduler *sched,
>  			       const struct panthor_sched_tick_ctx *ctx)
>  {
> -	/* We had space left, no need to reschedule until some external event happens. */
> -	if (!tick_ctx_is_full(sched, ctx))
> -		goto no_tick;
> +	u64 resched_target;
>  
> -	/* If idle groups were scheduled, no need to wake up until some external
> -	 * event happens (group unblocked, new job submitted, ...).
> -	 */
> -	if (ctx->idle_group_count)
> +	if (ctx->stop_tick)
>  		goto no_tick;
>  
>  	if (drm_WARN_ON(&sched->ptdev->base, ctx->min_priority >= PANTHOR_CSG_PRIORITY_COUNT))
>  		goto no_tick;
>  
> -	/* If there are groups of the same priority waiting, we need to
> -	 * keep the scheduler ticking, otherwise, we'll just wait for
> -	 * new groups with higher priority to be queued.
> -	 */
> -	if (!list_empty(&sched->groups.runnable[ctx->min_priority])) {
> -		u64 resched_target = sched->last_tick + sched->tick_period;
> +	resched_target = sched->last_tick + sched->tick_period;
>  
> -		if (time_before64(sched->resched_target, sched->last_tick) ||
> -		    time_before64(resched_target, sched->resched_target))
> -			sched->resched_target = resched_target;
> +	if (time_before64(sched->resched_target, sched->last_tick) ||
> +	    time_before64(resched_target, sched->resched_target))
> +		sched->resched_target = resched_target;
>  
> -		return sched->resched_target - sched->last_tick;
> -	}
> +	return sched->resched_target - sched->last_tick;
>  
>  no_tick:
>  	sched->resched_target = U64_MAX;

[PATCH v1 7/8] drm/panthor: Make sure we resume the tick when new jobs are submitted

[Boris Brezillon]

If the group is already assigned a slot but was idle before this job
submission, we need to make sure the priority rotation happens in the
future. Extract the existing logic living in group_schedule_locked()
and call this new sched_resume_tick() helper from the "group is
assigned a slot" path.

Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 43 +++++++++++++++++++------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 7b164228af7b..923816397751 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2541,14 +2541,33 @@ static void sync_upd_work(struct work_struct *work)
 		sched_queue_delayed_work(sched, tick, 0);
 }
 
+static void sched_resume_tick(struct panthor_device *ptdev)
+{
+	struct panthor_scheduler *sched = ptdev->scheduler;
+	u64 delay_jiffies, now;
+
+	drm_WARN_ON(&ptdev->base, sched->resched_target != U64_MAX);
+
+	/* Scheduler tick was off, recalculate the resched_target based on the
+	 * last tick event, and queue the scheduler work.
+	 */
+	now = get_jiffies_64();
+	sched->resched_target = sched->last_tick + sched->tick_period;
+	if (sched->used_csg_slot_count == sched->csg_slot_count &&
+	    time_before64(now, sched->resched_target))
+		delay_jiffies = min_t(unsigned long, sched->resched_target - now, ULONG_MAX);
+	else
+		delay_jiffies = 0;
+
+	sched_queue_delayed_work(sched, tick, delay_jiffies);
+}
+
 static void group_schedule_locked(struct panthor_group *group, u32 queue_mask)
 {
 	struct panthor_device *ptdev = group->ptdev;
 	struct panthor_scheduler *sched = ptdev->scheduler;
 	struct list_head *queue = &sched->groups.runnable[group->priority];
-	u64 delay_jiffies = 0;
 	bool was_idle;
-	u64 now;
 
 	if (!group_can_run(group))
 		return;
@@ -2593,13 +2612,7 @@ static void group_schedule_locked(struct panthor_group *group, u32 queue_mask)
 	/* Scheduler tick was off, recalculate the resched_target based on the
 	 * last tick event, and queue the scheduler work.
 	 */
-	now = get_jiffies_64();
-	sched->resched_target = sched->last_tick + sched->tick_period;
-	if (sched->used_csg_slot_count == sched->csg_slot_count &&
-	    time_before64(now, sched->resched_target))
-		delay_jiffies = min_t(unsigned long, sched->resched_target - now, ULONG_MAX);
-
-	sched_queue_delayed_work(sched, tick, delay_jiffies);
+	sched_resume_tick(ptdev);
 }
 
 static void queue_stop(struct panthor_queue *queue,
@@ -3200,6 +3213,18 @@ queue_run_job(struct drm_sched_job *sched_job)
 
 		group_schedule_locked(group, BIT(job->queue_idx));
 	} else {
+		u32 queue_mask = BIT(job->queue_idx);
+		bool resume_tick = group_is_idle(group) &&
+				   (group->idle_queues & queue_mask) &&
+				   !(group->blocked_queues & queue_mask) &&
+				   sched->resched_target == U64_MAX;
+
+		/* We just added something to the queue, so it's no longer idle. */
+		group->idle_queues &= ~BIT(job->queue_idx);
+
+		if (resume_tick)
+			sched_resume_tick(ptdev);
+
 		gpu_write(ptdev, CSF_DOORBELL(queue->doorbell_id), 1);
 		if (!sched->pm.has_ref &&
 		    !(group->blocked_queues & BIT(job->queue_idx))) {
-- 
2.51.1

Re: [PATCH v1 7/8] drm/panthor: Make sure we resume the tick when new jobs are submitted

[Steven Price]

On 06/11/2025 14:46, Boris Brezillon wrote:
> If the group is already assigned a slot but was idle before this job
> submission, we need to make sure the priority rotation happens in the
> future. Extract the existing logic living in group_schedule_locked()
> and call this new sched_resume_tick() helper from the "group is
> assigned a slot" path.
> 
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

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

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 43 +++++++++++++++++++------
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 7b164228af7b..923816397751 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2541,14 +2541,33 @@ static void sync_upd_work(struct work_struct *work)
>  		sched_queue_delayed_work(sched, tick, 0);
>  }
>  
> +static void sched_resume_tick(struct panthor_device *ptdev)
> +{
> +	struct panthor_scheduler *sched = ptdev->scheduler;
> +	u64 delay_jiffies, now;
> +
> +	drm_WARN_ON(&ptdev->base, sched->resched_target != U64_MAX);
> +
> +	/* Scheduler tick was off, recalculate the resched_target based on the
> +	 * last tick event, and queue the scheduler work.
> +	 */
> +	now = get_jiffies_64();
> +	sched->resched_target = sched->last_tick + sched->tick_period;
> +	if (sched->used_csg_slot_count == sched->csg_slot_count &&
> +	    time_before64(now, sched->resched_target))
> +		delay_jiffies = min_t(unsigned long, sched->resched_target - now, ULONG_MAX);
> +	else
> +		delay_jiffies = 0;
> +
> +	sched_queue_delayed_work(sched, tick, delay_jiffies);
> +}
> +
>  static void group_schedule_locked(struct panthor_group *group, u32 queue_mask)
>  {
>  	struct panthor_device *ptdev = group->ptdev;
>  	struct panthor_scheduler *sched = ptdev->scheduler;
>  	struct list_head *queue = &sched->groups.runnable[group->priority];
> -	u64 delay_jiffies = 0;
>  	bool was_idle;
> -	u64 now;
>  
>  	if (!group_can_run(group))
>  		return;
> @@ -2593,13 +2612,7 @@ static void group_schedule_locked(struct panthor_group *group, u32 queue_mask)
>  	/* Scheduler tick was off, recalculate the resched_target based on the
>  	 * last tick event, and queue the scheduler work.
>  	 */
> -	now = get_jiffies_64();
> -	sched->resched_target = sched->last_tick + sched->tick_period;
> -	if (sched->used_csg_slot_count == sched->csg_slot_count &&
> -	    time_before64(now, sched->resched_target))
> -		delay_jiffies = min_t(unsigned long, sched->resched_target - now, ULONG_MAX);
> -
> -	sched_queue_delayed_work(sched, tick, delay_jiffies);
> +	sched_resume_tick(ptdev);
>  }
>  
>  static void queue_stop(struct panthor_queue *queue,
> @@ -3200,6 +3213,18 @@ queue_run_job(struct drm_sched_job *sched_job)
>  
>  		group_schedule_locked(group, BIT(job->queue_idx));
>  	} else {
> +		u32 queue_mask = BIT(job->queue_idx);
> +		bool resume_tick = group_is_idle(group) &&
> +				   (group->idle_queues & queue_mask) &&
> +				   !(group->blocked_queues & queue_mask) &&
> +				   sched->resched_target == U64_MAX;
> +
> +		/* We just added something to the queue, so it's no longer idle. */
> +		group->idle_queues &= ~BIT(job->queue_idx);
> +
> +		if (resume_tick)
> +			sched_resume_tick(ptdev);
> +
>  		gpu_write(ptdev, CSF_DOORBELL(queue->doorbell_id), 1);
>  		if (!sched->pm.has_ref &&
>  		    !(group->blocked_queues & BIT(job->queue_idx))) {

[PATCH v1 8/8] drm/panthor: Kill panthor_sched_immediate_tick()

[Boris Brezillon]

It's only used in a couple places and everyone else is just using
sched_queue_delayed_work(sched, tick, 0) directly, so let's make
this consistent.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 923816397751..6b8428ca8145 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2668,13 +2668,6 @@ static void panthor_group_start(struct panthor_group *group)
 	group_put(group);
 }
 
-static void panthor_sched_immediate_tick(struct panthor_device *ptdev)
-{
-	struct panthor_scheduler *sched = ptdev->scheduler;
-
-	sched_queue_delayed_work(sched, tick, 0);
-}
-
 /**
  * panthor_sched_report_mmu_fault() - Report MMU faults to the scheduler.
  */
@@ -2682,13 +2675,13 @@ void panthor_sched_report_mmu_fault(struct panthor_device *ptdev)
 {
 	/* Force a tick to immediately kill faulty groups. */
 	if (ptdev->scheduler)
-		panthor_sched_immediate_tick(ptdev);
+		sched_queue_delayed_work(ptdev->scheduler, tick, 0);
 }
 
 void panthor_sched_resume(struct panthor_device *ptdev)
 {
 	/* Force a tick to re-evaluate after a resume. */
-	panthor_sched_immediate_tick(ptdev);
+	sched_queue_delayed_work(ptdev->scheduler, tick, 0);
 }
 
 void panthor_sched_suspend(struct panthor_device *ptdev)
-- 
2.51.1

Re: [PATCH v1 8/8] drm/panthor: Kill panthor_sched_immediate_tick()

[Steven Price]

On 06/11/2025 14:46, Boris Brezillon wrote:
> It's only used in a couple places and everyone else is just using
> sched_queue_delayed_work(sched, tick, 0) directly, so let's make
> this consistent.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

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

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 923816397751..6b8428ca8145 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2668,13 +2668,6 @@ static void panthor_group_start(struct panthor_group *group)
>  	group_put(group);
>  }
>  
> -static void panthor_sched_immediate_tick(struct panthor_device *ptdev)
> -{
> -	struct panthor_scheduler *sched = ptdev->scheduler;
> -
> -	sched_queue_delayed_work(sched, tick, 0);
> -}
> -
>  /**
>   * panthor_sched_report_mmu_fault() - Report MMU faults to the scheduler.
>   */
> @@ -2682,13 +2675,13 @@ void panthor_sched_report_mmu_fault(struct panthor_device *ptdev)
>  {
>  	/* Force a tick to immediately kill faulty groups. */
>  	if (ptdev->scheduler)
> -		panthor_sched_immediate_tick(ptdev);
> +		sched_queue_delayed_work(ptdev->scheduler, tick, 0);
>  }
>  
>  void panthor_sched_resume(struct panthor_device *ptdev)
>  {
>  	/* Force a tick to re-evaluate after a resume. */
> -	panthor_sched_immediate_tick(ptdev);
> +	sched_queue_delayed_work(ptdev->scheduler, tick, 0);
>  }
>  
>  void panthor_sched_suspend(struct panthor_device *ptdev)