[PATCH 3/3] drm/amdgpu: move static CSA address to top of address space

Tue Jan 23 12:51:21 UTC 2018

The ATC (or alternatively HMM) maps the CPU addresses into the GPU 
address space.

Now on Linux the lower range (0x0-0x7fffffffffff) is used for the 
userspace address space on the CPU, so when we want to use that on the 
GPU it must be free of other mappings.

GFX8 and GFX7 hardware had a separate flag which address space to use in 
the memory instructions, but on GFX9 it's all one big shared address 
space and we need to make sure that we don't kick each others feet.

Had to move the addresses Mesa use as well because of this and older VCE 
firmware unfortunately was buggy regarding this as well.

Regards,
Christian.

Am 23.01.2018 um 13:43 schrieb Liu, Monk:
>
> OK I see, but why HMM/ATC/SVN want  low 8MB range ? why they cannot 
> use high range address ?
>
>
> ------------------------------------------------------------------------
> *From:* Christian König <ckoenig.leichtzumerken at gmail.com>
> *Sent:* Tuesday, January 23, 2018 4:26:21 PM
> *To:* Liu, Monk; He, Roger; amd-gfx at lists.freedesktop.org
> *Subject:* Re: [PATCH 3/3] drm/amdgpu: move static CSA address to top 
> of address space
>> Any bug introduced by original design ? I don't see why 8MB- 8KB as 
>> the start vm address of CSA has any trouble compared with top range ?
> On gfx9 we need the lower address range (0x0-0x7fffffffffff) for 
> HMM/ATC/SVM. If we map the 8k CSA in there and hit it by accident then 
> the GPU will not do what it is supposed to do :)
>
> I could limit the change to only gfx9, but I think the code will be 
> simpler if we change both gfx8 and gfx9.
>
>> Besides, please note that you also need modify gfx8/9 source to align 
>> emit_cs/de_meta with your change
> That was the design flaw I was talking about. In other words I 
> couldn't find the code where that address was actually used.
>
> We should use the macro define in those functions as well. Going to 
> send fixed for this in a minute.
>
> Thanks,
> Christian.
>
> Am 23.01.2018 um 04:23 schrieb Liu, Monk:
>>
>> anyway I prefer no change on that part unless there issues or bug 
>> need to fix by the change.
>>
>> the CSA address is for use by CPG h/w not s/w, and since 8MB is a 
>> reserved range for each VM I don't see
>>
>> it's a design flaw with it
>>
>> ------------------------------------------------------------------------
>> *From:* Liu, Monk
>> *Sent:* Tuesday, January 23, 2018 11:20:35 AM
>> *To:* He, Roger; Christian König; amd-gfx at lists.freedesktop.org 
>> <mailto:amd-gfx at lists.freedesktop.org>
>> *Subject:* Re: [PATCH 3/3] drm/amdgpu: move static CSA address to top 
>> of address space
>>
>> Any bug introduced by original design ? I don't see why 8MB- 8KB as 
>> the start vm address of CSA has any trouble compared with top range ? 
>> Besides, please note that you also need modify gfx8/9 source to align 
>> emit_cs/de_meta with your change
>>
>>
>>
>> ------------------------------------------------------------------------
>> *From:* He, Roger
>> *Sent:* Tuesday, January 23, 2018 10:36:31 AM
>> *To:* Christian König; Liu, Monk; amd-gfx at lists.freedesktop.org 
>> <mailto:amd-gfx at lists.freedesktop.org>
>> *Subject:* RE: [PATCH 3/3] drm/amdgpu: move static CSA address to top 
>> of address space
>>
>>
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On 
>> Behalf Of Christian K?nig
>> Sent: Monday, January 22, 2018 6:44 PM
>> To: Liu, Monk <Monk.Liu at amd.com> <mailto:Monk.Liu at amd.com>; 
>> amd-gfx at lists.freedesktop.org <mailto:amd-gfx at lists.freedesktop.org>
>> Subject: [PATCH 3/3] drm/amdgpu: move static CSA address to top of 
>> address space
>>
>> There seems to be a design flaw with this since the address of the 
>> static CSA is never exported anywhere.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com> 
>> <mailto:christian.koenig at amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 15 +++++++-------- 
>> drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h |  6 ++++--
>>  2 files changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> index e7dfb7b44b4b..c7d24af03e3e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> @@ -55,11 +55,10 @@ void amdgpu_free_static_csa(struct amdgpu_device 
>> *adev) {
>>
>>  /*
>>   * amdgpu_map_static_csa should be called during amdgpu_vm_init
>> - * it maps virtual address "AMDGPU_VA_RESERVED_SIZE - AMDGPU_CSA_SIZE"
>> - * to this VM, and each command submission of GFX should use this 
>> virtual
>> - * address within META_DATA init package to support SRIOV gfx 
>> preemption.
>> + * it maps virtual address "AMDGPU_CSA_VADDR" to this VM, and each
>> + command
>> + * submission of GFX should use this virtual address within META_DATA
>> + init
>> + * package to support SRIOV gfx preemption.
>>   */
>> -
>>  int amdgpu_map_static_csa(struct amdgpu_device *adev, struct 
>> amdgpu_vm *vm,
>>                            struct amdgpu_bo_va **bo_va)
>>  {
>> @@ -90,7 +89,7 @@ int amdgpu_map_static_csa(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm,
>>                  return -ENOMEM;
>>          }
>>
>> -       r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm, 
>> AMDGPU_CSA_VADDR,
>> +       r = amdgpu_vm_alloc_pts(adev, (*bo_va)->base.vm,
>> +AMDGPU_CSA_VADDR(adev),
>>                                  AMDGPU_CSA_SIZE);
>>          if (r) {
>>                  DRM_ERROR("failed to allocate pts for static CSA, 
>> err=%d\n", r); @@ -99,9 +98,9 @@ int amdgpu_map_static_csa(struct 
>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>                  return r;
>>          }
>>
>> -       r = amdgpu_vm_bo_map(adev, *bo_va, AMDGPU_CSA_VADDR, 0, 
>> AMDGPU_CSA_SIZE,
>> -                            AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
>> -                            AMDGPU_PTE_EXECUTABLE);
>> +       r = amdgpu_vm_bo_map(adev, *bo_va, AMDGPU_CSA_VADDR(adev), 0,
>> +                            AMDGPU_CSA_SIZE, AMDGPU_PTE_READABLE |
>> +                            AMDGPU_PTE_WRITEABLE | 
>> AMDGPU_PTE_EXECUTABLE);
>>
>>          if (r) {
>>                  DRM_ERROR("failed to do bo_map on static CSA, 
>> err=%d\n", r); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
>> index 6a83425aa9ed..499362b55e45 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
>> @@ -251,8 +251,10 @@ struct amdgpu_virt {
>>          uint32_t gim_feature;
>>  };
>>
>> -#define AMDGPU_CSA_SIZE    (8 * 1024)
>> -#define AMDGPU_CSA_VADDR   (AMDGPU_VA_RESERVED_SIZE - AMDGPU_CSA_SIZE)
>> +#define AMDGPU_CSA_SIZE                (8 * 1024)
>> +#define AMDGPU_CSA_VADDR(adev) \
>>         +       (((adev)->vm_manager.max_pfn << 
>> AMDGPU_GPU_PAGE_SHIFT) + \
>>         +        AMDGPU_VA_RESERVED_SIZE)
>>
>> If I  understand your intention correctly, it should be that:
>> +       (((adev)->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT)  - \
>> +        AMDGPU_VA_RESERVED_SIZE)
>>
>> Thanks
>> Roger(Hongbo.He)
>>
>>  #define amdgpu_sriov_enabled(adev) \
>>  ((adev)->virt.caps & AMDGPU_SRIOV_CAPS_ENABLE_IOV)
>> --
>> 2.14.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org <mailto:amd-gfx at lists.freedesktop.org>
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180123/89eb568d/attachment-0001.html>