> 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 > *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 > *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>; 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> > --- >  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 > 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/f206689e/attachment-0001.html>
