<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<div class="moz-cite-prefix">Thanks again for the patch. I'm going
to apply this with some minor fixes. The headline should start
with "drm/amdgpu:". I'll also change the wording of the headline
and commit message:</div>
<blockquote>
<div class="moz-cite-prefix">drm/amdgpu: shrink critical section
in amdgpu_amdkfd_gpuvm_free_memory_of_gpu</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Reduce the mem->lock`s protected
code area, no need to protect pr_debug.</div>
<div class="moz-cite-prefix">This also simplifies error handling.</div>
</blockquote>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">There is one more cosmetic change I'm
going to make, see inline. I'll apply your patch with those
updates if you're OK with that.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">On 2020-04-21 2:48, Bernard Zhao wrote:<br>
</div>
<blockquote type="cite" cite="mid:20200421064818.129158-1-bernard@vivo.com">
<pre class="moz-quote-pre" wrap="">Maybe we could reduce the mutex_lock(&mem->lock)`s protected code area,
and no need to protect pr_debug.
Signed-off-by: Bernard Zhao <a class="moz-txt-link-rfc2396E" href="mailto:bernard@vivo.com"><bernard@vivo.com></a>
Changes since V1:
*commit message improve
Changes since V2:
*move comment along with the mutex_unlock
Link for V1:
*<a class="moz-txt-link-freetext" href="https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1226588%2F&data=02%7C01%7CFelix.Kuehling%40amd.com%7Cc715f5d02aae40846b8f08d7e5c001fd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637230485152789682&sdata=4UdZiWMAbW8eR1BS2%2F6qMvs5K6cHWy5c8I32ReQ4uz0%3D&reserved=0">https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1226588%2F&data=02%7C01%7CFelix.Kuehling%40amd.com%7Cc715f5d02aae40846b8f08d7e5c001fd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637230485152789682&sdata=4UdZiWMAbW8eR1BS2%2F6qMvs5K6cHWy5c8I32ReQ4uz0%3D&reserved=0</a>
Link for V2:
*<a class="moz-txt-link-freetext" href="https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1227907%2F&data=02%7C01%7CFelix.Kuehling%40amd.com%7Cc715f5d02aae40846b8f08d7e5c001fd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637230485152799673&sdata=Wt%2Bk7k4MtSX4zIDgmLEOB6ZKzfuqAd6GJZ3Creqf1aY%3D&reserved=0">https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1227907%2F&data=02%7C01%7CFelix.Kuehling%40amd.com%7Cc715f5d02aae40846b8f08d7e5c001fd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637230485152799673&sdata=Wt%2Bk7k4MtSX4zIDgmLEOB6ZKzfuqAd6GJZ3Creqf1aY%3D&reserved=0</a>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 327317c54f7c..f03d9843d723 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1285,21 +1285,21 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
struct bo_vm_reservation_context ctx;
struct ttm_validate_buffer *bo_list_entry;
int ret;
+ unsigned int mapped_to_gpu_memory;</pre>
</blockquote>
<p>I'll move this before the "int ret;" line. It makes the code more
readable if long declarations come before short ones.</p>
<p>Regards,<br>
Felix<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:20200421064818.129158-1-bernard@vivo.com">
<pre class="moz-quote-pre" wrap="">
mutex_lock(&mem->lock);
+ mapped_to_gpu_memory = mem->mapped_to_gpu_memory;
+ mutex_unlock(&mem->lock);
+ /* lock is not needed after this, since mem is unused and will
+ * be freed anyway
+ */
- if (mem->mapped_to_gpu_memory > 0) {
+ if (mapped_to_gpu_memory > 0) {
pr_debug("BO VA 0x%llx size 0x%lx is still mapped.\n",
mem->va, bo_size);
- mutex_unlock(&mem->lock);
return -EBUSY;
}
- mutex_unlock(&mem->lock);
- /* lock is not needed after this, since mem is unused and will
- * be freed anyway
- */
-
/* No more MMU notifiers */
amdgpu_mn_unregister(mem->bo);
</pre>
</blockquote>
<blockquote type="cite" cite="mid:20200421064818.129158-1-bernard@vivo.com">
</blockquote>
</body>
</html>