On 03/06/2026 10:14, Tvrtko Ursulin wrote:
On 03/06/2026 09:37, Philipp Stanner wrote:On Tue, 2026-06-02 at 16:33 +0100, Tvrtko Ursulin wrote:Commit28c5bf28763d ("drm/sched: Disallow initializing entities with no schedulers") failed to notice clearing of entity->rq in drm_sched_entity_init() is nowBy clearing you also mean the setting to NULL? I'd just use "initialization" consistently, like in the commit title.It is initialized properly a bit lower down so clearing is I think more accurate.redundant and can be removed.Given that entity->rq can now never be NULL, we also remove two impossiblechecks, from drm_sched_entity_kill() and drm_sched_entity_flush() respectively. Similarly, we can also remove the !entity->rq check indrm_sched_job_init(). And for the better, given that the error message, ifit ever triggered, would have dereferenced the yet un-initialized job-> sched (only initialized later in drm_sched_job_arm()). This appears to have been theoretically broken ever since commit56e449603f0a ("drm/sched: Convert the GPU scheduler to variable number of run-queues"). 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 | 11 ++--------- drivers/gpu/drm/scheduler/sched_main.c | 9 --------- 2 files changed, 2 insertions(+), 18 deletions(-)diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/ drm/scheduler/sched_entity.cindex 4ebb513255ed..c51101ec70c1 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c@@ -129,7 +129,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,return -ENOMEM; INIT_LIST_HEAD(&entity->list); - entity->rq = NULL;It would seem that has always been redundant because of the memset(0) directly above.True, ever since 1decbf6bb0b4 ("drm/sched: Fix entities with 0 rqs."). Just that in this patch the redundant is focusing on:entity->rq = NULL; ... entity->rq = &sched_list[0]->rq; Good enough or you want a respin and if so in what flavour?
Gentle reminder - can we merge this as is or you want me to tweak what specifically?
Regards, Tvrtko
entity->guilty = guilty; entity->priority = priority; entity->last_user = current->group_leader;@@ -280,9 +279,6 @@ void drm_sched_entity_kill(struct drm_sched_entity *entity)struct drm_sched_job *job; struct dma_fence *prev; - if (!entity->rq) - return; - spin_lock(&entity->lock); entity->stopped = true; drm_sched_rq_remove_entity(entity->rq, entity); @@ -329,14 +325,11 @@ EXPORT_SYMBOL(drm_sched_entity_kill); */long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout){ - struct drm_gpu_scheduler *sched; + struct drm_gpu_scheduler *sched = + container_of(entity->rq, typeof(*sched), rq); struct task_struct *last_user; long ret = timeout; - if (!entity->rq) - return 0; - - sched = container_of(entity->rq, typeof(*sched), rq); /* * The client will not queue more jobs during this fini - consume * existing queued ones, or discard them on SIGKILL.diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/ drm/scheduler/sched_main.cindex 818d3d4434b5..d2ca01b31ee4 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -588,15 +588,6 @@ int drm_sched_job_init(struct drm_sched_job *job, u32 credits, void *owner, uint64_t drm_client_id) { - if (!entity->rq) { - /* This will most likely be followed by missing frames - * or worse--a blank screen--leave a trail in the - * logs, so this can be debugged easier. - */ - dev_err(job->sched->dev, "%s: entity has no rq!\n", __func__); - return -ENOENT; - } - if (unlikely(!credits)) { pr_err("*ERROR* %s: credits cannot be 0!\n", __func__); return -EINVAL;But overall a very nice cleanup P.
