[PATCH 2/7] drm/amdgpu: fix up GDS/GWS/OA shifting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
> Christian König
> Sent: Saturday, September 15, 2018 3:52 AM
> To: Alex Deucher <alexdeucher at gmail.com>
> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
> Subject: Re: [PATCH 2/7] drm/amdgpu: fix up GDS/GWS/OA shifting
> 
> Am 14.09.2018 um 22:26 schrieb Alex Deucher:
> > On Fri, Sep 14, 2018 at 3:12 PM Christian König
> > <ckoenig.leichtzumerken at gmail.com> wrote:
> >> That only worked by pure coincident. Completely remove the shifting
> >> and always apply correct PAGE_SHIFT.
> >>
> >> Signed-off-by: Christian König <christian.koenig at amd.com>
> >> ---
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     | 12 ++++++------
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_gds.h    |  7 -------
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 12 +++---------
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    | 14 +++++++-------
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  6 +++++-
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    | 15 +++------------
> >>   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c      |  9 ---------
> >>   drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c      |  9 ---------
> >>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c      | 12 +-----------
> >>   9 files changed, 25 insertions(+), 71 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> >> index d762d78e5102..8836186eb5ef 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> >> @@ -721,16 +721,16 @@ static int amdgpu_cs_parser_bos(struct
> amdgpu_cs_parser *p,
> >>                  e->bo_va = amdgpu_vm_bo_find(vm,
> >> ttm_to_amdgpu_bo(e->tv.bo));
> >>
> >>          if (gds) {
> >> -               p->job->gds_base = amdgpu_bo_gpu_offset(gds);
> >> -               p->job->gds_size = amdgpu_bo_size(gds);
> >> +               p->job->gds_base = amdgpu_bo_gpu_offset(gds) >>
> PAGE_SHIFT;
> >> +               p->job->gds_size = amdgpu_bo_size(gds) >> PAGE_SHIFT;
> >>          }
> >>          if (gws) {
> >> -               p->job->gws_base = amdgpu_bo_gpu_offset(gws);
> >> -               p->job->gws_size = amdgpu_bo_size(gws);
> >> +               p->job->gws_base = amdgpu_bo_gpu_offset(gws) >>
> PAGE_SHIFT;
> >> +               p->job->gws_size = amdgpu_bo_size(gws) >> PAGE_SHIFT;
> >>          }
> >>          if (oa) {
> >> -               p->job->oa_base = amdgpu_bo_gpu_offset(oa);
> >> -               p->job->oa_size = amdgpu_bo_size(oa);
> >> +               p->job->oa_base = amdgpu_bo_gpu_offset(oa) >> PAGE_SHIFT;
> >> +               p->job->oa_size = amdgpu_bo_size(oa) >> PAGE_SHIFT;
> >>          }
> >>
> >>          if (!r && p->uf_entry.tv.bo) { diff --git
> >> a/drivers/gpu/drm/amd/amdgpu/amdgpu_gds.h
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gds.h
> >> index e73728d90388..ecbcefe49a98 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gds.h
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gds.h
> >> @@ -24,13 +24,6 @@
> >>   #ifndef __AMDGPU_GDS_H__
> >>   #define __AMDGPU_GDS_H__
> >>
> >> -/* Because TTM request that alloacted buffer should be PAGE_SIZE
> >> aligned,
> >> - * we should report GDS/GWS/OA size as PAGE_SIZE aligned
> >> - * */
> >> -#define AMDGPU_GDS_SHIFT       2
> >> -#define AMDGPU_GWS_SHIFT       PAGE_SHIFT
> >> -#define AMDGPU_OA_SHIFT                PAGE_SHIFT
> >> -
> >>   struct amdgpu_ring;
> >>   struct amdgpu_bo;
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >> index d30a0838851b..7b3d1ebda9df 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >> @@ -244,16 +244,10 @@ int amdgpu_gem_create_ioctl(struct
> drm_device *dev, void *data,
> >>                          return -EINVAL;
> >>                  }
> >>                  flags |= AMDGPU_GEM_CREATE_NO_CPU_ACCESS;
> >> -               if (args->in.domains == AMDGPU_GEM_DOMAIN_GDS)
> >> -                       size = size << AMDGPU_GDS_SHIFT;
> >> -               else if (args->in.domains == AMDGPU_GEM_DOMAIN_GWS)
> >> -                       size = size << AMDGPU_GWS_SHIFT;
> >> -               else if (args->in.domains == AMDGPU_GEM_DOMAIN_OA)
> >> -                       size = size << AMDGPU_OA_SHIFT;
> >> -               else
> >> -                       return -EINVAL;
> >> +               /* GDS allocations must be DW aligned */
> >> +               if (args->in.domains & AMDGPU_GEM_DOMAIN_GDS)
> >> +                       size = ALIGN(size, 4);
> >>          }
> >> -       size = roundup(size, PAGE_SIZE);
> >>
> >>          if (flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID) {
> >>                  r = amdgpu_bo_reserve(vm->root.base.bo, false); diff
> >> --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> >> index b766270d86cb..64cc483db973 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> >> @@ -528,13 +528,13 @@ static int amdgpu_info_ioctl(struct drm_device
> *dev, void *data, struct drm_file
> >>                  struct drm_amdgpu_info_gds gds_info;
> >>
> >>                  memset(&gds_info, 0, sizeof(gds_info));
> >> -               gds_info.gds_gfx_partition_size = adev-
> >gds.mem.gfx_partition_size >> AMDGPU_GDS_SHIFT;
> >> -               gds_info.compute_partition_size = adev-
> >gds.mem.cs_partition_size >> AMDGPU_GDS_SHIFT;
> >> -               gds_info.gds_total_size = adev->gds.mem.total_size >>
> AMDGPU_GDS_SHIFT;
> >> -               gds_info.gws_per_gfx_partition = adev-
> >gds.gws.gfx_partition_size >> AMDGPU_GWS_SHIFT;
> >> -               gds_info.gws_per_compute_partition = adev-
> >gds.gws.cs_partition_size >> AMDGPU_GWS_SHIFT;
> >> -               gds_info.oa_per_gfx_partition = adev->gds.oa.gfx_partition_size
> >> AMDGPU_OA_SHIFT;
> >> -               gds_info.oa_per_compute_partition = adev-
> >gds.oa.cs_partition_size >> AMDGPU_OA_SHIFT;
> >> +               gds_info.gds_gfx_partition_size = adev-
> >gds.mem.gfx_partition_size;
> >> +               gds_info.compute_partition_size = adev-
> >gds.mem.cs_partition_size;
> >> +               gds_info.gds_total_size = adev->gds.mem.total_size;
> >> +               gds_info.gws_per_gfx_partition = adev-
> >gds.gws.gfx_partition_size;
> >> +               gds_info.gws_per_compute_partition = adev-
> >gds.gws.cs_partition_size;
> >> +               gds_info.oa_per_gfx_partition = adev-
> >gds.oa.gfx_partition_size;
> >> +               gds_info.oa_per_compute_partition =
> >> + adev->gds.oa.cs_partition_size;
> >>                  return copy_to_user(out, &gds_info,
> >>                                      min((size_t)size, sizeof(gds_info))) ? -EFAULT : 0;
> >>          }
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >> index e6909252aefa..e1f32a196f6d 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >> @@ -433,7 +433,11 @@ static int amdgpu_bo_do_create(struct
> amdgpu_device *adev,
> >>          int r;
> >>
> >>          page_align = roundup(bp->byte_align, PAGE_SIZE) >> PAGE_SHIFT;
> >> -       size = ALIGN(size, PAGE_SIZE);
> >> +       if (bp->domain & (AMDGPU_GEM_DOMAIN_GDS |
> AMDGPU_GEM_DOMAIN_GWS |
> >> +                         AMDGPU_GEM_DOMAIN_OA))
> >> +               size <<= PAGE_SHIFT;
> >> +       else
> >> +               size = ALIGN(size, PAGE_SIZE);
> > Is this right for GDS?  I think we need to be in 4K units regardless
> > of the page size.  I have to admit, all the shifting is mixing me up.
> 
> Yeah, that is indeed correct. GDS only needs DW alignment.
> 
> The shifting is purely done because TTM wants to work with pages and not
> bytes.
> 
> Andrey already said he wanted to clean that up when there is time.


Acked-by: Alex Deucher <alexander.deucher at amd.com>



> 
> Christian.
> 
> >
> > Alex
> >
> >>          if (!amdgpu_bo_validate_size(adev, size, bp->domain))
> >>                  return -ENOMEM;
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >> index 1565344cc139..c691275cd1f0 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >> @@ -1825,19 +1825,10 @@ int amdgpu_ttm_init(struct amdgpu_device
> *adev)
> >>                   (unsigned)(gtt_size / (1024 * 1024)));
> >>
> >>          /* Initialize various on-chip memory pools */
> >> -       adev->gds.mem.total_size = adev->gds.mem.total_size <<
> AMDGPU_GDS_SHIFT;
> >> -       adev->gds.mem.gfx_partition_size = adev-
> >gds.mem.gfx_partition_size << AMDGPU_GDS_SHIFT;
> >> -       adev->gds.mem.cs_partition_size = adev-
> >gds.mem.cs_partition_size << AMDGPU_GDS_SHIFT;
> >> -       adev->gds.gws.total_size = adev->gds.gws.total_size <<
> AMDGPU_GWS_SHIFT;
> >> -       adev->gds.gws.gfx_partition_size = adev-
> >gds.gws.gfx_partition_size << AMDGPU_GWS_SHIFT;
> >> -       adev->gds.gws.cs_partition_size = adev->gds.gws.cs_partition_size
> << AMDGPU_GWS_SHIFT;
> >> -       adev->gds.oa.total_size = adev->gds.oa.total_size <<
> AMDGPU_OA_SHIFT;
> >> -       adev->gds.oa.gfx_partition_size = adev->gds.oa.gfx_partition_size
> << AMDGPU_OA_SHIFT;
> >> -       adev->gds.oa.cs_partition_size = adev->gds.oa.cs_partition_size <<
> AMDGPU_OA_SHIFT;
> >>          /* GDS Memory */
> >>          if (adev->gds.mem.total_size) {
> >>                  r = ttm_bo_init_mm(&adev->mman.bdev, AMDGPU_PL_GDS,
> >> -                                  adev->gds.mem.total_size >> PAGE_SHIFT);
> >> +                                  adev->gds.mem.total_size);
> >>                  if (r) {
> >>                          DRM_ERROR("Failed initializing GDS heap.\n");
> >>                          return r;
> >> @@ -1847,7 +1838,7 @@ int amdgpu_ttm_init(struct amdgpu_device
> *adev)
> >>          /* GWS */
> >>          if (adev->gds.gws.total_size) {
> >>                  r = ttm_bo_init_mm(&adev->mman.bdev, AMDGPU_PL_GWS,
> >> -                                  adev->gds.gws.total_size >> PAGE_SHIFT);
> >> +                                  adev->gds.gws.total_size);
> >>                  if (r) {
> >>                          DRM_ERROR("Failed initializing gws heap.\n");
> >>                          return r;
> >> @@ -1857,7 +1848,7 @@ int amdgpu_ttm_init(struct amdgpu_device
> *adev)
> >>          /* OA */
> >>          if (adev->gds.oa.total_size) {
> >>                  r = ttm_bo_init_mm(&adev->mman.bdev, AMDGPU_PL_OA,
> >> -                                  adev->gds.oa.total_size >> PAGE_SHIFT);
> >> +                                  adev->gds.oa.total_size);
> >>                  if (r) {
> >>                          DRM_ERROR("Failed initializing oa heap.\n");
> >>                          return r;
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> >> b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> >> index a15d9c0f233b..c0f9732cbaf7 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> >> @@ -4170,15 +4170,6 @@ static void
> gfx_v7_0_ring_emit_gds_switch(struct amdgpu_ring *ring,
> >>                                            uint32_t gws_base, uint32_t gws_size,
> >>                                            uint32_t oa_base, uint32_t oa_size)
> >>   {
> >> -       gds_base = gds_base >> AMDGPU_GDS_SHIFT;
> >> -       gds_size = gds_size >> AMDGPU_GDS_SHIFT;
> >> -
> >> -       gws_base = gws_base >> AMDGPU_GWS_SHIFT;
> >> -       gws_size = gws_size >> AMDGPU_GWS_SHIFT;
> >> -
> >> -       oa_base = oa_base >> AMDGPU_OA_SHIFT;
> >> -       oa_size = oa_size >> AMDGPU_OA_SHIFT;
> >> -
> >>          /* GDS Base */
> >>          amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
> >>          amdgpu_ring_write(ring, (WRITE_DATA_ENGINE_SEL(0) | diff
> >> --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> >> b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> >> index 3882689b2d8f..57e4b14e3bd1 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> >> @@ -5396,15 +5396,6 @@ static void
> gfx_v8_0_ring_emit_gds_switch(struct amdgpu_ring *ring,
> >>                                            uint32_t gws_base, uint32_t gws_size,
> >>                                            uint32_t oa_base, uint32_t oa_size)
> >>   {
> >> -       gds_base = gds_base >> AMDGPU_GDS_SHIFT;
> >> -       gds_size = gds_size >> AMDGPU_GDS_SHIFT;
> >> -
> >> -       gws_base = gws_base >> AMDGPU_GWS_SHIFT;
> >> -       gws_size = gws_size >> AMDGPU_GWS_SHIFT;
> >> -
> >> -       oa_base = oa_base >> AMDGPU_OA_SHIFT;
> >> -       oa_size = oa_size >> AMDGPU_OA_SHIFT;
> >> -
> >>          /* GDS Base */
> >>          amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
> >>          amdgpu_ring_write(ring, (WRITE_DATA_ENGINE_SEL(0) | diff
> >> --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> >> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> >> index 75a91663019f..d31a2bc00d61 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> >> @@ -1527,8 +1527,7 @@ static int gfx_v9_0_ngg_en(struct
> amdgpu_device *adev)
> >>          gfx_v9_0_write_data_to_reg(ring, 0, false,
> >>                                     SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE),
> >>                                     (adev->gds.mem.total_size +
> >> -                                   adev->gfx.ngg.gds_reserve_size) >>
> >> -                                  AMDGPU_GDS_SHIFT);
> >> +                                   adev->gfx.ngg.gds_reserve_size));
> >>
> >>          amdgpu_ring_write(ring, PACKET3(PACKET3_DMA_DATA, 5));
> >>          amdgpu_ring_write(ring, (PACKET3_DMA_DATA_CP_SYNC | @@
> >> -3472,15 +3471,6 @@ static void gfx_v9_0_ring_emit_gds_switch(struct
> amdgpu_ring *ring,
> >>   {
> >>          struct amdgpu_device *adev = ring->adev;
> >>
> >> -       gds_base = gds_base >> AMDGPU_GDS_SHIFT;
> >> -       gds_size = gds_size >> AMDGPU_GDS_SHIFT;
> >> -
> >> -       gws_base = gws_base >> AMDGPU_GWS_SHIFT;
> >> -       gws_size = gws_size >> AMDGPU_GWS_SHIFT;
> >> -
> >> -       oa_base = oa_base >> AMDGPU_OA_SHIFT;
> >> -       oa_size = oa_size >> AMDGPU_OA_SHIFT;
> >> -
> >>          /* GDS Base */
> >>          gfx_v9_0_write_data_to_reg(ring, 0, false,
> >>                                     SOC15_REG_OFFSET(GC, 0,
> >> mmGDS_VMID0_BASE) + 2 * vmid,
> >> --
> >> 2.14.1
> >>
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux