On 20/10/2026 13:08, Christian König wrote:
On 10/20/26 09:03, Tvrtko Ursulin wrote:
On 20/10/2026 07:47, Philipp Stanner wrote:
On Fri, 2026-10-16 at 09:23 +0100, Tvrtko Ursulin wrote:
On 15/10/2026 20:30, Olivier Kaloudoff wrote:
The hole was latent-but-safe until commit 2df5efb45425 ("drm/sched:
Remove redundant entity->rq initialization and checks") removed the
if (!entity->rq) early return that commit 1decbf6bb0b4 ("drm/sched:
Fix entities with 0 rqs.") had put in drm_sched_entity_flush(), on the
grounds that entity->rq can never be NULL. That holds only for
entities that completed drm_sched_entity_init() — not for the
never-initialized entity this teardown path reaches.
[…]
- drm_sched_entity_destroy(&adev->uvd.entity);
+ if (adev->uvd.entity.rq)
+ drm_sched_entity_destroy(&adev->uvd.entity);
for (j = 0; j < adev->uvd.num_uvd_inst; ++j) {
if (adev->uvd.harvest_config & (1 << j))
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
index 1f7a98cdc..3952af9ae 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
@@ -253,7 +253,8 @@ int amdgpu_vce_sw_fini(struct amdgpu_device *adev)
if (adev->vce.vcpu_bo == NULL)
return 0;
- drm_sched_entity_destroy(&adev->vce.entity);
+ if (adev->vce.entity.rq)
+ drm_sched_entity_destroy(&adev->vce.entity);
for (i = 0; i < adev->vce.num_rings; i++)
amdgpu_ring_fini(&adev->vce.ring[i]);
Dang, it looks like currently the ownership of entity initialization is
broken between the IP agnostic phase which does the init, and per-IP
phase which tears it down.
The fix will get in the way of me re-trying to embed the run-queue into
the scheduler but it does look correct.
Couldn't the status be tracked through the vce struct?
if (vce.entity_initialized)
Otherwise you rely on API-internals (again) which is what enabled that
bug AFAICS. If it's about amdgpu having called foo_init() or not, then
the most robust solution is to track within amdgpu "did I call that
function already?" Same applies to all constructors and destructors in
the kernel AFAIK.
Yep, it is such a horrible pattern to rely on being able to call destructors on
non-initialized objects. I am looking at some flavour of "track in amdgpu" as
priority options. Either by re-arranging the probe steps, or by tracking explicitly.
Yeah the problem goes deeper than the DRM scheduler here.
Normally the sw_fini callback should only be called when the sw_init callback
succeeded. Same for the hw_init/hw_fini.
Somehow amdgpu tries to tear down an IP block which was never full initialized
in the first place. The scheduler is just the first component where it becomes
obvious that something is going wrong here.
So we need to take a step back and ask why sw_fini is called for something that
was never initialized *OR* maybe why the DRM scheduler init is somewhere else
than the fini.
Problem is entity is initialized outside of sw_init (after), while
destroy is done on sw_fini.
Flow is roughly like this:
amdgpu_device_ip_init
sw_init
... danger zone ...
amdgpu_device_init_schedulers (this initializes the entity)
So any probe failure inside the danger zone calls (correctly) sw_fini
and that calls drm_sched_entity_destroy (incorrectly).
I tried moving entity init into the ip block domain, to fix the confused
"ownership", but it is all interleaved and not straightforward to
untangle. For now I gave up and have a local patch which adds an
explicit flag. I'll send it once this one propagates to some visible branch.
At least I have some new trivial cleanup patches as a bonus result,
which I will also send soon.
Regards,
Tvrtko
Reviewed-by: Tvrtko Ursulin <[email protected]>
I will see if I can re-work the init/cleanup phases so this can be
removed, or if not, try to add a proper DRM scheduler API to query the
status of an entity.
An API with which you can query whether a data object that you
allocated and that you own had the constructor run by yourself? Or am I
misunderstanding something?
Yeah it would be bad. I don't think it will come to it since there is at least
one better option.
Regards,
Tvrtko
