<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p><br>
</p>
<div class="moz-cite-prefix">On 6/10/20 6:15 AM, Thomas Hellström
(Intel) wrote:<br>
</div>
<blockquote type="cite" cite="mid:eb9e5896-1f16-2102-350a-1e64d9af7ea8@shipmail.org">
<p><br>
</p>
<div class="moz-cite-prefix">On 6/9/20 7:21 PM, Koenig, Christian
wrote:<br>
</div>
<blockquote type="cite" cite="mid:f36c1fa1-bbee-477a-9cb2-ed2726f27eef@email.android.com">
<div dir="auto">
<div><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">Am 09.06.2020 18:37 schrieb
"Grodzovsky, Andrey" <a class="moz-txt-link-rfc2396E" href="mailto:Andrey.Grodzovsky@amd.com" moz-do-not-send="true"><Andrey.Grodzovsky@amd.com></a>:<br type="attribution">
<blockquote class="quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><font size="2"><span style="font-size:11pt">
<div><br>
On 6/5/20 2:40 PM, Christian König wrote:<br>
> Am 05.06.20 um 16:29 schrieb Andrey
Grodzovsky:<br>
>><br>
>> On 5/11/20 2:45 AM, Christian König
wrote:<br>
>>> Am 09.05.20 um 20:51 schrieb
Andrey Grodzovsky:<br>
>>>> Signed-off-by: Andrey
Grodzovsky <a class="moz-txt-link-rfc2396E" href="mailto:andrey.grodzovsky@amd.com" moz-do-not-send="true"><andrey.grodzovsky@amd.com></a><br>
>>>> ---<br>
>>>>
drivers/gpu/drm/ttm/ttm_bo.c | 22
+++++++++++++++++++++-<br>
>>>>
include/drm/ttm/ttm_bo_driver.h | 2 ++<br>
>>>> 2 files changed, 23
insertions(+), 1 deletion(-)<br>
>>>><br>
>>>> diff --git
a/drivers/gpu/drm/ttm/ttm_bo.c <br>
>>>>
b/drivers/gpu/drm/ttm/ttm_bo.c<br>
>>>> index c5b516f..eae61cc 100644<br>
>>>> ---
a/drivers/gpu/drm/ttm/ttm_bo.c<br>
>>>> +++
b/drivers/gpu/drm/ttm/ttm_bo.c<br>
>>>> @@ -1750,9 +1750,29 @@ void
ttm_bo_unmap_virtual(struct <br>
>>>> ttm_buffer_object *bo)<br>
>>>>
ttm_bo_unmap_virtual_locked(bo);<br>
>>>> ttm_mem_io_unlock(man);<br>
>>>> }<br>
>>>>
+EXPORT_SYMBOL(ttm_bo_unmap_virtual);<br>
>>>> +void
ttm_bo_unmap_virtual_address_space(struct
ttm_bo_device *bdev)<br>
>>>> +{<!-- --><br>
>>>> + struct
ttm_mem_type_manager *man;<br>
>>>> + int i;<br>
>>>>
-EXPORT_SYMBOL(ttm_bo_unmap_virtual);<br>
>>><br>
>>>> + for (i = 0; i <
TTM_NUM_MEM_TYPES; i++) {<!-- --><br>
>>>> + man =
&bdev->man[i];<br>
>>>> + if (man->has_type
&& man->use_type)<br>
>>>> +
ttm_mem_io_lock(man, false);<br>
>>>> + }<br>
>>><br>
>>> You should drop that it will just
result in a deadlock warning for <br>
>>> Nouveau and has no effect at all.<br>
>>><br>
>>> Apart from that looks good to me,<br>
>>> Christian.<br>
>><br>
>><br>
>> As I am considering to re-include
this in V2 of the patchsets, can <br>
>> you clarify please why this will have
no effect at all ?<br>
><br>
> The locks are exclusive for Nouveau to
allocate/free the io address <br>
> space.<br>
><br>
> Since we don't do this here we don't need
the locks.<br>
><br>
> Christian.<br>
<br>
<br>
So basically calling unmap_mapping_range
doesn't require any extra <br>
locking around it and whatever locks are taken
within the function <br>
should be enough ?<br>
</div>
</span></font></div>
</blockquote>
</div>
</div>
</div>
<div dir="auto"><br>
</div>
<div dir="auto">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><font size="2"><span style="font-size:11pt"> </span></font></div>
</blockquote>
</div>
</div>
</div>
<div dir="auto"><br>
</div>
<div dir="auto">I think so, yes.</div>
<div dir="auto"><br>
</div>
<div dir="auto">Christian.</div>
</div>
</blockquote>
<p>Yes, that's true. However, without the bo reservation, nothing
stops a PTE from being immediately re-faulted back again. Even
while unmap_mapping_range() is running. </p>
</blockquote>
<p><br>
</p>
<p>Can you explain more on this - specifically, which function to
reserve the BO, why BO reservation would prevent re-fault of the
PTE ?</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:eb9e5896-1f16-2102-350a-1e64d9af7ea8@shipmail.org">
<p>So the device removed flag needs to be advertized before this
function is run,</p>
</blockquote>
<p><br>
</p>
<p>I indeed intend to call this right after calling drm_dev_unplug
from amdgpu_pci_remove while adding drm_dev_enter/exit in
ttm_bo_vm_fault (or in amdgpu specific wrapper since I don't see
how can I access struct drm_device from ttm_bo_vm_fault) and this
in my understanding should stop a PTE from being re-faulted back
as you pointed out - so again I don't see how bo reservation
would prevent it so it looks like I am missing something...</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:eb9e5896-1f16-2102-350a-1e64d9af7ea8@shipmail.org">
<p> (perhaps with a memory barrier pair). </p>
</blockquote>
<p><br>
</p>
<p> drm_dev_unplug and drm_dev_enter/exit are RCU synchronized and
so I don't think require any extra memory barriers for visibility
of the removed flag being set<br>
</p>
<p><br>
</p>
<p>Andrey</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:eb9e5896-1f16-2102-350a-1e64d9af7ea8@shipmail.org">
<p>That should probably be added to the function documentation. <br>
</p>
<p>(Other than that, please add a commit message if respinning).</p>
<p>/Thomas</p>
<p><br>
</p>
<p><br>
</p>
</blockquote>
</body>
</html>