[PATCH 9/9] drm/amdgpu: Lock the attached dmabuf in unpopulate

[Felix Kuehling]

Am 2021-04-14 um 3:33 a.m. schrieb Christian König:
> Am 14.04.21 um 08:46 schrieb Felix Kuehling:
>> amdgpu_ttm_tt_unpopulate can be called during bo_destroy. The
>> dmabuf->resv
>> must not be held by the caller or dma_buf_detach will deadlock. This is
>> probably not the right fix. I get a recursive lock warning with the
>> reservation held in ttm_bo_release. Should unmap_attachment move to
>> backend_unbind instead?
>
> Yes probably, but I'm really wondering if we should call unpopulate
> without holding the reservation lock.

There is an error handling code path in ttm_tt_populate that calls
unpopulate. I believe that has to be holding the reservation lock. The
other cases (destroy and swapout) do not hold the lock, AIUI.

Regards,
  Felix


>
> Christian.
>
>>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 936b3cfdde55..257750921eed 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -1216,9 +1216,22 @@ static void amdgpu_ttm_tt_unpopulate(struct
>> ttm_device *bdev,
>>         if (ttm->sg && gtt->gobj->import_attach) {
>>           struct dma_buf_attachment *attach;
>> +        bool locked;
>>             attach = gtt->gobj->import_attach;
>> +        /* FIXME: unpopulate can be called during bo_destroy.
>> +         * The dmabuf->resv must not be held by the caller or
>> +         * dma_buf_detach will deadlock. This is probably not
>> +         * the right fix. I get a recursive lock warning with the
>> +         * reservation held in ttm_bo_releas.. Should
>> +         * unmap_attachment move to backend_unbind instead?
>> +         */
>> +        locked = dma_resv_is_locked(attach->dmabuf->resv);
>> +        if (!locked)
>> +            dma_resv_lock(attach->dmabuf->resv, NULL);
>>           dma_buf_unmap_attachment(attach, ttm->sg, DMA_BIDIRECTIONAL);
>> +        if (!locked)
>> +            dma_resv_unlock(attach->dmabuf->resv);
>>           ttm->sg = NULL;
>>           return;
>>       }
>