Am 10.06.20 um 15:54 schrieb Andrey Grodzovsky:
On 6/10/20 6:15 AM, Thomas Hellström (Intel) wrote:
On 6/9/20 7:21 PM, Koenig, Christian wrote:
Am 09.06.2020 18:37 schrieb "Grodzovsky, Andrey"
<andrey.grodzov...@amd.com>:
On 6/5/20 2:40 PM, Christian König wrote:
> Am 05.06.20 um 16:29 schrieb Andrey Grodzovsky:
>>
>> On 5/11/20 2:45 AM, Christian König wrote:
>>> Am 09.05.20 um 20:51 schrieb Andrey Grodzovsky:
>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzov...@amd.com>
>>>> ---
>>>> drivers/gpu/drm/ttm/ttm_bo.c | 22 +++++++++++++++++++++-
>>>> include/drm/ttm/ttm_bo_driver.h | 2 ++
>>>> 2 files changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
>>>> b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> index c5b516f..eae61cc 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> @@ -1750,9 +1750,29 @@ void ttm_bo_unmap_virtual(struct
>>>> ttm_buffer_object *bo)
>>>> ttm_bo_unmap_virtual_locked(bo);
>>>> ttm_mem_io_unlock(man);
>>>> }
>>>> +EXPORT_SYMBOL(ttm_bo_unmap_virtual);
>>>> +void ttm_bo_unmap_virtual_address_space(struct
ttm_bo_device *bdev)
>>>> +{
>>>> + struct ttm_mem_type_manager *man;
>>>> + int i;
>>>> -EXPORT_SYMBOL(ttm_bo_unmap_virtual);
>>>
>>>> + for (i = 0; i < TTM_NUM_MEM_TYPES; i++) {
>>>> + man = &bdev->man[i];
>>>> + if (man->has_type && man->use_type)
>>>> + ttm_mem_io_lock(man, false);
>>>> + }
>>>
>>> You should drop that it will just result in a deadlock
warning for
>>> Nouveau and has no effect at all.
>>>
>>> Apart from that looks good to me,
>>> Christian.
>>
>>
>> As I am considering to re-include this in V2 of the
patchsets, can
>> you clarify please why this will have no effect at all ?
>
> The locks are exclusive for Nouveau to allocate/free the io
address
> space.
>
> Since we don't do this here we don't need the locks.
>
> Christian.
So basically calling unmap_mapping_range doesn't require any extra
locking around it and whatever locks are taken within the function
should be enough ?
I think so, yes.
Christian.
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.
Can you explain more on this - specifically, which function to reserve
the BO, why BO reservation would prevent re-fault of the PTE ?
Thomas is talking about ttm_bo_reserver()/ttm_bo_unreserve(), but we
don't need this because we unmap everything because the whole device is
gone and not just manipulate a single BO.
So the device removed flag needs to be advertized before this
function is run,
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...
(perhaps with a memory barrier pair).
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
As far as I can see that should be perfectly sufficient.
Christian.
Andrey
That should probably be added to the function documentation.
(Other than that, please add a commit message if respinning).
/Thomas
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
