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.
Regards,
Christian.
>
>>>
>>> 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
>
