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

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

 

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.

>
>>
>>>
>>> > -/* 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.

>
> 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.koenig at 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
>>>>
>>>>
>>
[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux