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 <[email protected]>
Reviewed-by: Steven Price <[email protected]>
> ---
> 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;
