<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Exchange Server"> <style></style> </head> <body> <meta content="text/html; charset=UTF-8"> <style type="text/css" style="">  </style> <div dir="ltr"> <div id="x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:#000000; font-family:Calibri,Arial,Helvetica,sans-serif"> <p></p> <div>Hi Jerry,<br> <br> Let me clarify my understanding of the problem and the intention of the fix.<br> <br> Address range per page: 4KB<br> Address range per PT (level 3): 2MB<br> Address range per PD (level 2): 1GB<br> Address range per PD (level 1): 512GB<br> Address range per PD (level 0): 256TB<br> <br> Imagine for example a mapping that spans multiple level 2 page directories. Say from 0.5GB to 2.5GB.<br> <br> At level 0:<br> from = 0<br> to = 0<br> <br> At level 1:<br> from = 0<br> to = 2<br> <br> So for level 2 amdgpu_vm_alloc_levels gets called 3 times (pt_idx = [0..2]). Without my change, it gets called with the same saddr and eaddr 3 times. So it calculates the same "from" and "to" each time:<br> from = 256 % 512 = 256<br> to = 767 % 512 = 255<br> <br> Obviously that doesn't work. What we really want is this (from..to in level 2 when called for pt_idx values from level 1):<br> <br> For pt_idx=0 we want to fill the second half of the level 2 page directory with page tables:<br> from = 256<br> to = 511<br> <br> For pt_idx=1 we need to fill the entire level 2 page directories with page tables:<br> from = 0<br> to = 511<br> <br> For pt_idx=2 we want to fill the first half of the level 2 page directory with page tables:<br> from = 0<br> to = 255<br> <br> This creates a contiguous range of page tables across the three level 2 page directories that are spanned by the allocation. My change achieves that.<br> <br> I think you're right, there are some extra high bits in saddr and eaddr, but they get discarded by the modulo-division in the next recursion level. For added clarity the modulo division (or masking of high bits) could be moved up to the caller.<br> <br> Regards,<br> Felix<br> </div> <p></p> <p><br> </p> <div id="x_Signature"> <div style="font-family:Tahoma; font-size:13px"> <div style="font-family:Tahoma; font-size:13px"> <div style="font-family:Tahoma; font-size:13px"> <div style="font-family:Tahoma; font-size:13px"> <div style="font-family:Tahoma; font-size:13px"><font size="2" face="Courier New"></font> <div align="left"> <div> <div class="x_BodyFragment"><font size="2" face="Courier New"> <div class="x_PlainText"><font size="2"><span style="font-size:10pt">--<br> F e l i x K u e h l i n g<br> SMTS Software Development Engineer | Vertical Workstation/Compute<br> 1 Commerce Valley Dr. East, Markham, ON L3T 7X6 Canada<br> (O) +1(289)695-1597<br> _ _ _ _____ _____<br> / \ | \ / | | _ \ \ _ |<br> / A \ | \M/ | | |D) ) /|_| |<br> /_/ \_\ |_| |_| |_____/ |__/ \| facebook.com/AMD | amd.com</span></font><br> </div> </font></div> </div> </div> <font size="2" face="Courier New"></font></div> </div> </div> </div> </div> </div> </div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> Zhang, Jerry<br> <b>Sent:</b> Tuesday, March 28, 2017 10:54:03 PM<br> <b>To:</b> Kuehling, Felix; amd-gfx@lists.freedesktop.org; Koenig, Christian; Zhou, David(ChunMing)<br> <b>Cc:</b> Deucher, Alexander; Russell, Kent<br> <b>Subject:</b> Re: [PATCH 3/3] drm/amdgpu: Fix multi-level page table bugs for large BOs</font> <div> </div> </div> </div> <font size="2"><span style="font-size:10pt;"> <div class="PlainText">On 03/29/2017 09:00 AM, Felix Kuehling wrote:<br> > Fix the start/end address calculation for address ranges that span<br> > multiple page directories in amdgpu_vm_alloc_levels.<br> ><br> > Add WARN_ONs if page tables aren't found. Otherwise the page table<br> > update would just fail silently.<br> ><br> > Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com><br> > ---<br> > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++++---<br> > 1 file changed, 10 insertions(+), 3 deletions(-)<br> ><br> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c<br> > index 818747f..44aa039 100644<br> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c<br> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c<br> > @@ -278,6 +278,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,<br> > from = (saddr >> shift) % amdgpu_vm_num_entries(adev, level);<br> > to = (eaddr >> shift) % amdgpu_vm_num_entries(adev, level);<br> ><br> > + WARN_ON(from > to);<br> > +<br> > if (to > parent->last_entry_used)<br> > parent->last_entry_used = to;<br> ><br> > @@ -312,8 +314,11 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,<br> > }<br> ><br> > if (level < adev->vm_manager.num_level) {<br> > - r = amdgpu_vm_alloc_levels(adev, vm, entry, saddr,<br> > - eaddr, level);<br> > + uint64_t sub_saddr = (pt_idx == from) ? saddr : 0;<br> > + uint64_t sub_eaddr = (pt_idx == to) ? eaddr :<br> > + ((1 << shift) - 1);<br> > + r = amdgpu_vm_alloc_levels(adev, vm, entry, sub_saddr,<br> > + sub_eaddr, level);<br> <br> Do you mean to create a full sub-pt if pt_idx != from and != to?<br> I didn't catch it up clear.<br> <br> In my perspective, we may need to clear the saddr and eaddr upper level bit <br> before going on to allocate the next level PT.<br> Otherwise, the number of next PT entry will be incorrect, more than num_entries <br> as the upper level PT number calculated in.<br> <br> e.g.<br> there is a address contains<br> PD + PT1 + PT2 + PT3<br> <br> for the first round, we could get correct "from" and "to" address with right <br> "shift"(3 blocks), then walk over the address space in PD range.<br> <br> Then for the 2nd round, we cannot get the expected "from" and "to" address with <br> "shift"(2 blocks), as it walks over the address space in PD + PT1 range.<br> While we'd like it's in range PT1 indeed.<br> <br> The fault way goes on with later round.<br> <br> Perhaps, that's the root cause about the issue you face.<br> <br> Jerry.<br> <br> > if (r)<br> > return r;<br> > }<br> > @@ -957,9 +962,11 @@ static struct amdgpu_bo *amdgpu_vm_get_pt(struct amdgpu_pte_update_params *p,<br> > entry = &entry->entries[idx];<br> > }<br> ><br> > - if (level)<br> > + if (WARN_ON(level))<br> > return NULL;<br> ><br> > + WARN_ON(!entry->bo);<br> > +<br> > return entry->bo;<br> > }<br> ><br> ><br> </div> </span></font> </body> </html>