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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 11, 2020 at 08:12:37AM +0200, Thomas Hellström (Intel) wrote:
> 
> On 6/10/20 11:16 PM, Daniel Vetter wrote:
> > On Wed, Jun 10, 2020 at 10:30 PM Thomas Hellström (Intel)
> > <thomas_os@shipmail.org> wrote:
> > > 
> > > On 6/10/20 5:30 PM, Daniel Vetter wrote:
> > > > On Wed, Jun 10, 2020 at 04:05:04PM +0200, Christian König wrote:
> > > > > 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.
> > > > Only if you have a drm_dev_enter/exit pair in your fault handler.
> > > > Otherwise you're still open to the races Thomas described. But aside from
> > > > that the drm_dev_unplug stuff has all the barriers and stuff to make sure
> > > > nothing escapes.
> > > > 
> > > > Failure to drm_dev_enter could then also trigger the special case where we
> > > > put a dummy page in place.
> > > > -Daniel
> > > Hmm, Yes, indeed advertizing the flag before the call to
> > > unmap_mapping_range isn't enough, since there might be fault handlers
> > > running that haven't picked up the flag when unmap_mapping_range is
> > > launched.
> > Hm ... Now I'm not sure drm_dev_enter/exit is actually good enough. I
> > guess if you use vmf_insert_pfn within the drm_dev_enter/exit critical
> > section, it should be fine. But I think you can also do fault handlers
> > that just return the struct page and then let core handle the pte
> > wrangling, those would indeed race and we can't have that I think.
> 
> For the TTM drivers, having a fault handler that defers the pte insertion to
> the core would break also the bo synchronization so I don't think that will
> ever happen. To make sure we could perhaps add a return value warning at the
> end of the fault handler with a comment explaining why this is a bad idea.

Yeah good thing at least is that vram drivers all use ttm thus far, so
that worry is handled.

And for usb/spi and other panels/ports connected over some bus that can't
do mmio, the mmaps all point at system memory. So we don't have that
problem there.
-Daniel

> 
> > 
> > I think we should try and make sure (as much as possible) that this is
> > done all done in helpers and not some open coded stuff in drivers, or
> > we'll just get it all wrong in the details.
> 
> If doable, considering all the various fault handlers we have in DRM, I
> agree.
> 
> /Thomas
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx