Re: [PATCH v6 10/14] drm/panthor: Add the scheduler logical block

[Boris Brezillon <boris.brezillon@collabora.com>]

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,