amd-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* Re: [RFC PATCH 1/2] drm/amdgpu: add direct ib pool
@ 2020-03-26  6:51 Koenig, Christian
  2020-03-26  7:11 ` Pan, Xinhui
  0 siblings, 1 reply; 8+ messages in thread
From: Koenig, Christian @ 2020-03-26  6:51 UTC (permalink / raw)
  To: Pan, Xinhui; +Cc: Deucher, Alexander, Kuehling, Felix, amd-gfx


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



Am 26.03.2020 07:45 schrieb "Pan, Xinhui" <Xinhui.Pan@amd.com>:


> 2020年3月26日 14:36,Koenig, Christian <Christian.Koenig@amd.com> 写道:
>
>
>
> Am 26.03.2020 07:15 schrieb "Pan, Xinhui" <Xinhui.Pan@amd.com>:
>
>
> > 2020年3月26日 13:38,Koenig, Christian <Christian.Koenig@amd.com> 写道:
> >
> > Yeah that's on my TODO list for quite a while as well.
> >
> > But we even need three IB pools. One very small for the IB tests, one for direct VM updates and one for the rest.
> >
> > So please make the pool a parameter to ib_get() and not the hack you have below.
>
> yep, I will make IB pool  a parameter.
>
> IB tests for gfx need many IBs, PAGE_SIZE for ib pool is still not enough.
> but the default size for ib pool is 2MB now, just one hugepage, today we have memory in TB.
> so no need make a different size for IB tests pool.
>
> 2MB is probably a bit much and we don't have huge page optimisation for kernel allocations at the moment anyway. Keep in mind that we have only limited space in the GART.
gart table is just 512MB.
do you mean every entry in gart table just points to one 4KB page? and need 5 gart table entries for one 2M hugepage?

Yes, we need 512 * 4KB entries for a 2MB page in GART. The table for the system VM is flat because of hardware restrictions.

IIRC we tried 256MB for the GART initially and in general we try to keep that as small as possible because it eats up visible VRAM space.

Christian.


>
> Maybe make this 4*PAGE_SIZE for the new IB pool for now and test if that works or not.
>
> Christian.
>
>
>
>
> >
> > Thanks,
> > Christian.
> >
> > Am 26.03.2020 03:02 schrieb "Pan, Xinhui" <Xinhui.Pan@amd.com>:
> > Another ib poll for direct submit.
> > Any jobs schedule IBs without dependence on gpu scheduler should use
> > this pool firstly.
> >
> > Signed-off-by: xinhui pan <xinhui.pan@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  2 +-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 12 ++++++++++--
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c |  8 +++++++-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_job.h |  3 ++-
> >  5 files changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index 7dd74253e7b6..c01423ffb8ed 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -849,6 +849,7 @@ struct amdgpu_device {
> >          struct amdgpu_ring              *rings[AMDGPU_MAX_RINGS];
> >          bool                            ib_pool_ready;
> >          struct amdgpu_sa_manager        ring_tmp_bo;
> > +       struct amdgpu_sa_manager        ring_tmp_bo_direct;
> >
> >          /* interrupts */
> >          struct amdgpu_irq               irq;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > index 8304d0c87899..28be4efb3d5b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > @@ -920,7 +920,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev,
> >                  parser->entity = entity;
> >
> >                  ring = to_amdgpu_ring(entity->rq->sched);
> > -               r =  amdgpu_ib_get(adev, vm, ring->funcs->parse_cs ?
> > +               r =  amdgpu_ib_get(adev, (unsigned long )vm|0x1, ring->funcs->parse_cs ?
> >                                     chunk_ib->ib_bytes : 0, ib);
> >                  if (r) {
> >                          DRM_ERROR("Failed to get ib !\n");
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> > index bece01f1cf09..f2e08c372d57 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> > @@ -66,7 +66,7 @@ int amdgpu_ib_get(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> >          int r;
> >
> >          if (size) {
> > -               r = amdgpu_sa_bo_new(&adev->ring_tmp_bo,
> > +               r = amdgpu_sa_bo_new(vm ? &adev->ring_tmp_bo : &adev->ring_tmp_bo_direct,
> >                                        &ib->sa_bo, size, 256);
> >                  if (r) {
> >                          dev_err(adev->dev, "failed to get a new IB (%d)\n", r);
> > @@ -75,7 +75,7 @@ int amdgpu_ib_get(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> >
> >                  ib->ptr = amdgpu_sa_bo_cpu_addr(ib->sa_bo);
> >
> > -               if (!vm)
> > +               if (!((unsigned long)vm & ~0x1))
> >                          ib->gpu_addr = amdgpu_sa_bo_gpu_addr(ib->sa_bo);
> >          }
> >
> > @@ -310,6 +310,13 @@ int amdgpu_ib_pool_init(struct amdgpu_device *adev)
> >                  return r;
> >          }
> >
> > +       r = amdgpu_sa_bo_manager_init(adev, &adev->ring_tmp_bo_direct,
> > +                                     AMDGPU_IB_POOL_SIZE*64*1024,
> > +                                     AMDGPU_GPU_PAGE_SIZE,
> > +                                     AMDGPU_GEM_DOMAIN_GTT);
> > +       if (r) {
> > +               return r;
> > +       }
> >          adev->ib_pool_ready = true;
> >
> >          return 0;
> > @@ -327,6 +334,7 @@ void amdgpu_ib_pool_fini(struct amdgpu_device *adev)
> >  {
> >          if (adev->ib_pool_ready) {
> >                  amdgpu_sa_bo_manager_fini(adev, &adev->ring_tmp_bo);
> > +               amdgpu_sa_bo_manager_fini(adev, &adev->ring_tmp_bo_direct);
> >                  adev->ib_pool_ready = false;
> >          }
> >  }
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > index 4981e443a884..6a63826c6760 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > @@ -88,6 +88,12 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
> >
> >  int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, unsigned size,
> >                               struct amdgpu_job **job)
> > +{
> > +       return amdgpu_job_alloc_with_ib_direct(adev, size, job, 0);
> > +}
> > +
> > +int amdgpu_job_alloc_with_ib_direct(struct amdgpu_device *adev, unsigned size,
> > +                            struct amdgpu_job **job, int direct)
> >  {
> >          int r;
> >
> > @@ -95,7 +101,7 @@ int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, unsigned size,
> >          if (r)
> >                  return r;
> >
> > -       r = amdgpu_ib_get(adev, NULL, size, &(*job)->ibs[0]);
> > +       r = amdgpu_ib_get(adev, direct ? NULL : 0x1, size, &(*job)->ibs[0]);
> >          if (r)
> >                  kfree(*job);
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> > index 2e2110dddb76..be9dd72b9912 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> > @@ -67,7 +67,8 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
> >                       struct amdgpu_job **job, struct amdgpu_vm *vm);
> >  int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, unsigned size,
> >                               struct amdgpu_job **job);
> > -
> > +int amdgpu_job_alloc_with_ib_direct(struct amdgpu_device *adev, unsigned size,
> > +                            struct amdgpu_job **job, int direct);
> >  void amdgpu_job_free_resources(struct amdgpu_job *job);
> >  void amdgpu_job_free(struct amdgpu_job *job);
> >  int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
> > --
> > 2.17.1
>
>



[-- Attachment #1.2: Type: text/html, Size: 15657 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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [RFC PATCH 1/2] drm/amdgpu: add direct ib pool
@ 2020-03-26  6:36 Koenig, Christian
  2020-03-26  6:45 ` Pan, Xinhui
  0 siblings, 1 reply; 8+ messages in thread
From: Koenig, Christian @ 2020-03-26  6:36 UTC (permalink / raw)
  To: Pan, Xinhui; +Cc: Deucher, Alexander, Kuehling, Felix, amd-gfx


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



Am 26.03.2020 07:15 schrieb "Pan, Xinhui" <Xinhui.Pan@amd.com>:


> 2020年3月26日 13:38,Koenig, Christian <Christian.Koenig@amd.com> 写道:
>
> Yeah that's on my TODO list for quite a while as well.
>
> But we even need three IB pools. One very small for the IB tests, one for direct VM updates and one for the rest.
>
> So please make the pool a parameter to ib_get() and not the hack you have below.

yep, I will make IB pool  a parameter.

IB tests for gfx need many IBs, PAGE_SIZE for ib pool is still not enough.
but the default size for ib pool is 2MB now, just one hugepage, today we have memory in TB.
so no need make a different size for IB tests pool.

2MB is probably a bit much and we don't have huge page optimisation for kernel allocations at the moment anyway. Keep in mind that we have only limited space in the GART.

Maybe make this 4*PAGE_SIZE for the new IB pool for now and test if that works or not.

Christian.




>
> Thanks,
> Christian.
>
> Am 26.03.2020 03:02 schrieb "Pan, Xinhui" <Xinhui.Pan@amd.com>:
> Another ib poll for direct submit.
> Any jobs schedule IBs without dependence on gpu scheduler should use
> this pool firstly.
>
> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 12 ++++++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c |  8 +++++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.h |  3 ++-
>  5 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 7dd74253e7b6..c01423ffb8ed 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -849,6 +849,7 @@ struct amdgpu_device {
>          struct amdgpu_ring              *rings[AMDGPU_MAX_RINGS];
>          bool                            ib_pool_ready;
>          struct amdgpu_sa_manager        ring_tmp_bo;
> +       struct amdgpu_sa_manager        ring_tmp_bo_direct;
>
>          /* interrupts */
>          struct amdgpu_irq               irq;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 8304d0c87899..28be4efb3d5b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -920,7 +920,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev,
>                  parser->entity = entity;
>
>                  ring = to_amdgpu_ring(entity->rq->sched);
> -               r =  amdgpu_ib_get(adev, vm, ring->funcs->parse_cs ?
> +               r =  amdgpu_ib_get(adev, (unsigned long )vm|0x1, ring->funcs->parse_cs ?
>                                     chunk_ib->ib_bytes : 0, ib);
>                  if (r) {
>                          DRM_ERROR("Failed to get ib !\n");
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index bece01f1cf09..f2e08c372d57 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -66,7 +66,7 @@ int amdgpu_ib_get(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>          int r;
>
>          if (size) {
> -               r = amdgpu_sa_bo_new(&adev->ring_tmp_bo,
> +               r = amdgpu_sa_bo_new(vm ? &adev->ring_tmp_bo : &adev->ring_tmp_bo_direct,
>                                        &ib->sa_bo, size, 256);
>                  if (r) {
>                          dev_err(adev->dev, "failed to get a new IB (%d)\n", r);
> @@ -75,7 +75,7 @@ int amdgpu_ib_get(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>
>                  ib->ptr = amdgpu_sa_bo_cpu_addr(ib->sa_bo);
>
> -               if (!vm)
> +               if (!((unsigned long)vm & ~0x1))
>                          ib->gpu_addr = amdgpu_sa_bo_gpu_addr(ib->sa_bo);
>          }
>
> @@ -310,6 +310,13 @@ int amdgpu_ib_pool_init(struct amdgpu_device *adev)
>                  return r;
>          }
>
> +       r = amdgpu_sa_bo_manager_init(adev, &adev->ring_tmp_bo_direct,
> +                                     AMDGPU_IB_POOL_SIZE*64*1024,
> +                                     AMDGPU_GPU_PAGE_SIZE,
> +                                     AMDGPU_GEM_DOMAIN_GTT);
> +       if (r) {
> +               return r;
> +       }
>          adev->ib_pool_ready = true;
>
>          return 0;
> @@ -327,6 +334,7 @@ void amdgpu_ib_pool_fini(struct amdgpu_device *adev)
>  {
>          if (adev->ib_pool_ready) {
>                  amdgpu_sa_bo_manager_fini(adev, &adev->ring_tmp_bo);
> +               amdgpu_sa_bo_manager_fini(adev, &adev->ring_tmp_bo_direct);
>                  adev->ib_pool_ready = false;
>          }
>  }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 4981e443a884..6a63826c6760 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -88,6 +88,12 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
>
>  int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, unsigned size,
>                               struct amdgpu_job **job)
> +{
> +       return amdgpu_job_alloc_with_ib_direct(adev, size, job, 0);
> +}
> +
> +int amdgpu_job_alloc_with_ib_direct(struct amdgpu_device *adev, unsigned size,
> +                            struct amdgpu_job **job, int direct)
>  {
>          int r;
>
> @@ -95,7 +101,7 @@ int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, unsigned size,
>          if (r)
>                  return r;
>
> -       r = amdgpu_ib_get(adev, NULL, size, &(*job)->ibs[0]);
> +       r = amdgpu_ib_get(adev, direct ? NULL : 0x1, size, &(*job)->ibs[0]);
>          if (r)
>                  kfree(*job);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> index 2e2110dddb76..be9dd72b9912 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> @@ -67,7 +67,8 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
>                       struct amdgpu_job **job, struct amdgpu_vm *vm);
>  int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, unsigned size,
>                               struct amdgpu_job **job);
> -
> +int amdgpu_job_alloc_with_ib_direct(struct amdgpu_device *adev, unsigned size,
> +                            struct amdgpu_job **job, int direct);
>  void amdgpu_job_free_resources(struct amdgpu_job *job);
>  void amdgpu_job_free(struct amdgpu_job *job);
>  int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
> --
> 2.17.1



[-- Attachment #1.2: Type: text/html, Size: 14207 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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [RFC PATCH 0/2] add direct IB pool
@ 2020-03-26  2:01 xinhui pan
  2020-03-26  2:01 ` [RFC PATCH 1/2] drm/amdgpu: add direct ib pool xinhui pan
  0 siblings, 1 reply; 8+ messages in thread
From: xinhui pan @ 2020-03-26  2:01 UTC (permalink / raw)
  To: amd-gfx; +Cc: alexander.deucher, Felix.Kuehling, xinhui pan, christian.koenig

druing gpu recovery, we alloc ibs for ring tests to test if recovery
succeed or not.

As gpu recovery parked the gpu scheduler thread, any pending jobs hold the ib
resource has no chance to free. So the ring test above got stuck if no
ib to alloc.

If we schedule IBs directly in job_submit_direct, we can alloc ibs in
the new ib pool. It should have less contention.

If the IB could be freed in time, IOW, not depending on any scheduler,
nor any other blocking code. It is better to alloc ibs in direct pool.

xinhui pan (2):
  drm/amdgpu: add direct ib pool
  drm/amdgpu: use new job alloc variation if possible

 drivers/gpu/drm/amd/amdgpu/amdgpu.h         |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c      | 12 ++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c     |  8 +++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.h     |  3 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c    |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c     |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c     |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c     |  6 +++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c |  3 ++-
 drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c       |  4 ++--
 drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c       |  4 ++--
 13 files changed, 35 insertions(+), 18 deletions(-)

-- 
2.17.1

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-03-26 14:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26  6:51 [RFC PATCH 1/2] drm/amdgpu: add direct ib pool Koenig, Christian
2020-03-26  7:11 ` Pan, Xinhui
2020-03-26 14:36   ` Felix Kuehling
  -- strict thread matches above, loose matches on Subject: below --
2020-03-26  6:36 Koenig, Christian
2020-03-26  6:45 ` Pan, Xinhui
2020-03-26  2:01 [RFC PATCH 0/2] add direct IB pool xinhui pan
2020-03-26  2:01 ` [RFC PATCH 1/2] drm/amdgpu: add direct ib pool xinhui pan
2020-03-26  5:38   ` Koenig, Christian
2020-03-26  6:15     ` Pan, Xinhui

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).