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

["Christian König" <christian.koenig@amd.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4178 bytes --]

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.Grodzovsky@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@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
>>
>>
>>


[-- Attachment #1.2: Type: text/html, Size: 10609 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx