Message ID | 20200728032545.19878-1-airlied@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | nouveau: use ttm populate mapping functions. (v2) | expand |
>-----Original Message----- >From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >Dave Airlie >Sent: Monday, July 27, 2020 11:26 PM >To: dri-devel@lists.freedesktop.org >Cc: bskeggs@redhat.com >Subject: [PATCH] nouveau: use ttm populate mapping functions. (v2) > >From: Dave Airlie <airlied@redhat.com> > >Instead of rolling driver copies of them. > >v2: cleanup return handling (Ben) >Signed-off-by: Dave Airlie <airlied@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? 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@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, 29 Jul 2020 at 02:08, Ruhl, Michael J <michael.j.ruhl@intel.com> wrote: > > >-----Original Message----- > >From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of > >Dave Airlie > >Sent: Monday, July 27, 2020 11:26 PM > >To: dri-devel@lists.freedesktop.org > >Cc: bskeggs@redhat.com > >Subject: [PATCH] nouveau: use ttm populate mapping functions. (v2) > > > >From: Dave Airlie <airlied@redhat.com> > > > >Instead of rolling driver copies of them. > > > >v2: cleanup return handling (Ben) > >Signed-off-by: Dave Airlie <airlied@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. 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@lists.freedesktop.org > >https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>-----Original Message----- >From: Ben Skeggs <skeggsb@gmail.com> >Sent: Tuesday, July 28, 2020 4:16 PM >To: Ruhl, Michael J <michael.j.ruhl@intel.com> >Cc: Dave Airlie <airlied@gmail.com>; dri-devel@lists.freedesktop.org; >bskeggs@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@intel.com> >wrote: >> >> >-----Original Message----- >> >From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >> >Dave Airlie >> >Sent: Monday, July 27, 2020 11:26 PM >> >To: dri-devel@lists.freedesktop.org >> >Cc: bskeggs@redhat.com >> >Subject: [PATCH] nouveau: use ttm populate mapping functions. (v2) >> > >> >From: Dave Airlie <airlied@redhat.com> >> > >> >Instead of rolling driver copies of them. >> > >> >v2: cleanup return handling (Ben) >> >Signed-off-by: Dave Airlie <airlied@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.
On Wed, 29 Jul 2020 at 06:33, Ruhl, Michael J <michael.j.ruhl@intel.com> wrote: > >-----Original Message----- > >From: Ben Skeggs <skeggsb@gmail.com> > >Sent: Tuesday, July 28, 2020 4:16 PM > >To: Ruhl, Michael J <michael.j.ruhl@intel.com> > >Cc: Dave Airlie <airlied@gmail.com>; dri-devel@lists.freedesktop.org; > >bskeggs@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@intel.com> > >wrote: > >> > >> >-----Original Message----- > >> >From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of > >> >Dave Airlie > >> >Sent: Monday, July 27, 2020 11:26 PM > >> >To: dri-devel@lists.freedesktop.org > >> >Cc: bskeggs@redhat.com > >> >Subject: [PATCH] nouveau: use ttm populate mapping functions. (v2) > >> > > >> >From: Dave Airlie <airlied@redhat.com> > >> > > >> >Instead of rolling driver copies of them. > >> > > >> >v2: cleanup return handling (Ben) > >> >Signed-off-by: Dave Airlie <airlied@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.
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); } 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