[PATCH v5 4/5] drm/amdgpu: use bulk moves for efficient VM LRU handling (v5)

[Huang Rui]

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,
Ray