[PATCH] drm/amdgpu: Reserve fence slots for command submission

[Zhang, Jerry (Junwei)]

On 07/04/2018 05:15 PM, Michel Dänzer wrote:
> On 2018-07-04 11:01 AM, Christian König wrote:
>> Am 04.07.2018 um 09:57 schrieb Zhang, Jerry (Junwei):
>>> On 07/04/2018 03:21 PM, Michel Dänzer wrote:
>>>> On 2018-07-04 08:55 AM, Zhang, Jerry (Junwei) wrote:
>>>>> On 07/04/2018 02:34 PM, Christian König wrote:
>>>>>> Am 04.07.2018 um 05:02 schrieb Junwei Zhang:
>>>>>>> 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)
>>>>>>> v3:
>>>>>>> * Reserve slots for command submission after VM updates (Christian
>>>>>>> König)
>>>>>>>
>>>>>>> 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>
>>>>>>> Signed-off-by: Junwei Zhang <Jerry.Zhang at amd.com>
>>>>>>
>>>>>> I would put that at the end of amdgpu_bo_vm_update_pte(), but that is
>>>>>> only a minor nit pick.
>>>>>
>>>>> At first, I really put it at the end of amdgpu_bo_vm_update_pte().
>>>>> On the 2nd thought, that func may be called by others(although it's not
>>>>> for now), so I move it out of there to the caller.
>>
>> Well that is exactly the reason I wanted to have it in
>> amdgpu_bo_vm_update_pte() :)
>>
>> But you are right, there are good arguments for both approach and as
>> long as we don't have a second user it doesn't matter.
>
> In general, I think it's better to keep the
> reservation_object_reserve_shared calls as close as possible to the
> corresponding reservation_object_add_shared_fence calls, otherwise it's
> hard to keep track of whether or not a slot is reserved at any given point.

Agree, I also consider that previously, just before the command submission.
while I think 2 more things then:

1) amdgpu_cs_ib_vm_chunk() is unique func during cs ioctl,
in another word, it's a MUST part of that, so it will not impact others.
Additionally it also checks if vm used, otherwise, the reserve is not needed either.

2) adding the reserve with vm checking in amdgpu_cs_submit() looks not so good as well.
IMO. so I decide to insert that in amdgpu_cs_ib_vm_chunk() vm existing case.

BTW, we may add some comments in code as well to better reference in the future.

Jerry

>
>