[PATCH 1/6] drm/ttm: Add unampping of the entire device address space

[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.Grodzovsky at 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.grodzovsky at 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 ?


> 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


Andrey


> That should probably be added to the function documentation.
>
> (Other than that, please add a commit message if respinning).
>
> /Thomas
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20200610/6a6b97f0/attachment.htm>