On 14/10/2025 09:52, Philipp Stanner wrote:
On Tue, 2025-10-14 at 08:26 +0100, Tvrtko Ursulin wrote:On 14/10/2025 07:53, Philipp Stanner wrote:On Sat, 2025-10-11 at 15:19 +0100, Tvrtko Ursulin wrote:On 10/10/2025 11:49, Philipp Stanner wrote:On Wed, 2025-10-08 at 09:53 +0100, Tvrtko Ursulin wrote:Move the code dealing with entities entering and exiting run queues to helpers to logically separate it from jobs entering and exiting 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_entity.c | 64 ++------------- drivers/gpu/drm/scheduler/sched_internal.h | 8 +- drivers/gpu/drm/scheduler/sched_main.c | 95 +++++++++++++++++++--- 3 files changed, 91 insertions(+), 76 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 4852006f2308..7a0a52ba87bf 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -456,24 +456,9 @@ drm_sched_job_dependency(struct drm_sched_job *job, return NULL; }-static ktime_t-drm_sched_rq_get_rr_ts(struct drm_sched_rq *rq, struct drm_sched_entity *entity) -{ - ktime_t ts; - - lockdep_assert_held(&entity->lock); - lockdep_assert_held(&rq->lock); - - ts = ktime_add_ns(rq->rr_ts, 1); - entity->rr_ts = ts; - rq->rr_ts = ts; - - return ts; -} - struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) { - struct drm_sched_job *sched_job, *next_job; + struct drm_sched_job *sched_job;`next_job` has been added in a previous job. Have you tried whether patch-order can be reversed? Just asking; I don't want to cause unnecessary work hereYou are correct that there would be some knock on effect on a few other patches in the series but it is definitely doable. Because for certain argument can be made it would be logical to have it like that. Both this patch and "drm/sched: Move run queue related code into a separate file" would be then moved ahead of "drm/sched: Implement RR via FIFO". If you prefer it like that I can reshuffle no problem.I mean, it seems to make the overall git diff smaller, which is nice? If you don't see a significant reason against it, I'd say it's a good idea.Okay deal. It isn't anything significant, just re-ordering patches with compile testing patches to ensure every step still builds.
Completed locally.
sched_job = drm_sched_entity_queue_peek(entity);if (!sched_job) @@ -502,26 +487,7 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)spsc_queue_pop(&entity->job_queue); - /*- * Update the entity's location in the min heap according to - * the timestamp of the next job, if any. - */ - next_job = drm_sched_entity_queue_peek(entity); - if (next_job) { - struct drm_sched_rq *rq; - 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); - spin_unlock(&rq->lock); - spin_unlock(&entity->lock); - } + drm_sched_rq_pop_entity(entity);/* Jobs and entities might have different lifecycles. Since we're* removing the job from the entities queue, set the jobs entity pointer @@ -611,30 +577,10 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job) /* first job wakes up scheduler */ if (first) { struct drm_gpu_scheduler *sched; - struct drm_sched_rq *rq;- /* Add the entity to the run queue */- spin_lock(&entity->lock); - if (entity->stopped) { - spin_unlock(&entity->lock); - - DRM_ERROR("Trying to push to a killed entity\n"); - return; - } - - rq = entity->rq; - sched = rq->sched; - - spin_lock(&rq->lock); - drm_sched_rq_add_entity(rq, entity); - if (drm_sched_policy == DRM_SCHED_POLICY_RR) - submit_ts = entity->rr_ts; - drm_sched_rq_update_fifo_locked(entity, rq, submit_ts); - - spin_unlock(&rq->lock); - spin_unlock(&entity->lock); - - drm_sched_wakeup(sched); + sched = drm_sched_rq_add_entity(entity, submit_ts); + if (sched) + drm_sched_wakeup(sched); } } EXPORT_SYMBOL(drm_sched_entity_push_job); diff --git a/drivers/gpu/drm/scheduler/sched_internal.h b/drivers/gpu/drm/scheduler/sched_internal.h index 7ea5a6736f98..8269c5392a82 100644 --- a/drivers/gpu/drm/scheduler/sched_internal.h +++ b/drivers/gpu/drm/scheduler/sched_internal.h @@ -12,13 +12,11 @@ extern int drm_sched_policy;void drm_sched_wakeup(struct drm_gpu_scheduler *sched); -void drm_sched_rq_add_entity(struct drm_sched_rq *rq,- struct drm_sched_entity *entity); +struct drm_gpu_scheduler * +drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts); void drm_sched_rq_remove_entity(struct drm_sched_rq *rq, struct drm_sched_entity *entity); - -void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity, - struct drm_sched_rq *rq, ktime_t ts); +void drm_sched_rq_pop_entity(struct drm_sched_entity *entity);void drm_sched_entity_select_rq(struct drm_sched_entity *entity);struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity); diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 8e62541b439a..e5d02c28665c 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -151,9 +151,9 @@ static void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity, } }-void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,- struct drm_sched_rq *rq, - ktime_t ts) +static void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity, + struct drm_sched_rq *rq, + ktime_t ts) { /* * Both locks need to be grabbed, one to protect from entity->rq change @@ -191,22 +191,45 @@ static void drm_sched_rq_init(struct drm_sched_rq *rq, /** * drm_sched_rq_add_entity - add an entity * - * @rq: scheduler run queue * @entity: scheduler entity + * @ts: submission timestamp * * Adds a scheduler entity to the run queue. + * + * Returns a DRM scheduler pre-selected to handle this entity. */ -void drm_sched_rq_add_entity(struct drm_sched_rq *rq, - struct drm_sched_entity *entity) +struct drm_gpu_scheduler * +drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts) {I'm not sure if it's a good idea to have the scheduler returned from that function. That doesn't make a whole lot of sense semantically. At the very least the function's docstring, maybe even its name, should be adjusted to detail why this makes sense. The commit message, too. It's not trivially understood. I think I get why it's being done, but writing it down black on white gives us something to grasp. Sth like "adds an entity to a runqueue, selects to appropriate scheduler and returns it for the purpose of XYZ"Yeah. Remeber your unlocked rq access slide and the discussion around it?Sure. Is that related, though? The slide was about many readers being totally unlocked. The current drm_sched_entity_push_job() locks readers correctly if I'm not mistaken.Currently we have this: drm_sched_entity_push_job() { ... spin_lock(&entity->lock); ... rq = entity->rq; sched = rq->sched; ... spin_unlock(&rq->lock); spin_unlock(&entity->lock); drm_sched_wakeup(sched); Ie. we know entity->rq and rq->sched are guaranteed to be stable and present at this point because job is already in the queue and drm_sched_entity_select_rq() guarantees that. In this patch I moved all this block into drm_sched_rq_add_entity() but I wanted to leave drm_sched_wakeup() outside. Because I thought it is not the job of the run queue handling, and semantically the logic was "only once added to the entity we know the rq and scheduler for certain". That would open the door for future improvements and late rq/scheduler selection. But now I think it is premature and it would be better I simply move the wakekup inside drm_sched_rq_add_entity() together with all the rest. Does that sound like a plan for now?Hmmm. What I'm wondering most about if it really is a good idea to have drm_sched_wakeup() in rq_add_entity(). Do you think that makes semantically more sense than just reading: drm_sched_entity_push_job() { foo bar more_foo /* New job was added. Right time to wake up scheduler. */ drm_sched_wakeup();Problem here always is you need a sched pointer so question is simply how and where to get it.I think both can make sense, but the above / current version seems to make more sense to me.Current as in this patch or current as in the upstream codebase? In all cases the knowledge it is safe to use sched after unlocking is implicit. I see only two options: current) drm_sched_entity_push_job() { ... spin_unlock(&rq->lock); spin_unlock(&entity->lock); drm_sched_wakeup(sched); a) drm_sched_entity_push_job() { ... sched = drm_sched_rq_add_entity(entity, submit_ts); if (sched) drm_sched_wakeup(sched); b) drm_sched_rq_add_entity() { ... spin_unlock(&rq->lock); spin_unlock(&entity->lock); drm_sched_wakeup(sched); drm_sched_entity_push_job() { ... drm_sched_rq_add_entity(entity, submit_ts); b) is the same as today, a) perhaps a bit premature. Which do you prefer?Alright, I looked through everything now. The thing is just that I believe that it's a semantically confusing and unclean concept of having drm_sched_rq_add_entity() return a scheduler – except for when the entity is stopped. Then "there is no scheduler" actually means "there is a scheduler, but that entity is stopped" In an ideal world: a) drm_sched_entity_push_job() wakes up the scheduler (as in your code, and as in the current mainline code) and b) drm_sched_entity_push_job() is the one who checks whether the entity is stopped. rq_add_entity() should just, well, add an entity to a runqueue. Option b) then would need locks again and could race. So that's not so cool. Possible solutions I can see is: 1. Have drm_sched_rq_add_entity() return an ERR_PTR instead of NULL.
Maybe I am misunderstanding the idea, but what would be the benefit of this option?
To clarify, I have:
drm_sched_rq_add_entity()
{
...
if (entity->stopped) {
...
return NULL;
drm_sched_entity_push_job()
{
...
sched = drm_sched_rq_add_entity(entity);
if (sched)
drm_sched_wakeup(sched);
And you propose:
drm_sched_rq_add_entity()
{
...
if (entity->stopped) {
...
return ERR_PTR(-ESOMETHING);
drm_sched_entity_push_job()
{
...
sched = drm_sched_rq_add_entity(entity);
if (!IS_ERR(sched))
drm_sched_wakeup(sched);
?
2. Rename rq_add_entity()
You mean to something signify it is also doing the wakeup? Or simply drm_sched_rq_add_first_entity()?
3. Potentially leave it as is? I guess that doesn't work for your rq- simplification?
Leave drm_sched_wakeup in push job? Yeah that doesn't work for moving the rq handling into own helpers.
> Option 1 would almost be my preference. What do you think?Lets see if I understand the option 1. I am fine with that one as described, and also with option 2.
Regards, Tvrtko
