[PATCH] nouveau: use ttm populate mapping functions. (v2)

[Ben Skeggs]

On Wed, 29 Jul 2020 at 06:33, Ruhl, Michael J <michael.j.ruhl at intel.com>
wrote:

> >-----Original Message-----
> >From: Ben Skeggs <skeggsb at gmail.com>
> >Sent: Tuesday, July 28, 2020 4:16 PM
> >To: Ruhl, Michael J <michael.j.ruhl at intel.com>
> >Cc: Dave Airlie <airlied at gmail.com>; dri-devel at lists.freedesktop.org;
> >bskeggs at redhat.com
> >Subject: Re: [PATCH] nouveau: use ttm populate mapping functions. (v2)
> >
> >On Wed, 29 Jul 2020 at 02:08, Ruhl, Michael J <michael.j.ruhl at intel.com>
> >wrote:
> >>
> >> >-----Original Message-----
> >> >From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf Of
> >> >Dave Airlie
> >> >Sent: Monday, July 27, 2020 11:26 PM
> >> >To: dri-devel at lists.freedesktop.org
> >> >Cc: bskeggs at redhat.com
> >> >Subject: [PATCH] nouveau: use ttm populate mapping functions. (v2)
> >> >
> >> >From: Dave Airlie <airlied at redhat.com>
> >> >
> >> >Instead of rolling driver copies of them.
> >> >
> >> >v2: cleanup return handling (Ben)
> >> >Signed-off-by: Dave Airlie <airlied at redhat.com>
> >> >---
> >> > drivers/gpu/drm/nouveau/nouveau_bo.c | 38 ++--------------------------
> >> > 1 file changed, 2 insertions(+), 36 deletions(-)
> >> >
> >> >diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
> >> >b/drivers/gpu/drm/nouveau/nouveau_bo.c
> >> >index 7806278dce57..6ef5085c9a91 100644
> >> >--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> >> >+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> >> >@@ -1231,8 +1231,6 @@ nouveau_ttm_tt_populate(struct ttm_tt *ttm,
> >> >struct ttm_operation_ctx *ctx)
> >> >       struct ttm_dma_tt *ttm_dma = (void *)ttm;
> >> >       struct nouveau_drm *drm;
> >> >       struct device *dev;
> >> >-      unsigned i;
> >> >-      int r;
> >> >       bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
> >> >
> >> >       if (ttm->state != tt_unpopulated)
> >> >@@ -1260,31 +1258,7 @@ nouveau_ttm_tt_populate(struct ttm_tt *ttm,
> >> >struct ttm_operation_ctx *ctx)
> >> >               return ttm_dma_populate((void *)ttm, dev, ctx);
> >> >       }
> >> > #endif
> >> >-
> >> >-      r = ttm_pool_populate(ttm, ctx);
> >> >-      if (r) {
> >> >-              return r;
> >> >-      }
> >> >-
> >> >-      for (i = 0; i < ttm->num_pages; i++) {
> >> >-              dma_addr_t addr;
> >> >-
> >> >-              addr = dma_map_page(dev, ttm->pages[i], 0, PAGE_SIZE,
> >> >-                                  DMA_BIDIRECTIONAL);
> >> >-
> >> >-              if (dma_mapping_error(dev, addr)) {
> >> >-                      while (i--) {
> >> >-                              dma_unmap_page(dev, ttm_dma-
> >> >>dma_address[i],
> >> >-                                             PAGE_SIZE,
> >> >DMA_BIDIRECTIONAL);
> >> >-                              ttm_dma->dma_address[i] = 0;
> >> >-                      }
> >> >-                      ttm_pool_unpopulate(ttm);
> >> >-                      return -EFAULT;
> >> >-              }
> >> >-
> >> >-              ttm_dma->dma_address[i] = addr;
> >> >-      }
> >> >-      return 0;
> >> >+      return ttm_populate_and_map_pages(dev, ttm_dma, ctx);
> >>
> >> This is not a completely straight code replacement.
> >>
> >> ttm_populate_and_map_pages() also has code to deal with pages that are
> >> contiguous (consolidates them).
> >>
> >> Is it possible that the nouveau HW can't handle a contiguous buffer
> larger
> >> than PAG_SIZE?
> >I think it's fine.  The function appears to consolidate the pages for
> >the dma_map_page() call, but otherwise leave dma_address[] in
> >PAGE_SIZE chunks, I don't believe anything else in the driver will
> >care.
>
> Ahh..  I misread it.   This is limiting the calls to dma_map_page().
> Instead
> of calling it for each page, just do it for the first one...
>
> Thanks for setting me straight. 😊
>
> Reviewed-by: Michael J. Ruhl <michael.j.ruhl at intel.com>
>
Thanks for the review!  I've got the patch in my tree.

Ben.


>
> Mike
>
>
> >Ben.
> >
> >>
> >> Thanks,
> >>
> >> Mike
> >>
> >> > }
> >> >
> >> > static void
> >> >@@ -1293,7 +1267,6 @@ nouveau_ttm_tt_unpopulate(struct ttm_tt *ttm)
> >> >       struct ttm_dma_tt *ttm_dma = (void *)ttm;
> >> >       struct nouveau_drm *drm;
> >> >       struct device *dev;
> >> >-      unsigned i;
> >> >       bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
> >> >
> >> >       if (slave)
> >> >@@ -1316,14 +1289,7 @@ nouveau_ttm_tt_unpopulate(struct ttm_tt
> >*ttm)
> >> >       }
> >> > #endif
> >> >
> >> >-      for (i = 0; i < ttm->num_pages; i++) {
> >> >-              if (ttm_dma->dma_address[i]) {
> >> >-                      dma_unmap_page(dev, ttm_dma->dma_address[i],
> >> >PAGE_SIZE,
> >> >-                                     DMA_BIDIRECTIONAL);
> >> >-              }
> >> >-      }
> >> >-
> >> >-      ttm_pool_unpopulate(ttm);
> >> >+      ttm_unmap_and_unpopulate_pages(dev, ttm_dma);
> >> > }
> >> >
> >> > void
> >> >--
> >> >2.26.2
> >> >
> >> >_______________________________________________
> >> >dri-devel mailing list
> >> >dri-devel at lists.freedesktop.org
> >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200729/fc9d10cf/attachment-0001.htm>