On Wed, Aug 22, 2018 at 04:45:27PM +0800, Zhang, Jerry (Junwei) wrote:
> 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.
>
Nevermind. :-)
I will use amdgpu_vm_move_to_lru_tail() directly in next version as your
comments.
Thanks,
Ray
