On 12/05/2025 14:03, Philipp Stanner wrote:
On Fri, 2025-04-25 at 11:20 +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.
That there is no need to do so doesn't imply that you can't keep them
around. The commit message doesn't make the motivation clear
Signed-off-by: Tvrtko Ursulin <tvrtko.ursu...@igalia.com>
Cc: Christian König <christian.koe...@amd.com>
Cc: Danilo Krummrich <d...@kernel.org>
Cc: Matthew Brost <matthew.br...@intel.com>
Cc: Philipp Stanner <pha...@kernel.org>
---
drivers/gpu/drm/scheduler/sched_rq.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
Since this doesn't simplify the code base, I think the only
justification would be a somewhat decent performance gain. Does this
patch result in that?
Otherwise it's probably better to keep git-blame intact here.
I needed this for one of the earlier approaches and I *think* what
remains with the latest is just the fact it makes the run-queue contain
only runnable entities (which makes sense and is logical; run-queue <->
runnable). And that rb-tree re-balancing is cheaper with smaller trees
but in the grand scheme of things it is not something I even considered
attempting to measure.
I will re-consider the fate of this patch once more feedback on the
series as overall is received. Until then I don't think it makes sense
to churn it.
Btw another angle to this, which we touched upon with Christian before
is, if we end up not pruning the tree from unrunnable entities, then we
could drop the drm_sched_rq->entities list. Making a handful of caller
which walk it walk the tree instead.
Regards,
Tvrtko
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c
b/drivers/gpu/drm/scheduler/sched_rq.c
index d477a027feb9..2cde89cf25fb 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -149,25 +149,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;
- if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
- ts = next_job->submit_ts;
- else
- ts = drm_sched_rq_get_rr_deadline(rq);
+ if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
+ ts = next_job->submit_ts;
+ else
+ ts = drm_sched_rq_get_rr_deadline(rq);
- spin_lock(&entity->lock);
- rq = entity->rq;
- spin_lock(&rq->lock);
- drm_sched_rq_update_fifo_locked(entity, rq, ts);
+ 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);
}
