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

[Boris Brezillon]

On Tue, 18 Aug 2026 18:20:58 +0100
Liviu Dudau <liviu.dudau@xxxxxxx> wrote:

> On Tue, Aug 11, 2026 at 01:23:43PM +0200, 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.  
> 
> Not sure why group_bind_locked() had to move. Aestethics?

Yeah, I tend to keep do/undo helpers close to each other in general. I
can make the code motion a separate commit if you prefer.

> 
> Makes the patch a bit harder to review as one function is copied
> verbatim while the other gets a line added. Minor sigh in the
> end, so please ignore.
> 
> Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>
> 
> Best regards,
> Liviu
> 
> > 
> > Reviewed-by: Steven Price <steven.price@xxxxxxx>
> > Reviewed-by: Chia-I Wu <olvaffe@xxxxxxxxx>
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/panthor/panthor_sched.c | 182 +++++++++++++++-----------------
> >  1 file changed, 84 insertions(+), 98 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> > index 8016b0a55173..57308cd90e96 100644
> > --- a/drivers/gpu/drm/panthor/panthor_sched.c
> > +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> > @@ -989,87 +989,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, &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, &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);
> > -	if (cancel_work(&group->tiler_oom_work))
> > -		group_put(group);
> > -
> > -	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)
> >  {
> > @@ -1979,6 +1898,89 @@ 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, &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, &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);
> > +	if (cancel_work(&group->tiler_oom_work))
> > +		group_put(group);
> > +
> > +	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";
> > @@ -2405,18 +2407,8 @@ 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)(&sched->events_lock);
> > -				sched_process_csg_irq_locked(ptdev, group->csg_id);
> > -			}
> > -
> > +		list_for_each_entry(group, &ctx->old_groups[prio], run_node)
> >  			group_unbind_locked(group);
> > -		}
> >  	}
> >  
> >  	for (i = 0; i < sched->csg_slot_count; i++) {
> > @@ -3010,12 +3002,6 @@ void panthor_sched_suspend(struct panthor_device *ptdev)
> >  			continue;
> >  
> >  		group_get(group);
> > -
> > -		if (group->csg_id >= 0) {
> > -			guard(spinlock)(&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));
> > 
> > -- 
> > 2.55.0
> >   
>