On 10/31/25 12:50, Tvrtko Ursulin wrote:
>
> On 31/10/2025 09:07, Pierre-Eric Pelloux-Prayer wrote:
>> The Mesa issue referenced below pointed out a possible deadlock:
>>
>> [ 1231.611031] Possible interrupt unsafe locking scenario:
>>
>> [ 1231.611033] CPU0 CPU1
>> [ 1231.611034] ---- ----
>> [ 1231.611035] lock(&xa->xa_lock#17);
>> [ 1231.611038] local_irq_disable();
>> [ 1231.611039] lock(&fence->lock);
>> [ 1231.611041] lock(&xa->xa_lock#17);
>> [ 1231.611044] <Interrupt>
>> [ 1231.611045] lock(&fence->lock);
>> [ 1231.611047]
>> *** DEADLOCK ***
>>
>> In this example, CPU0 would be any function accessing job->dependencies
>> through the xa_* functions that doesn't disable interrupts (eg:
>> drm_sched_job_add_dependency, drm_sched_entity_kill_jobs_cb).
>>
>> CPU1 is executing drm_sched_entity_kill_jobs_cb as a fence signalling
>> callback so in an interrupt context. It will deadlock when trying to
>> grab the xa_lock which is already held by CPU0.
>>
>> Replacing all xa_* usage by their xa_*_irq counterparts would fix
>> this issue, but Christian pointed out another issue: dma_fence_signal
>> takes fence.lock and so does dma_fence_add_callback.
>>
>> dma_fence_signal() // locks f1.lock
>> -> drm_sched_entity_kill_jobs_cb()
>> -> foreach dependencies
>> -> dma_fence_add_callback() // locks f2.lock
>>
>> This will deadlock if f1 and f2 share the same spinlock.
>
> Is it possible to hit this case?
>
> Same lock means same execution timeline
Nope, exactly that is incorrect. It's completely up to the implementation what
they use this lock for.
>, which should mean dependency should have been squashed in
>drm_sched_job_add_dependency(), no?
This makes it less likely, but not impossible to trigger.
Regards,
Christian.
>
> Or would sharing the lock but not sharing the entity->fence_context be
> considered legal? It would be surprising at least.
>
> Also, would anyone have time to add a kunit test? ;)
>
> Regards,
>
> Tvrtko
>
>> To fix both issues, the code iterating on dependencies and re-arming them
>> is moved out to drm_sched_entity_kill_jobs_work.
>>
>> Link: https://gitlab.freedesktop.org/mesa/mesa/-/issues/13908
>> Reported-by: Mikhail Gavrilov <[email protected]>
>> Suggested-by: Christian König <[email protected]>
>> Reviewed-by: Christian König <[email protected]>
>> Signed-off-by: Pierre-Eric Pelloux-Prayer
>> <[email protected]>
>> ---
>> drivers/gpu/drm/scheduler/sched_entity.c | 34 +++++++++++++-----------
>> 1 file changed, 19 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
>> b/drivers/gpu/drm/scheduler/sched_entity.c
>> index c8e949f4a568..fe174a4857be 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -173,26 +173,15 @@ int drm_sched_entity_error(struct drm_sched_entity
>> *entity)
>> }
>> EXPORT_SYMBOL(drm_sched_entity_error);
>> +static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
>> + struct dma_fence_cb *cb);
>> +
>> static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk)
>> {
>> struct drm_sched_job *job = container_of(wrk, typeof(*job), work);
>> -
>> - drm_sched_fence_scheduled(job->s_fence, NULL);
>> - drm_sched_fence_finished(job->s_fence, -ESRCH);
>> - WARN_ON(job->s_fence->parent);
>> - job->sched->ops->free_job(job);
>> -}
>> -
>> -/* Signal the scheduler finished fence when the entity in question is
>> killed. */
>> -static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
>> - struct dma_fence_cb *cb)
>> -{
>> - struct drm_sched_job *job = container_of(cb, struct drm_sched_job,
>> - finish_cb);
>> + struct dma_fence *f;
>> unsigned long index;
>> - dma_fence_put(f);
>> -
>> /* Wait for all dependencies to avoid data corruptions */
>> xa_for_each(&job->dependencies, index, f) {
>> struct drm_sched_fence *s_fence = to_drm_sched_fence(f);
>> @@ -220,6 +209,21 @@ static void drm_sched_entity_kill_jobs_cb(struct
>> dma_fence *f,
>> dma_fence_put(f);
>> }
>> + drm_sched_fence_scheduled(job->s_fence, NULL);
>> + drm_sched_fence_finished(job->s_fence, -ESRCH);
>> + WARN_ON(job->s_fence->parent);
>> + job->sched->ops->free_job(job);
>> +}
>> +
>> +/* Signal the scheduler finished fence when the entity in question is
>> killed. */
>> +static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
>> + struct dma_fence_cb *cb)
>> +{
>> + struct drm_sched_job *job = container_of(cb, struct drm_sched_job,
>> + finish_cb);
>> +
>> + dma_fence_put(f);
>> +
>> INIT_WORK(&job->work, drm_sched_entity_kill_jobs_work);
>> schedule_work(&job->work);
>> }
>
