<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 2021-04-21 3:22 a.m., Christian König wrote: </div> <blockquote type="cite" cite="mid:6d4d7698-381a-f1d7-2eed-b71047ddc70d@gmail.com">Am 20.04.21 um 22:21 schrieb Philip Yang: <blockquote type="cite">Add interface to remove address from fault filter ring by resetting fault ring entry of the fault address timestamp to 0, then future vm fault on the address will be processed to recover. Check fault address from fault ring, add address into fault ring and remove address from fault ring are serialized in same interrupt deferred work, don't have race condition. </blockquote> That might not work on Vega20. We call amdgpu_gmc_filter_faults() from the the IH while the fault handling id done from the delegated IH processing. </blockquote> Added spinlock for VG20. <blockquote type="cite" cite="mid:6d4d7698-381a-f1d7-2eed-b71047ddc70d@gmail.com"> More comments below. <blockquote type="cite">Signed-off-by: Philip Yang <a class="moz-txt-link-rfc2396E" href="mailto:Philip.Yang@amd.com"><Philip.Yang@amd.com></a> --- drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 24 ++++++++++++++++++++++++ drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h | 2 ++ 2 files changed, 26 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c index c39ed9eb0987..338e45fa66cb 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c @@ -387,6 +387,30 @@ bool amdgpu_gmc_filter_faults(struct amdgpu_device *adev, uint64_t addr, return false; } +/** + * amdgpu_gmc_filter_faults_remove - remove address from VM faults filter + * + * @adev: amdgpu device structure + * @addr: address of the VM fault + * @pasid: PASID of the process causing the fault + * + * Remove the address from fault filter, then future vm fault on this address + * will pass to retry fault handler to recover. + */ +void amdgpu_gmc_filter_faults_remove(struct amdgpu_device *adev, uint64_t addr, + uint16_t pasid) +{ + struct amdgpu_gmc *gmc = &adev->gmc; + + uint64_t key = addr << 4 | pasid; </blockquote> We should probably have a function for this now. </blockquote> add function fault_key in v2. <blockquote type="cite" cite="mid:6d4d7698-381a-f1d7-2eed-b71047ddc70d@gmail.com"> <blockquote type="cite">+ struct amdgpu_gmc_fault *fault; + uint32_t hash; + + hash = hash_64(key, AMDGPU_GMC_FAULT_HASH_ORDER); + fault = &gmc->fault_ring[gmc->fault_hash[hash].idx]; + fault->timestamp = 0; </blockquote> There is no guarantee that the ring entry you found for the fault is the one for this address. After all that is just an 8 bit hash for a 64bit values :) You need to double check the key and walk the chain by looking at the next entry to eventually find the right one. </blockquote> I am not completely understand how fault->next and gmc->last_fault works, as it keep increasing. Please help review patch v2. Thanks, Philip <blockquote type="cite" cite="mid:6d4d7698-381a-f1d7-2eed-b71047ddc70d@gmail.com">Christian. <blockquote type="cite">+} + int amdgpu_gmc_ras_late_init(struct amdgpu_device *adev) { int r; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h index 9d11c02a3938..498a7a0d5a9e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h @@ -318,6 +318,8 @@ void amdgpu_gmc_agp_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc); bool amdgpu_gmc_filter_faults(struct amdgpu_device *adev, uint64_t addr, uint16_t pasid, uint64_t timestamp); +void amdgpu_gmc_filter_faults_remove(struct amdgpu_device *adev, uint64_t addr, + uint16_t pasid); int amdgpu_gmc_ras_late_init(struct amdgpu_device *adev); void amdgpu_gmc_ras_fini(struct amdgpu_device *adev); int amdgpu_gmc_allocate_vm_inv_eng(struct amdgpu_device *adev); </blockquote> </blockquote> </body> </html>