On Thu, 2025-09-11 at 16:06 +0100, Tvrtko Ursulin wrote: > > On 11/09/2025 15:32, Philipp Stanner wrote: > > 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? > > Both is true. There is currently no reason idle entities _need_ to be in > the tree. Removing them would improve O(log n) on the rbtree. But also > fair scheduler relies on it, see below... > > > > Apart from that, the upcoming fair scheduling algorithm will rely on the > > > tree only containing runnable entities. > > ... ^^^ here.
Yes, I saw that. I wanted to stress where I'm coming from: generic code improvements should ideally be posted as separate patches, because that makes it easier to review and quicker to merge (and easier to revert should a problem be detected before the subsequent CFS series is merged) So, can you submit this patch separately without too much effort? :) > > > > 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? > > Yes, drm_sched_rq_pop_entity() is the new caller, which needs to take > both locks on its own. So IMO makes sense to add the asserts. > > > Maybe you want to investigate the other lockdep assertions and, if > > there's room for improvement, address that in a dedicated patch. > > They look okay to me. Are you seeing something is off? No, they do look correct. It's just that we have a bit of redundancy then, but that's probably a good thing for robustness. P. > > Regards, > > Tvrtko > > > > + > > > 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); > > > } > > >
