RE: [PATCH] drm/amdgpu: fix VM sync with always valid BOs v2

["Zhou, David(ChunMing)" <David1.Zhou-5C7GfCeVMHo@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 7396 bytes --]

My mean is you should add a condition in code to check if BO is kernel BO or not. I expect V3.

Regards,
David


发自坚果 Pro

He, Roger <Hongbo.He@amd.com> 于 2017年9月11日 下午6:57写道：

Reviewed-by: Roger He <Hongbo.He@amd.com>

Thanks
Roger(Hongbo.He)
-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Christian K?nig
Sent: Monday, September 11, 2017 6:53 PM
To: Zhou, David(ChunMing) <David1.Zhou@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: fix VM sync with always valid BOs v2

Am 11.09.2017 um 11:10 schrieb zhoucm1:
>
>
> On 2017年09月11日 17:04, Christian König wrote:
>>> NAK, as Roger pointed, the performance of Vulkan Dota2 drops from
>>> 72fps to 24fps.
>>>
>>> This patches will kill Per-vm-BO advantages, even drop to 1/3 of
>>> previous non-per-vm-bo.
>>
>> This is irrelevant and actually a foreseen consequence of per VM BOs.
>>
>> If the performance drops is in-acceptable then the UMD shouldn't use
>> this feature.
> I got your mean, if UMD doesn't use this feature, then all BOs should
> be kernel only, then this change should only be limited to kernel VM
> BOs, not include UMD BOs.
> With this limitation, I think the change also can fix your issue.

Correct, yes. Can I get your rb or ab now? We need to get this fixed asap.

Thanks,
Christian.

>
> Regards,
> David Zhou
>>
>>> all moved and relocated fence have already synced, we just need to
>>> catch the va mapping but which is CS itself required, as my proposal
>>> in another thread, we maybe need to expand va_ioctl to return
>>> mapping fence to UMD, then UMD passes it to CS as dependency.
>>
>> That can be an addition to the existing handling, but first of all
>> the current state must be corrected.
>>
>> There are at least two bug reports on the open driver fixed by this,
>> so please review.
>>
>> Thanks,
>> Christian.
>>
>> Am 11.09.2017 um 10:59 schrieb zhoucm1:
>>> NAK, as Roger pointed, the performance of Vulkan Dota2 drops from
>>> 72fps to 24fps.
>>>
>>> This patches will kill Per-vm-BO advantages, even drop to 1/3 of
>>> previous non-per-vm-bo.
>>>
>>> all moved and relocated fence have already synced, we just need to
>>> catch the va mapping but which is CS itself required, as my proposal
>>> in another thread, we maybe need to expand va_ioctl to return
>>> mapping fence to UMD, then UMD passes it to CS as dependency.
>>>
>>>
>>> Regards,
>>>
>>> David Zhou
>>>
>>>
>>> On 2017年09月11日 15:53, Christian König wrote:
>>>> From: Christian König <christian.koenig@amd.com>
>>>>
>>>> All users of a VM must always wait for updates with always valid
>>>> BOs to be completed.
>>>>
>>>> v2: remove debugging leftovers, rename struct member
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 10 ++++++----
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 15 ++++++++++-----
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  2 +-
>>>>   3 files changed, 17 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index 8aa37e0..4681dcc 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -752,10 +752,6 @@ static int amdgpu_bo_vm_update_pte(struct
>>>> amdgpu_cs_parser *p)
>>>>       if (r)
>>>>           return r;
>>>>   -    r = amdgpu_sync_fence(adev, &p->job->sync,
>>>> vm->last_dir_update);
>>>> -    if (r)
>>>> -        return r;
>>>> -
>>>>       r = amdgpu_vm_clear_freed(adev, vm, NULL);
>>>>       if (r)
>>>>           return r;
>>>> @@ -810,6 +806,12 @@ static int amdgpu_bo_vm_update_pte(struct
>>>> amdgpu_cs_parser *p)
>>>>       }
>>>>         r = amdgpu_vm_handle_moved(adev, vm, &p->job->sync);
>>>> +    if (r)
>>>> +        return r;
>>>> +
>>>> +    r = amdgpu_sync_fence(adev, &p->job->sync, vm->last_update);
>>>> +    if (r)
>>>> +        return r;
>>>>         if (amdgpu_vm_debug && p->bo_list) {
>>>>           /* Invalidate all BOs to test for userspace bugs */ diff
>>>> --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 55f1ecb..5042f09 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1140,9 +1140,8 @@ static int amdgpu_vm_update_level(struct
>>>> amdgpu_device *adev,
>>>>                   goto error_free;
>>>>                 amdgpu_bo_fence(parent->base.bo, fence, true);
>>>> -            dma_fence_put(vm->last_dir_update);
>>>> -            vm->last_dir_update = dma_fence_get(fence);
>>>> -            dma_fence_put(fence);
>>>> +            dma_fence_put(vm->last_update);
>>>> +            vm->last_update = fence;
>>>>           }
>>>>       }
>>>>   @@ -1803,6 +1802,12 @@ int amdgpu_vm_bo_update(struct
>>>> amdgpu_device *adev,
>>>>               trace_amdgpu_vm_bo_mapping(mapping);
>>>>       }
>>>>   +    if (bo_va->base.bo &&
>>>> +        bo_va->base.bo->tbo.resv == vm->root.base.bo->tbo.resv) {
>>>> +        dma_fence_put(vm->last_update);
>>>> +        vm->last_update = dma_fence_get(bo_va->last_pt_update);
>>>> +    }
>>>> +
>>>>       return 0;
>>>>   }
>>>>   @@ -2586,7 +2591,7 @@ int amdgpu_vm_init(struct amdgpu_device
>>>> *adev, struct amdgpu_vm *vm,
>>>>                vm->use_cpu_for_update ? "CPU" : "SDMA");
>>>>       WARN_ONCE((vm->use_cpu_for_update &
>>>> !amdgpu_vm_is_large_bar(adev)),
>>>>             "CPU update of VM recommended only for large BAR
>>>> system\n");
>>>> -    vm->last_dir_update = NULL;
>>>> +    vm->last_update = NULL;
>>>>         flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
>>>>               AMDGPU_GEM_CREATE_VRAM_CLEARED; @@ -2692,7 +2697,7 @@
>>>> void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm
>>>> *vm)
>>>>       }
>>>>         amdgpu_vm_free_levels(&vm->root);
>>>> -    dma_fence_put(vm->last_dir_update);
>>>> +    dma_fence_put(vm->last_update);
>>>>       for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
>>>>           amdgpu_vm_free_reserved_vmid(adev, vm, i);
>>>>   }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> index c1accd1..cb6a622 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> @@ -140,7 +140,7 @@ struct amdgpu_vm {
>>>>         /* contains the page directory */
>>>>       struct amdgpu_vm_pt     root;
>>>> -    struct dma_fence    *last_dir_update;
>>>> +    struct dma_fence    *last_update;
>>>>         /* protecting freed */
>>>>       spinlock_t        freed_lock;
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[-- Attachment #1.2: Type: text/html, Size: 14016 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx