<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Hi Oak, <blockquote type="cite">>In other words when you request the address of a PT you get the address where memory management has moved it, not the address where the hardware is processing the fault from. Do you mean the address in the PT is what vm has mapped and the faulty address need to be mapped in the PT? </blockquote> No, the memory management is pipelined to avoid stalls during eviction. This means that the state we manage is always the future state, e.g. when you walk the page tables in the VM structure you see the state after all the running operations completed. In other words when a page table is pipelined for eviction you see the evicted state and not the current executing one. But what we need for page fault handling is the current state and not the future state. <blockquote type="cite">The amdgpu_vm and amdgpu_vm_pt data structure are invented for the purpose of management of page table BO. Why we need invent extra structure?</blockquote> We will need something like a double entry accounting for the page tables where we both keep the future as well as the current state. <blockquote type="cite">Think about the normal case of the page fault handling when the process and vm are still alive. In the normal case, the amdgpu_vm structure can meet exactly purpose of page table update.</blockquote> No, the problem is not if the VM is still alive or dead but rather that in the case of an eviction you wouldn't be able to handle page faults any more. <blockquote type="cite">The said extra data structure must look very similar to the amdgpu_vm/vm_pt structure. So the best way is to reuse the existing vm structure, either in normal case or in process termination case.</blockquote> Completely agree that we will probably need the same structure as the VM hierarchy offers, just with another set of information. <blockquote type="cite">Is it possible to delay the destroy of vm during process termination?</blockquote> Yeah, thought about that as well. The problem with that is the OOM killer and that we are still not able to reliable kill running command submissions. Ok you convinced me that we should probably work on that instead. E.g. when a process has bound its address space to a VM and exits we need to find a way to kill all still executing jobs. We should let the UMD handle that case gracefully when they really need this. Thanks, Christian. Am 07.09.2018 um 23:52 schrieb Zeng, Oak: </div> <blockquote type="cite" cite="mid:BN6PR12MB1651CCA8BB73DE64815EB0B280000@BN6PR12MB1651.namprd12.prod.outlook.com"> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> <div class="WordSection1"> Hi Christian,<o:p></o:p> <o:p> </o:p> See comments inline<o:p></o:p> <o:p> </o:p> <div> Thanks,<o:p></o:p> Oak<o:p></o:p> </div> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Koenig, Christian Sent: Monday, September 10, 2018 7:44 AM To: Zeng, Oak <a class="moz-txt-link-rfc2396E" href="mailto:Oak.Zeng@amd.com"><Oak.Zeng@amd.com></a>; Oak Zeng <a class="moz-txt-link-rfc2396E" href="mailto:zengshanjun@gmail.com"><zengshanjun@gmail.com></a>; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a>; Yang, Philip <a class="moz-txt-link-rfc2396E" href="mailto:Philip.Yang@amd.com"><Philip.Yang@amd.com></a> Subject: Re: [PATCH 1/2] drm/amdgpu: Moved fault hash table to amdgpu vm<o:p></o:p> </div> </div> <o:p> </o:p> <div> >The VM doesn't know that information either.<o:p></o:p> <o:p> </o:p> Yes, VM doesn’t know the information. More specifically I meant we need VM data structure to update PTs during page fault handling. For example, we walk the 4 level page table (the first level is pointed by vm->root) to figure out the entries need to be filled, allocate page table entry Bos if necessary, allocate physical page for the faulty address, setup 4 level page table for the faulty virtual address to pointing to the allocated physical page. >In other words when you request the address of a PT you get the address where memory management has moved it, not the address where the hardware is processing the fault from.<o:p></o:p> Do you mean the address in the PT is what vm has mapped and the faulty address need to be mapped in the PT?<o:p></o:p> I see only a few possible solutions for that: >1. We use an extra structure in the kernel driver which holds the current address of the PTs and is feed by memory management when buffers move around.<o:p></o:p> The amdgpu_vm and amdgpu_vm_pt data structure are invented for the purpose of management of page table BO. Why we need invent extra structure? Think about the normal case of the page fault handling when the process and vm are still alive. In the normal case, the amdgpu_vm structure can meet exactly purpose of page table update.<o:p></o:p> <o:p> </o:p> The said extra data structure must look very similar to the amdgpu_vm/vm_pt structure. So the best way is to reuse the existing vm structure, either in normal case or in process termination case. Is it possible to delay the destroy of vm during process termination?<o:p></o:p> >2. We change the SDMA firmware to do the walk for us and update the PTEs based on the information in the page directory.<o:p></o:p> As the page table BO need to be freed later. You still need to manage them in the driver. SDMA FW can walk and update but it need information from driver side. Driver still need a similar struct like amdgpu_vm.<o:p></o:p> >3. We use the UTCL walker to get us to the correct address which is then updated with the SDMA. I don’t quite understand your point here. What do you mean by “correct address”? Do you mean the physical address to be filled to the page table? If this is your meaning, how UTCL2 knows this information – the physical address has to be allocated/validated through driver.<o:p></o:p> <o:p> </o:p> Regards, Christian. Am 07.09.2018 um 17:30 schrieb Zeng, Oak:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> Without a VM, how can you get the physical address of the faulty virtual address? Where this information should be hold?<o:p></o:p> <o:p></o:p> <div> Thanks,<o:p></o:p> Oak<o:p></o:p> </div> <o:p></o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Koenig, Christian Sent: Monday, September 10, 2018 7:20 AM To: Zeng, Oak <a href="mailto:Oak.Zeng@amd.com" moz-do-not-send="true"><Oak.Zeng@amd.com></a>; Oak Zeng <a href="mailto:zengshanjun@gmail.com" moz-do-not-send="true"><zengshanjun@gmail.com></a>; <a href="mailto:amd-gfx@lists.freedesktop.org" moz-do-not-send="true"> amd-gfx@lists.freedesktop.org</a>; Yang, Philip <a href="mailto:Philip.Yang@amd.com" moz-do-not-send="true"> <Philip.Yang@amd.com></a> Subject: Re: [PATCH 1/2] drm/amdgpu: Moved fault hash table to amdgpu vm<o:p></o:p> </div> </div> <o:p></o:p> <div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> Am I right that the handling of page fault need a valid VM? <o:p></o:p> </blockquote> No, exactly that view is incorrect. The VM is meant to be a memory management structure of page tables and is completely pointless fault processing because it represent the future state of the page tables and not the current one. What we need for fault processing is a new structure build around PASIDs which is feed by the with addresses when page tables are moved around. Alternatively I hope that we can use the SDMA to walk the page tables and update the required entries by just using the address. Regards, Christian. Am 07.09.2018 um 16:55 schrieb Zeng, Oak:<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> What is the value of delay the destroy of hash to after vm is destroyed? Since vm is destroyed, you basically don’t have enough information to paging in the correct page to gpuvm. Am I right that the handling of page fault need a valid VM? For example, you need the VM to get the corresponding physical address of the faulty page. <o:p></o:p> <o:p></o:p> After vm is destroyed, the retry interrupt can be directly discard as you can’t find vm through pasid. You can think this is also one kind of prescreen. <o:p></o:p> <o:p></o:p> So I am stand for the point that, there is no need to delay the destroy of hash to after vm is destroyed. Prescreening hash can be destroyed at the time of vm_fini.<o:p></o:p> <o:p></o:p> <div> Thanks,<o:p></o:p> Oak<o:p></o:p> </div> <o:p></o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Christian König <a href="mailto:ckoenig.leichtzumerken@gmail.com" moz-do-not-send="true"><ckoenig.leichtzumerken@gmail.com></a> Sent: Friday, September 07, 2018 4:39 AM To: Zeng, Oak <a href="mailto:Oak.Zeng@amd.com" moz-do-not-send="true"><Oak.Zeng@amd.com></a>; Oak Zeng <a href="mailto:zengshanjun@gmail.com" moz-do-not-send="true"><zengshanjun@gmail.com></a>; <a href="mailto:amd-gfx@lists.freedesktop.org" moz-do-not-send="true"> amd-gfx@lists.freedesktop.org</a> Cc: Zeng, Oak <a href="mailto:Oak.Zeng@amd.com" moz-do-not-send="true"><Oak.Zeng@amd.com></a> Subject: Re: [PATCH 1/2] drm/amdgpu: Moved fault hash table to amdgpu vm<o:p></o:p> </div> </div> <o:p></o:p> <div> Hi Oak, yeah, but this is still a NAK. Making the hash per PASID was not a suggestion but rather a must have. The VM structures must be destroyed while the processing is still ongoing, or otherwise we would not have a clean OOM handling. The IDR for PASID lockup can be put into amdgpu_ids.c, you can just replace the IDA already used there with an IDR. Since the PASID is already freed up delayed we should have the grace period for interrupt processing included. If that still isn't sufficient we can still add some delayed work for this. Regards, Christian. Am 07.09.2018 um 06:16 schrieb ozeng:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> Hi Christian, <o:p></o:p> In this implementation, fault hash is made per vm, not per pasid as suggested, based on below considerations:<o:p></o:p> <ul type="disc"> <li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo3"> Delay the destroy of hash introduces more effort like how to set the proper grace period after which no retry interrupt will be generated. We want to avoid those complication if we can. <o:p></o:p></li> <li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo3"> The problem of the per vm implementation is that it is hard to delay the destroy of fault hash, because when vm is destroyed, prescreen function won't be able to retrieve the fault hash. But in this case, the prescreen function can either let the interrupt go through (to gmc) or ignore it. From this perspective, it is not very necessary to delay the destroy of hash table. <o:p></o:p></li> <li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo3"> On the other hand, since ASICs after vega10 have the HW CAM feature. So the SW IV prescreen is only useful for vega10. For all the HW implemented CAM, we can consider the delayed CAM acknowledgment. <o:p></o:p></li> <li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo3"> Making it per pasid need to introduce another idr to correlate pasid and hash table - where to put the idr? You will have to make it a global variable which is not very desirable. <o:p></o:p></li> </ul> The main purpose of this patch is to solve the inter-process lock issue (when hash table is full), while still keep codes simple.<o:p></o:p> Also with this patch, the faults kfifo is not necessary any more. See patch 2. <o:p></o:p> Regards,<o:p></o:p> Oak<o:p></o:p> <o:p></o:p> <div> On 09/06/2018 11:28 PM, Oak Zeng wrote:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>In stead of share one fault hash table per device, make it<o:p></o:p></pre> <pre>per vm. This can avoid inter-process lock issue when fault<o:p></o:p></pre> <pre>hash table is full.<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Change-Id: I5d1281b7c41eddc8e26113e010516557588d3708<o:p></o:p></pre> <pre>Signed-off-by: Oak Zeng <a href="mailto:Oak.Zeng@amd.com" moz-do-not-send="true"><Oak.Zeng@amd.com></a><o:p></o:p></pre> <pre>Suggested-by: Christian Konig <a href="mailto:Christian.Koenig@amd.com" moz-do-not-send="true"><Christian.Koenig@amd.com></a><o:p></o:p></pre> <pre>Suggested-by: Felix Kuehling <a href="mailto:Felix.Kuehling@amd.com" moz-do-not-send="true"><Felix.Kuehling@amd.com></a><o:p></o:p></pre> <pre>---<o:p></o:p></pre> <pre> drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c | 75 ------------------------<o:p></o:p></pre> <pre> drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h | 10 ----<o:p></o:p></pre> <pre> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 102 ++++++++++++++++++++++++++++++++-<o:p></o:p></pre> <pre> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 12 ++++<o:p></o:p></pre> <pre> drivers/gpu/drm/amd/amdgpu/vega10_ih.c | 38 +++++-------<o:p></o:p></pre> <pre> 5 files changed, 127 insertions(+), 110 deletions(-)<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c<o:p></o:p></pre> <pre>index 06373d4..4ed8621 100644<o:p></o:p></pre> <pre>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c<o:p></o:p></pre> <pre>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c<o:p></o:p></pre> <pre>@@ -197,78 +197,3 @@ int amdgpu_ih_process(struct amdgpu_device *adev)<o:p></o:p></pre> <pre> return IRQ_HANDLED;<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>-/**<o:p></o:p></pre> <pre>- * amdgpu_ih_add_fault - Add a page fault record<o:p></o:p></pre> <pre>- *<o:p></o:p></pre> <pre>- * @adev: amdgpu device pointer<o:p></o:p></pre> <pre>- * @key: 64-bit encoding of PASID and address<o:p></o:p></pre> <pre>- *<o:p></o:p></pre> <pre>- * This should be called when a retry page fault interrupt is<o:p></o:p></pre> <pre>- * received. If this is a new page fault, it will be added to a hash<o:p></o:p></pre> <pre>- * table. The return value indicates whether this is a new fault, or<o:p></o:p></pre> <pre>- * a fault that was already known and is already being handled.<o:p></o:p></pre> <pre>- *<o:p></o:p></pre> <pre>- * If there are too many pending page faults, this will fail. Retry<o:p></o:p></pre> <pre>- * interrupts should be ignored in this case until there is enough<o:p></o:p></pre> <pre>- * free space.<o:p></o:p></pre> <pre>- *<o:p></o:p></pre> <pre>- * Returns 0 if the fault was added, 1 if the fault was already known,<o:p></o:p></pre> <pre>- * -ENOSPC if there are too many pending faults.<o:p></o:p></pre> <pre>- */<o:p></o:p></pre> <pre>-int amdgpu_ih_add_fault(struct amdgpu_device *adev, u64 key)<o:p></o:p></pre> <pre>-{<o:p></o:p></pre> <pre>- unsigned long flags;<o:p></o:p></pre> <pre>- int r = -ENOSPC;<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>- if (WARN_ON_ONCE(!adev->irq.ih.faults))<o:p></o:p></pre> <pre>- /* Should be allocated in <IP>_ih_sw_init on GPUs that<o:p></o:p></pre> <pre>- * support retry faults and require retry filtering.<o:p></o:p></pre> <pre>- */<o:p></o:p></pre> <pre>- return r;<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>- spin_lock_irqsave(&adev->irq.ih.faults->lock, flags);<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>- /* Only let the hash table fill up to 50% for best performance */<o:p></o:p></pre> <pre>- if (adev->irq.ih.faults->count >= (1 << (AMDGPU_PAGEFAULT_HASH_BITS-1)))<o:p></o:p></pre> <pre>- goto unlock_out;<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>- r = chash_table_copy_in(&adev->irq.ih.faults->hash, key, NULL);<o:p></o:p></pre> <pre>- if (!r)<o:p></o:p></pre> <pre>- adev->irq.ih.faults->count++;<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>- /* chash_table_copy_in should never fail unless we're losing count */<o:p></o:p></pre> <pre>- WARN_ON_ONCE(r < 0);<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>-unlock_out:<o:p></o:p></pre> <pre>- spin_unlock_irqrestore(&adev->irq.ih.faults->lock, flags);<o:p></o:p></pre> <pre>- return r;<o:p></o:p></pre> <pre>-}<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>-/**<o:p></o:p></pre> <pre>- * amdgpu_ih_clear_fault - Remove a page fault record<o:p></o:p></pre> <pre>- *<o:p></o:p></pre> <pre>- * @adev: amdgpu device pointer<o:p></o:p></pre> <pre>- * @key: 64-bit encoding of PASID and address<o:p></o:p></pre> <pre>- *<o:p></o:p></pre> <pre>- * This should be called when a page fault has been handled. Any<o:p></o:p></pre> <pre>- * future interrupt with this key will be processed as a new<o:p></o:p></pre> <pre>- * page fault.<o:p></o:p></pre> <pre>- */<o:p></o:p></pre> <pre>-void amdgpu_ih_clear_fault(struct amdgpu_device *adev, u64 key)<o:p></o:p></pre> <pre>-{<o:p></o:p></pre> <pre>- unsigned long flags;<o:p></o:p></pre> <pre>- int r;<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>- if (!adev->irq.ih.faults)<o:p></o:p></pre> <pre>- return;<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>- spin_lock_irqsave(&adev->irq.ih.faults->lock, flags);<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>- r = chash_table_remove(&adev->irq.ih.faults->hash, key, NULL);<o:p></o:p></pre> <pre>- if (!WARN_ON_ONCE(r < 0)) {<o:p></o:p></pre> <pre>- adev->irq.ih.faults->count--;<o:p></o:p></pre> <pre>- WARN_ON_ONCE(adev->irq.ih.faults->count < 0);<o:p></o:p></pre> <pre>- }<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>- spin_unlock_irqrestore(&adev->irq.ih.faults->lock, flags);<o:p></o:p></pre> <pre>-}<o:p></o:p></pre> <pre>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h<o:p></o:p></pre> <pre>index a23e1c0..f411ffb 100644<o:p></o:p></pre> <pre>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h<o:p></o:p></pre> <pre>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h<o:p></o:p></pre> <pre>@@ -32,13 +32,6 @@ struct amdgpu_device;<o:p></o:p></pre> <pre> #define AMDGPU_IH_CLIENTID_LEGACY 0<o:p></o:p></pre> <pre> #define AMDGPU_IH_CLIENTID_MAX SOC15_IH_CLIENTID_MAX<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>-#define AMDGPU_PAGEFAULT_HASH_BITS 8<o:p></o:p></pre> <pre>-struct amdgpu_retryfault_hashtable {<o:p></o:p></pre> <pre>- DECLARE_CHASH_TABLE(hash, AMDGPU_PAGEFAULT_HASH_BITS, 8, 0);<o:p></o:p></pre> <pre>- spinlock_t lock;<o:p></o:p></pre> <pre>- int count;<o:p></o:p></pre> <pre>-};<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre> /*<o:p></o:p></pre> <pre> * R6xx+ IH ring<o:p></o:p></pre> <pre> */<o:p></o:p></pre> <pre>@@ -57,7 +50,6 @@ struct amdgpu_ih_ring {<o:p></o:p></pre> <pre> bool use_doorbell;<o:p></o:p></pre> <pre> bool use_bus_addr;<o:p></o:p></pre> <pre> dma_addr_t rb_dma_addr; /* only used when use_bus_addr = true */<o:p></o:p></pre> <pre>- struct amdgpu_retryfault_hashtable *faults;<o:p></o:p></pre> <pre> };<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> #define AMDGPU_IH_SRC_DATA_MAX_SIZE_DW 4<o:p></o:p></pre> <pre>@@ -95,7 +87,5 @@ int amdgpu_ih_ring_init(struct amdgpu_device *adev, unsigned ring_size,<o:p></o:p></pre> <pre> bool use_bus_addr);<o:p></o:p></pre> <pre> void amdgpu_ih_ring_fini(struct amdgpu_device *adev);<o:p></o:p></pre> <pre> int amdgpu_ih_process(struct amdgpu_device *adev);<o:p></o:p></pre> <pre>-int amdgpu_ih_add_fault(struct amdgpu_device *adev, u64 key);<o:p></o:p></pre> <pre>-void amdgpu_ih_clear_fault(struct amdgpu_device *adev, u64 key);<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> #endif<o:p></o:p></pre> <pre>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c<o:p></o:p></pre> <pre>index 1d7e3c1..8b220e9 100644<o:p></o:p></pre> <pre>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c<o:p></o:p></pre> <pre>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c<o:p></o:p></pre> <pre>@@ -2692,6 +2692,22 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,<o:p></o:p></pre> <pre> adev->vm_manager.fragment_size);<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>+static struct amdgpu_retryfault_hashtable *init_fault_hash(void)<o:p></o:p></pre> <pre>+{<o:p></o:p></pre> <pre>+ struct amdgpu_retryfault_hashtable *fault_hash;<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ fault_hash = kmalloc(sizeof(*fault_hash), GFP_KERNEL);<o:p></o:p></pre> <pre>+ if (!fault_hash)<o:p></o:p></pre> <pre>+ return fault_hash;<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ INIT_CHASH_TABLE(fault_hash->hash,<o:p></o:p></pre> <pre>+ AMDGPU_PAGEFAULT_HASH_BITS, 8, 0);<o:p></o:p></pre> <pre>+ spin_lock_init(&fault_hash->lock);<o:p></o:p></pre> <pre>+ fault_hash->count = 0;<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ return fault_hash;<o:p></o:p></pre> <pre>+}<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre> /**<o:p></o:p></pre> <pre> * amdgpu_vm_init - initialize a vm instance<o:p></o:p></pre> <pre> *<o:p></o:p></pre> <pre>@@ -2780,6 +2796,12 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,<o:p></o:p></pre> <pre> vm->pasid = pasid;<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>+ vm->fault_hash = init_fault_hash();<o:p></o:p></pre> <pre>+ if (!vm->fault_hash) {<o:p></o:p></pre> <pre>+ r = -ENOMEM;<o:p></o:p></pre> <pre>+ goto error_free_root;<o:p></o:p></pre> <pre>+ }<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre> INIT_KFIFO(vm->faults);<o:p></o:p></pre> <pre> vm->fault_credit = 16;<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>@@ -2973,7 +2995,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> /* Clear pending page faults from IH when the VM is destroyed */<o:p></o:p></pre> <pre> while (kfifo_get(&vm->faults, &fault))<o:p></o:p></pre> <pre>- amdgpu_ih_clear_fault(adev, fault);<o:p></o:p></pre> <pre>+ amdgpu_vm_clear_fault(vm->fault_hash, fault);<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> if (vm->pasid) {<o:p></o:p></pre> <pre> unsigned long flags;<o:p></o:p></pre> <pre>@@ -2983,6 +3005,9 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)<o:p></o:p></pre> <pre> spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>+ kfree(vm->fault_hash);<o:p></o:p></pre> <pre>+ vm->fault_hash = NULL;<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre> drm_sched_entity_destroy(&vm->entity);<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> if (!RB_EMPTY_ROOT(&vm->va.rb_root)) {<o:p></o:p></pre> <pre>@@ -3183,3 +3208,78 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+/**<o:p></o:p></pre> <pre>+ * amdgpu_vm_add_fault - Add a page fault record to fault hash table<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * @fault_hash: fault hash table<o:p></o:p></pre> <pre>+ * @key: 64-bit encoding of PASID and address<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * This should be called when a retry page fault interrupt is<o:p></o:p></pre> <pre>+ * received. If this is a new page fault, it will be added to a hash<o:p></o:p></pre> <pre>+ * table. The return value indicates whether this is a new fault, or<o:p></o:p></pre> <pre>+ * a fault that was already known and is already being handled.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * If there are too many pending page faults, this will fail. Retry<o:p></o:p></pre> <pre>+ * interrupts should be ignored in this case until there is enough<o:p></o:p></pre> <pre>+ * free space.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * Returns 0 if the fault was added, 1 if the fault was already known,<o:p></o:p></pre> <pre>+ * -ENOSPC if there are too many pending faults.<o:p></o:p></pre> <pre>+ */<o:p></o:p></pre> <pre>+int amdgpu_vm_add_fault(struct amdgpu_retryfault_hashtable *fault_hash, u64 key)<o:p></o:p></pre> <pre>+{<o:p></o:p></pre> <pre>+ unsigned long flags;<o:p></o:p></pre> <pre>+ int r = -ENOSPC;<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ if (WARN_ON_ONCE(!fault_hash))<o:p></o:p></pre> <pre>+ /* Should be allocated in amdgpu_vm_init<o:p></o:p></pre> <pre>+ */<o:p></o:p></pre> <pre>+ return r;<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ spin_lock_irqsave(&fault_hash->lock, flags);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ /* Only let the hash table fill up to 50% for best performance */<o:p></o:p></pre> <pre>+ if (fault_hash->count >= (1 << (AMDGPU_PAGEFAULT_HASH_BITS-1)))<o:p></o:p></pre> <pre>+ goto unlock_out;<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ r = chash_table_copy_in(&fault_hash->hash, key, NULL);<o:p></o:p></pre> <pre>+ if (!r)<o:p></o:p></pre> <pre>+ fault_hash->count++;<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ /* chash_table_copy_in should never fail unless we're losing count */<o:p></o:p></pre> <pre>+ WARN_ON_ONCE(r < 0);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+unlock_out:<o:p></o:p></pre> <pre>+ spin_unlock_irqrestore(&fault_hash->lock, flags);<o:p></o:p></pre> <pre>+ return r;<o:p></o:p></pre> <pre>+}<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+/**<o:p></o:p></pre> <pre>+ * amdgpu_vm_clear_fault - Remove a page fault record<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * @fault_hash: fault hash table<o:p></o:p></pre> <pre>+ * @key: 64-bit encoding of PASID and address<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * This should be called when a page fault has been handled. Any<o:p></o:p></pre> <pre>+ * future interrupt with this key will be processed as a new<o:p></o:p></pre> <pre>+ * page fault.<o:p></o:p></pre> <pre>+ */<o:p></o:p></pre> <pre>+void amdgpu_vm_clear_fault(struct amdgpu_retryfault_hashtable *fault_hash, u64 key)<o:p></o:p></pre> <pre>+{<o:p></o:p></pre> <pre>+ unsigned long flags;<o:p></o:p></pre> <pre>+ int r;<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ if (!fault_hash)<o:p></o:p></pre> <pre>+ return;<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ spin_lock_irqsave(&fault_hash->lock, flags);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ r = chash_table_remove(&fault_hash->hash, key, NULL);<o:p></o:p></pre> <pre>+ if (!WARN_ON_ONCE(r < 0)) {<o:p></o:p></pre> <pre>+ fault_hash->count--;<o:p></o:p></pre> <pre>+ WARN_ON_ONCE(fault_hash->count < 0);<o:p></o:p></pre> <pre>+ }<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ spin_unlock_irqrestore(&fault_hash->lock, flags);<o:p></o:p></pre> <pre>+}<o:p></o:p></pre> <pre>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h<o:p></o:p></pre> <pre>index e275ee7..6eb1da1 100644<o:p></o:p></pre> <pre>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h<o:p></o:p></pre> <pre>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h<o:p></o:p></pre> <pre>@@ -178,6 +178,13 @@ struct amdgpu_task_info {<o:p></o:p></pre> <pre> pid_t tgid;<o:p></o:p></pre> <pre> };<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>+#define AMDGPU_PAGEFAULT_HASH_BITS 8<o:p></o:p></pre> <pre>+struct amdgpu_retryfault_hashtable {<o:p></o:p></pre> <pre>+ DECLARE_CHASH_TABLE(hash, AMDGPU_PAGEFAULT_HASH_BITS, 8, 0);<o:p></o:p></pre> <pre>+ spinlock_t lock;<o:p></o:p></pre> <pre>+ int count;<o:p></o:p></pre> <pre>+};<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre> struct amdgpu_vm {<o:p></o:p></pre> <pre> /* tree of virtual addresses mapped */<o:p></o:p></pre> <pre> struct rb_root_cached va;<o:p></o:p></pre> <pre>@@ -240,6 +247,7 @@ struct amdgpu_vm {<o:p></o:p></pre> <pre> struct ttm_lru_bulk_move lru_bulk_move;<o:p></o:p></pre> <pre> /* mark whether can do the bulk move */<o:p></o:p></pre> <pre> bool bulk_moveable;<o:p></o:p></pre> <pre>+ struct amdgpu_retryfault_hashtable *fault_hash;<o:p></o:p></pre> <pre> };<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> struct amdgpu_vm_manager {<o:p></o:p></pre> <pre>@@ -355,4 +363,8 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);<o:p></o:p></pre> <pre> void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,<o:p></o:p></pre> <pre> struct amdgpu_vm *vm);<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>+int amdgpu_vm_add_fault(struct amdgpu_retryfault_hashtable *fault_hash, u64 key);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+void amdgpu_vm_clear_fault(struct amdgpu_retryfault_hashtable *fault_hash, u64 key);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre> #endif<o:p></o:p></pre> <pre>diff --git a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c<o:p></o:p></pre> <pre>index 5ae5ed2..acbe5a7 100644<o:p></o:p></pre> <pre>--- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c<o:p></o:p></pre> <pre>+++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c<o:p></o:p></pre> <pre>@@ -265,35 +265,36 @@ static bool vega10_ih_prescreen_iv(struct amdgpu_device *adev)<o:p></o:p></pre> <pre> return true;<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>- addr = ((u64)(dw5 & 0xf) << 44) | ((u64)dw4 << 12);<o:p></o:p></pre> <pre>- key = AMDGPU_VM_FAULT(pasid, addr);<o:p></o:p></pre> <pre>- r = amdgpu_ih_add_fault(adev, key);<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre>- /* Hash table is full or the fault is already being processed,<o:p></o:p></pre> <pre>- * ignore further page faults<o:p></o:p></pre> <pre>- */<o:p></o:p></pre> <pre>- if (r != 0)<o:p></o:p></pre> <pre>- goto ignore_iv;<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre> /* Track retry faults in per-VM fault FIFO. */<o:p></o:p></pre> <pre> spin_lock(&adev->vm_manager.pasid_lock);<o:p></o:p></pre> <pre> vm = idr_find(&adev->vm_manager.pasid_idr, pasid);<o:p></o:p></pre> <pre>+ addr = ((u64)(dw5 & 0xf) << 44) | ((u64)dw4 << 12);<o:p></o:p></pre> <pre>+ key = AMDGPU_VM_FAULT(pasid, addr);<o:p></o:p></pre> <pre> if (!vm) {<o:p></o:p></pre> <pre> /* VM not found, process it normally */<o:p></o:p></pre> <pre> spin_unlock(&adev->vm_manager.pasid_lock);<o:p></o:p></pre> <pre>- amdgpu_ih_clear_fault(adev, key);<o:p></o:p></pre> <pre> return true;<o:p></o:p></pre> <pre>+ } else {<o:p></o:p></pre> <pre>+ r = amdgpu_vm_add_fault(vm->fault_hash, key);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ /* Hash table is full or the fault is already being processed,<o:p></o:p></pre> <pre>+ * ignore further page faults<o:p></o:p></pre> <pre>+ */<o:p></o:p></pre> <pre>+ if (r != 0) {<o:p></o:p></pre> <pre>+ spin_unlock(&adev->vm_manager.pasid_lock);<o:p></o:p></pre> <pre>+ goto ignore_iv;<o:p></o:p></pre> <pre>+ }<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre> /* No locking required with single writer and single reader */<o:p></o:p></pre> <pre> r = kfifo_put(&vm->faults, key);<o:p></o:p></pre> <pre> if (!r) {<o:p></o:p></pre> <pre> /* FIFO is full. Ignore it until there is space */<o:p></o:p></pre> <pre>+ amdgpu_vm_clear_fault(vm->fault_hash, key);<o:p></o:p></pre> <pre> spin_unlock(&adev->vm_manager.pasid_lock);<o:p></o:p></pre> <pre>- amdgpu_ih_clear_fault(adev, key);<o:p></o:p></pre> <pre> goto ignore_iv;<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre>- spin_unlock(&adev->vm_manager.pasid_lock);<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>+ spin_unlock(&adev->vm_manager.pasid_lock);<o:p></o:p></pre> <pre> /* It's the first fault for this address, process it normally */<o:p></o:p></pre> <pre> return true;<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>@@ -386,14 +387,6 @@ static int vega10_ih_sw_init(void *handle)<o:p></o:p></pre> <pre> adev->irq.ih.use_doorbell = true;<o:p></o:p></pre> <pre> adev->irq.ih.doorbell_index = AMDGPU_DOORBELL64_IH << 1;<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>- adev->irq.ih.faults = kmalloc(sizeof(*adev->irq.ih.faults), GFP_KERNEL);<o:p></o:p></pre> <pre>- if (!adev->irq.ih.faults)<o:p></o:p></pre> <pre>- return -ENOMEM;<o:p></o:p></pre> <pre>- INIT_CHASH_TABLE(adev->irq.ih.faults->hash,<o:p></o:p></pre> <pre>- AMDGPU_PAGEFAULT_HASH_BITS, 8, 0);<o:p></o:p></pre> <pre>- spin_lock_init(&adev->irq.ih.faults->lock);<o:p></o:p></pre> <pre>- adev->irq.ih.faults->count = 0;<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre> r = amdgpu_irq_init(adev);<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> return r;<o:p></o:p></pre> <pre>@@ -406,9 +399,6 @@ static int vega10_ih_sw_fini(void *handle)<o:p></o:p></pre> <pre> amdgpu_irq_fini(adev);<o:p></o:p></pre> <pre> amdgpu_ih_ring_fini(adev);<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>- kfree(adev->irq.ih.faults);<o:p></o:p></pre> <pre>- adev->irq.ih.faults = NULL;<o:p></o:p></pre> <pre>-<o:p></o:p></pre> <pre> return 0;<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre> <o:p></o:p></pre> </blockquote> <o:p></o:p> <pre>_______________________________________________<o:p></o:p></pre> <pre>amd-gfx mailing list<o:p></o:p></pre> <pre><a href="mailto:amd-gfx@lists.freedesktop.org" moz-do-not-send="true">amd-gfx@lists.freedesktop.org</a><o:p></o:p></pre> <pre><a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" moz-do-not-send="true">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><o:p></o:p></pre> </blockquote> <o:p></o:p> </blockquote> <o:p></o:p> </blockquote> <o:p> </o:p> </div> </blockquote> </body> </html>