* Re: [PATCH v6 10/14] drm/panthor: Add the scheduler logical block
[not found] ` <20240229162230.2634044-11-boris.brezillon@collabora.com>
@ 2024-03-28 15:38 ` Nathan Chancellor
2024-03-28 15:51 ` Boris Brezillon
0 siblings, 1 reply; 2+ messages in thread
From: Nathan Chancellor @ 2024-03-28 15:38 UTC (permalink / raw)
To: Boris Brezillon
Cc: dri-devel, Daniel Vetter, Marty E . Plummer, Rob Herring,
Clément Péron, Nicolas Boichat, Neil Armstrong,
Faith Ekstrand, Daniel Stone, Liviu Dudau, Steven Price,
Robin Murphy, kernel, Heiko Stuebner, Tatsuyuki Ishi,
Chris Diamand, Ketil Johnsen, Maxime Ripard, llvm
Hi Boris,
On Thu, Feb 29, 2024 at 05:22:24PM +0100, Boris Brezillon wrote:
<snip>
> ---
> drivers/gpu/drm/panthor/panthor_sched.c | 3502 +++++++++++++++++++++++
> drivers/gpu/drm/panthor/panthor_sched.h | 50 +
> 2 files changed, 3552 insertions(+)
> create mode 100644 drivers/gpu/drm/panthor/panthor_sched.c
> create mode 100644 drivers/gpu/drm/panthor/panthor_sched.h
>
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> new file mode 100644
> index 000000000000..5f7803b6fc48
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
<snip>
> +static void
> +tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *ctx)
> +{
> + struct panthor_group *group, *tmp;
> + struct panthor_device *ptdev = sched->ptdev;
> + struct panthor_csg_slot *csg_slot;
> + int prio, new_csg_prio = MAX_CSG_PRIO, i;
> + u32 csg_mod_mask = 0, free_csg_slots = 0;
> + struct panthor_csg_slots_upd_ctx upd_ctx;
> + int ret;
> +
> + csgs_upd_ctx_init(&upd_ctx);
> +
> + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) {
> + /* Suspend or terminate evicted groups. */
> + list_for_each_entry(group, &ctx->old_groups[prio], run_node) {
> + bool term = !group_can_run(group);
> + int csg_id = group->csg_id;
> +
> + if (drm_WARN_ON(&ptdev->base, csg_id < 0))
> + continue;
> +
> + csg_slot = &sched->csg_slots[csg_id];
> + csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
> + term ? CSG_STATE_TERMINATE : CSG_STATE_SUSPEND,
> + CSG_STATE_MASK);
> + }
> +
> + /* Update priorities on already running groups. */
> + list_for_each_entry(group, &ctx->groups[prio], run_node) {
> + struct panthor_fw_csg_iface *csg_iface;
> + int csg_id = group->csg_id;
> +
> + if (csg_id < 0) {
> + new_csg_prio--;
> + continue;
> + }
> +
> + csg_slot = &sched->csg_slots[csg_id];
> + csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> + if (csg_slot->priority == new_csg_prio) {
> + new_csg_prio--;
> + continue;
> + }
> +
> + panthor_fw_update_reqs(csg_iface, endpoint_req,
> + CSG_EP_REQ_PRIORITY(new_csg_prio),
> + CSG_EP_REQ_PRIORITY_MASK);
> + csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
> + csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG,
> + CSG_ENDPOINT_CONFIG);
> + new_csg_prio--;
> + }
> + }
> +
> + ret = csgs_upd_ctx_apply_locked(ptdev, &upd_ctx);
> + if (ret) {
> + panthor_device_schedule_reset(ptdev);
> + ctx->csg_upd_failed_mask |= upd_ctx.timedout_mask;
> + return;
> + }
> +
> + /* Unbind evicted groups. */
> + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) {
> + list_for_each_entry(group, &ctx->old_groups[prio], run_node) {
> + /* This group is gone. Process interrupts to clear
> + * any pending interrupts before we start the new
> + * group.
> + */
> + if (group->csg_id >= 0)
> + sched_process_csg_irq_locked(ptdev, group->csg_id);
> +
> + group_unbind_locked(group);
> + }
> + }
> +
> + for (i = 0; i < sched->csg_slot_count; i++) {
> + if (!sched->csg_slots[i].group)
> + free_csg_slots |= BIT(i);
> + }
> +
> + csgs_upd_ctx_init(&upd_ctx);
> + new_csg_prio = MAX_CSG_PRIO;
> +
> + /* Start new groups. */
> + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) {
> + list_for_each_entry(group, &ctx->groups[prio], run_node) {
> + int csg_id = group->csg_id;
> + struct panthor_fw_csg_iface *csg_iface;
> +
> + if (csg_id >= 0) {
> + new_csg_prio--;
> + continue;
> + }
> +
> + csg_id = ffs(free_csg_slots) - 1;
> + if (drm_WARN_ON(&ptdev->base, csg_id < 0))
> + break;
> +
> + csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> + csg_slot = &sched->csg_slots[csg_id];
> + csg_mod_mask |= BIT(csg_id);
> + group_bind_locked(group, csg_id);
> + csg_slot_prog_locked(ptdev, csg_id, new_csg_prio--);
> + csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
> + group->state == PANTHOR_CS_GROUP_SUSPENDED ?
> + CSG_STATE_RESUME : CSG_STATE_START,
> + CSG_STATE_MASK);
> + csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
> + csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG,
> + CSG_ENDPOINT_CONFIG);
> + free_csg_slots &= ~BIT(csg_id);
> + }
> + }
> +
> + ret = csgs_upd_ctx_apply_locked(ptdev, &upd_ctx);
> + if (ret) {
> + panthor_device_schedule_reset(ptdev);
> + ctx->csg_upd_failed_mask |= upd_ctx.timedout_mask;
> + return;
> + }
> +
> + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) {
> + list_for_each_entry_safe(group, tmp, &ctx->groups[prio], run_node) {
> + list_del_init(&group->run_node);
> +
> + /* If the group has been destroyed while we were
> + * scheduling, ask for an immediate tick to
> + * re-evaluate as soon as possible and get rid of
> + * this dangling group.
> + */
> + if (group->destroyed)
> + ctx->immediate_tick = true;
> + group_put(group);
> + }
> +
> + /* Return evicted groups to the idle or run queues. Groups
> + * that can no longer be run (because they've been destroyed
> + * or experienced an unrecoverable error) will be scheduled
> + * for destruction in tick_ctx_cleanup().
> + */
> + list_for_each_entry_safe(group, tmp, &ctx->old_groups[prio], run_node) {
> + if (!group_can_run(group))
> + continue;
> +
> + if (group_is_idle(group))
> + list_move_tail(&group->run_node, &sched->groups.idle[prio]);
> + else
> + list_move_tail(&group->run_node, &sched->groups.runnable[prio]);
> + group_put(group);
> + }
> + }
> +
> + sched->used_csg_slot_count = ctx->group_count;
> + sched->might_have_idle_groups = ctx->idle_group_count > 0;
> +}
Clang builds fail after this change in -next as commit
de8548813824 ("drm/panthor: Add the scheduler logical block"):
drivers/gpu/drm/panthor/panthor_sched.c:2048:6: error: variable 'csg_mod_mask' set but not used [-Werror,-Wunused-but-set-variable]
2048 | u32 csg_mod_mask = 0, free_csg_slots = 0;
| ^
1 error generated.
It appears legitimate to me, csg_mod_mask is only updated with '|=' but
never accessed in any other manner. Should the variable be removed or
was it supposed to be used somewhere else?
Cheers,
Nathan
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 5f7803b6fc48..e5a710f190d2 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2045,7 +2045,7 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c
struct panthor_device *ptdev = sched->ptdev;
struct panthor_csg_slot *csg_slot;
int prio, new_csg_prio = MAX_CSG_PRIO, i;
- u32 csg_mod_mask = 0, free_csg_slots = 0;
+ u32 free_csg_slots = 0;
struct panthor_csg_slots_upd_ctx upd_ctx;
int ret;
@@ -2139,7 +2139,6 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c
csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
csg_slot = &sched->csg_slots[csg_id];
- csg_mod_mask |= BIT(csg_id);
group_bind_locked(group, csg_id);
csg_slot_prog_locked(ptdev, csg_id, new_csg_prio--);
csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v6 10/14] drm/panthor: Add the scheduler logical block
2024-03-28 15:38 ` [PATCH v6 10/14] drm/panthor: Add the scheduler logical block Nathan Chancellor
@ 2024-03-28 15:51 ` Boris Brezillon
0 siblings, 0 replies; 2+ messages in thread
From: Boris Brezillon @ 2024-03-28 15:51 UTC (permalink / raw)
To: Nathan Chancellor
Cc: dri-devel, Daniel Vetter, Marty E . Plummer, Rob Herring,
Clément Péron, Nicolas Boichat, Neil Armstrong,
Faith Ekstrand, Daniel Stone, Liviu Dudau, Steven Price,
Robin Murphy, kernel, Heiko Stuebner, Tatsuyuki Ishi,
Chris Diamand, Ketil Johnsen, Maxime Ripard, llvm
Hi Nathan,
On Thu, 28 Mar 2024 08:38:09 -0700
Nathan Chancellor <nathan@kernel.org> wrote:
> Hi Boris,
>
> On Thu, Feb 29, 2024 at 05:22:24PM +0100, Boris Brezillon wrote:
> <snip>
> > ---
> > drivers/gpu/drm/panthor/panthor_sched.c | 3502 +++++++++++++++++++++++
> > drivers/gpu/drm/panthor/panthor_sched.h | 50 +
> > 2 files changed, 3552 insertions(+)
> > create mode 100644 drivers/gpu/drm/panthor/panthor_sched.c
> > create mode 100644 drivers/gpu/drm/panthor/panthor_sched.h
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> > new file mode 100644
> > index 000000000000..5f7803b6fc48
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> <snip>
> > +static void
> > +tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *ctx)
> > +{
> > + struct panthor_group *group, *tmp;
> > + struct panthor_device *ptdev = sched->ptdev;
> > + struct panthor_csg_slot *csg_slot;
> > + int prio, new_csg_prio = MAX_CSG_PRIO, i;
> > + u32 csg_mod_mask = 0, free_csg_slots = 0;
> > + struct panthor_csg_slots_upd_ctx upd_ctx;
> > + int ret;
> > +
> > + csgs_upd_ctx_init(&upd_ctx);
> > +
> > + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) {
> > + /* Suspend or terminate evicted groups. */
> > + list_for_each_entry(group, &ctx->old_groups[prio], run_node) {
> > + bool term = !group_can_run(group);
> > + int csg_id = group->csg_id;
> > +
> > + if (drm_WARN_ON(&ptdev->base, csg_id < 0))
> > + continue;
> > +
> > + csg_slot = &sched->csg_slots[csg_id];
> > + csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
> > + term ? CSG_STATE_TERMINATE : CSG_STATE_SUSPEND,
> > + CSG_STATE_MASK);
> > + }
> > +
> > + /* Update priorities on already running groups. */
> > + list_for_each_entry(group, &ctx->groups[prio], run_node) {
> > + struct panthor_fw_csg_iface *csg_iface;
> > + int csg_id = group->csg_id;
> > +
> > + if (csg_id < 0) {
> > + new_csg_prio--;
> > + continue;
> > + }
> > +
> > + csg_slot = &sched->csg_slots[csg_id];
> > + csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> > + if (csg_slot->priority == new_csg_prio) {
> > + new_csg_prio--;
> > + continue;
> > + }
> > +
> > + panthor_fw_update_reqs(csg_iface, endpoint_req,
> > + CSG_EP_REQ_PRIORITY(new_csg_prio),
> > + CSG_EP_REQ_PRIORITY_MASK);
> > + csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
> > + csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG,
> > + CSG_ENDPOINT_CONFIG);
> > + new_csg_prio--;
> > + }
> > + }
> > +
> > + ret = csgs_upd_ctx_apply_locked(ptdev, &upd_ctx);
> > + if (ret) {
> > + panthor_device_schedule_reset(ptdev);
> > + ctx->csg_upd_failed_mask |= upd_ctx.timedout_mask;
> > + return;
> > + }
> > +
> > + /* Unbind evicted groups. */
> > + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) {
> > + list_for_each_entry(group, &ctx->old_groups[prio], run_node) {
> > + /* This group is gone. Process interrupts to clear
> > + * any pending interrupts before we start the new
> > + * group.
> > + */
> > + if (group->csg_id >= 0)
> > + sched_process_csg_irq_locked(ptdev, group->csg_id);
> > +
> > + group_unbind_locked(group);
> > + }
> > + }
> > +
> > + for (i = 0; i < sched->csg_slot_count; i++) {
> > + if (!sched->csg_slots[i].group)
> > + free_csg_slots |= BIT(i);
> > + }
> > +
> > + csgs_upd_ctx_init(&upd_ctx);
> > + new_csg_prio = MAX_CSG_PRIO;
> > +
> > + /* Start new groups. */
> > + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) {
> > + list_for_each_entry(group, &ctx->groups[prio], run_node) {
> > + int csg_id = group->csg_id;
> > + struct panthor_fw_csg_iface *csg_iface;
> > +
> > + if (csg_id >= 0) {
> > + new_csg_prio--;
> > + continue;
> > + }
> > +
> > + csg_id = ffs(free_csg_slots) - 1;
> > + if (drm_WARN_ON(&ptdev->base, csg_id < 0))
> > + break;
> > +
> > + csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> > + csg_slot = &sched->csg_slots[csg_id];
> > + csg_mod_mask |= BIT(csg_id);
> > + group_bind_locked(group, csg_id);
> > + csg_slot_prog_locked(ptdev, csg_id, new_csg_prio--);
> > + csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
> > + group->state == PANTHOR_CS_GROUP_SUSPENDED ?
> > + CSG_STATE_RESUME : CSG_STATE_START,
> > + CSG_STATE_MASK);
> > + csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
> > + csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG,
> > + CSG_ENDPOINT_CONFIG);
> > + free_csg_slots &= ~BIT(csg_id);
> > + }
> > + }
> > +
> > + ret = csgs_upd_ctx_apply_locked(ptdev, &upd_ctx);
> > + if (ret) {
> > + panthor_device_schedule_reset(ptdev);
> > + ctx->csg_upd_failed_mask |= upd_ctx.timedout_mask;
> > + return;
> > + }
> > +
> > + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) {
> > + list_for_each_entry_safe(group, tmp, &ctx->groups[prio], run_node) {
> > + list_del_init(&group->run_node);
> > +
> > + /* If the group has been destroyed while we were
> > + * scheduling, ask for an immediate tick to
> > + * re-evaluate as soon as possible and get rid of
> > + * this dangling group.
> > + */
> > + if (group->destroyed)
> > + ctx->immediate_tick = true;
> > + group_put(group);
> > + }
> > +
> > + /* Return evicted groups to the idle or run queues. Groups
> > + * that can no longer be run (because they've been destroyed
> > + * or experienced an unrecoverable error) will be scheduled
> > + * for destruction in tick_ctx_cleanup().
> > + */
> > + list_for_each_entry_safe(group, tmp, &ctx->old_groups[prio], run_node) {
> > + if (!group_can_run(group))
> > + continue;
> > +
> > + if (group_is_idle(group))
> > + list_move_tail(&group->run_node, &sched->groups.idle[prio]);
> > + else
> > + list_move_tail(&group->run_node, &sched->groups.runnable[prio]);
> > + group_put(group);
> > + }
> > + }
> > +
> > + sched->used_csg_slot_count = ctx->group_count;
> > + sched->might_have_idle_groups = ctx->idle_group_count > 0;
> > +}
>
> Clang builds fail after this change in -next as commit
> de8548813824 ("drm/panthor: Add the scheduler logical block"):
>
> drivers/gpu/drm/panthor/panthor_sched.c:2048:6: error: variable 'csg_mod_mask' set but not used [-Werror,-Wunused-but-set-variable]
> 2048 | u32 csg_mod_mask = 0, free_csg_slots = 0;
> | ^
> 1 error generated.
>
> It appears legitimate to me, csg_mod_mask is only updated with '|=' but
> never accessed in any other manner. Should the variable be removed or
> was it supposed to be used somewhere else?
Thanks for the report!
It's a leftover from a refactor that happened in v2 or v3 of this
patchset, csg_mod_mask can be dropped (like you do in your diff).
Mind sending a proper patch to dri-devel@lists.freedesktop.org with me,
Steven and Liviu in Cc?
Regards,
Boris
>
> Cheers,
> Nathan
>
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 5f7803b6fc48..e5a710f190d2 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2045,7 +2045,7 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c
> struct panthor_device *ptdev = sched->ptdev;
> struct panthor_csg_slot *csg_slot;
> int prio, new_csg_prio = MAX_CSG_PRIO, i;
> - u32 csg_mod_mask = 0, free_csg_slots = 0;
> + u32 free_csg_slots = 0;
> struct panthor_csg_slots_upd_ctx upd_ctx;
> int ret;
>
> @@ -2139,7 +2139,6 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c
>
> csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> csg_slot = &sched->csg_slots[csg_id];
> - csg_mod_mask |= BIT(csg_id);
> group_bind_locked(group, csg_id);
> csg_slot_prog_locked(ptdev, csg_id, new_csg_prio--);
> csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-03-28 15:51 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240229162230.2634044-1-boris.brezillon@collabora.com>
[not found] ` <20240229162230.2634044-11-boris.brezillon@collabora.com>
2024-03-28 15:38 ` [PATCH v6 10/14] drm/panthor: Add the scheduler logical block Nathan Chancellor
2024-03-28 15:51 ` Boris Brezillon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox