On Thu, Nov 5, 2020 at 2:01 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> On Thu, Nov 5, 2020 at 1:12 PM Christian König
> <ckoenig.leichtzumerken@xxxxxxxxx> wrote:
> >
> > Am 04.11.20 um 18:38 schrieb Daniel Vetter:
> > > On Wed, Nov 04, 2020 at 02:00:21PM +0100, Christian König wrote:
> > >> This is deprecated.
> > >>
> > >> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > > So I tried to prove to myself that ttm doesn't access ->pages for these
> > > cases, and kinda couldn't. We still seem to allocate the pages array and
> > > all that, and there's lots of code using ->pages all over. And between
> > > ttm_bo_type_sg and TTM_PAGE_FLAG_SG I didn't manage to chase a whole lot
> > > of paths to their full conclusion.
> >
> > Nope, see the amdgpu code:
> >
> > > if (ttm_sg_tt_init(>t->ttm, bo, page_flags, caching)) {
> >
> > And then what ttm_sg_tt_init() does:
> >
> > > if (page_flags & TTM_PAGE_FLAG_SG)
> > > ret = ttm_sg_tt_alloc_page_directory(ttm);
> > > else
> > > ret = ttm_dma_tt_alloc_page_directory(ttm);
> >
> > And then finally what ttm_sg_tt_alloc_page_directory() does:
> >
> > ttm->dma_address = kvmalloc_array(ttm->num_pages,....
> >
> > ttm->pages should be NULL in this case if I'm not completely mistaken.
> >
> > For or imported DMA-buf s we shouldn't have a ttm->pages in amdgpu for
> > quite a while and that works perfectly fine.
> >
> >
> >
> >
> > > So I reduced my ambitions and wanted to prove that at least for dma-buf
> > > imports aka ttm_bo_type_sg, we're guaranteed that we don't try to mmap
> > > these to userspace. And also failed to find that check.
> >
> > See ttm_bo_vm_reserve():
> > > /*
> > > * Refuse to fault imported pages. This should be handled
> > > * (if at all) by redirecting mmap to the exporter.
> > > */
> > > if (bo->ttm && (bo->ttm->page_flags & TTM_PAGE_FLAG_SG)) {
> > > dma_resv_unlock(bo->base.resv);
> > > return VM_FAULT_SIGBUS;
> > > }
E.g. I think if this here would check for gem_bo->import_attach
instead, that would be a ton clearer. And we can do that nowadays I
think, since vmwgfx doesn't import foreign dma_buf, ever.
-Daniel
> > > btw this is across all drivers, mostly ttm code, not radeon specific.
>
> Ok, this makes sense, and you can have my r-b for the amdgpu patch.
>
> > Well instead of those patches we could as well switch over radeon and
> > nouveau to using ttm_sg_tt_init() as well (or merge ttm_sg_tt_init()
> > into ttm_dma_tt_init).
> >
> > That's probably the much cleaner approach, but when I wrote
> > ttm_sg_tt_init() I wasn't sure if radeon/nouveau where using ttm->pages
> > for something, e.g. basically the same concern you have.
>
> I guess that might be useful as an intermediate step. Still a pile of
> work to review drivers, but at least the ttm paths work all the same.
>
> One thing that's somewhat annoying here is that ttm_bo_type_sg
> conflates how the backing memory is stored (it's in an sg list) with
> where it's from (it also means "this is a dma-buf, don't look at the
> cpu side, don't mmap it"). That's kinda awkward, just in case you have
> a driver that might want to use sg lists for everything to not
> duplicate code - ttm_tt otoh seems to be the "everything goes"
> combinatorial chaos. But that's an entirely unrelated rant :-)
>
> Then there's also the issue that dma-buf import goes through a
> bazillion layers with lots of driver duplication, which isn't helping
> in reviewing to make sure they really all end up with ttm_bo_type_sg.
> But I think that's the case from what I can see after lots of
> grepping. Maybe ttm_bo_init should have a WARN_ON if there's an sg but
> type isn't ttm_bo_type_sg.
> -Daniel
>
> >
> > Regards,
> > Christian.
> >
> > > So conclusion, still a mess here that at least I can't see throug clearly
> > > :-/ here = ttm_tt and the entire backing storage handling and everything
> > > that ties into it. Probably the area that still has the most midlayer feel
> > > to ttm with all the refactoring in-flight in still.
> > >
> > > tldr; tried to review patches 1-3, gave up.
> > >
> > > Cheers, Daniel
> > >
> > >> ---
> > >> drivers/gpu/drm/radeon/radeon_ttm.c | 9 +++++----
> > >> 1 file changed, 5 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> > >> index 95038ac3382e..f41fcee35f70 100644
> > >> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> > >> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> > >> @@ -494,8 +494,8 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_bo_device *bdev, struct ttm_tt *
> > >> if (r)
> > >> goto release_sg;
> > >>
> > >> - drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
> > >> - gtt->ttm.dma_address, ttm->num_pages);
> > >> + drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL, gtt->ttm.dma_address,
> > >> + ttm->num_pages);
> > >>
> > >> return 0;
> > >>
> > >> @@ -673,8 +673,9 @@ static int radeon_ttm_tt_populate(struct ttm_bo_device *bdev,
> > >> }
> > >>
> > >> if (slave && ttm->sg) {
> > >> - drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
> > >> - gtt->ttm.dma_address, ttm->num_pages);
> > >> + drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL,
> > >> + gtt->ttm.dma_address,
> > >> + ttm->num_pages);
> > >> return 0;
> > >> }
> > >>
> > >> --
> > >> 2.25.1
> > >>
> > >> _______________________________________________
> > >> dri-devel mailing list
> > >> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
