On 08/22/2018 04:38 PM, Huang Rui wrote:
> On Wed, Aug 22, 2018 at 04:33:30PM +0800, Huang Rui wrote:
>> On Wed, Aug 22, 2018 at 04:07:20PM +0800, Zhang, Jerry wrote:
>>> On 08/22/2018 03:52 PM, Huang Rui wrote:
>>>> I continue to work for bulk moving that based on the proposal by Christian.
>>>>
>>>> Background:
>>>> amdgpu driver will move all PD/PT and PerVM BOs into idle list. Then move all of
>>>> them on the end of LRU list one by one. Thus, that cause so many BOs moved to
>>>> the end of the LRU, and impact performance seriously.
>>>>
>>>> Then Christian provided a workaround to not move PD/PT BOs on LRU with below
>>>> patch:
>>>> Commit 0bbf32026cf5ba41e9922b30e26e1bed1ecd38ae ("drm/amdgpu: band aid
>>>> validating VM PTs")
>>>>
>>>> However, the final solution should bulk move all PD/PT and PerVM BOs on the LRU
>>>> instead of one by one.
>>>>
>>>> Whenever amdgpu_vm_validate_pt_bos() is called and we have BOs which need to be
>>>> validated we move all BOs together to the end of the LRU without dropping the
>>>> lock for the LRU.
>>>>
>>>> While doing so we note the beginning and end of this block in the LRU list.
>>>>
>>>> Now when amdgpu_vm_validate_pt_bos() is called and we don't have anything to do,
>>>> we don't move every BO one by one, but instead cut the LRU list into pieces so
>>>> that we bulk move everything to the end in just one operation.
>>>>
>>>> Test data:
>>>> +--------------+-----------------+-----------+---------------------------------------+
>>>> | |The Talos |Clpeak(OCL)|BusSpeedReadback(OCL) |
>>>> | |Principle(Vulkan)| | |
>>>> +------------------------------------------------------------------------------------+
>>>> | | | |0.319 ms(1k) 0.314 ms(2K) 0.308 ms(4K) |
>>>> | Original | 147.7 FPS | 76.86 us |0.307 ms(8K) 0.310 ms(16K) |
>>>> +------------------------------------------------------------------------------------+
>>>> | Orignial + WA| | |0.254 ms(1K) 0.241 ms(2K) |
>>>> |(don't move | 162.1 FPS | 42.15 us |0.230 ms(4K) 0.223 ms(8K) 0.204 ms(16K)|
>>>> |PT BOs on LRU)| | | |
>>>> +------------------------------------------------------------------------------------+
>>>> | Bulk move | 163.1 FPS | 40.52 us |0.244 ms(1K) 0.252 ms(2K) 0.213 ms(4K) |
>>>> | | | |0.214 ms(8K) 0.225 ms(16K) |
>>>> +--------------+-----------------+-----------+---------------------------------------+
>>>>
>>>> After test them with above three benchmarks include vulkan and opencl. We can
>>>> see the visible improvement than original, and even better than original with
>>>> workaround.
>>>>
>>>> v2: move all BOs include idle, relocated, and moved list to the end of LRU and
>>>> put them together.
>>>> v3: remove unused parameter and use list_for_each_entry instead of the one with
>>>> save entry.
>>>> v4: move the amdgpu_vm_move_to_lru_tail after command submission, at that time,
>>>> all bo will be back on idle list.
>>>> v5: remove amdgpu_vm_move_to_lru_tail_by_list(), use bulk_moveable instread of
>>>> validated, and move ttm_bo_bulk_move_lru_tail() also into
>>>> amdgpu_vm_move_to_lru_tail().
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> Signed-off-by: Huang Rui <ray.huang at amd.com>
>>>> Tested-by: Mike Lothian <mike at fireburn.co.uk>
>>>> Tested-by: Dieter Nützel <Dieter at nuetzel-hh.de>
>>>> Acked-by: Chunming Zhou <david1.zhou at amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 10 ++++++
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 66 +++++++++++++++++++++++-----------
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 11 +++++-
>>>> 3 files changed, 65 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index 502b94f..4efdbd2 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -1260,6 +1260,15 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>>>> return 0;
>>>> }
>>>>
>>>> +static void amdgpu_cs_vm_move_on_lru(struct amdgpu_device *adev,
>>>> + struct amdgpu_cs_parser *p)
>>>> +{
>>>> + struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
>>>> + struct amdgpu_vm *vm = &fpriv->vm;
>>>> +
>>>> + amdgpu_vm_move_to_lru_tail(adev, vm);
>>>> +}
>>>> +
>>>> int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>>>> {
>>>> struct amdgpu_device *adev = dev->dev_private;
>>>> @@ -1310,6 +1319,7 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>>>>
>>>> r = amdgpu_cs_submit(&parser, cs);
>>>>
>>>> + amdgpu_cs_vm_move_on_lru(adev, &parser);
>>>
>>> Looks we can call amdgpu_vm_move_to_lru_tail() directly.
>>
>> Both ok, here, I just
>>
>
> Missed this comment. My intention is to align vm member in vm functions.
> Anyway, both is ok for me.
Thanks for explanation, got it.
BTW, Personally I'd prefer to call vm function directly, especially in kernel space.
Regards,
Jerry
>
> Thanks,
> Ray
>
