Re: [PATCH] drm/amdgpu: correctly sign extend 48bit addresses v3

[Zhang, Jerry (Junwei)]

On 08/30/2018 02:48 PM, Christian König wrote:

Am 30.08.2018 um 04:43 schrieb Zhang, Jerry (Junwei):

On 08/29/2018 05:39 PM, Christian König wrote:

Am 29.08.2018 um 04:03 schrieb Zhang, Jerry (Junwei):

On 08/28/2018 08:17 PM, Christian König wrote:

Correct sign extend the GMC addresses to 48bit.

Could you explain a bit more why to extend the sign?

The hardware works like this, in other words when bit 47 is set we must extend 
that into bits 48-63.

Thanks. fine.

the address is uint64_t. is if failed in some case?

What do you mean?

Sorry for the typo without finishing the sentence before sending.

I mean even if the address is uint64_t, still needs to extend the sign?
what I was thinking is that the int64_t needs to do this.

Well, no. What we would need is an int48_t type, but such thing doesn't exists 
and isn't easily implementable in C.

If so, it would be better to understand.
Thanks.

> -/* VA hole for 48bit addresses on Vega10 */
> -#define AMDGPU_VA_HOLE_START 0x0000800000000000ULL
> -#define AMDGPU_VA_HOLE_END 0xffff800000000000ULL

BTW, the hole for 48bit is actually 47 bit left, any background for that?

Well bits start counting at zero. So the 48bit addresses have bits 0-47.

The VA hole is going to catch the VA address out of normal range, which for 
vega10 is 48-bit?

Yes, exactly.

if so, 0x8000_0000_0000 ULL holds from 0~46 bits, starting from 128TB, but 
vega10 VA is 256TB

Correct, the lower range is from 0x0-0x8000_0000_0000 and the higher range is 
from 0xffff_8000_0000_0000-0xffff_ffff_ffff_ffff.

it also could be found in old code gmc_v9.c, the mc_mask holds 48-bits address, 
like below:

    adev->gmc.mc_mask = 0xffff_ffff_ffff ULL; /* 48 bit MC */

But the VA hole start address is 0x8000_0000_0000 ULL, then libdrm gets 
virtual_address_max

    dev_info.virtual_address_max = min(vm_size, AMDGPU_VA_HOLE_START)
    // that's 0x8000_0000_0000 ULL actually

We limit the reported VA size for backward compatibility with old userspace 
here.

fine, got it.
Thanks.

Regards,
Jerry

Above all, it looks the VA hole start should be 0x1_0000_0000_0000 UL.

Nope, that isn't correct. The hole is between 0x8000_0000_0000 and 
0xffff_8000_0000_0000.

Regards,
Christian.

Regards,
Jerry

Regards,
Christian.

Regards,
Jerry

v2: sign extending turned out easier than thought.
v3: clean up the defines and move them into amdgpu_gmc.h as well

Signed-off-by: Christian König <christian.koe...@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c |  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     |  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 10 ++++-----
  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    | 26 ++++++++++++++++++++++
  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c    |  8 +++----
  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c   |  6 ++---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     |  7 +++---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     | 13 -----------
  9 files changed, 44 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index 8c652ecc4f9a..bc5ccfca68c5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -135,7 +135,7 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
              .num_queue_per_pipe = adev->gfx.mec.num_queue_per_pipe,
              .gpuvm_size = min(adev->vm_manager.max_pfn
                        << AMDGPU_GPU_PAGE_SHIFT,
-                      AMDGPU_VA_HOLE_START),
+                      AMDGPU_GMC_HOLE_START),
              .drm_render_minor = adev->ddev->render->index
          };

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index dd734970e167..ef2bfc04b41c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -835,7 +835,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
              if (chunk->chunk_id != AMDGPU_CHUNK_ID_IB)
                  continue;

-            va_start = chunk_ib->va_start & AMDGPU_VA_HOLE_MASK;
+            va_start = chunk_ib->va_start & AMDGPU_GMC_HOLE_MASK;
              r = amdgpu_cs_find_mapping(p, va_start, &aobj, &m);
              if (r) {
                  DRM_ERROR("IB va_start is invalid\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 71792d820ae0..d30a0838851b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -572,16 +572,16 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void 
*data,
          return -EINVAL;
      }

-    if (args->va_address >= AMDGPU_VA_HOLE_START &&
-        args->va_address < AMDGPU_VA_HOLE_END) {
+    if (args->va_address >= AMDGPU_GMC_HOLE_START &&
+        args->va_address < AMDGPU_GMC_HOLE_END) {
          dev_dbg(&dev->pdev->dev,
              "va_address 0x%LX is in VA hole 0x%LX-0x%LX\n",
-            args->va_address, AMDGPU_VA_HOLE_START,
-            AMDGPU_VA_HOLE_END);
+            args->va_address, AMDGPU_GMC_HOLE_START,
+            AMDGPU_GMC_HOLE_END);
          return -EINVAL;
      }

-    args->va_address &= AMDGPU_VA_HOLE_MASK;
+    args->va_address &= AMDGPU_GMC_HOLE_MASK;

      if ((args->flags & ~valid_flags) && (args->flags & ~prt_flags)) {
          dev_dbg(&dev->pdev->dev, "invalid flags combination 0x%08X\n",
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
index 0d2c9f65ca13..9d9c7a9f54e4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
@@ -30,6 +30,19 @@

  #include "amdgpu_irq.h"

+/* VA hole for 48bit addresses on Vega10 */
+#define AMDGPU_GMC_HOLE_START    0x0000800000000000ULL
+#define AMDGPU_GMC_HOLE_END    0xffff800000000000ULL
+
+/*
+ * Hardware is programmed as if the hole doesn't exists with start and end
+ * address values.
+ *
+ * This mask is used to remove the upper 16bits of the VA and so come up with
+ * the linear addr value.
+ */
+#define AMDGPU_GMC_HOLE_MASK    0x0000ffffffffffffULL
+
  struct firmware;

  /*
@@ -131,6 +144,19 @@ static inline bool amdgpu_gmc_vram_full_visible(struct 
amdgpu_gmc *gmc)
      return (gmc->real_vram_size == gmc->visible_vram_size);
  }

+/**
+ * amdgpu_gmc_sign_extend - sign extend the given gmc address
+ *
+ * @addr: address to extend
+ */
+static inline uint64_t amdgpu_gmc_sign_extend(uint64_t addr)
+{
+    if (addr >= AMDGPU_GMC_HOLE_START)
+        addr |= AMDGPU_GMC_HOLE_END;
+
+    return addr;
+}
+
  void amdgpu_gmc_get_pde_for_bo(struct amdgpu_bo *bo, int level,
                     uint64_t *addr, uint64_t *flags);
  uint64_t amdgpu_gmc_pd_addr(struct amdgpu_bo *bo);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 9c4e45936ade..29ac3873eeb0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -655,11 +655,11 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void 
*data, struct drm_file

          dev_info.virtual_address_offset = AMDGPU_VA_RESERVED_SIZE;
          dev_info.virtual_address_max =
-            min(vm_size, AMDGPU_VA_HOLE_START);
+            min(vm_size, AMDGPU_GMC_HOLE_START);

-        if (vm_size > AMDGPU_VA_HOLE_START) {
-            dev_info.high_va_offset = AMDGPU_VA_HOLE_END;
-            dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
+        if (vm_size > AMDGPU_GMC_HOLE_START) {
+            dev_info.high_va_offset = AMDGPU_GMC_HOLE_END;
+            dev_info.high_va_max = AMDGPU_GMC_HOLE_END | vm_size;
          }
          dev_info.virtual_address_alignment = max((int)PAGE_SIZE, 
AMDGPU_GPU_PAGE_SIZE);
          dev_info.pte_fragment_size = (1 << adev->vm_manager.fragment_size) * 
AMDGPU_GPU_PAGE_SIZE;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 5ddd4e87480b..9c3cc23488c2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1370,7 +1370,7 @@ u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo)
      WARN_ON_ONCE(bo->tbo.mem.mem_type == TTM_PL_VRAM &&
               !(bo->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS));

-    return bo->tbo.offset;
+    return amdgpu_gmc_sign_extend(bo->tbo.offset);
  }

  /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index 38856365580d..f2f358aa0597 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -28,9 +28,7 @@ uint64_t amdgpu_csa_vaddr(struct amdgpu_device *adev)
      uint64_t addr = adev->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT;

      addr -= AMDGPU_VA_RESERVED_SIZE;
-
-    if (addr >= AMDGPU_VA_HOLE_START)
-        addr |= AMDGPU_VA_HOLE_END;
+    addr = amdgpu_gmc_sign_extend(addr);

      return addr;
  }
@@ -73,7 +71,7 @@ void amdgpu_free_static_csa(struct amdgpu_device *adev) {
  int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,
                struct amdgpu_bo_va **bo_va)
  {
-    uint64_t csa_addr = amdgpu_csa_vaddr(adev) & AMDGPU_VA_HOLE_MASK;
+    uint64_t csa_addr = amdgpu_csa_vaddr(adev) & AMDGPU_GMC_HOLE_MASK;
      struct ww_acquire_ctx ticket;
      struct list_head list;
      struct amdgpu_bo_list_entry pd;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 1f4b8dfc0456..8bbba0127e42 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -399,7 +399,7 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
          if (level == adev->vm_manager.root_level) {
              ats_entries = amdgpu_vm_level_shift(adev, level);
              ats_entries += AMDGPU_GPU_PAGE_SHIFT;
-            ats_entries = AMDGPU_VA_HOLE_START >> ats_entries;
+            ats_entries = AMDGPU_GMC_HOLE_START >> ats_entries;
              ats_entries = min(ats_entries, entries);
              entries -= ats_entries;
          } else {
@@ -629,7 +629,7 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
      eaddr = saddr + size - 1;

      if (vm->pte_support_ats)
-        ats = saddr < AMDGPU_VA_HOLE_START;
+        ats = saddr < AMDGPU_GMC_HOLE_START;

      saddr /= AMDGPU_GPU_PAGE_SIZE;
      eaddr /= AMDGPU_GPU_PAGE_SIZE;
@@ -1934,7 +1934,8 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
              struct amdgpu_bo_va_mapping, list);
          list_del(&mapping->list);

-        if (vm->pte_support_ats && mapping->start < AMDGPU_VA_HOLE_START)
+        if (vm->pte_support_ats &&
+            mapping->start < AMDGPU_GMC_HOLE_START)
              init_pte_value = AMDGPU_PTE_DEFAULT_ATC;

          r = amdgpu_vm_bo_update_mapping(adev, NULL, NULL, vm,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 94fe47890adf..2ce452198017 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -101,19 +101,6 @@ struct amdgpu_bo_list_entry;
  /* hardcode that limit for now */
  #define AMDGPU_VA_RESERVED_SIZE            (1ULL << 20)

-/* VA hole for 48bit addresses on Vega10 */
-#define AMDGPU_VA_HOLE_START 0x0000800000000000ULL
-#define AMDGPU_VA_HOLE_END            0xffff800000000000ULL
-
-/*
- * Hardware is programmed as if the hole doesn't exists with start and end
- * address values.
- *
- * This mask is used to remove the upper 16bits of the VA and so come up with
- * the linear addr value.
- */
-#define AMDGPU_VA_HOLE_MASK 0x0000ffffffffffffULL
-
  /* max vmids dedicated for process */
  #define AMDGPU_VM_MAX_RESERVED_VMID    1

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