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

[Steven Price <steven.price@arm.com>]

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;