* [PATCH v2 1/8] drm/sched: Fix out of bounds entity priority check
2026-10-20 15:41 [PATCH v2 0/8] DRM scheduler assorted cleanups Tvrtko Ursulin
@ 2026-10-20 15:41 ` Tvrtko Ursulin
2026-10-20 15:41 ` [PATCH v2 2/8] drm/sched: Disallow initializing entities with un-initialized schedulers Tvrtko Ursulin
` (7 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2026-10-20 15:41 UTC (permalink / raw)
To: dri-devel
Cc: amd-gfx, kernel-dev, Tvrtko Ursulin, Christian König,
Danilo Krummrich, Matthew Brost, Philipp Stanner, stable
When the out of range priority at entity init time is clamped to the
number of run queues in the supplied scheduler the internal run-queue
priority needs to be adjusted as well.
There is no impact since none of the drivers hit the clamping path, but
we still need to address it by moving the entity->rq_priority assignment
to after the clamping is done.
Note that a lot of this will hopefully get removed once again when the
single run-queue simplification lands.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: 2fa4d8e2c109 ("drm/sched: Add fair scheduling policy")
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: <stable@vger.kernel.org> # v7.2+
---
drivers/gpu/drm/scheduler/sched_entity.c | 27 +++++++++++-------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 7be0387bac3c..e12709b7bd0c 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -130,10 +130,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
INIT_LIST_HEAD(&entity->list);
entity->guilty = guilty;
- entity->priority = priority;
entity->last_user = current->group_leader;
- entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ?
- DRM_SCHED_PRIORITY_KERNEL : priority;
entity->num_sched_list = num_sched_list;
entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
RB_CLEAR_NODE(&entity->rb_tree_node);
@@ -144,25 +141,25 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
* not to do this and to fix their DRM calling order.
*/
pr_warn("%s: called with uninitialized scheduler\n", __func__);
- } else {
- enum drm_sched_priority p = entity->priority;
-
+ } else if (priority >= sched_list[0]->num_user_rqs) {
/*
* The "priority" of an entity cannot exceed the number of
* run-queues of a scheduler. Protect against num_rqs being 0,
* by converting to signed. Choose the lowest priority
* available.
*/
- if (p >= sched_list[0]->num_user_rqs) {
- dev_err(sched_list[0]->dev, "entity with out-of-bounds priority:%u num_user_rqs:%u\n",
- p, sched_list[0]->num_user_rqs);
- p = max_t(s32,
- (s32)sched_list[0]->num_user_rqs - 1,
- (s32)DRM_SCHED_PRIORITY_KERNEL);
- entity->priority = p;
- }
+ dev_err(sched_list[0]->dev, "entity with out-of-bounds priority:%u num_user_rqs:%u\n",
+ priority, sched_list[0]->num_user_rqs);
+ priority = max_t(s32,
+ (s32)sched_list[0]->num_user_rqs - 1,
+ (s32)DRM_SCHED_PRIORITY_KERNEL);
+ }
+
+ entity->priority = priority;
+ entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ?
+ DRM_SCHED_PRIORITY_KERNEL : priority;
+ if (sched_list[0]->sched_rq)
entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
- }
init_completion(&entity->entity_idle);
--
2.56.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 2/8] drm/sched: Disallow initializing entities with un-initialized schedulers
2026-10-20 15:41 [PATCH v2 0/8] DRM scheduler assorted cleanups Tvrtko Ursulin
2026-10-20 15:41 ` [PATCH v2 1/8] drm/sched: Fix out of bounds entity priority check Tvrtko Ursulin
@ 2026-10-20 15:41 ` Tvrtko Ursulin
2026-10-20 16:02 ` sashiko-bot
2026-10-20 15:41 ` [PATCH v2 3/8] drm/sched: Reject double scheduler initialization Tvrtko Ursulin
` (6 subsequent siblings)
8 siblings, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2026-10-20 15:41 UTC (permalink / raw)
To: dri-devel
Cc: amd-gfx, kernel-dev, Tvrtko Ursulin, Christian König,
Danilo Krummrich, Matthew Brost, Philipp Stanner
Simplify the code and theoretical runtime combinations by disallowing
initializing entities with un-initialized schedulers.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
I do not *think* any of the drivers relies on this inverted initialization
order but it needs more eyes for sure.
---
drivers/gpu/drm/scheduler/sched_entity.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index e12709b7bd0c..95bb79b3d003 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -119,9 +119,16 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
unsigned int num_sched_list,
atomic_t *guilty)
{
- if (!entity || !sched_list || !num_sched_list || !sched_list[0])
+ unsigned int i;
+
+ if (!entity || !sched_list || !num_sched_list)
return -EINVAL;
+ for (i = 0; i < num_sched_list; i++) {
+ if (!sched_list[i] || !sched_list[i]->sched_rq)
+ return -EINVAL;
+ }
+
memset(entity, 0, sizeof(struct drm_sched_entity));
entity->stats = drm_sched_entity_stats_new();
@@ -135,13 +142,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
RB_CLEAR_NODE(&entity->rb_tree_node);
- if (!sched_list[0]->sched_rq) {
- /* Since every entry covered by num_sched_list
- * should be non-NULL and therefore we warn drivers
- * not to do this and to fix their DRM calling order.
- */
- pr_warn("%s: called with uninitialized scheduler\n", __func__);
- } else if (priority >= sched_list[0]->num_user_rqs) {
+ if (priority >= sched_list[0]->num_user_rqs) {
/*
* The "priority" of an entity cannot exceed the number of
* run-queues of a scheduler. Protect against num_rqs being 0,
@@ -158,8 +159,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
entity->priority = priority;
entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ?
DRM_SCHED_PRIORITY_KERNEL : priority;
- if (sched_list[0]->sched_rq)
- entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
+ entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
init_completion(&entity->entity_idle);
--
2.56.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 2/8] drm/sched: Disallow initializing entities with un-initialized schedulers
2026-10-20 15:41 ` [PATCH v2 2/8] drm/sched: Disallow initializing entities with un-initialized schedulers Tvrtko Ursulin
@ 2026-10-20 16:02 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-10-20 16:02 UTC (permalink / raw)
To: Tvrtko Ursulin, dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/sched: drm_sched_entity_init() breaks v3d_open() on V3D 3.3
--
commit 607520d70e20fa56f0a8fa9eade2db9f019b764f
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
drm/sched: Disallow initializing entities with un-initialized schedulers
This patch updates drm_sched_entity_init() to reject schedulers whose
run-queues have not been initialized, returning -EINVAL instead of logging
a warning and continuing.
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index e12709b7bd0c3..95bb79b3d003a 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -119,9 +119,16 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
> unsigned int num_sched_list,
> atomic_t *guilty)
> {
> - if (!entity || !sched_list || !num_sched_list || !sched_list[0])
> + unsigned int i;
> +
> + if (!entity || !sched_list || !num_sched_list)
> return -EINVAL;
>
> + for (i = 0; i < num_sched_list; i++) {
> + if (!sched_list[i] || !sched_list[i]->sched_rq)
> + return -EINVAL;
> + }
[Severity: High]
Does returning -EINVAL here when !sched_list[i]->sched_rq break
v3d_open() on V3D 3.3 hardware (V3D_GEN_33)?
During probe, v3d_sched_init() in drivers/gpu/drm/v3d/v3d_sched.c only
initializes the V3D_CSD and V3D_CACHE_CLEAN schedulers when
v3d_has_csd(v3d) is true:
drivers/gpu/drm/v3d/v3d_sched.c:v3d_sched_init() {
...
if (v3d_has_csd(v3d)) {
ret = v3d_queue_sched_init(v3d, &v3d_csd_sched_ops, V3D_CSD,
"v3d_csd");
if (ret)
goto fail;
ret = v3d_queue_sched_init(v3d, &v3d_cache_clean_sched_ops,
V3D_CACHE_CLEAN, "v3d_cache_clean");
if (ret)
goto fail;
}
...
}
On V3D 3.3 platforms where !v3d_has_csd(v3d), those two schedulers are
never initialized via drm_sched_init(), leaving their sched_rq pointers
NULL.
However, when userspace opens the DRM device node, v3d_open() in
drivers/gpu/drm/v3d/v3d_drv.c unconditionally loops over all
V3D_MAX_QUEUES queues and fails if drm_sched_entity_init() returns an
error:
drivers/gpu/drm/v3d/v3d_drv.c:v3d_open() {
...
for (i = 0; i < V3D_MAX_QUEUES; i++) {
v3d_priv->stats[i] = v3d_stats_alloc();
if (!v3d_priv->stats[i]) {
ret = -ENOMEM;
goto err_stats;
}
sched = &v3d->queue[i].sched;
ret = drm_sched_entity_init(&v3d_priv->sched_entity[i],
DRM_SCHED_PRIORITY_NORMAL, &sched,
1, NULL);
if (ret)
goto err_sched;
}
...
}
Previously, drm_sched_entity_init() logged a warning via pr_warn() and
returned 0, allowing v3d_open() to succeed. With this change, won't
every v3d_open() call on V3D 3.3 fail with -EINVAL?
[ ... ]
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261020154123.33631-1-tvrtko.ursulin@igalia.com?part=2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/8] drm/sched: Reject double scheduler initialization
2026-10-20 15:41 [PATCH v2 0/8] DRM scheduler assorted cleanups Tvrtko Ursulin
2026-10-20 15:41 ` [PATCH v2 1/8] drm/sched: Fix out of bounds entity priority check Tvrtko Ursulin
2026-10-20 15:41 ` [PATCH v2 2/8] drm/sched: Disallow initializing entities with un-initialized schedulers Tvrtko Ursulin
@ 2026-10-20 15:41 ` Tvrtko Ursulin
2026-10-20 15:41 ` [PATCH v2 4/8] drm/sched: Remove log message of questionable use Tvrtko Ursulin
` (5 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2026-10-20 15:41 UTC (permalink / raw)
To: dri-devel
Cc: amd-gfx, kernel-dev, Tvrtko Ursulin, Christian König,
Danilo Krummrich, Matthew Brost, Philipp Stanner
There is no point in having code and logging whose only purpose is to
notify developers of incorrect usage of the API while bringing up a new
driver. Lets simply return an error and let the developers figure it out.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
drivers/gpu/drm/scheduler/sched_main.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 99252674e266..ca1665bb49cc 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1101,6 +1101,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
{
int i;
+ if (sched->sched_rq)
+ return -EINVAL;
+
sched->ops = args->ops;
sched->credit_limit = args->credit_limit;
sched->name = args->name;
@@ -1116,13 +1119,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
dev_err(sched->dev, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
__func__);
return -EINVAL;
- } else if (sched->sched_rq) {
- /* Not an error, but warn anyway so drivers can
- * fine-tune their DRM calling order, and return all
- * is good.
- */
- dev_warn(sched->dev, "%s: scheduler already initialized!\n", __func__);
- return 0;
}
if (args->submit_wq) {
--
2.56.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 4/8] drm/sched: Remove log message of questionable use
2026-10-20 15:41 [PATCH v2 0/8] DRM scheduler assorted cleanups Tvrtko Ursulin
` (2 preceding siblings ...)
2026-10-20 15:41 ` [PATCH v2 3/8] drm/sched: Reject double scheduler initialization Tvrtko Ursulin
@ 2026-10-20 15:41 ` Tvrtko Ursulin
2026-10-20 15:41 ` [PATCH v2 5/8] drm/sched: Remove useless log message on memory exhaustion Tvrtko Ursulin
` (4 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2026-10-20 15:41 UTC (permalink / raw)
To: dri-devel
Cc: amd-gfx, kernel-dev, Tvrtko Ursulin, Christian König,
Danilo Krummrich, Matthew Brost, Philipp Stanner
There is no point in having a log message whose only purpose is to
notify developers of incorrect usage of the API while bringing up a new
driver. Lets simply return an error and let the developers figure it out.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
drivers/gpu/drm/scheduler/sched_main.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index ca1665bb49cc..d86738611b1e 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1104,6 +1104,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
if (sched->sched_rq)
return -EINVAL;
+ if (args->num_rqs > DRM_SCHED_PRIORITY_COUNT)
+ return -EINVAL;
+
sched->ops = args->ops;
sched->credit_limit = args->credit_limit;
sched->name = args->name;
@@ -1113,14 +1116,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
sched->score = args->score ? args->score : &sched->_score;
sched->dev = args->dev;
- if (args->num_rqs > DRM_SCHED_PRIORITY_COUNT) {
- /* This is a gross violation--tell drivers what the problem is.
- */
- dev_err(sched->dev, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
- __func__);
- return -EINVAL;
- }
-
if (args->submit_wq) {
sched->submit_wq = args->submit_wq;
sched->own_submit_wq = false;
--
2.56.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 5/8] drm/sched: Remove useless log message on memory exhaustion
2026-10-20 15:41 [PATCH v2 0/8] DRM scheduler assorted cleanups Tvrtko Ursulin
` (3 preceding siblings ...)
2026-10-20 15:41 ` [PATCH v2 4/8] drm/sched: Remove log message of questionable use Tvrtko Ursulin
@ 2026-10-20 15:41 ` Tvrtko Ursulin
2026-10-20 15:41 ` [PATCH v2 6/8] drm/sched: Ignore stopped entity attempt to change its run-queue Tvrtko Ursulin
` (3 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2026-10-20 15:41 UTC (permalink / raw)
To: dri-devel
Cc: amd-gfx, kernel-dev, Tvrtko Ursulin, Christian König,
Danilo Krummrich, Matthew Brost, Philipp Stanner
If scheduler initialization has failed due failure to allocate a handful
of bytes for the run-queue backing data structure, there will be plenty of
loud messages in the kernel log. We can safely remove the one
drm_sched_init() logs.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
drivers/gpu/drm/scheduler/sched_main.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index d86738611b1e..85a49e219837 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1164,7 +1164,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
Out_check_own:
if (sched->own_submit_wq)
destroy_workqueue(sched->submit_wq);
- dev_err(sched->dev, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
return -ENOMEM;
}
EXPORT_SYMBOL(drm_sched_init);
--
2.56.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 6/8] drm/sched: Ignore stopped entity attempt to change its run-queue
2026-10-20 15:41 [PATCH v2 0/8] DRM scheduler assorted cleanups Tvrtko Ursulin
` (4 preceding siblings ...)
2026-10-20 15:41 ` [PATCH v2 5/8] drm/sched: Remove useless log message on memory exhaustion Tvrtko Ursulin
@ 2026-10-20 15:41 ` Tvrtko Ursulin
2026-10-20 15:41 ` [PATCH v2 7/8] drm/sched: Replace completion with a flush Tvrtko Ursulin
` (2 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2026-10-20 15:41 UTC (permalink / raw)
To: dri-devel
Cc: amd-gfx, kernel-dev, Tvrtko Ursulin, Christian König,
Danilo Krummrich, Matthew Brost, Philipp Stanner
Jobs submitted to a stopped entity do not go anywhere due how they are
handled in drm_sched_rq_add_entity() so there is no point to allow them
to change their run-queue. We can simply bail out early which makes
entity->stopped a clearer boundary than it currently is.
This is completely safe because it is always guaranteed entity->rq is
valid if drm_sched_entity_init() succeeded.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
drivers/gpu/drm/scheduler/sched_entity.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 95bb79b3d003..3ca671af02ed 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -584,13 +584,16 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
return;
spin_lock(&entity->lock);
+
+ /* Cannot submit to stopped entity anyway so bail out early. */
+ if (entity->stopped)
+ goto out;
+
fence = entity->last_scheduled;
/* stay on the same engine if the previous job hasn't finished */
- if (fence && !dma_fence_is_signaled(fence)) {
- spin_unlock(&entity->lock);
- return;
- }
+ if (fence && !dma_fence_is_signaled(fence))
+ goto out;
sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
rq = sched ? sched->sched_rq[entity->rq_priority] : NULL;
@@ -602,6 +605,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
if (entity->num_sched_list == 1)
entity->sched_list = NULL;
+out:
spin_unlock(&entity->lock);
}
--
2.56.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 7/8] drm/sched: Replace completion with a flush
2026-10-20 15:41 [PATCH v2 0/8] DRM scheduler assorted cleanups Tvrtko Ursulin
` (5 preceding siblings ...)
2026-10-20 15:41 ` [PATCH v2 6/8] drm/sched: Ignore stopped entity attempt to change its run-queue Tvrtko Ursulin
@ 2026-10-20 15:41 ` Tvrtko Ursulin
2026-10-20 16:07 ` sashiko-bot
2026-10-20 15:41 ` [PATCH v2 8/8] drm/sched: Untangle the scheduler list mess Tvrtko Ursulin
2026-10-20 16:04 ` [PATCH v2 0/8] DRM scheduler assorted cleanups Philipp Stanner
8 siblings, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2026-10-20 15:41 UTC (permalink / raw)
To: dri-devel
Cc: amd-gfx, kernel-dev, Tvrtko Ursulin, Christian König,
Danilo Krummrich, Matthew Brost, Philipp Stanner, intel-xe
Due the scheduler locking design, and the inability to always lock both
the entity and the run-queue in the consistent order, a completion exists
which effectively marks the entity as in use from a call path which is not
able to lock it.
When entity is selected from the run job worker, its completion is marked
as non-idle all until the code is sure it will not be dereferencing it any
more, at which point it signals it as idle, releasing the potential
parallel cleanup path.
We can remove the need for this completion by implementing the identical
guarantee by simply flushing the run job work from the cleanup path, after
having removed the entity from the run queue.
We then know that the entity is no longer reachable by the run queue
selection logic, so as soon as any pending work is done the cleanup can
safely proceed. And because we have marked the entity as stopped, we also
know that the entity cannot re-enter the run queue.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: amd-gfx@lists.freedesktop.org
Cc: intel-xe@lists.freedesktop.org
---
drivers/gpu/drm/scheduler/sched_entity.c | 18 ++++++++++--------
drivers/gpu/drm/scheduler/sched_internal.h | 14 ++++++++++++--
drivers/gpu/drm/scheduler/sched_main.c | 2 --
drivers/gpu/drm/scheduler/sched_rq.c | 19 +++++++++++++------
include/drm/gpu_scheduler.h | 9 ---------
5 files changed, 35 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 3ca671af02ed..79ea20838328 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -161,11 +161,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
DRM_SCHED_PRIORITY_KERNEL : priority;
entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
- init_completion(&entity->entity_idle);
-
- /* We start in an idle state. */
- complete_all(&entity->entity_idle);
-
spin_lock_init(&entity->lock);
spsc_queue_init(&entity->job_queue);
@@ -304,16 +299,23 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
*/
void drm_sched_entity_kill(struct drm_sched_entity *entity)
{
+ struct drm_gpu_scheduler *sched;
struct drm_sched_job *job;
struct dma_fence *prev;
spin_lock(&entity->lock);
entity->stopped = true;
- drm_sched_rq_remove_entity(entity->rq, entity);
+ sched = drm_sched_rq_remove_entity(entity->rq, entity);
spin_unlock(&entity->lock);
- /* Make sure this entity is not used by the scheduler at the moment */
- wait_for_completion(&entity->entity_idle);
+ /*
+ * Make sure this entity is not used by the scheduler at the moment.
+ *
+ * Scheduler is guaranteed to be stable after the entity was stopped and
+ * removed from the run-queue.
+ */
+ if (sched)
+ drm_sched_flush_run_work(sched);
spin_lock(&entity->lock);
prev = dma_fence_get(entity->last_scheduled);
diff --git a/drivers/gpu/drm/scheduler/sched_internal.h b/drivers/gpu/drm/scheduler/sched_internal.h
index 32d3ddb820be..ca724a8a4433 100644
--- a/drivers/gpu/drm/scheduler/sched_internal.h
+++ b/drivers/gpu/drm/scheduler/sched_internal.h
@@ -42,13 +42,23 @@ bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
struct drm_sched_entity *entity);
void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
+/**
+ * drm_sched_flush_run_work - flush the run-job work
+ * @sched: scheduler instance
+ */
+static inline void drm_sched_flush_run_work(struct drm_gpu_scheduler *sched)
+{
+ flush_work(&sched->work_run_job);
+}
+
void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
struct drm_sched_rq *rq);
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);
+struct drm_gpu_scheduler *
+drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
+ struct drm_sched_entity *entity);
void drm_sched_rq_pop_entity(struct drm_sched_entity *entity);
struct drm_sched_entity *
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 85a49e219837..281a1e7f89a9 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1033,7 +1033,6 @@ static void drm_sched_run_job_work(struct work_struct *w)
sched_job = drm_sched_entity_pop_job(entity);
if (!sched_job) {
- complete_all(&entity->entity_idle);
drm_sched_run_job_queue(sched);
return;
}
@@ -1049,7 +1048,6 @@ static void drm_sched_run_job_work(struct work_struct *w)
* refcount has been incremented for the scheduler already.
*/
fence = sched->ops->run_job(sched_job);
- complete_all(&entity->entity_idle);
drm_sched_fence_scheduled(s_fence, fence);
if (!IS_ERR_OR_NULL(fence)) {
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
index 23f46ec610e7..79f7bdb5b3f0 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -297,23 +297,32 @@ drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts)
* @entity: scheduler entity
*
* Removes a scheduler entity from the run queue.
+ *
+ * Return: DRM scheduler selected to handle this entity or NULL if entity has
+ * already been removed.
*/
-void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
- struct drm_sched_entity *entity)
+struct drm_gpu_scheduler *
+drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
+ struct drm_sched_entity *entity)
{
+ struct drm_gpu_scheduler *sched;
+
lockdep_assert_held(&entity->lock);
if (list_empty(&entity->list))
- return;
+ return NULL;
spin_lock(&rq->lock);
- atomic_dec(rq->sched->score);
+ sched = rq->sched;
+ atomic_dec(sched->score);
list_del_init(&entity->list);
drm_sched_rq_remove_fifo_locked(entity, rq);
spin_unlock(&rq->lock);
+
+ return sched;
}
static ktime_t
@@ -407,8 +416,6 @@ drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
spin_unlock(&rq->lock);
return ERR_PTR(-ENOSPC);
}
-
- reinit_completion(&entity->entity_idle);
break;
}
}
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 3b0978fa8cf0..c89a581f54bf 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -27,7 +27,6 @@
#include <drm/spsc_queue.h>
#include <linux/average.h>
#include <linux/dma-fence.h>
-#include <linux/completion.h>
#include <linux/xarray.h>
#include <linux/workqueue.h>
@@ -232,14 +231,6 @@ struct drm_sched_entity {
*/
bool stopped;
- /**
- * @entity_idle:
- *
- * Signals when entity is not in use, used to sequence entity cleanup in
- * drm_sched_entity_fini().
- */
- struct completion entity_idle;
-
/**
* @oldest_job_waiting:
*
--
2.56.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 7/8] drm/sched: Replace completion with a flush
2026-10-20 15:41 ` [PATCH v2 7/8] drm/sched: Replace completion with a flush Tvrtko Ursulin
@ 2026-10-20 16:07 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-10-20 16:07 UTC (permalink / raw)
To: Tvrtko Ursulin, dri-devel; +Cc: intel-xe
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] drm/sched: stopped entity re-enters rbtree in drm_sched_rq_pop_entity()
- [Critical] drm/sched: concurrent drm_sched_entity_kill() skips run_work flush
- [High] drm/sched: workqueue deadlock in drm_sched_entity_kill() from free_job
--
commit 271930dfdc1a5516f290d150182ce8d6b1d2358b
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
drm/sched: Replace completion with a flush
This commit replaces struct drm_sched_entity.entity_idle with a flush of
the scheduler's work_run_job work item in drm_sched_entity_kill() after
removing the entity from the run-queue.
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 3ca671af02ed5..79ea20838328d 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
[ ... ]
> @@ -304,16 +299,23 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
> */
> void drm_sched_entity_kill(struct drm_sched_entity *entity)
> {
> + struct drm_gpu_scheduler *sched;
> struct drm_sched_job *job;
> struct dma_fence *prev;
>
> spin_lock(&entity->lock);
> entity->stopped = true;
> - drm_sched_rq_remove_entity(entity->rq, entity);
> + sched = drm_sched_rq_remove_entity(entity->rq, entity);
> spin_unlock(&entity->lock);
>
> - /* Make sure this entity is not used by the scheduler at the moment */
> - wait_for_completion(&entity->entity_idle);
> + /*
> + * Make sure this entity is not used by the scheduler at the moment.
> + *
> + * Scheduler is guaranteed to be stable after the entity was stopped and
> + * removed from the run-queue.
> + */
> + if (sched)
> + drm_sched_flush_run_work(sched);
[Severity: Critical]
Can an in-flight drm_sched_run_job_work() re-insert the stopped entity
back into rq->rb_tree_root while drm_sched_entity_kill() is waiting inside
drm_sched_flush_run_work()?
When an entity has two or more queued jobs and drm_sched_run_job_work()
selects it via drm_sched_select_entity() -> drm_sched_rq_select_entity(),
rq->lock is dropped before calling drm_sched_entity_pop_job(entity), which
peeks the first job and checks its dependencies without holding rq->lock
or entity->lock.
If drm_sched_entity_kill() runs during that unlocked window, it sets
entity->stopped = true, removes entity->list and entity->rb_tree_node via
drm_sched_rq_remove_entity(), and blocks in drm_sched_flush_run_work()
before draining entity->job_queue.
The in-flight drm_sched_entity_pop_job() then acquires entity->lock, pops
the first job, and calls drm_sched_rq_pop_entity(entity):
drivers/gpu/drm/scheduler/sched_entity.c:drm_sched_entity_pop_job() {
...
spin_lock(&entity->lock);
prev_last_scheduled = entity->last_scheduled;
entity->last_scheduled = dma_fence_get(&sched_job->s_fence->finished);
spsc_queue_pop(&entity->job_queue);
drm_sched_rq_pop_entity(entity);
spin_unlock(&entity->lock);
...
}
In drm_sched_rq_pop_entity(), neither entity->stopped nor
list_empty(&entity->list) is checked, and because the second job is still
in entity->job_queue, next_job is non-NULL:
drivers/gpu/drm/scheduler/sched_rq.c:drm_sched_rq_pop_entity() {
...
next_job = drm_sched_entity_queue_peek(entity);
if (next_job) {
ktime_t ts;
...
drm_sched_rq_update_fifo_locked(entity, rq, ts);
}
...
}
This re-inserts entity->rb_tree_node into rq->rb_tree_root before
drm_sched_run_job_work() re-queues sched->work_run_job and finishes.
Once drm_sched_flush_run_work() returns, won't the re-queued
drm_sched_run_job_work() be able to select the entity again concurrently
with drm_sched_entity_kill() draining entity->job_queue and
drm_sched_entity_fini() freeing the entity, or leave a dangling
entity->rb_tree_node in rq->rb_tree_root because subsequent calls to
drm_sched_rq_remove_entity() see list_empty(&entity->list) and return NULL
early?
[Severity: High]
Can calling drm_sched_flush_run_work(sched) here deadlock when
drm_sched_entity_kill() is invoked from a driver's free_job() callback on
sched->submit_wq?
When an entity pushes a job, drm_sched_rq_add_entity() links entity->list
onto rq->entities, and entity->list stays linked even after all jobs have
been popped and completed until drm_sched_rq_remove_entity() is called in
drm_sched_entity_kill(). As a result, drm_sched_rq_remove_entity() returns
a non-NULL sched for any entity that has ever submitted a job, causing
drm_sched_entity_kill() to call flush_work(&sched->work_run_job).
By default, sched->submit_wq is allocated via drm_sched_alloc_wq() as an
ordered workqueue (max_active = 1) shared by sched->work_run_job and
sched->work_free_job. In drivers such as msm and panfrost, freeing a job in
drm_sched_free_job_work() -> sched->ops->free_job() can drop the last
reference to a closed context or submitqueue and tear down its entities:
drm_sched_free_job_work()
-> msm_job_free()
-> msm_gem_submit_put()
-> __msm_gem_submit_destroy()
-> msm_submitqueue_put() / msm_context_put()
-> __msm_context_destroy()
-> drm_sched_entity_destroy()
-> drm_sched_entity_fini()
-> drm_sched_entity_kill()
-> drm_sched_flush_run_work()
-> flush_work(&sched->work_run_job)
and similarly via panfrost_job_free() -> panfrost_job_put() ->
panfrost_job_cleanup() -> panfrost_jm_ctx_put() ->
panfrost_jm_ctx_release() -> drm_sched_entity_destroy().
If sched->work_run_job is queued on the ordered sched->submit_wq when
drm_sched_free_job_work() runs this path, won't flush_work(&sched->work_run_job)
deadlock waiting for work_run_job, which cannot start until
drm_sched_free_job_work() finishes (and trigger a recursive lockdep
warning on drm_sched_lockdep_map)?
>
> spin_lock(&entity->lock);
> prev = dma_fence_get(entity->last_scheduled);
[ ... ]
> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
> index 23f46ec610e78..79f7bdb5b3f0a 100644
> --- a/drivers/gpu/drm/scheduler/sched_rq.c
> +++ b/drivers/gpu/drm/scheduler/sched_rq.c
> @@ -297,23 +297,32 @@ drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts)
> * @entity: scheduler entity
> *
> * Removes a scheduler entity from the run queue.
> + *
> + * Return: DRM scheduler selected to handle this entity or NULL if entity has
> + * already been removed.
> */
> -void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
> - struct drm_sched_entity *entity)
> +struct drm_gpu_scheduler *
> +drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
> + struct drm_sched_entity *entity)
> {
> + struct drm_gpu_scheduler *sched;
> +
> lockdep_assert_held(&entity->lock);
>
> if (list_empty(&entity->list))
> - return;
> + return NULL;
>
> spin_lock(&rq->lock);
>
> - atomic_dec(rq->sched->score);
> + sched = rq->sched;
> + atomic_dec(sched->score);
> list_del_init(&entity->list);
>
> drm_sched_rq_remove_fifo_locked(entity, rq);
>
> spin_unlock(&rq->lock);
> +
> + return sched;
> }
[Severity: Critical]
What happens if two threads call drm_sched_entity_kill() on the same
entity concurrently (for instance, one thread in file_operations->flush
calling drm_sched_entity_flush() -> drm_sched_entity_kill() upon SIGKILL,
and another thread closing the context and calling drm_sched_entity_fini()
-> drm_sched_entity_kill()) while drm_sched_run_job_work() is using the
entity?
Previously, both callers waited on wait_for_completion(&entity->entity_idle)
until drm_sched_run_job_work() called complete_all(&entity->entity_idle).
With this change, the first caller into drm_sched_rq_remove_entity() runs
list_del_init(&entity->list), receives the non-NULL sched pointer, drops
entity->lock, and blocks in drm_sched_flush_run_work(sched).
When the second caller enters drm_sched_rq_remove_entity(), it sees
list_empty(&entity->list) and gets NULL back, so in drm_sched_entity_kill()
it skips drm_sched_flush_run_work(sched) and proceeds directly to draining
entity->job_queue and freeing the entity in drm_sched_entity_fini() while
drm_sched_run_job_work() is still running. Can this cause a use-after-free
and concurrent SPSC queue access on entity->job_queue?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261020154123.33631-1-tvrtko.ursulin@igalia.com?part=7
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 8/8] drm/sched: Untangle the scheduler list mess
2026-10-20 15:41 [PATCH v2 0/8] DRM scheduler assorted cleanups Tvrtko Ursulin
` (6 preceding siblings ...)
2026-10-20 15:41 ` [PATCH v2 7/8] drm/sched: Replace completion with a flush Tvrtko Ursulin
@ 2026-10-20 15:41 ` Tvrtko Ursulin
2026-10-20 16:18 ` sashiko-bot
2026-10-20 16:04 ` [PATCH v2 0/8] DRM scheduler assorted cleanups Philipp Stanner
8 siblings, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2026-10-20 15:41 UTC (permalink / raw)
To: dri-devel
Cc: amd-gfx, kernel-dev, Tvrtko Ursulin, Christian König,
Alex Deucher, Luben Tuikov, Matthew Brost, Simona Vetter,
Danilo Krummrich, Philipp Stanner, Tomeu Vizoso
Long time ago in commit
b3ac17667f11 ("drm/scheduler: rework entity creation")
a change was made which prevented priority changes for entities with only
one assigned scheduler.
That commit reduced drm_sched_entity_set_priority() to simply update the
entities priority, but the run queue selection logic in
drm_sched_entity_select_rq() was never able to actually change the
originally assigned run queue.
In practice that only affected amdgpu, being the only driver which can do
dynamic priority changes, and it appears it was attempted to rectify this
in commit
2316a86bde49 ("drm/amdgpu: change hw sched list on ctx priority override")
.
A few unresolved problems remained - this only fixed
drm_sched_entity_set_priority() if drm_sched_entity_modify_sched() was
called first, which was not documented anywhere.
Secondly, that only worked if drm_sched_entity_modify_sched() is actually
called, which in amdgpu's case is true only for gfx and compute. Priority
changes for other engines, which have only one scheduler assigned such as
jpeg and video decode still did not work.
(Note that this was also noticed in 981b04d96856 ("drm/sched: improve docs
around drm_sched_entity").)
Completely different set of non-obvious confusion was that, whereas
drm_sched_entity_init() was not keeping the passed in list of schedulers
(courtesy of commit
8c23056bdc7a ("drm/scheduler: do not keep a copy of sched list")
), drm_sched_entity_modify_sched() was disagreeing with that and
would simply assign the single item list. That incosistency appears to
have been semi-silently fixed in commit
ac4eb83ab255 ("drm/sched: select new rq even if there is only one v3")
.
What was also not documented is why it was important to not keep the list
of schedulers when there is only one scheduler given. This was in fact due
many of the callers passing in a stack container for this case. For the
multiple schedulers on the other hand, amdgpu is the only such caller, and
there the container is not on the stack. Keeping a stack backed list in
the entity would obviously be undefined behaviour if the list was kept.
Amdgpu however did only stop passing in stack backed container for the
more than one scheduler case in commit
977f7e1068be ("drm/amdgpu: allocate entities on demand")
. Until then dereferencing freed stack from drm_sched_entity_select_rq()
was a possibility.
In order to untangle all this lets make this area behave consistently and
sensibly and document who owns what. We make the entity own its own copy
of the scheduler list, which allows removal of the special cases which
needed to artificially clear entity->sched_list for a single scheduler
case. Individual scheduler pointers are still owned by the driver with
unchanged lifetime rules.
To avoid growing the entity structure and also avoid runtime allocation of
the list, where the latter would be a significant challenge due the
current API is to not expect drm_sched_entity_modify_sched() to fail, we
do this by only having a single built in scheduler slot in the entity,
while the drivers (amdgpu and rocket only, former only in some cases),
have the responsibility to allocate their entities with enough trailing
storage to accomodate as large a scheduler list thy need for the trailing
flex array member.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: b3ac17667f11 ("drm/scheduler: rework entity creation")
References: 8c23056bdc7a ("drm/scheduler: do not keep a copy of sched list")
References: 977f7e1068be ("drm/amdgpu: allocate entities on demand")
References: 2316a86bde49 ("drm/amdgpu: change hw sched list on ctx priority override")
References: ac4eb83ab255 ("drm/sched: select new rq even if there is only one v3")
References: 981b04d96856 ("drm/sched: improve docs around drm_sched_entity")
References: 77a6809f1dc3 ("drm/sched: Remove FIFO and RR and simplify to a single run queue")
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Luben Tuikov <ltuikov89@gmail.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Tomeu Vizoso <tomeu@tomeuvizoso.net>
Cc: amd-gfx@lists.freedesktop.org
---
Rocket only compile tested!
In a previous version I had the scheduler allocate the list storage but
that was problematic in the way how it made
drm_sched_entity_modify_sched() having to start returning a return code,
and how that percolated through amdgpu. Both from the error unwind and
allocating under locks perspective.
So in this version I took a different approach by mandating callers
provide enough storage for the drm_sched_entity flex
array storage.
It is also not the prettiest but at least it addresses the problems
enumerated in the commit message.
---
drivers/accel/rocket/rocket_drv.h | 2 +-
drivers/accel/rocket/rocket_job.c | 20 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 5 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 3 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 15 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 +-
drivers/gpu/drm/scheduler/sched_entity.c | 48 ++++++++-----
include/drm/gpu_scheduler.h | 91 +++++++++++++++++++-----
10 files changed, 136 insertions(+), 58 deletions(-)
diff --git a/drivers/accel/rocket/rocket_drv.h b/drivers/accel/rocket/rocket_drv.h
index 2c673bb99ccc..1dbade2850ee 100644
--- a/drivers/accel/rocket/rocket_drv.h
+++ b/drivers/accel/rocket/rocket_drv.h
@@ -23,7 +23,7 @@ struct rocket_file_priv {
struct drm_mm mm;
struct mutex mm_lock;
- struct drm_sched_entity sched_entity;
+ DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE(sched_entity, sched_list, 3);
};
struct rocket_iommu_domain *rocket_iommu_domain_get(struct rocket_file_priv *rocket_priv);
diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index c3edb94355a3..87fd77b838c2 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -533,18 +533,21 @@ void rocket_job_fini(struct rocket_core *core)
int rocket_job_open(struct rocket_file_priv *rocket_priv)
{
struct rocket_device *rdev = rocket_priv->rdev;
- struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
- rdev->num_cores);
+ unsigned int num_cores = rdev->num_cores;
unsigned int core;
int ret;
- for (core = 0; core < rdev->num_cores; core++)
- scheds[core] = &rdev->cores[core].sched;
+ if (WARN_ON_ONCE(num_cores > ARRAY_SIZE(rocket_priv->sched_list)))
+ return -EINVAL;
- ret = drm_sched_entity_init(&rocket_priv->sched_entity,
- DRM_SCHED_PRIORITY_NORMAL,
- scheds,
- rdev->num_cores, NULL);
+ for (core = 0; core < num_cores; core++)
+ rocket_priv->sched_list[core] = &rdev->cores[core].sched;
+
+ ret = __drm_sched_entity_init(&rocket_priv->sched_entity,
+ DRM_SCHED_PRIORITY_NORMAL,
+ rocket_priv->sched_list,
+ ARRAY_SIZE(rocket_priv->sched_list),
+ num_cores, NULL);
if (WARN_ON(ret))
return ret;
@@ -555,7 +558,6 @@ void rocket_job_close(struct rocket_file_priv *rocket_priv)
{
struct drm_sched_entity *entity = &rocket_priv->sched_entity;
- kfree(entity->sched_list);
drm_sched_entity_destroy(entity);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 2f0349233695..a746c57ec384 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -255,8 +255,9 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, u32 hw_ip,
num_scheds = 1;
}
- r = drm_sched_entity_init(&entity->entity, drm_prio, scheds, num_scheds,
- NULL);
+ r = __drm_sched_entity_init(&entity->entity, drm_prio, scheds,
+ ARRAY_SIZE(entity->sched_list), num_scheds,
+ NULL);
if (r)
goto error_free_entity;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index a4b89eca4169..ba395406f37a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -39,7 +39,7 @@ struct amdgpu_ctx_mgr;
struct amdgpu_ctx_entity {
uint32_t hw_ip;
uint64_t sequence;
- struct drm_sched_entity entity;
+ DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE(entity, sched_list, AMDGPU_MAX_HWIP_RINGS);
struct dma_fence *fences[];
};
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 8f0720a77520..42371a5ef596 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -2087,7 +2087,9 @@ static int amdgpu_ttm_buffer_entity_init(struct amdgpu_gtt_mgr *mgr,
if (ARRAY_SIZE(entity->gart_window_offs) < num_gart_windows)
return -EINVAL;
- r = drm_sched_entity_init(&entity->base, prio, scheds, num_schedulers, NULL);
+ r = __drm_sched_entity_init(&entity->base, prio, scheds,
+ ARRAY_SIZE(entity->sched_list),
+ num_schedulers, NULL);
if (r)
return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
index 34b4b35a20f1..9a677bf2fcc9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -30,6 +30,7 @@
#include "amdgpu_vram_mgr.h"
#include "amdgpu_hmm.h"
#include "amdgpu_gmc.h"
+#include "amdgpu_sdma_types.h"
#define AMDGPU_PL_GDS (TTM_PL_PRIV + 0)
#define AMDGPU_PL_GWS (TTM_PL_PRIV + 1)
@@ -55,7 +56,7 @@ struct amdgpu_gtt_mgr {
};
struct amdgpu_ttm_buffer_entity {
- struct drm_sched_entity base;
+ DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE(base, sched_list, AMDGPU_MAX_SDMA_INSTANCES);
struct mutex lock;
struct drm_mm_node gart_node;
u64 gart_window_offs[2];
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 467ba16bd603..21536227c576 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -522,15 +522,18 @@ static int amdgpu_vm_init_entities(struct amdgpu_device *adev,
{
int r;
- r = drm_sched_entity_init(&vm->immediate, DRM_SCHED_PRIORITY_NORMAL,
- adev->vm_manager.vm_pte_scheds,
- adev->vm_manager.vm_pte_num_scheds, NULL);
+ r = __drm_sched_entity_init(&vm->immediate, DRM_SCHED_PRIORITY_NORMAL,
+ adev->vm_manager.vm_pte_scheds,
+ ARRAY_SIZE(vm->immediate_sched_list),
+ adev->vm_manager.vm_pte_num_scheds, NULL);
if (r)
goto error;
- return drm_sched_entity_init(&vm->delayed, DRM_SCHED_PRIORITY_NORMAL,
- adev->vm_manager.vm_pte_scheds,
- adev->vm_manager.vm_pte_num_scheds, NULL);
+ return __drm_sched_entity_init(&vm->delayed, DRM_SCHED_PRIORITY_NORMAL,
+ adev->vm_manager.vm_pte_scheds,
+ ARRAY_SIZE(vm->delayed_sched_list),
+ adev->vm_manager.vm_pte_num_scheds,
+ NULL);
error:
drm_sched_entity_destroy(&vm->immediate);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 98c48c45ce61..1dec90be622c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -325,8 +325,8 @@ struct amdgpu_vm {
struct dma_fence *last_update;
/* Scheduler entities for page table updates */
- struct drm_sched_entity immediate;
- struct drm_sched_entity delayed;
+ DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE(immediate, immediate_sched_list, AMDGPU_MAX_SDMA_INSTANCES);
+ DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE(delayed, delayed_sched_list, AMDGPU_MAX_SDMA_INSTANCES);
/* Last finished delayed update */
atomic64_t tlb_seq;
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 79ea20838328..d4956c6de6ed 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -91,18 +91,24 @@ ktime_t drm_sched_entity_stats_job_add_gpu_time(struct drm_sched_job *job)
}
/**
- * drm_sched_entity_init - Init a context entity used by scheduler when
+ * __drm_sched_entity_init - Init a context entity used by scheduler when
* submit to HW ring.
*
* @entity: scheduler entity to init
* @priority: priority of the entity
* @sched_list: the list of drm scheds on which jobs from this
* entity can be submitted
+ * @max_sched_list: the number of driver provided @sched_list slots
* @num_sched_list: number of drm sched in sched_list
* @guilty: atomic_t set to 1 when a job on this queue
* is found to be guilty causing a timeout
*
- * Note that the &sched_list must have at least one element to schedule the entity.
+ * Note that the &sched_list must have at least one element to schedule the
+ * entity.
+ *
+ * The individual drm_gpu_scheduler pointers have borrow semantics, ie.
+ * they must remain valid during entities lifetime, while the containing
+ * array can be freed after this call returns.
*
* For changing @priority later on at runtime see
* drm_sched_entity_set_priority(). For changing the set of schedulers
@@ -113,15 +119,17 @@ ktime_t drm_sched_entity_stats_job_add_gpu_time(struct drm_sched_job *job)
*
* Returns 0 on success or a negative error code on failure.
*/
-int drm_sched_entity_init(struct drm_sched_entity *entity,
+int __drm_sched_entity_init(struct drm_sched_entity *entity,
enum drm_sched_priority priority,
struct drm_gpu_scheduler **sched_list,
+ unsigned int max_sched_list,
unsigned int num_sched_list,
atomic_t *guilty)
{
unsigned int i;
- if (!entity || !sched_list || !num_sched_list)
+ if (!entity || !sched_list || !num_sched_list ||
+ num_sched_list > max_sched_list)
return -EINVAL;
for (i = 0; i < num_sched_list; i++) {
@@ -138,8 +146,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
INIT_LIST_HEAD(&entity->list);
entity->guilty = guilty;
entity->last_user = current->group_leader;
- entity->num_sched_list = num_sched_list;
- entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
RB_CLEAR_NODE(&entity->rb_tree_node);
if (priority >= sched_list[0]->num_user_rqs) {
@@ -159,17 +165,20 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
entity->priority = priority;
entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ?
DRM_SCHED_PRIORITY_KERNEL : priority;
- entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
spin_lock_init(&entity->lock);
spsc_queue_init(&entity->job_queue);
+ entity->max_sched_list = max_sched_list;
+ drm_sched_entity_modify_sched(entity, sched_list, num_sched_list);
+ drm_sched_entity_select_rq(entity);
+
atomic_set(&entity->fence_seq, 0);
entity->fence_context = dma_fence_context_alloc(2);
return 0;
}
-EXPORT_SYMBOL(drm_sched_entity_init);
+EXPORT_SYMBOL(__drm_sched_entity_init);
/**
* drm_sched_entity_modify_sched - Modify sched of an entity
@@ -178,20 +187,28 @@ EXPORT_SYMBOL(drm_sched_entity_init);
* existing entity->sched_list
* @num_sched_list: number of drm sched in sched_list
*
+ * The individual drm_gpu_scheduler pointers have borrow semantics, ie.
+ * they must remain valid during entities lifetime, while the containing
+ * array can be freed after this call returns.
+ *
* Note that this must be called under the same common lock for @entity as
* drm_sched_job_arm() and drm_sched_entity_push_job(), or the driver needs to
* guarantee through some other means that this is never called while new jobs
* can be pushed to @entity.
*/
void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
- struct drm_gpu_scheduler **sched_list,
- unsigned int num_sched_list)
+ struct drm_gpu_scheduler **sched_list,
+ unsigned int num_sched_list)
{
- WARN_ON(!num_sched_list || !sched_list);
+ WARN_ON_ONCE(!num_sched_list || !sched_list);
+
+ if (WARN_ON_ONCE(num_sched_list > entity->max_sched_list))
+ num_sched_list = entity->max_sched_list;
spin_lock(&entity->lock);
- entity->sched_list = sched_list;
entity->num_sched_list = num_sched_list;
+ memmove(entity->sched_list, sched_list,
+ sizeof(*sched_list) * num_sched_list);
spin_unlock(&entity->lock);
}
EXPORT_SYMBOL(drm_sched_entity_modify_sched);
@@ -577,10 +594,6 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
struct drm_gpu_scheduler *sched;
struct drm_sched_rq *rq;
- /* single possible engine and already selected */
- if (!entity->sched_list)
- return;
-
/* queue non-empty, stay on the same engine */
if (spsc_queue_count(&entity->job_queue))
return;
@@ -604,9 +617,6 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
entity->rq = rq;
}
- if (entity->num_sched_list == 1)
- entity->sched_list = NULL;
-
out:
spin_unlock(&entity->lock);
}
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index c89a581f54bf..051a7af47aeb 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -121,22 +121,11 @@ struct drm_sched_entity {
struct drm_sched_entity_stats *stats;
/**
- * @sched_list:
+ * @max_sched_list:
*
- * A list of schedulers (struct drm_gpu_scheduler). Jobs from this entity can
- * be scheduled on any scheduler on this list.
- *
- * This can be modified by calling drm_sched_entity_modify_sched().
- * Locking is entirely up to the driver, see the above function for more
- * details.
- *
- * This will be set to NULL if &num_sched_list equals 1 and @rq has been
- * set already.
- *
- * FIXME: This means priority changes through
- * drm_sched_entity_set_priority() will be lost henceforth in this case.
+ * Maximum number of drm_gpu_schedulers in the @sched_list.
*/
- struct drm_gpu_scheduler **sched_list;
+ unsigned int max_sched_list;
/**
* @num_sched_list:
@@ -245,8 +234,38 @@ struct drm_sched_entity {
*/
struct rb_node rb_tree_node;
+ union {
+ /**
+ * @__sched__:
+ *
+ * Built-in storage for callers who only need a single slot.
+ *
+ * MUST NOT be accessed.
+ */
+ struct drm_gpu_scheduler *__sched__;
+
+ /**
+ * @sched_list:
+ *
+ * Flexible array of schedulers this entity can pick from, with
+ * the driver reponsibility to size the array correctly at
+ * declaration and initialization time, and populate the array
+ * via drm_sched_entity_init() and/or
+ * drm_sched_entity_modify_sched().
+ *
+ * It can be modified by calling
+ * drm_sched_entity_modify_sched().
+ */
+ DECLARE_FLEX_ARRAY(struct drm_gpu_scheduler *, sched_list);
+ };
};
+#define DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE(name, listname, max_scheds) \
+struct { \
+ struct drm_sched_entity (name); \
+ struct drm_gpu_scheduler *(listname)[(max_scheds)]; \
+}
+
/**
* struct drm_sched_rq - queue of entities to be scheduled.
*
@@ -708,12 +727,52 @@ static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
}
/* Entities */
-
-int drm_sched_entity_init(struct drm_sched_entity *entity,
+int __drm_sched_entity_init(struct drm_sched_entity *entity,
enum drm_sched_priority priority,
struct drm_gpu_scheduler **sched_list,
+ unsigned int max_sched_list,
unsigned int num_sched_list,
atomic_t *guilty);
+
+/**
+ * drm_sched_entity_init - Init a context entity used by scheduler when
+ * submit to HW ring.
+ *
+ * @entity: scheduler entity to init
+ * @priority: priority of the entity
+ * @sched_list: the list of drm scheds on which jobs from this
+ * entity can be submitted
+ * @num_sched_list: number of drm sched in sched_list
+ * @guilty: atomic_t set to 1 when a job on this queue
+ * is found to be guilty causing a timeout
+ *
+ * Note that the &sched_list must have exactly one element to schedule the
+ * entity.
+ *
+ * The individual drm_gpu_scheduler pointers have borrow semantics, ie.
+ * they must remain valid during entities lifetime, while the containing
+ * array can be freed after this call returns.
+ *
+ * For changing @priority later on at runtime see
+ * drm_sched_entity_set_priority(). For changing the set of schedulers
+ * @sched_list at runtime see drm_sched_entity_modify_sched().
+ *
+ * An entity is cleaned up by calling drm_sched_entity_fini(). See also
+ * drm_sched_entity_destroy().
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+static inline int
+drm_sched_entity_init(struct drm_sched_entity *entity,
+ enum drm_sched_priority priority,
+ struct drm_gpu_scheduler **sched_list,
+ unsigned int num_sched_list,
+ atomic_t *guilty)
+{
+ return __drm_sched_entity_init(entity, priority, sched_list, 1,
+ num_sched_list, guilty);
+}
+
long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout);
void drm_sched_entity_kill(struct drm_sched_entity *entity);
void drm_sched_entity_fini(struct drm_sched_entity *entity);
--
2.56.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 8/8] drm/sched: Untangle the scheduler list mess
2026-10-20 15:41 ` [PATCH v2 8/8] drm/sched: Untangle the scheduler list mess Tvrtko Ursulin
@ 2026-10-20 16:18 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-10-20 16:18 UTC (permalink / raw)
To: Tvrtko Ursulin, dri-devel
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/sched: NULL entity->rq dereference via drm_sched_entity_select_rq()
- [High] drm/sched: invalid flex-array embedding and layout in drm_sched_entity
- [Medium] drm/sched: drm_sched_entity_set_priority() leaves rq_priority stale
--
commit ea4d5caf1dfadd7f3c593ae875d3ce707b5c11f4
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
drm/sched: Untangle the scheduler list mess
This commit reworks drm_sched_entity so that it owns a copy of its
scheduler list via a flexible array member (with a built-in single
scheduler slot), and updates rocket and amdgpu to provide trailing storage
for multi-scheduler entities.
> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 4bc4f9c8ee403..e29ec5755280d 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -511,18 +511,21 @@ void rocket_job_fini(struct rocket_core *core)
> int rocket_job_open(struct rocket_file_priv *rocket_priv)
> {
> struct rocket_device *rdev = rocket_priv->rdev;
> - struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
> - rdev->num_cores);
> + unsigned int num_cores = rdev->num_cores;
> unsigned int core;
> int ret;
>
> - for (core = 0; core < rdev->num_cores; core++)
> - scheds[core] = &rdev->cores[core].sched;
> + if (WARN_ON_ONCE(num_cores > ARRAY_SIZE(rocket_priv->sched_list)))
> + return -EINVAL;
> +
> + for (core = 0; core < num_cores; core++)
> + rocket_priv->sched_list[core] = &rdev->cores[core].sched;
>
> - ret = drm_sched_entity_init(&rocket_priv->sched_entity,
> - DRM_SCHED_PRIORITY_NORMAL,
> - scheds,
> - rdev->num_cores, NULL);
> + ret = __drm_sched_entity_init(&rocket_priv->sched_entity,
> + DRM_SCHED_PRIORITY_NORMAL,
> + rocket_priv->sched_list,
> + ARRAY_SIZE(rocket_priv->sched_list),
> + num_cores, NULL);
[Severity: High]
Because struct drm_sched_entity already includes one pointer slot in its
union (__sched__ and sched_list[]), rocket_priv->sched_list[0] is located
at &rocket_priv->sched_entity.sched_list[1] rather than at index 0.
When rocket_job_open() populates rocket_priv->sched_list[] and passes it
to __drm_sched_entity_init() -> drm_sched_entity_modify_sched(), memmove()
copies num_cores pointers from &sched_entity.sched_list[1] down into
&sched_entity.sched_list[0], shifting the entries in rocket_priv->sched_list
and leaving the two arrays out of sync.
More broadly, adding DECLARE_FLEX_ARRAY(struct drm_gpu_scheduler *,
sched_list) to struct drm_sched_entity makes it a structure with a
flexible array member, which C99/C11 and GCC 14+
(-Wflex-array-member-not-at-end) disallow from being embedded in the
middle of structs or in arrays. However, struct drm_sched_entity is
embedded in DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE itself, in the middle of
struct amdgpu_ctx_entity (before fences[]), struct
amdgpu_ttm_buffer_entity, and struct amdgpu_vm, as well as in arrays across
several DRM drivers (such as amdgpu_mman::move_entities[],
etnaviv_file_private::sched_entity[], panfrost_jm_ctx::slot_entity[], and
v3d_file_priv::sched_entity[]).
Additionally, when drm_sched_entity_modify_sched() writes more than one
element to entity->sched_list, the write crosses the sub-object boundary
of struct drm_sched_entity into the adjacent listname array, which can
trip FORTIFY_SOURCE or UBSAN bounds checks.
Could DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE use TRAILING_OVERLAP() (or a
different storage representation that avoids a flexible array member in
struct drm_sched_entity) so that struct drm_sched_entity can still be
safely embedded in structs and arrays, and listname[0] aliases
sched_list[0]?
[ ... ]
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 79ea20838328d..d4956c6de6ed1 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
[ ... ]
> @@ -159,17 +165,20 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
> entity->priority = priority;
> entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ?
> DRM_SCHED_PRIORITY_KERNEL : priority;
> - entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
>
> spin_lock_init(&entity->lock);
> spsc_queue_init(&entity->job_queue);
>
> + entity->max_sched_list = max_sched_list;
> + drm_sched_entity_modify_sched(entity, sched_list, num_sched_list);
> + drm_sched_entity_select_rq(entity);
[Severity: High]
What happens if all schedulers in sched_list have sched->ready == false
when __drm_sched_entity_init() or drm_sched_entity_select_rq() runs (for
example in amdgpu when a ring fails IB ring tests during init/reset/resume
while ring->sched.sched_rq remains allocated)?
Previously, drm_sched_entity_init() unconditionally set:
entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
and for single-scheduler entities (num_sched_list == 1), entity->sched_list
was set to NULL so drm_sched_entity_select_rq() returned early without
clearing entity->rq.
With this change, __drm_sched_entity_init() leaves entity->rq as NULL from
memset() and calls drm_sched_entity_select_rq():
sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
rq = sched ? sched->sched_rq[entity->rq_priority] : NULL;
if (rq != entity->rq) {
drm_sched_rq_remove_entity(entity->rq, entity);
entity->rq = rq;
}
Because drm_sched_pick_best() skips any scheduler where !sched->ready, it
returns NULL when no scheduler is ready. __drm_sched_entity_init() then
succeeds (returning 0) with entity->rq == NULL, and
drm_sched_entity_select_rq() can also overwrite an existing non-NULL
entity->rq with NULL.
Won't subsequent calls to drm_sched_entity_destroy() ->
drm_sched_entity_flush():
sched = entity->rq->sched;
or drm_sched_job_arm():
drm_sched_entity_select_rq(entity);
sched = entity->rq->sched;
or amdgpu_ctx_fini_entity() -> amdgpu_xcp_release_sched() trigger a NULL
pointer dereference on entity->rq?
[ ... ]
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 76637e21284ac..38b50a84a8210 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -121,22 +121,11 @@ struct drm_sched_entity {
> struct drm_sched_entity_stats *stats;
>
> /**
> - * @sched_list:
> + * @max_sched_list:
> *
> - * A list of schedulers (struct drm_gpu_scheduler). Jobs from this entity can
> - * be scheduled on any scheduler on this list.
> - *
> - * This can be modified by calling drm_sched_entity_modify_sched().
> - * Locking is entirely up to the driver, see the above function for more
> - * details.
> - *
> - * This will be set to NULL if &num_sched_list equals 1 and @rq has been
> - * set already.
> - *
> - * FIXME: This means priority changes through
> - * drm_sched_entity_set_priority() will be lost henceforth in this case.
> + * Maximum number of drm_gpu_schedulers in the @sched_list.
> */
> - struct drm_gpu_scheduler **sched_list;
> + unsigned int max_sched_list;
[Severity: Medium]
Does drm_sched_entity_set_priority() actually switch run-queues when
per-priority run-queues (such as DRM_SCHED_POLICY_FIFO or
DRM_SCHED_POLICY_RR) are used?
In drivers/gpu/drm/scheduler/sched_entity.c, __drm_sched_entity_init()
initializes both priority fields:
entity->priority = priority;
entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ?
DRM_SCHED_PRIORITY_KERNEL : priority;
However, drm_sched_entity_set_priority() only updates entity->priority:
void drm_sched_entity_set_priority(struct drm_sched_entity *entity,
enum drm_sched_priority priority)
{
spin_lock(&entity->lock);
entity->priority = priority;
spin_unlock(&entity->lock);
}
When drm_sched_entity_select_rq() later selects a run-queue, it indexes
sched->sched_rq using entity->rq_priority:
sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
rq = sched ? sched->sched_rq[entity->rq_priority] : NULL;
Since entity->rq_priority is never updated by
drm_sched_entity_set_priority(), won't the entity remain on its original
priority's run-queue when drm_sched_policy is not DRM_SCHED_POLICY_FAIR?
[ ... ]
> @@ -245,8 +234,38 @@ struct drm_sched_entity {
> */
> struct rb_node rb_tree_node;
>
> + union {
> + /**
> + * @__sched__:
> + *
> + * Built-in storage for callers who only need a single slot.
> + *
> + * MUST NOT be accessed.
> + */
> + struct drm_gpu_scheduler *__sched__;
> +
> + /**
> + * @sched_list:
> + *
> + * Flexible array of schedulers this entity can pick from, with
> + * the driver reponsibility to size the array correctly at
> + * declaration and initialization time, and populate the array
> + * via drm_sched_entity_init() and/or
> + * drm_sched_entity_modify_sched().
> + *
> + * It can be modified by calling
> + * drm_sched_entity_modify_sched().
> + */
> + DECLARE_FLEX_ARRAY(struct drm_gpu_scheduler *, sched_list);
> + };
> };
>
> +#define DECLARE_DRM_SCHED_ENTITY_WITH_STORAGE(name, listname, max_scheds) \
> +struct { \
> + struct drm_sched_entity (name); \
> + struct drm_gpu_scheduler *(listname)[(max_scheds)]; \
> +}
[ ... ]
>|}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261020154123.33631-1-tvrtko.ursulin@igalia.com?part=8
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/8] DRM scheduler assorted cleanups
2026-10-20 15:41 [PATCH v2 0/8] DRM scheduler assorted cleanups Tvrtko Ursulin
` (7 preceding siblings ...)
2026-10-20 15:41 ` [PATCH v2 8/8] drm/sched: Untangle the scheduler list mess Tvrtko Ursulin
@ 2026-10-20 16:04 ` Philipp Stanner
8 siblings, 0 replies; 13+ messages in thread
From: Philipp Stanner @ 2026-10-20 16:04 UTC (permalink / raw)
To: Tvrtko Ursulin, dri-devel
Cc: amd-gfx, kernel-dev, Christian König, Danilo Krummrich,
Matthew Brost, Philipp Stanner
On Tue, 2026-10-20 at 16:41 +0100, Tvrtko Ursulin wrote:
> A little bit of a dump of my patch queue leading towards the SPSC queue removal
> (which is still WIP). Primarily because Philipp asked me to rebase and re-post
> "drm/sched: Replace completion with a flush".
What's your merge plan for this? Since it's a series one would expect
that things depend on one another, but patches like 4 look surprisingly
independent.
> Then I realised we agreed to split out one part of that in a separate patch.
completion ⇒ flush patch now need to be part of a series? It was
independent before.
> Specifically "drm/sched: Ignore stopped entity attempt to change its run-queue".
> And I also realized that one is easier to argue for if we also try to tighten
> the rules around entity initialization. Which is "drm/sched: Disallow
> initializing entities with un-initialized schedulers".
>
> Then there are some trivial cleanups in the same area which should be easy
> reviews.
>
> The first patch of the series feel free to ignore. I have sent it separately
> already but need to include it so the series actually applies, both for CI, and
> should someone want to apply the series.
But why is it a series when so many things are independent? :(
P.
^ permalink raw reply [flat|nested] 13+ messages in thread