* [PATCH v2 1/8] drm/panthor: Simplify group idleness tracking
2025-11-12 11:51 [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
@ 2025-11-12 11:51 ` Boris Brezillon
2025-11-12 11:51 ` [PATCH v2 2/8] drm/panthor: Don't try to enable extract events Boris Brezillon
` (7 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Boris Brezillon @ 2025-11-12 11:51 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, Florent Tomasin, Heinrich Fink, kernel
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.
v2:
- Add R-b
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
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;
}
--
2.51.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v2 2/8] drm/panthor: Don't try to enable extract events
2025-11-12 11:51 [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
2025-11-12 11:51 ` [PATCH v2 1/8] drm/panthor: Simplify group idleness tracking Boris Brezillon
@ 2025-11-12 11:51 ` Boris Brezillon
2025-11-12 11:51 ` [PATCH v2 3/8] drm/panthor: Fix the group priority rotation logic Boris Brezillon
` (6 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Boris Brezillon @ 2025-11-12 11:51 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, Florent Tomasin, Heinrich Fink, kernel
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.
v2:
- Add R-b
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;
--
2.51.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v2 3/8] drm/panthor: Fix the group priority rotation logic
2025-11-12 11:51 [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
2025-11-12 11:51 ` [PATCH v2 1/8] drm/panthor: Simplify group idleness tracking Boris Brezillon
2025-11-12 11:51 ` [PATCH v2 2/8] drm/panthor: Don't try to enable extract events Boris Brezillon
@ 2025-11-12 11:51 ` Boris Brezillon
2025-11-26 21:48 ` Chia-I Wu
2025-11-12 11:51 ` [PATCH v2 4/8] drm/panthor: Fix the full_tick check Boris Brezillon
` (5 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Boris Brezillon @ 2025-11-12 11:51 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, Florent Tomasin, Heinrich Fink, kernel
When rotating group priorities, we want the group with the
highest priority to go back to the end of the queue, and all
other active groups to get their priority bumped, otherwise
some groups will never get a chance to run with the highest
priority. This implies moving the rotation itself to
tick_work(), and only dealing with old group ordering in
tick_ctx_insert_old_group().
v2:
- Add R-b
- Fix the commit message
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 | 47 +++++++++++++++----------
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 94514d464e64..f08a05d36fc0 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,29 @@ 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 3/8] drm/panthor: Fix the group priority rotation logic
2025-11-12 11:51 ` [PATCH v2 3/8] drm/panthor: Fix the group priority rotation logic Boris Brezillon
@ 2025-11-26 21:48 ` Chia-I Wu
0 siblings, 0 replies; 18+ messages in thread
From: Chia-I Wu @ 2025-11-26 21:48 UTC (permalink / raw)
To: Boris Brezillon
Cc: Steven Price, Liviu Dudau, Adrián Larumbe, dri-devel,
Florent Tomasin, Heinrich Fink, kernel
On Wed, Nov 12, 2025 at 3:52 AM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> When rotating group priorities, we want the group with the
> highest priority to go back to the end of the queue, and all
> other active groups to get their priority bumped, otherwise
> some groups will never get a chance to run with the highest
> priority. This implies moving the rotation itself to
> tick_work(), and only dealing with old group ordering in
> tick_ctx_insert_old_group().
>
> v2:
> - Add R-b
> - Fix the commit message
>
> 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 | 47 +++++++++++++++----------
> 1 file changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 94514d464e64..f08a05d36fc0 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);
This drops the only user of full_tick. We can omit the parameter from
tick_ctx_init.
> csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, i,
> csg_iface->output->ack ^ CSG_STATUS_UPDATE,
> CSG_STATUS_UPDATE);
> @@ -2399,9 +2390,29 @@ 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 4/8] drm/panthor: Fix the full_tick check
2025-11-12 11:51 [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
` (2 preceding siblings ...)
2025-11-12 11:51 ` [PATCH v2 3/8] drm/panthor: Fix the group priority rotation logic Boris Brezillon
@ 2025-11-12 11:51 ` Boris Brezillon
2025-11-12 11:51 ` [PATCH v2 5/8] drm/panthor: Fix immediate ticking on a disabled tick Boris Brezillon
` (4 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Boris Brezillon @ 2025-11-12 11:51 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, Florent Tomasin, Heinrich Fink, kernel
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.
v2:
- Add R-b
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 f08a05d36fc0..dd3a3b8cd5ad 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v2 5/8] drm/panthor: Fix immediate ticking on a disabled tick
2025-11-12 11:51 [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
` (3 preceding siblings ...)
2025-11-12 11:51 ` [PATCH v2 4/8] drm/panthor: Fix the full_tick check Boris Brezillon
@ 2025-11-12 11:51 ` Boris Brezillon
2025-11-12 11:51 ` [PATCH v2 6/8] drm/panthor: Fix the logic that decides when to stop ticking Boris Brezillon
` (3 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Boris Brezillon @ 2025-11-12 11:51 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, Florent Tomasin, Heinrich Fink, kernel
We have a few paths 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.
v2:
- Fix typo
- Simplify the code as suggested by Steve
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 | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index dd3a3b8cd5ad..4c137581fe40 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2346,6 +2346,7 @@ static void tick_work(struct work_struct *work)
tick_work.work);
struct panthor_device *ptdev = sched->ptdev;
struct panthor_sched_tick_ctx ctx;
+ u64 resched_target = sched->resched_target;
u64 remaining_jiffies = 0, resched_delay;
u64 now = get_jiffies_64();
int prio, ret, cookie;
@@ -2358,8 +2359,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))
- remaining_jiffies = sched->resched_target - now;
+ /* 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;
full_tick = remaining_jiffies == 0;
--
2.51.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v2 6/8] drm/panthor: Fix the logic that decides when to stop ticking
2025-11-12 11:51 [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
` (4 preceding siblings ...)
2025-11-12 11:51 ` [PATCH v2 5/8] drm/panthor: Fix immediate ticking on a disabled tick Boris Brezillon
@ 2025-11-12 11:51 ` Boris Brezillon
2025-11-26 22:28 ` Chia-I Wu
2025-11-12 11:51 ` [PATCH v2 7/8] drm/panthor: Make sure we resume the tick when new jobs are submitted Boris Brezillon
` (2 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Boris Brezillon @ 2025-11-12 11:51 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, Florent Tomasin, Heinrich Fink, kernel
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.
v2:
- Add R-b
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 4c137581fe40..0394f377c284 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 6/8] drm/panthor: Fix the logic that decides when to stop ticking
2025-11-12 11:51 ` [PATCH v2 6/8] drm/panthor: Fix the logic that decides when to stop ticking Boris Brezillon
@ 2025-11-26 22:28 ` Chia-I Wu
0 siblings, 0 replies; 18+ messages in thread
From: Chia-I Wu @ 2025-11-26 22:28 UTC (permalink / raw)
To: Boris Brezillon
Cc: Steven Price, Liviu Dudau, Adrián Larumbe, dri-devel,
Florent Tomasin, Heinrich Fink, kernel
On Wed, Nov 12, 2025 at 4:21 AM Boris Brezillon
<boris.brezillon@collabora.com> 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.
>
> v2:
> - Add R-b
>
> 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 4c137581fe40..0394f377c284 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])) {
It looks like we can drop ctx->min_priority completely. It was mainly
a safeguard to avoid OOB access here.
> - 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 7/8] drm/panthor: Make sure we resume the tick when new jobs are submitted
2025-11-12 11:51 [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
` (5 preceding siblings ...)
2025-11-12 11:51 ` [PATCH v2 6/8] drm/panthor: Fix the logic that decides when to stop ticking Boris Brezillon
@ 2025-11-12 11:51 ` Boris Brezillon
2025-11-26 22:57 ` Chia-I Wu
2025-11-12 11:51 ` [PATCH v2 8/8] drm/panthor: Kill panthor_sched_immediate_tick() Boris Brezillon
2025-11-26 12:14 ` [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
8 siblings, 1 reply; 18+ messages in thread
From: Boris Brezillon @ 2025-11-12 11:51 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, Florent Tomasin, Heinrich Fink, kernel
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.
v2:
- Add R-b
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 0394f377c284..d1484f4a1c5b 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2543,14 +2543,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;
@@ -2595,13 +2614,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,
@@ -3202,6 +3215,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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 7/8] drm/panthor: Make sure we resume the tick when new jobs are submitted
2025-11-12 11:51 ` [PATCH v2 7/8] drm/panthor: Make sure we resume the tick when new jobs are submitted Boris Brezillon
@ 2025-11-26 22:57 ` Chia-I Wu
2025-11-27 8:41 ` Boris Brezillon
2025-11-27 14:46 ` Boris Brezillon
0 siblings, 2 replies; 18+ messages in thread
From: Chia-I Wu @ 2025-11-26 22:57 UTC (permalink / raw)
To: Boris Brezillon
Cc: Steven Price, Liviu Dudau, Adrián Larumbe, dri-devel,
Florent Tomasin, Heinrich Fink, kernel
On Thu, Nov 13, 2025 at 7:32 AM Boris Brezillon
<boris.brezillon@collabora.com> 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.
>
> v2:
> - Add R-b
>
> 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 0394f377c284..d1484f4a1c5b 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2543,14 +2543,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;
> @@ -2595,13 +2614,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,
> @@ -3202,6 +3215,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;
The logic here should be the same as the first part of
group_schedule_locked. I wonder if we can refactor that as well.
> +
> + /* We just added something to the queue, so it's no longer idle. */
> + group->idle_queues &= ~BIT(job->queue_idx);
group->idle_queues &= queue_mask;
> +
> + 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 7/8] drm/panthor: Make sure we resume the tick when new jobs are submitted
2025-11-26 22:57 ` Chia-I Wu
@ 2025-11-27 8:41 ` Boris Brezillon
2025-11-27 14:46 ` Boris Brezillon
1 sibling, 0 replies; 18+ messages in thread
From: Boris Brezillon @ 2025-11-27 8:41 UTC (permalink / raw)
To: Chia-I Wu
Cc: Steven Price, Liviu Dudau, Adrián Larumbe, dri-devel,
Florent Tomasin, Heinrich Fink, kernel
On Wed, 26 Nov 2025 14:57:32 -0800
Chia-I Wu <olvaffe@gmail.com> wrote:
> On Thu, Nov 13, 2025 at 7:32 AM Boris Brezillon
> <boris.brezillon@collabora.com> 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.
> >
> > v2:
> > - Add R-b
> >
> > 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 0394f377c284..d1484f4a1c5b 100644
> > --- a/drivers/gpu/drm/panthor/panthor_sched.c
> > +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> > @@ -2543,14 +2543,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;
> > @@ -2595,13 +2614,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,
> > @@ -3202,6 +3215,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;
> The logic here should be the same as the first part of
> group_schedule_locked. I wonder if we can refactor that as well.
Could be, yes. I'll give it a try.
>
> > +
> > + /* We just added something to the queue, so it's no longer idle. */
> > + group->idle_queues &= ~BIT(job->queue_idx);
> group->idle_queues &= queue_mask;
You mean
group->idle_queues &= ~queue_mask;
I guess.
>
> > +
> > + 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
> >
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 7/8] drm/panthor: Make sure we resume the tick when new jobs are submitted
2025-11-26 22:57 ` Chia-I Wu
2025-11-27 8:41 ` Boris Brezillon
@ 2025-11-27 14:46 ` Boris Brezillon
2025-11-27 18:08 ` Chia-I Wu
1 sibling, 1 reply; 18+ messages in thread
From: Boris Brezillon @ 2025-11-27 14:46 UTC (permalink / raw)
To: Chia-I Wu
Cc: Steven Price, Liviu Dudau, Adrián Larumbe, dri-devel,
Florent Tomasin, Heinrich Fink, kernel
On Wed, 26 Nov 2025 14:57:32 -0800
Chia-I Wu <olvaffe@gmail.com> wrote:
> > static void queue_stop(struct panthor_queue *queue,
> > @@ -3202,6 +3215,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;
> The logic here should be the same as the first part of
> group_schedule_locked. I wonder if we can refactor that as well.
I addressed everything you pointed out, except for this. The tests in
group_schedule_locked() are two intricated with the rest of the logic to
be easily extracted into some helper. I'm happy to review such a patch
though.
>
> > +
> > + /* We just added something to the queue, so it's no longer idle. */
> > + group->idle_queues &= ~BIT(job->queue_idx);
> group->idle_queues &= queue_mask;
>
> > +
> > + 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))) {
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 7/8] drm/panthor: Make sure we resume the tick when new jobs are submitted
2025-11-27 14:46 ` Boris Brezillon
@ 2025-11-27 18:08 ` Chia-I Wu
0 siblings, 0 replies; 18+ messages in thread
From: Chia-I Wu @ 2025-11-27 18:08 UTC (permalink / raw)
To: Boris Brezillon
Cc: Steven Price, Liviu Dudau, Adrián Larumbe, dri-devel,
Florent Tomasin, Heinrich Fink, kernel
On Thu, Nov 27, 2025 at 6:47 AM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Wed, 26 Nov 2025 14:57:32 -0800
> Chia-I Wu <olvaffe@gmail.com> wrote:
>
> > > static void queue_stop(struct panthor_queue *queue,
> > > @@ -3202,6 +3215,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;
> > The logic here should be the same as the first part of
> > group_schedule_locked. I wonder if we can refactor that as well.
>
> I addressed everything you pointed out, except for this. The tests in
> group_schedule_locked() are two intricated with the rest of the logic to
> be easily extracted into some helper. I'm happy to review such a patch
> though.
Sounds good.
>
> >
> > > +
> > > + /* We just added something to the queue, so it's no longer idle. */
> > > + group->idle_queues &= ~BIT(job->queue_idx);
> > group->idle_queues &= queue_mask;
Right, should have been "group->idle_queues &= ~queue_mask;".
> >
> > > +
> > > + 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))) {
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 8/8] drm/panthor: Kill panthor_sched_immediate_tick()
2025-11-12 11:51 [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
` (6 preceding siblings ...)
2025-11-12 11:51 ` [PATCH v2 7/8] drm/panthor: Make sure we resume the tick when new jobs are submitted Boris Brezillon
@ 2025-11-12 11:51 ` Boris Brezillon
2025-11-26 12:14 ` [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
8 siblings, 0 replies; 18+ messages in thread
From: Boris Brezillon @ 2025-11-12 11:51 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, Florent Tomasin, Heinrich Fink, kernel
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.
v2:
- Add R-b
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 d1484f4a1c5b..4342e3b2b30a 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2670,13 +2670,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.
*/
@@ -2684,13 +2677,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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 0/8] drm/panthor: Misc scheduler fixes
2025-11-12 11:51 [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
` (7 preceding siblings ...)
2025-11-12 11:51 ` [PATCH v2 8/8] drm/panthor: Kill panthor_sched_immediate_tick() Boris Brezillon
@ 2025-11-26 12:14 ` Boris Brezillon
2025-11-26 16:06 ` Steven Price
8 siblings, 1 reply; 18+ messages in thread
From: Boris Brezillon @ 2025-11-26 12:14 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, Florent Tomasin, Heinrich Fink, kernel
On Wed, 12 Nov 2025 12:51:34 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 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.
I've posted the IGT patches to make sure the bugs fixed here are caught
if it ever regresses again. If there's no objection, I'd like to merge
those patches before the end of the week (given we're passed the 6.19
branching point, this should give us plenty of time to address
regressions, if any).
>
> No major changes in this version, for more details, check the changelog
> in each patch.
>
> 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 | 190 ++++++++++++------------
> 1 file changed, 98 insertions(+), 92 deletions(-)
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 0/8] drm/panthor: Misc scheduler fixes
2025-11-26 12:14 ` [PATCH v2 0/8] drm/panthor: Misc scheduler fixes Boris Brezillon
@ 2025-11-26 16:06 ` Steven Price
2025-11-26 23:02 ` Chia-I Wu
0 siblings, 1 reply; 18+ messages in thread
From: Steven Price @ 2025-11-26 16:06 UTC (permalink / raw)
To: Boris Brezillon, Liviu Dudau, Adrián Larumbe
Cc: dri-devel, Florent Tomasin, Heinrich Fink, kernel
On 26/11/2025 12:14, Boris Brezillon wrote:
> On Wed, 12 Nov 2025 12:51:34 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>
>> 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.
>
> I've posted the IGT patches to make sure the bugs fixed here are caught
> if it ever regresses again. If there's no objection, I'd like to merge
> those patches before the end of the week (given we're passed the 6.19
> branching point, this should give us plenty of time to address
> regressions, if any).
I believe you've already got my R-b on all the patches, so from my point
of view: merge away! ;)
Thanks,
Steve
>>
>> No major changes in this version, for more details, check the changelog
>> in each patch.
>>
>> 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 | 190 ++++++++++++------------
>> 1 file changed, 98 insertions(+), 92 deletions(-)
>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/8] drm/panthor: Misc scheduler fixes
2025-11-26 16:06 ` Steven Price
@ 2025-11-26 23:02 ` Chia-I Wu
0 siblings, 0 replies; 18+ messages in thread
From: Chia-I Wu @ 2025-11-26 23:02 UTC (permalink / raw)
To: Steven Price
Cc: Boris Brezillon, Liviu Dudau, Adrián Larumbe, dri-devel,
Florent Tomasin, Heinrich Fink, kernel
On Wed, Nov 26, 2025 at 8:06 AM Steven Price <steven.price@arm.com> wrote:
>
> On 26/11/2025 12:14, Boris Brezillon wrote:
> > On Wed, 12 Nov 2025 12:51:34 +0100
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >
> >> 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.
> >
> > I've posted the IGT patches to make sure the bugs fixed here are caught
> > if it ever regresses again. If there's no objection, I'd like to merge
> > those patches before the end of the week (given we're passed the 6.19
> > branching point, this should give us plenty of time to address
> > regressions, if any).
>
> I believe you've already got my R-b on all the patches, so from my point
> of view: merge away! ;)
I made a few minor comments. Whether they are addressed or not, feel free to add
Reviewed-by: Chia-I Wu <olvaffe@gmail.com>
And because they are so minor, feel free to merge without sending out v3.
>
> Thanks,
> Steve
>
> >>
> >> No major changes in this version, for more details, check the changelog
> >> in each patch.
> >>
> >> 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 | 190 ++++++++++++------------
> >> 1 file changed, 98 insertions(+), 92 deletions(-)
> >>
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread