* Re: [PATCH 1/2] drm/sched: keep the current runqueue when no scheduler is ready
2026-09-28 1:59 [PATCH 1/2] drm/sched: keep the current runqueue when no scheduler is ready vitaly.prosyak
@ 2026-09-28 2:20 ` Matthew Brost
2026-09-28 8:52 ` Danilo Krummrich
2026-09-28 7:49 ` Philipp Stanner
2026-09-28 10:12 ` Christian König
2 siblings, 1 reply; 5+ messages in thread
From: Matthew Brost @ 2026-09-28 2:20 UTC (permalink / raw)
To: vitaly.prosyak
Cc: amd-gfx, dri-devel, Christian König, Alex Deucher,
Danilo Krummrich, Philipp Stanner
On Sun, Sep 27, 2026 at 09:59:02PM -0400, vitaly.prosyak@amd.com wrote:
> From: Vitaly Prosyak <vitaly.prosyak@amd.com>
>
> The IGT amd_dispatch test exposed a NULL pointer dereference in the
> AMDGPU CS submission path when the GPU schedulers were not ready.
>
> drm_sched_pick_best() returns NULL when every scheduler in an entity's
> list is marked not ready. drm_sched_entity_select_rq() then replaces
> the entity's existing runqueue with NULL.
>
> A subsequent drm_sched_job_arm() retains that invalid runqueue.
> When AMDGPU CS submission calls drm_sched_entity_push_job(), the
> scheduler pointer derived from entity->rq is invalid and the access
> to sched->score faults. The reported oops shows the sequence:
>
> [drm] scheduler comp_1.1.0 is not ready, skipping
> [drm] scheduler comp_1.2.0 is not ready, skipping
> BUG: kernel NULL pointer dereference, address: 0000000000000268
> RIP: drm_sched_entity_push_job+0x4f/0x2b0 [gpu_sched]
> Call Trace:
> amdgpu_cs_ioctl+0x1e9e/0x2530 [amdgpu]
>
> Keep the previously selected runqueue when no ready replacement is
> found. This prevents scheduler selection from turning a valid entity
> runqueue into NULL; it does not make a stopped scheduler ready or
> guarantee that the submitted job will execute.
>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Philipp Stanner <phasta@kernel.org>
> Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
> ---
> drivers/gpu/drm/scheduler/sched_entity.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 4ebb513255ed..b11e1dddabd0 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -584,8 +584,8 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
>
> spin_lock(&entity->lock);
> sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
This entire thing is broken, badly.
So I assume this pops too in drm_sched_pick_best?
969 if (!sched->ready) {
970 DRM_WARN("scheduler %s is not ready, skipping",
971 sched->name);
972 continue;
973 }
To me, this looks like a lifetime issue that should be fixed in AMDGPU.
Either that, or we need to rework DRM to have proper lifetime management,
or, of course, just deprecate drm_sched.
In other words, we need refcounting so that drm_sched_fini() cannot be
called while jobs are still in flight, nor can jobs be submitted before
drm_sched_init(). Alternatively, drm_dep could serve as a replacement.
So this is a NAK from me. As it stands, this is papering over a larger
issue. You won't hit a NULL pointer dereference, but sched->ready on
entity->rq will be false. At that point, what does sched->ready even
mean?
Matt
> - rq = sched ? &sched->rq : NULL;
> - if (rq != entity->rq) {
> + if (sched && &sched->rq != entity->rq) {
> + rq = &sched->rq;
> drm_sched_rq_remove_entity(entity->rq, entity);
> entity->rq = rq;
> }
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] drm/sched: keep the current runqueue when no scheduler is ready
2026-09-28 2:20 ` Matthew Brost
@ 2026-09-28 8:52 ` Danilo Krummrich
0 siblings, 0 replies; 5+ messages in thread
From: Danilo Krummrich @ 2026-09-28 8:52 UTC (permalink / raw)
To: Matthew Brost
Cc: vitaly.prosyak, amd-gfx, dri-devel, Christian König,
Alex Deucher, Philipp Stanner
On Mon Sep 28, 2026 at 4:20 AM CEST, Matthew Brost wrote:
> To me, this looks like a lifetime issue that should be fixed in AMDGPU.
> Either that, or we need to rework DRM to have proper lifetime management,
> or, of course, just deprecate drm_sched.
Agreed. The lifetime and ownership model in drm_sched is not fundamentally
wrong, but the implementation is not consistently keeping it up and drivers also
don't always honor it.
> In other words, we need refcounting so that drm_sched_fini() cannot be
> called while jobs are still in flight, nor can jobs be submitted before
> drm_sched_init(). Alternatively, drm_dep could serve as a replacement.
I'm not a huge fan of refcounting for those kind of things because it
fundamentally incentivises the wrong lifetime model in the context of the driver
model. The driver model requires a bounded lifetime scope, but refcounting
incentivises an unbounded lifetime model, which leads to other problems.
So, especially for the sake of deferring drm_sched_fini() from running jobs,
drivers still have to make sure that all jobs are torn down and drm_sched_fini()
is called *before* driver unbind completes. IOW, drivers should tear down the
hardware and hence all jobs latest in remove() and then call drm_sched_fini()
subsequently.
Refcounting does not provide a lot of value in this regard, because we have to
somehow guarantee that the hardware and all jobs are torn down at this specific
boundary anyway.
This is also my biggest concern about drm_dep, it seems to be designed with
exactly the idea of an unbounded lifetime model, which is not the correct design
for anything that represents a device resource (e.g. a GPU job).
Now, to be fair, refcounting is really the only mechanism that we have in C to
manage lifetimes, everything else is more or less just a convention. That said,
I'm not all against refcounting in general, but we have to be careful about how
it influences the design in terms of a bounded and unbounded lifetime model.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] drm/sched: keep the current runqueue when no scheduler is ready
2026-09-28 1:59 [PATCH 1/2] drm/sched: keep the current runqueue when no scheduler is ready vitaly.prosyak
2026-09-28 2:20 ` Matthew Brost
@ 2026-09-28 7:49 ` Philipp Stanner
2026-09-28 10:12 ` Christian König
2 siblings, 0 replies; 5+ messages in thread
From: Philipp Stanner @ 2026-09-28 7:49 UTC (permalink / raw)
To: vitaly.prosyak, amd-gfx, dri-devel
Cc: Christian König, Alex Deucher, Matthew Brost,
Danilo Krummrich, Philipp Stanner
On Sun, 2026-09-27 at 21:59 -0400, vitaly.prosyak@amd.com wrote:
> From: Vitaly Prosyak <vitaly.prosyak@amd.com>
>
> The IGT amd_dispatch test exposed a NULL pointer dereference in the
> AMDGPU CS submission path when the GPU schedulers were not ready.
>
> drm_sched_pick_best() returns NULL when every scheduler in an entity's
> list is marked not ready. drm_sched_entity_select_rq() then replaces
> the entity's existing runqueue with NULL.
>
> A subsequent drm_sched_job_arm() retains that invalid runqueue.
> When AMDGPU CS submission calls drm_sched_entity_push_job(), the
> scheduler pointer derived from entity->rq is invalid and the access
> to sched->score faults. The reported oops shows the sequence:
>
> [drm] scheduler comp_1.1.0 is not ready, skipping
> [drm] scheduler comp_1.2.0 is not ready, skipping
> BUG: kernel NULL pointer dereference, address: 0000000000000268
> RIP: drm_sched_entity_push_job+0x4f/0x2b0 [gpu_sched]
That seems to be the issue that the LLM was hinting at a few weeks ago:
https://lore.kernel.org/dri-devel/20260625121102.3AB3A1F000E9@smtp.kernel.org/
> Call Trace:
> amdgpu_cs_ioctl+0x1e9e/0x2530 [amdgpu]
>
> Keep the previously selected runqueue when no ready replacement is
> found. This prevents scheduler selection from turning a valid entity
> runqueue into NULL; it does not make a stopped scheduler ready or
> guarantee that the submitted job will execute.
>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Philipp Stanner <phasta@kernel.org>
> Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
> ---
> drivers/gpu/drm/scheduler/sched_entity.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 4ebb513255ed..b11e1dddabd0 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -584,8 +584,8 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
>
> spin_lock(&entity->lock);
> sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
> - rq = sched ? &sched->rq : NULL;
> - if (rq != entity->rq) {
> + if (sched && &sched->rq != entity->rq) {
> + rq = &sched->rq;
I'm not sure if either explicit or implicit "fix" within drm_sched
would solve the issue at hand, because the entire "ready" state
tracking is actually a responsibility of the driver – AFAIK its
existence has to do with AMD GPUs' ring resetting? I had a conversation
with Christian about that a while ago; I think we agree that that flag
should be carried by the driver in its own data structure.
Does amdgpu still modify sched->ready without going through a drm_sched
API? A first grep hints at about ~135 places where this might be the
case.
What I'm especially wondering about is whether different threads
participate in handling that state; dealing with the flag is in no way
synchronized in drm_sched_init() and drm_sched_fini(). If yes, the
proposed fix would likely still be UB.
So since this patch seems to stem from a broken test, not an actual bug
at a customer, my hope would be that we have some time to fix it and
that AMD, thus, can help with reducing drm_sched's tech debt by
removing that flag and have amdgpu invoke the scheduler API in the
correct order.
Thx
P.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] drm/sched: keep the current runqueue when no scheduler is ready
2026-09-28 1:59 [PATCH 1/2] drm/sched: keep the current runqueue when no scheduler is ready vitaly.prosyak
2026-09-28 2:20 ` Matthew Brost
2026-09-28 7:49 ` Philipp Stanner
@ 2026-09-28 10:12 ` Christian König
2 siblings, 0 replies; 5+ messages in thread
From: Christian König @ 2026-09-28 10:12 UTC (permalink / raw)
To: vitaly.prosyak, amd-gfx, dri-devel
Cc: Alex Deucher, Matthew Brost, Danilo Krummrich, Philipp Stanner
On 9/28/26 03:59, vitaly.prosyak@amd.com wrote:
> From: Vitaly Prosyak <vitaly.prosyak@amd.com>
>
> The IGT amd_dispatch test exposed a NULL pointer dereference in the
> AMDGPU CS submission path when the GPU schedulers were not ready.
>
> drm_sched_pick_best() returns NULL when every scheduler in an entity's
> list is marked not ready. drm_sched_entity_select_rq() then replaces
> the entity's existing runqueue with NULL.
That is perfectly correct behavior as far as I can see.
The schedulers should only be marked not ready when they are permanently dead and in this case keeping the existing rq doesn't make sense any more either.
>
> A subsequent drm_sched_job_arm() retains that invalid runqueue.
> When AMDGPU CS submission calls drm_sched_entity_push_job(), the
> scheduler pointer derived from entity->rq is invalid and the access
> to sched->score faults. The reported oops shows the sequence:
>
> [drm] scheduler comp_1.1.0 is not ready, skipping
> [drm] scheduler comp_1.2.0 is not ready, skipping
> BUG: kernel NULL pointer dereference, address: 0000000000000268
> RIP: drm_sched_entity_push_job+0x4f/0x2b0 [gpu_sched]
> Call Trace:
> amdgpu_cs_ioctl+0x1e9e/0x2530 [amdgpu]
That is clearly a bug in amdgpu. The scheduler behavior here is correct.
Regards,
Christian.
>
> Keep the previously selected runqueue when no ready replacement is
> found. This prevents scheduler selection from turning a valid entity
> runqueue into NULL; it does not make a stopped scheduler ready or
> guarantee that the submitted job will execute.
>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Philipp Stanner <phasta@kernel.org>
> Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
> ---
> drivers/gpu/drm/scheduler/sched_entity.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 4ebb513255ed..b11e1dddabd0 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -584,8 +584,8 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
>
> spin_lock(&entity->lock);
> sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
> - rq = sched ? &sched->rq : NULL;
> - if (rq != entity->rq) {
> + if (sched && &sched->rq != entity->rq) {
> + rq = &sched->rq;
> drm_sched_rq_remove_entity(entity->rq, entity);
> entity->rq = rq;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread