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 <[email protected]>
> > > Signed-off-by: Huang Rui <[email protected]>
> > > Tested-by: Mike Lothian <[email protected]>
> > > Tested-by: Dieter Nützel <[email protected]>
> > > Acked-by: Chunming Zhou <[email protected]>
> > > ---
> > > 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
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
