Re: [PATCH 07/10] drm/panthor: Automate CSG IRQ processing at group unbind time

Steven Price
2026-05-01T13:53:26Z

On 29/04/2026 10:38, Boris Brezillon wrote:
> Make the sched_process_csg_irq_locked() call part of
> group_unbind_locked() so we don't have to manually call it in
> tick_ctx_apply()/panthor_sched_suspend().
>
> This implies moving group_[un]bind_locked() around to avoid a
> forward declaration.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> drivers/gpu/drm/panthor/panthor_sched.c | 178 +++++++++++++++-----------------
> 1 file changed, 82 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index c197bdc4b2c7..601a9bff1485 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -982,86 +982,6 @@ group_get(struct panthor_group *group)
> return group;
> }
>
> -/**
> - * group_bind_locked() - Bind a group to a group slot
> - * @group: Group.
> - * @csg_id: Slot.
> - *
> - * Return: 0 on success, a negative error code otherwise.
> - */
> -static int
> -group_bind_locked(struct panthor_group *group, u32 csg_id)
> -{
> - struct panthor_device *ptdev = group->ptdev;
> - int ret;
> -
> - lockdep_assert_held(&ptdev->scheduler->lock);
> -
> - if (drm_WARN_ON(&ptdev->base, group->csg_id != -1 || csg_id >= MAX_CSGS ||
> - ptdev->scheduler->csg_slots[csg_id].group))
> - return -EINVAL;
> -
> - ret = panthor_vm_active(group->vm);
> - if (ret)
> - return ret;
> -
> - group_get(group);
> -
> - /* Dummy doorbell allocation: doorbell is assigned to the group and
> - * all queues use the same doorbell.
> - *
> - * TODO: Implement LRU-based doorbell assignment, so the most often
> - * updated queues get their own doorbell, thus avoiding useless checks
> - * on queues belonging to the same group that are rarely updated.
> - */
> - for (u32 i = 0; i < group->queue_count; i++)
> - group->queues[i]->doorbell_id = csg_id + 1;
> -
> - scoped_guard(spinlock_irqsave, &ptdev->scheduler->events_lock) {
> - ptdev->scheduler->csg_slots[csg_id].group = group;
> - group->csg_id = csg_id;
> - }
> -
> - return 0;
> -}
> -
> -/**
> - * group_unbind_locked() - Unbind a group from a slot.
> - * @group: Group to unbind.
> - *
> - * Return: 0 on success, a negative error code otherwise.
> - */
> -static int
> -group_unbind_locked(struct panthor_group *group)
> -{
> - struct panthor_device *ptdev = group->ptdev;
> -
> - lockdep_assert_held(&ptdev->scheduler->lock);
> -
> - if (drm_WARN_ON(&ptdev->base, group->csg_id < 0 || group->csg_id >= MAX_CSGS))
> - return -EINVAL;
> -
> - if (drm_WARN_ON(&ptdev->base, group->state == PANTHOR_CS_GROUP_ACTIVE))
> - return -EINVAL;
> -
> - scoped_guard(spinlock_irqsave, &ptdev->scheduler->events_lock) {
> - ptdev->scheduler->csg_slots[group->csg_id].group = NULL;
> - group->csg_id = -1;
> - }
> -
> - panthor_vm_idle(group->vm);
> -
> - /* Tiler OOM events will be re-issued next time the group is scheduled. */
> - atomic_set(&group->tiler_oom, 0);
> - cancel_work(&group->tiler_oom_work);
> -
> - for (u32 i = 0; i < group->queue_count; i++)
> - group->queues[i]->doorbell_id = -1;
> -
> - group_put(group);
> - return 0;
> -}
> -
> static bool
> group_is_idle(struct panthor_group *group)
> {
> @@ -1968,6 +1888,88 @@ void panthor_sched_report_fw_events(struct panthor_device *ptdev, u32 events)
> }
> }
>
> +/**
> + * group_bind_locked() - Bind a group to a group slot
> + * @group: Group.
> + * @csg_id: Slot.
> + *
> + * Return: 0 on success, a negative error code otherwise.
> + */
> +static int
> +group_bind_locked(struct panthor_group *group, u32 csg_id)
> +{
> + struct panthor_device *ptdev = group->ptdev;
> + int ret;
> +
> + lockdep_assert_held(&ptdev->scheduler->lock);
> +
> + if (drm_WARN_ON(&ptdev->base, group->csg_id != -1 || csg_id >= MAX_CSGS ||
> + ptdev->scheduler->csg_slots[csg_id].group))
> + return -EINVAL;
> +
> + ret = panthor_vm_active(group->vm);
> + if (ret)
> + return ret;
> +
> + group_get(group);
> +
> + /* Dummy doorbell allocation: doorbell is assigned to the group and
> + * all queues use the same doorbell.
> + *
> + * TODO: Implement LRU-based doorbell assignment, so the most often
> + * updated queues get their own doorbell, thus avoiding useless checks
> + * on queues belonging to the same group that are rarely updated.
> + */
> + for (u32 i = 0; i < group->queue_count; i++)
> + group->queues[i]->doorbell_id = csg_id + 1;
> +
> + scoped_guard(spinlock_irqsave, &ptdev->scheduler->events_lock) {
> + ptdev->scheduler->csg_slots[csg_id].group = group;
> + group->csg_id = csg_id;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * group_unbind_locked() - Unbind a group from a slot.
> + * @group: Group to unbind.
> + *
> + * Return: 0 on success, a negative error code otherwise.
> + */
> +static int
> +group_unbind_locked(struct panthor_group *group)
> +{
> + struct panthor_device *ptdev = group->ptdev;
> +
> + lockdep_assert_held(&ptdev->scheduler->lock);
> +
> + if (drm_WARN_ON(&ptdev->base, group->csg_id < 0 || group->csg_id >= MAX_CSGS))
> + return -EINVAL;
> +
> + if (drm_WARN_ON(&ptdev->base, group->state == PANTHOR_CS_GROUP_ACTIVE))
> + return -EINVAL;
> +
> + scoped_guard(spinlock_irqsave, &ptdev->scheduler->events_lock) {
> + /* Process all pending IRQs before returning the slot. */
> + sched_process_csg_irq_locked(ptdev, group->csg_id);
> + ptdev->scheduler->csg_slots[group->csg_id].group = NULL;
> + group->csg_id = -1;
> + }
> +
> + panthor_vm_idle(group->vm);
> +
> + /* Tiler OOM events will be re-issued next time the group is scheduled. */
> + atomic_set(&group->tiler_oom, 0);
> + cancel_work(&group->tiler_oom_work);
> +
> + for (u32 i = 0; i < group->queue_count; i++)
> + group->queues[i]->doorbell_id = -1;
> +
> + group_put(group);
> + return 0;
> +}
> +
> static const char *fence_get_driver_name(struct dma_fence *fence)
> {
> return "panthor";
> @@ -2396,15 +2398,6 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c
> /* 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) {
> - guard(spinlock_irqsave)(&sched->events_lock);
> - sched_process_csg_irq_locked(ptdev, group->csg_id);
> - }
> -
> group_unbind_locked(group);
> }
> }
> @@ -2970,8 +2963,6 @@ void panthor_sched_suspend(struct panthor_device *ptdev)
>
> if (flush_caches_failed)
> csg_slot->group->state = PANTHOR_CS_GROUP_TERMINATED;
> - else
> - csg_slot_sync_update_locked(ptdev, csg_id);

The justification for this change doesn't seem to be included in the
commit message and looks suspicious. Although AFAICT the events_lock
wouldn't be held here so it could trigger a lockdep assert before this
change...

Thanks,
Steve

>
> slot_mask &= ~BIT(csg_id);
> }
> @@ -2986,11 +2977,6 @@ void panthor_sched_suspend(struct panthor_device *ptdev)
>
> group_get(group);
>
> - if (group->csg_id >= 0) {
> - guard(spinlock_irqsave)(&sched->events_lock);
> - sched_process_csg_irq_locked(ptdev, group->csg_id);
> - }
> -
> group_unbind_locked(group);
>
> drm_WARN_ON(&group->ptdev->base, !list_empty(&group->run_node));
>