[PATCH v2 1/3] drm/amdgpu: Correct unlocked update fence handling

[Christian König]

Am 28.03.22 um 15:35 schrieb philip yang:
>
>
> On 2022-03-28 9:14 a.m., Christian König wrote:
>> Am 28.03.22 um 15:06 schrieb Philip Yang:
>>> To fix two issues with unlocked update fence:
>>>
>>> 1. vm->last_unlocked store the latest fence without taking refcount.
>>> 2. amdgpu_vm_bo_update_mapping returns old fence, not the latest fence.
>>
>> NAK, that code here is perfectly correct.
>>
>> vm->last_unlocked gets a reference to the last unlocked operation.
>>
>> Otherwise the last locked operation is added directly to the root 
>> resv object.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Signed-off-by: Philip Yang <Philip.Yang at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> index dbb551762805..69fba68ff88e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> @@ -109,7 +109,7 @@ static int amdgpu_vm_sdma_commit(struct 
>>> amdgpu_vm_update_params *p,
>>>       if (p->unlocked) {
>>>           struct dma_fence *tmp = dma_fence_get(f);
>>>   -        swap(p->vm->last_unlocked, f);
>>> +        swap(p->vm->last_unlocked, tmp);
>
>           dma_fence_put(tmp);
>
> tmp is the new unlock fence, so this drop the new fence refcount, fix 
> is to move the old fence to tmp, drop the old fence refcount, take the 
> new fence reference.
>
> f is return fence, because f swap to old fence, so old fence is 
> returned, not new fence, fix will not change f.
>

No, swap() will exchange the parameters a and b.

So tmp is pointing to the old fence when dma_fence_put() is called on it.

That reference counting is correct as far as I can see.

Regards,
Christian.

> Regards,
>
> Philip
>
>>>       } else {
>>>           amdgpu_bo_fence(p->vm->root.bo, f, true);
>>