<div dir="ltr"><div dir="ltr">On Wed, 29 Jul 2020 at 06:33, Ruhl, Michael J <<a href="mailto:michael.j.ruhl@intel.com">michael.j.ruhl@intel.com</a>> wrote: </div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">>-----Original Message----- >From: Ben Skeggs <<a href="mailto:skeggsb@gmail.com" target="_blank">skeggsb@gmail.com</a>> >Sent: Tuesday, July 28, 2020 4:16 PM >To: Ruhl, Michael J <<a href="mailto:michael.j.ruhl@intel.com" target="_blank">michael.j.ruhl@intel.com</a>> >Cc: Dave Airlie <<a href="mailto:airlied@gmail.com" target="_blank">airlied@gmail.com</a>>; <a href="mailto:dri-devel@lists.freedesktop.org" target="_blank">dri-devel@lists.freedesktop.org</a>; ><a href="mailto:bskeggs@redhat.com" target="_blank">bskeggs@redhat.com</a> >Subject: Re: [PATCH] nouveau: use ttm populate mapping functions. (v2) > >On Wed, 29 Jul 2020 at 02:08, Ruhl, Michael J <<a href="mailto:michael.j.ruhl@intel.com" target="_blank">michael.j.ruhl@intel.com</a>> >wrote: >> >> >-----Original Message----- >> >From: dri-devel <<a href="mailto:dri-devel-bounces@lists.freedesktop.org" target="_blank">dri-devel-bounces@lists.freedesktop.org</a>> On Behalf Of >> >Dave Airlie >> >Sent: Monday, July 27, 2020 11:26 PM >> >To: <a href="mailto:dri-devel@lists.freedesktop.org" target="_blank">dri-devel@lists.freedesktop.org</a> >> >Cc: <a href="mailto:bskeggs@redhat.com" target="_blank">bskeggs@redhat.com</a> >> >Subject: [PATCH] nouveau: use ttm populate mapping functions. (v2) >> > >> >From: Dave Airlie <<a href="mailto:airlied@redhat.com" target="_blank">airlied@redhat.com</a>> >> > >> >Instead of rolling driver copies of them. >> > >> >v2: cleanup return handling (Ben) >> >Signed-off-by: Dave Airlie <<a href="mailto:airlied@redhat.com" target="_blank">airlied@redhat.com</a>> >> >--- >> > 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 <<a href="mailto:michael.j.ruhl@intel.com" target="_blank">michael.j.ruhl@intel.com</a>> </blockquote><div>Thanks for the review! I've got the patch in my tree.</div><div> </div><div>Ben.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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 >> ><a href="mailto:dri-devel@lists.freedesktop.org" target="_blank">dri-devel@lists.freedesktop.org</a> >> ><a href="https://lists.freedesktop.org/mailman/listinfo/dri-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/dri-devel</a> >> _______________________________________________ >> dri-devel mailing list >> <a href="mailto:dri-devel@lists.freedesktop.org" target="_blank">dri-devel@lists.freedesktop.org</a> >> <a href="https://lists.freedesktop.org/mailman/listinfo/dri-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/dri-devel</a> </blockquote></div></div>