On Tue, Aug 11, 2026 at 01:23:40PM +0200, Boris Brezillon wrote: > Add a specific spinlock for events processing so we can selectively > move some event processing to the threaded IRQ handler. For events to be > processed, we need to have access to the group attached to the CSG slot > which also forces us to protect the csg_slots[] updates with this > lock. > > Note that fatal_queues/timedout are turned into atomics to avoid having > to take the events_lock every time those are checked or updated. > > Signed-off-by: Boris Brezillon <[email protected]>
Reviewed-by: Liviu Dudau <[email protected]> Best regards, Liviu > --- > drivers/gpu/drm/panthor/panthor_sched.c | 123 > ++++++++++++++++++++------------ > 1 file changed, 78 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c > b/drivers/gpu/drm/panthor/panthor_sched.c > index 9adf1e21eb83..794105e239e6 100644 > --- a/drivers/gpu/drm/panthor/panthor_sched.c > +++ b/drivers/gpu/drm/panthor/panthor_sched.c > @@ -254,8 +254,21 @@ struct panthor_scheduler { > struct list_head waiting; > } groups; > > + /** > + * @events_lock: Lock taken when processing events. > + * > + * This also needs to be taken when csg_slots are updated, to make sure > + * the event processing logic doesn't touch groups that have left the > CSG > + * slot. > + */ > + spinlock_t events_lock; > + > /** > * @csg_slots: FW command stream group slots. > + * > + * Updates to these slots must happen with both panthor_scheduler::lock > and > + * panthor_scheduler::events_lock held. As a result, reads can happen > with > + * either of these locks held. > */ > struct panthor_csg_slot csg_slots[MAX_CSGS]; > > @@ -565,8 +578,13 @@ struct panthor_group { > /** @idle_queues: Bitmask reflecting the idle queues. */ > u32 idle_queues; > > - /** @fatal_queues: Bitmask reflecting the queues that hit a fatal > exception. */ > - u32 fatal_queues; > + /** > + * @fatal_queues: Bitmask reflecting the queues that hit a fatal > exception. > + * > + * This is an atomic because we don't want to acquire the events_lock > + * every time we need to check the group state. > + */ > + atomic_t fatal_queues; > > /** @tiler_oom: Mask of queues that have a tiler OOM event to process. > */ > atomic_t tiler_oom; > @@ -602,8 +620,14 @@ struct panthor_group { > * any timeout situation is unrecoverable, and the group becomes > useless. We > * simply wait for all references to be dropped so we can release the > group > * object. > + * > + * This is an atomic because it can be set from both a scheduling > context > + * (protected with panthor_scheduler::lock) and an event processing > context > + * (protected with panthor_scheduler::events_lock). We could protect > access > + * with the events_lock, but this is simpler to make it an atomic since > the > + * only allowed transition is false -> true. > */ > - bool timedout; > + atomic_t timedout; > > /** > * @innocent: True when the group becomes unusable because the group > suspension > @@ -996,7 +1020,6 @@ static int > group_bind_locked(struct panthor_group *group, u32 csg_id) > { > struct panthor_device *ptdev = group->ptdev; > - struct panthor_csg_slot *csg_slot; > int ret; > > lockdep_assert_held(&ptdev->scheduler->lock); > @@ -1009,9 +1032,7 @@ group_bind_locked(struct panthor_group *group, u32 > csg_id) > if (ret) > return ret; > > - csg_slot = &ptdev->scheduler->csg_slots[csg_id]; > group_get(group); > - group->csg_id = csg_id; > > /* Dummy doorbell allocation: doorbell is assigned to the group and > * all queues use the same doorbell. > @@ -1023,7 +1044,10 @@ group_bind_locked(struct panthor_group *group, u32 > csg_id) > for (u32 i = 0; i < group->queue_count; i++) > group->queues[i]->doorbell_id = csg_id + 1; > > - csg_slot->group = group; > + scoped_guard(spinlock, &ptdev->scheduler->events_lock) { > + ptdev->scheduler->csg_slots[csg_id].group = group; > + group->csg_id = csg_id; > + } > > return 0; > } > @@ -1038,7 +1062,6 @@ static int > group_unbind_locked(struct panthor_group *group) > { > struct panthor_device *ptdev = group->ptdev; > - struct panthor_csg_slot *slot; > > lockdep_assert_held(&ptdev->scheduler->lock); > > @@ -1048,9 +1071,12 @@ group_unbind_locked(struct panthor_group *group) > if (drm_WARN_ON(&ptdev->base, group->state == PANTHOR_CS_GROUP_ACTIVE)) > return -EINVAL; > > - slot = &ptdev->scheduler->csg_slots[group->csg_id]; > + 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); > - group->csg_id = -1; > > /* Tiler OOM events will be re-issued next time the group is scheduled. > */ > atomic_set(&group->tiler_oom, 0); > @@ -1060,8 +1086,6 @@ group_unbind_locked(struct panthor_group *group) > for (u32 i = 0; i < group->queue_count; i++) > group->queues[i]->doorbell_id = -1; > > - slot->group = NULL; > - > group_put(group); > return 0; > } > @@ -1079,8 +1103,9 @@ group_can_run(struct panthor_group *group) > { > return group->state != PANTHOR_CS_GROUP_TERMINATED && > group->state != PANTHOR_CS_GROUP_UNKNOWN_STATE && > - !group->destroyed && group->fatal_queues == 0 && > - !group->timedout; > + !group->destroyed && > + !atomic_read(&group->fatal_queues) && > + !atomic_read(&group->timedout); > } > > static bool > @@ -1479,7 +1504,7 @@ cs_slot_process_fatal_event_locked(struct > panthor_device *ptdev, > u32 fatal; > u64 info; > > - lockdep_assert_held(&sched->lock); > + lockdep_assert_held(&sched->events_lock); > > cs_iface = panthor_fw_get_cs_iface(ptdev, csg_id, cs_id); > fatal = cs_iface->output->fatal; > @@ -1489,7 +1514,7 @@ cs_slot_process_fatal_event_locked(struct > panthor_device *ptdev, > drm_warn(&ptdev->base, "CS_FATAL: pid=%d, comm=%s\n", > group->task_info.pid, group->task_info.comm); > > - group->fatal_queues |= BIT(cs_id); > + atomic_or(BIT(cs_id), &group->fatal_queues); > } > > if (CS_EXCEPTION_TYPE(fatal) == DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE) > { > @@ -1527,7 +1552,7 @@ cs_slot_process_fault_event_locked(struct > panthor_device *ptdev, > u32 fault; > u64 info; > > - lockdep_assert_held(&sched->lock); > + lockdep_assert_held(&sched->events_lock); > > cs_iface = panthor_fw_get_cs_iface(ptdev, csg_id, cs_id); > fault = cs_iface->output->fault; > @@ -1619,7 +1644,7 @@ static int group_process_tiler_oom(struct panthor_group > *group, u32 cs_id) > */ > if (ret && ret != -ENOMEM) { > drm_warn(&ptdev->base, "Failed to extend the tiler heap\n"); > - group->fatal_queues |= BIT(cs_id); > + atomic_or(BIT(cs_id), &group->fatal_queues); > sched_queue_delayed_work(sched, tick, 0); > goto out_put_heap_pool; > } > @@ -1679,7 +1704,7 @@ cs_slot_process_tiler_oom_event_locked(struct > panthor_device *ptdev, > struct panthor_csg_slot *csg_slot = &sched->csg_slots[csg_id]; > struct panthor_group *group = csg_slot->group; > > - lockdep_assert_held(&sched->lock); > + lockdep_assert_held(&sched->events_lock); > > if (drm_WARN_ON(&ptdev->base, !group)) > return; > @@ -1700,7 +1725,7 @@ static bool cs_slot_process_irq_locked(struct > panthor_device *ptdev, > struct panthor_fw_cs_iface *cs_iface; > u32 req, ack, events; > > - lockdep_assert_held(&ptdev->scheduler->lock); > + lockdep_assert_held(&ptdev->scheduler->events_lock); > > cs_iface = panthor_fw_get_cs_iface(ptdev, csg_id, cs_id); > req = cs_iface->input->req; > @@ -1728,7 +1753,7 @@ static void csg_slot_process_idle_event_locked(struct > panthor_device *ptdev, u32 > { > struct panthor_scheduler *sched = ptdev->scheduler; > > - lockdep_assert_held(&sched->lock); > + lockdep_assert_held(&sched->events_lock); > > /* Schedule a tick so we can evict idle groups and schedule non-idle > * ones. This will also update runtime PM and devfreq busy/idle states, > @@ -1743,7 +1768,7 @@ static void csg_slot_sync_update_locked(struct > panthor_device *ptdev, > struct panthor_csg_slot *csg_slot = > &ptdev->scheduler->csg_slots[csg_id]; > struct panthor_group *group = csg_slot->group; > > - lockdep_assert_held(&ptdev->scheduler->lock); > + lockdep_assert_held(&ptdev->scheduler->events_lock); > > if (group) > group_queue_work(group, sync_upd); > @@ -1758,14 +1783,14 @@ csg_slot_process_progress_timer_event_locked(struct > panthor_device *ptdev, u32 c > struct panthor_csg_slot *csg_slot = &sched->csg_slots[csg_id]; > struct panthor_group *group = csg_slot->group; > > - lockdep_assert_held(&sched->lock); > + lockdep_assert_held(&sched->events_lock); > > group = csg_slot->group; > if (!drm_WARN_ON(&ptdev->base, !group)) { > drm_warn(&ptdev->base, "CSG_PROGRESS_TIMER_EVENT: pid=%d, > comm=%s\n", > group->task_info.pid, group->task_info.comm); > > - group->timedout = true; > + atomic_set(&group->timedout, true); > } > > drm_warn(&ptdev->base, "CSG slot %d progress timeout\n", csg_id); > @@ -1779,7 +1804,7 @@ static void sched_process_csg_irq_locked(struct > panthor_device *ptdev, u32 csg_i > struct panthor_fw_csg_iface *csg_iface; > u32 ring_cs_db_mask = 0; > > - lockdep_assert_held(&ptdev->scheduler->lock); > + lockdep_assert_held(&ptdev->scheduler->events_lock); > > if (drm_WARN_ON(&ptdev->base, csg_id >= > ptdev->scheduler->csg_slot_count)) > return; > @@ -1837,7 +1862,7 @@ static void sched_process_idle_event_locked(struct > panthor_device *ptdev) > { > struct panthor_fw_global_iface *glb_iface = > panthor_fw_get_glb_iface(ptdev); > > - lockdep_assert_held(&ptdev->scheduler->lock); > + lockdep_assert_held(&ptdev->scheduler->events_lock); > > /* Acknowledge the idle event and schedule a tick. */ > panthor_fw_update_reqs(glb_iface, req, glb_iface->output->ack, > GLB_IDLE); > @@ -1853,7 +1878,7 @@ static void sched_process_global_irq_locked(struct > panthor_device *ptdev) > struct panthor_fw_global_iface *glb_iface = > panthor_fw_get_glb_iface(ptdev); > u32 req, ack, evts; > > - lockdep_assert_held(&ptdev->scheduler->lock); > + lockdep_assert_held(&ptdev->scheduler->events_lock); > > req = READ_ONCE(glb_iface->input->req); > ack = READ_ONCE(glb_iface->output->ack); > @@ -1870,7 +1895,7 @@ static void process_fw_events_work(struct work_struct > *work) > u32 events = atomic_xchg(&sched->fw_events, 0); > struct panthor_device *ptdev = sched->ptdev; > > - mutex_lock(&sched->lock); > + guard(spinlock)(&sched->events_lock); > > if (events & JOB_INT_GLOBAL_IF) { > sched_process_global_irq_locked(ptdev); > @@ -1883,8 +1908,6 @@ static void process_fw_events_work(struct work_struct > *work) > sched_process_csg_irq_locked(ptdev, csg_id); > events &= ~BIT(csg_id); > } > - > - mutex_unlock(&sched->lock); > } > > /** > @@ -2131,11 +2154,12 @@ tick_ctx_init(struct panthor_scheduler *sched, > * CSG IRQs, so we can flag the faulty queue. > */ > if (panthor_vm_has_unhandled_faults(group->vm)) { > - sched_process_csg_irq_locked(ptdev, i); > + scoped_guard(spinlock, &sched->events_lock) > + sched_process_csg_irq_locked(ptdev, i); > > /* No fatal fault reported, flag all queues as faulty. > */ > - if (!group->fatal_queues) > - group->fatal_queues |= > GENMASK(group->queue_count - 1, 0); > + atomic_cmpxchg(&group->fatal_queues, 0, > + GENMASK(group->queue_count - 1, 0)); > } > > tick_ctx_insert_old_group(sched, ctx, group); > @@ -2168,9 +2192,9 @@ group_term_post_processing(struct panthor_group *group) > struct panthor_syncobj_64b *syncobj; > int err; > > - if (group->fatal_queues & BIT(i)) > + if (atomic_read(&group->fatal_queues) & BIT(i)) > err = -EINVAL; > - else if (group->timedout) > + else if (atomic_read(&group->timedout)) > err = -ETIMEDOUT; > else > err = -ECANCELED; > @@ -2331,8 +2355,10 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct > panthor_sched_tick_ctx *c > * any pending interrupts before we start the new > * group. > */ > - if (group->csg_id >= 0) > + if (group->csg_id >= 0) { > + guard(spinlock)(&sched->events_lock); > sched_process_csg_irq_locked(ptdev, > group->csg_id); > + } > > group_unbind_locked(group); > } > @@ -2861,7 +2887,7 @@ void panthor_sched_suspend(struct panthor_device *ptdev) > /* We consider group suspension failures as fatal and > flag the > * group as unusable by setting timedout=true. > */ > - csg_slot->group->timedout = true; > + atomic_set(&csg_slot->group->timedout, true); > > csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id, > CSG_STATE_TERMINATE, > @@ -2910,10 +2936,12 @@ void panthor_sched_suspend(struct panthor_device > *ptdev) > u32 csg_id = ffs(slot_mask) - 1; > struct panthor_csg_slot *csg_slot = > &sched->csg_slots[csg_id]; > > - if (flush_caches_failed) > + if (flush_caches_failed) { > csg_slot->group->state = > PANTHOR_CS_GROUP_TERMINATED; > - else > + } else { > + guard(spinlock)(&sched->events_lock); > csg_slot_sync_update_locked(ptdev, csg_id); > + } > > slot_mask &= ~BIT(csg_id); > } > @@ -2928,8 +2956,10 @@ void panthor_sched_suspend(struct panthor_device > *ptdev) > > group_get(group); > > - if (group->csg_id >= 0) > + if (group->csg_id >= 0) { > + guard(spinlock)(&sched->events_lock); > sched_process_csg_irq_locked(ptdev, group->csg_id); > + } > > group_unbind_locked(group); > > @@ -3421,7 +3451,7 @@ queue_timedout_job(struct drm_sched_job *sched_job) > queue_stop(queue, job); > > mutex_lock(&sched->lock); > - group->timedout = true; > + atomic_set(&group->timedout, true); > if (group->csg_id >= 0) { > sched_queue_delayed_work(ptdev->scheduler, tick, 0); > } else { > @@ -3843,12 +3873,13 @@ int panthor_group_get_state(struct panthor_file > *pfile, > memset(get_state, 0, sizeof(*get_state)); > > mutex_lock(&sched->lock); > - if (group->timedout) > + if (atomic_read(&group->timedout)) > get_state->state |= DRM_PANTHOR_GROUP_STATE_TIMEDOUT; > - if (group->fatal_queues) { > + > + get_state->fatal_queues = atomic_read(&group->fatal_queues); > + if (get_state->fatal_queues) > get_state->state |= DRM_PANTHOR_GROUP_STATE_FATAL_FAULT; > - get_state->fatal_queues = group->fatal_queues; > - } > + > if (group->innocent) > get_state->state |= DRM_PANTHOR_GROUP_STATE_INNOCENT; > mutex_unlock(&sched->lock); > @@ -4146,6 +4177,8 @@ int panthor_sched_init(struct panthor_device *ptdev) > INIT_WORK(&sched->sync_upd_work, sync_upd_work); > INIT_WORK(&sched->fw_events_work, process_fw_events_work); > > + spin_lock_init(&sched->events_lock); > + > ret = drmm_mutex_init(&ptdev->base, &sched->lock); > if (ret) > return ret; > > -- > 2.55.0 > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯
