[PATCH v2] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Christian König]

Am 03.07.2018 um 03:59 schrieb Zhang, Jerry (Junwei):
> On 07/02/2018 05:23 PM, Christian König wrote:
>> Am 25.06.2018 um 11:07 schrieb Michel Dänzer:
>>> From: Michel Dänzer <michel.daenzer at amd.com>
>>>
>>> Without this, there could not be enough slots, which could trigger the
>>> BUG_ON in reservation_object_add_shared_fence.
>>>
>>> v2:
>>> * Jump to the error label instead of returning directly (Jerry Zhang)
>>
>> Well good catch, but NAK that is not the right solution to the issue.
>>
>> amdgpu_vm_update_directories() is only called by two places, the 
>> first one is amdgpu_bo_vm_update_pte() and the second 
>> amdgpu_gem_va_update_vm().
>>
>> In both cases the directory is reserved using 
>> ttm_eu_reserve_buffers() with "entry->tv.shared = true;".
>>
>> What most likely needs to be done instead is to call 
>> reservation_object_reserve_shared() after all the VM operations in 
>> amdgpu_bo_vm_update_pte() to make sure that we still have one 
>> additional slot
>> for the command submission.
>
> In this case, it looks like amdgpu_vm_bo_update_mapping() handling, 
> inside ttm_eu_reserve_buffers() as well.
>
> Is there any specific reason to call it at the end of 
> amdgpu_bo_vm_update_pte()?
>
> AFAIW, that has to be called right before where to add shared slot.

The reservation_object_reserve_shared() function needs to be called 
before you make submissions to the hardware to make sure that you have a 
free slot after you made the submission.

The problem is now that we always reserve one free slot during command 
submission, but for the page directory reservation object it can happen 
that this free slot is used by VM updates.

So the safest approach is to make sure that we have another free slot 
after the VM updates are done.

Michel's patch nearly did that, because we rarely have only one 
submission in amdgpu_vm_update_directories(). But it isn't 100% correct.

Christian.

>
> Jerry
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Cc: stable at vger.kernel.org
>>> Bugzilla: https://bugs.freedesktop.org/106418
>>> Reported-by: mikhail.v.gavrilov at gmail.com
>>> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 590db78b8c72..579bcc33d544 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1111,6 +1111,10 @@ int amdgpu_vm_update_directories(struct 
>>> amdgpu_device *adev,
>>>           struct amdgpu_ring *ring;
>>>           struct dma_fence *fence;
>>> +        r = reservation_object_reserve_shared(root->tbo.resv);
>>> +        if (r)
>>> +            goto error;
>>> +
>>>           ring = container_of(vm->entity.sched, struct amdgpu_ring,
>>>                       sched);
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx