On Wed, 2025-09-03 at 11:18 +0100, Tvrtko Ursulin wrote: > There is no need to keep entities with no jobs in the tree so lets remove > it once the last job is consumed. This keeps the tree smaller which is > nicer and more efficient as entities are removed and re-added on every > popped job.
This reads suspiciously as if it could be an independent patch, not necessarily tied to this series. I see it depends on the _pop() function you added. I think you'd want to make it a bit more obvious that this is not so much a general improvement as it is a preparation for followup work. Or could it be made generic for the current in-tree scheduler? > > Apart from that, the upcoming fair scheduling algorithm will rely on the > tree only containing runnable entities. > > Signed-off-by: Tvrtko Ursulin <[email protected]> > Cc: Christian König <[email protected]> > Cc: Danilo Krummrich <[email protected]> > Cc: Matthew Brost <[email protected]> > Cc: Philipp Stanner <[email protected]> > --- > drivers/gpu/drm/scheduler/sched_rq.c | 28 +++++++++++++++++----------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/scheduler/sched_rq.c > b/drivers/gpu/drm/scheduler/sched_rq.c > index 16d57461765e..67804815ca0d 100644 > --- a/drivers/gpu/drm/scheduler/sched_rq.c > +++ b/drivers/gpu/drm/scheduler/sched_rq.c > @@ -19,6 +19,9 @@ drm_sched_entity_compare_before(struct rb_node *a, const > struct rb_node *b) > static void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity, > struct drm_sched_rq *rq) > { > + lockdep_assert_held(&entity->lock); > + lockdep_assert_held(&rq->lock); The callers of drm_sched_rq_remove_fifo_locked() already have some lockdep asserts, have you seen them? Are those here redundant / additional? And are they strictly related to this patch? Maybe you want to investigate the other lockdep assertions and, if there's room for improvement, address that in a dedicated patch. P. > + > if (!RB_EMPTY_NODE(&entity->rb_tree_node)) { > rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root); > RB_CLEAR_NODE(&entity->rb_tree_node); > @@ -158,24 +161,27 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity > *entity) > { > struct drm_sched_job *next_job; > struct drm_sched_rq *rq; > - ktime_t ts; > > /* > * Update the entity's location in the min heap according to > * the timestamp of the next job, if any. > */ > + spin_lock(&entity->lock); > + rq = entity->rq; > + spin_lock(&rq->lock); > next_job = drm_sched_entity_queue_peek(entity); > - if (!next_job) > - return; > + if (next_job) { > + ktime_t ts; > > - spin_lock(&entity->lock); > - rq = entity->rq; > - spin_lock(&rq->lock); > - if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) > - ts = next_job->submit_ts; > - else > - ts = drm_sched_rq_get_rr_ts(rq, entity); > - drm_sched_rq_update_fifo_locked(entity, rq, ts); > + if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) > + ts = next_job->submit_ts; > + else > + ts = drm_sched_rq_get_rr_ts(rq, entity); > + > + drm_sched_rq_update_fifo_locked(entity, rq, ts); > + } else { > + drm_sched_rq_remove_fifo_locked(entity, rq); > + } > spin_unlock(&rq->lock); > spin_unlock(&entity->lock); > }
