* 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