[PATCH 1/4] drm/radeon: stop using pages with drm_prime_sg_to_page_addr_arrays

[Daniel Vetter]

On Thu, Nov 5, 2020 at 1:12 PM Christian König
<ckoenig.leichtzumerken at gmail.com> 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 at amd.com>
> > 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(&gtt->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;
> >         }
>
>
> > 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 at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch