<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body bgcolor="white" lang="EN-US" link="#0563C1" vlink="#954F72"> <div class="WordSection1"> Hi Christian <o:p></o:p> <o:p> </o:p> Thanks for explaining the HOLD for us, <o:p></o:p> <o:p> </o:p> My understanding is we still could put CSA to 0~HOLE_START, because we can report UMD the max space is HOLD_START-CSA_SIZE , thus no colliding will hit.<o:p></o:p> <o:p> </o:p> > Now on Vega/Raven/Picasso etc.. (everything with a GFX9) the lower range (0x0-0x8000 0000 0000) is reserved for SVA/ATC use. Since we unfortunately didn't knew that initially we exposed those to older userspace as usable and also put the CSA in there. <o:p></o:p> Regarding with above sentence, do you mean this range (0->HOLE_START) shouldn’t be exposed to user space ? I don’t get your point here …<o:p></o:p> <o:p> </o:p> Putting CSA in 0~HOLD_START is the legacy approach we selected for a long time since very early stage, how comes that you think it is a problem now ?<o:p></o:p> <o:p> </o:p> /Monk<o:p></o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Koenig, Christian Sent: Thursday, January 17, 2019 4:30 PM To: Liu, Monk <Monk.Liu@amd.com>; Lou, Wentao <Wentao.Lou@amd.com>; amd-gfx@lists.freedesktop.org; Zhu, Rex <Rex.Zhu@amd.com> Cc: Deng, Emily <Emily.Deng@amd.com> Subject: Re: [PATCH] drm/amdgpu: csa_vaddr should not larger than AMDGPU_GMC_HOLE_START<o:p></o:p> </div> </div> <o:p> </o:p> <div> Hi Monk, ok let me explain a bit more how the hardware works. The GMC manages a virtual 64bit address space, but only 48bit of that virtual address space are handled by the page table walker. The 48bits of address space are sign extended, so bit 47 of that are extended into bits 48-63. This gives us the following memory layout: 0x0 .... virtual address space 0x8000 0000 0000 .... hole 0xFFFF 8000 0000 0000 .... virtual address space 0xFFFF FFFF FFFF FFFF Trying to access the hole results in a range fault interrupt IIRC. When doing the VM page table walk the topmost 16bits are ignored, so when programming the page table walker you cut those of and use a linear address again. This is what AMDGPU_GMC_HOLE_MASK is good for. Now on Vega/Raven/Picasso etc.. (everything with a GFX9) the lower range (0x0-0x8000 0000 0000) is reserved for SVA/ATC use. Since we unfortunately didn't knew that initially we exposed those to older userspace as usable and also put the CSA in there. The most likely cause of this is that we still have a bug somewhere about this, e.g. not correctly using sign extended addresses *OR* using sign extended addresses where we should use linear instead. Regards, Christian. Am 17.01.19 um 09:04 schrieb Liu, Monk:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> Hi Christian <o:p></o:p> <o:p></o:p> I believe Wentao can fix the issue we it by below step:<o:p></o:p> <ol style="margin-top:0in" start="1" type="1"> <li class="MsoNormal" style="color:windowtext;mso-list:l1 level1 lfo3">Return Virtual_address_max (UMD use it) to HOLE_START – RESERVED_SIZE <o:p></o:p></li><li class="MsoNormal" style="color:windowtext;mso-list:l1 level1 lfo3">[optional] Still Keep virtual_address_offset to RESERVED_SIZE (current way, I think it’s because previously we put CSA in 0 à RESERVED_SIZE space) <o:p></o:p></li><li class="MsoNormal" style="color:windowtext;mso-list:l1 level1 lfo3">Put CSA in HOLE_START – RESERVED_SIZE è HOLE_START (it’s current design) <o:p> </o:p></li></ol> <o:p></o:p> I don’t get where above scheme is not correct … can you give more explain for the GMC_HOLE_START ?<o:p></o:p> <o:p></o:p> e.g.<o:p></o:p> <ol style="margin-top:0in" start="1" type="1"> <li class="MsoNormal" style="color:windowtext;margin-left:2.5pt;mso-list:l2 level1 lfo6"> why you set GMC_HOLE_START to 0x8’000’0000’0000 (half size of MAX of 48bit address space) ? is it for HSA purpose to make sure GPU address can also be used for CPU address ? <o:p></o:p></li><li class="MsoNormal" style="color:windowtext;margin-left:2.5pt;mso-list:l2 level1 lfo6"> now MAX_PFN is 1’000’0000’0000, do you need to change GMC_HOLE_START ? <o:p></o:p></li></ol> <o:p></o:p> thanks <o:p></o:p> we need some catch up <o:p></o:p> <o:p></o:p> /Monk<o:p></o:p> <o:p></o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: amd-gfx <a href="mailto:amd-gfx-bounces@lists.freedesktop.org"><amd-gfx-bounces@lists.freedesktop.org></a> On Behalf Of Koenig, Christian Sent: Thursday, January 17, 2019 3:39 PM To: Lou, Wentao <a href="mailto:Wentao.Lou@amd.com"><Wentao.Lou@amd.com></a>; Liu, Monk <a href="mailto:Monk.Liu@amd.com"><Monk.Liu@amd.com></a>; <a href="mailto:amd-gfx@lists.freedesktop.org"> amd-gfx@lists.freedesktop.org</a>; Zhu, Rex <a href="mailto:Rex.Zhu@amd.com"><Rex.Zhu@amd.com></a> Cc: Deng, Emily <a href="mailto:Emily.Deng@amd.com"><Emily.Deng@amd.com></a> Subject: Re: [PATCH] drm/amdgpu: csa_vaddr should not larger than AMDGPU_GMC_HOLE_START<o:p></o:p> </div> </div> <o:p></o:p> <div> Am 17.01.19 um 04:17 schrieb Lou, Wentao:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> Hi Christian,<o:p></o:p> <o:p></o:p> Your solution as:<o:p></o:p> addr = (max_pfn - (AMDGPU_VA_RESERVED_SIZE >> AMDGPU_PAGE_SHIFT)) << AMDGPU_PAGE_SHIFT;<o:p></o:p> now max_pfn = 0x10 0000 0000, AMDGPU_VA_RESERVED_SIZE = 0x10 0000, AMDGPU_PAGE_SHIFT = 12<o:p></o:p> Still got addr = 0xFFFF FFF0 0000, which would cause ring gfx timeout.<o:p></o:p> </blockquote> But 0xFFFF FFF0 0000 is the correct address, if that is causing a problem then there is a bug somewhere else. Please try to use AMDGPU_GMC_HOLE_START-AMDGPU_VA_RESERVED_SIZE as well. Does that work? <o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <o:p></o:p> Before commit 1bf621c42137926ac249af761c0190a9258aa0db, vm_size was 32GB, and csa_addr was under AMDGPU_GMC_HOLE_START.<o:p></o:p> </blockquote> Wait a second why was the vm_size 32GB? This is on a Vega10 isn't it? <o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> I didn’t understand why csa_addr need to be above AMDGPU_GMC_HOLE_START now.<o:p></o:p> </blockquote> On Vega10 the lower range, e.g. everything below AMDGPU_GMC_HOLE_START is reserved for SVA. Regards, Christian. <o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> Thanks.<o:p></o:p> <o:p></o:p> BR,<o:p></o:p> Wentao<o:p></o:p> <o:p></o:p> <o:p></o:p> <o:p></o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Koenig, Christian <a href="mailto:Christian.Koenig@amd.com"> <Christian.Koenig@amd.com></a> Sent: Wednesday, January 16, 2019 5:48 PM To: Lou, Wentao <a href="mailto:Wentao.Lou@amd.com"><Wentao.Lou@amd.com></a>; Liu, Monk <a href="mailto:Monk.Liu@amd.com"><Monk.Liu@amd.com></a>; <a href="mailto:amd-gfx@lists.freedesktop.org"> amd-gfx@lists.freedesktop.org</a>; Zhu, Rex <a href="mailto:Rex.Zhu@amd.com"><Rex.Zhu@amd.com></a> Cc: Deng, Emily <a href="mailto:Emily.Deng@amd.com"><Emily.Deng@amd.com></a> Subject: Re: [PATCH] drm/amdgpu: csa_vaddr should not larger than AMDGPU_GMC_HOLE_START<o:p></o:p> </div> </div> <o:p></o:p> <div> Hi Wentao, well the problem is you don't seem to understand how the hardware works. See the engines see an MC address space with a hole in the middle, similar to the how x86 64bit CPU address space works. But the page tables are programmed linearly. So the calculation in amdgpu_driver_open_kms() is correct because it takes the MC address and mages a linear page table index from it again. The only thing we might need to fix here is shifting max_pfn before the subtraction and I doubt that even that is necessary. Regards, Christian. Am 16.01.19 um 10:34 schrieb Lou, Wentao:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> Hi Christian,<o:p></o:p> <o:p></o:p> Now vm_size was set to 0x4 0000 GB by below commit:<o:p></o:p> 1bf621c42137926ac249af761c0190a9258aa0db drm/amdgpu: Remove unnecessary VM size calculations<o:p></o:p> <o:p></o:p> So that max_pfn would be 0x10 0000 0000.<o:p></o:p> amdgpu_csa_vaddr would make max_pfn << 12 to get 0x1 0000 0000 0000, and then minus AMDGPU_VA_RESERVED_SIZE, to get 0xFFFF FFF0 0000<o:p></o:p> unfortunately this number was between AMDGPU_GMC_HOLE_START and AMDGPU_GMC_HOLE_END, so that amdgpu_gmc_sign_extend was called to make it 0xFFFF FFFF FFF0 0000<o:p></o:p> <o:p></o:p> in amdgpu_driver_open_kms, extended csa_addr cannot be passed into amdgpu_map_static_csa directly, it would be above the limit of max_pfn.<o:p></o:p> So that csa_addr was restricted by AMDGPU_GMC_HOLE_MASK to make it possible for amdgpu_vm_alloc_pts.<o:p></o:p> But this restriction by AMDGPU_GMC_HOLE_MASK would make the address fall back into AMDGPU_GMC_HOLE again, which causing GPU reset.<o:p></o:p> We just put amdgpu_csa_vaddr back to AMDGPU_GMC_HOLE_START, to avoid the address touching AMDGPU_GMC_HOLE.<o:p></o:p> By the way, if max_pfn was shift much to the left, it would always get zero, with or without min(*,*).<o:p></o:p> <o:p></o:p> <o:p></o:p> BR,<o:p></o:p> Wentao<o:p></o:p> <o:p></o:p> <o:p></o:p> <o:p></o:p> -----Original Message----- From: Koenig, Christian <a href="mailto:Christian.Koenig@amd.com"><Christian.Koenig@amd.com></a> Sent: Tuesday, January 15, 2019 4:02 PM To: Liu, Monk <a href="mailto:Monk.Liu@amd.com"><Monk.Liu@amd.com></a>; Lou, Wentao <a href="mailto:Wentao.Lou@amd.com"><Wentao.Lou@amd.com></a>; <a href="mailto:amd-gfx@lists.freedesktop.org"> amd-gfx@lists.freedesktop.org</a>; Zhu, Rex <a href="mailto:Rex.Zhu@amd.com"><Rex.Zhu@amd.com></a> Subject: Re: [PATCH] drm/amdgpu: csa_vaddr should not larger than AMDGPU_GMC_HOLE_START<o:p></o:p> <o:p></o:p> Am 15.01.19 um 07:19 schrieb Liu, Monk:<o:p></o:p> > The max_pfn is now 1'0000'0000'0000'0000 (bytes) which is above 48 bit now, and it with AMDGPU_GMC_HOLE_MASK make it to zero ....<o:p></o:p> > <o:p></o:p> > And in code "amdgpu_driver_open_kms()" I saw @Zhu, Rex write the code as :<o:p></o:p> > <o:p></o:p> > "csa_addr = amdgpu_csa_vadr(adev) & AMDGPU_GMC_HOLE_MASK", I think this is wrong since you intentionally place the csa above GMC hole, right ?<o:p></o:p> <o:p></o:p> The fix is just completely incorrect since min(adev->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT, AMDGPU_GMC_HOLE_START) still gives you 0 when we shift max_pfn to much to the left.<o:p></o:p> <o:p></o:p> The correct solution is to substract the reserved size first and then shift. E.g.:<o:p></o:p> <o:p></o:p> addr = (max_pfn - (AMDGPU_VA_RESERVED_SIZE >> AMDGPU_PAGE_SHIFT)) << AMDGPU_PAGE_SHIFT;<o:p></o:p> <o:p></o:p> Regards,<o:p></o:p> Christian.<o:p></o:p> <o:p></o:p> > <o:p></o:p> > Looks like we should modify this place<o:p></o:p> > <o:p></o:p> > /Monk<o:p></o:p> > <o:p></o:p> > -----Original Message-----<o:p></o:p> > From: amd-gfx <<a href="mailto:amd-gfx-bounces@lists.freedesktop.org">amd-gfx-bounces@lists.freedesktop.org</a>> On Behalf Of <o:p></o:p> > Christian K?nig<o:p></o:p> > Sent: Monday, January 14, 2019 9:05 PM<o:p></o:p> > To: Lou, Wentao <<a href="mailto:Wentao.Lou@amd.com">Wentao.Lou@amd.com</a>>; <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a><o:p></o:p> > Subject: Re: [PATCH] drm/amdgpu: csa_vaddr should not larger than <o:p></o:p> > AMDGPU_GMC_HOLE_START<o:p></o:p> > <o:p></o:p> > Am 14.01.19 um 09:40 schrieb wentalou:<o:p></o:p> >> After removing unnecessary VM size calculations, vm_manager.max_pfn <o:p></o:p> >> would reach 0x10,0000,0000 max_pfn << AMDGPU_GPU_PAGE_SHIFT exceeding <o:p></o:p> >> AMDGPU_GMC_HOLE_START would caused GPU reset.<o:p></o:p> >> <o:p></o:p> >> Change-Id: I47ad0be2b0bd9fb7490c4e1d7bb7bdacf71132cb<o:p></o:p> >> Signed-off-by: wentalou <<a href="mailto:Wentao.Lou@amd.com">Wentao.Lou@amd.com</a>><o:p></o:p> > NAK, that is incorrect. We intentionally place the csa above the GMC hole.<o:p></o:p> > <o:p></o:p> > Regards,<o:p></o:p> > Christian.<o:p></o:p> > <o:p></o:p> >> ---<o:p></o:p> >> drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 3 ++-<o:p></o:p> >> 1 file changed, 2 insertions(+), 1 deletion(-)<o:p></o:p> >> <o:p></o:p> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c<o:p></o:p> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c<o:p></o:p> >> index 7e22be7..dd3bd01 100644<o:p></o:p> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c<o:p></o:p> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c<o:p></o:p> >> @@ -26,7 +26,8 @@<o:p></o:p> >> <o:p></o:p> >> uint64_t amdgpu_csa_vaddr(struct amdgpu_device *adev)<o:p></o:p> >> {<o:p></o:p> >> - uint64_t addr = adev->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT;<o:p></o:p> >> + uint64_t addr = min(adev->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT,<o:p></o:p> >> + AMDGPU_GMC_HOLE_START);<o:p></o:p> >> <o:p></o:p> >> addr -= AMDGPU_VA_RESERVED_SIZE;<o:p></o:p> >> addr = amdgpu_gmc_sign_extend(addr);<o:p></o:p> > _______________________________________________<o:p></o:p> > amd-gfx mailing list<o:p></o:p> > <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a><o:p></o:p> > <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx"> https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><o:p></o:p> <o:p></o:p> </blockquote> <o:p></o:p> </blockquote> <o:p></o:p> </blockquote> <o:p> </o:p> </div> </body> </html>