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.
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?
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);
}
