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

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

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 */  
>