Adding Jason to this discussion.
Hi Bobby,
Subject: [PATCH net-next v2 2/4] udmabuf: emit one sg entry per pinned folio
From: Bobby Eshleman bobbyeshleman@meta.com
get_sg_table() emitted one PAGE_SIZE sg entry per page even when the underlying folio was larger.
Instead, walk folios[] and emit one sg entry per folio. When folios
We have recently merged a patch (that will make it into 7.2) from Jason that replaced sg_set_folio() with sg_alloc_table_from_pages() in udmabuf driver: https://gitlab.freedesktop.org/drm/tip/-/commit/5bf888673e0dda5a53220fa0c495...
Since you are relying on sg_set_folio(), the core argument against its usage in udmabuf is that it doesn't work well with offsets > PAGE_SIZE, resulting in a malformed scatterlist. Not sure if this can be fixed easily.
represent large pages (as is for MFD_HUGETLB), each sg entry is a large page. Normal PAGE_SIZE sg tables are unchanged.
This is helpful for importers like net/core/devmem that expect dmabuf sg
IMO, udmabuf needs to detect whether importers can handle segments that are > PAGE_SIZE and set the entries appropriately. Please look into how the GPU drivers and other dmabuf exporters/importers handle this situation, so that we can adopt best practices to address this issue.
Thanks, Vivek
entries to be size and length aligned. Prior to this patch udmabuf handed over one PAGE_SIZE sg entry per page, so devmem only saw PAGE_SIZE chunks regardless of the underlying folio size.
dma_map_sgtable() does not always merge contiguous pages for us, so we do this internally before exporting.
Signed-off-by: Bobby Eshleman bobbyeshleman@meta.com
drivers/dma-buf/udmabuf.c | 52 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 5 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index 94b8ecb892bb..9b751dd98b12 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -141,26 +141,68 @@ static void vunmap_udmabuf(struct dma_buf *buf, struct iosys_map *map) vm_unmap_ram(map->vaddr, ubuf->pagecount); }
+/* Return the number of contiguous pages backed by the folio at @i.
- A udmabuf may map only part of a folio, or reference the same folio
- in multiple non-contiguous runs, so folio_nr_pages() can't be used.
- */
+static pgoff_t udmabuf_folio_nr_pages(struct udmabuf *ubuf, pgoff_t i) +{
- struct folio *f = ubuf->folios[i];
- pgoff_t j;
- for (j = 1; i + j < ubuf->pagecount; j++) {
if (ubuf->folios[i + j] != f)break;/* Same folio, but not a sequential offset within it. */if (ubuf->offsets[i + j] != ubuf->offsets[i] + j * PAGE_SIZE)break;- }
- return j;
+}
+/* Count the contiguous folio runs in @ubuf, one sg entry per run.
- Coalescing folios into a single sg entry up front lets importers actually
- see large chunks. We can't rely on dma_map_sgtable() to do this for us
as
- the dma_map_direct() path preserves the input scatterlist lengths
verbatim.
- */
+static unsigned int udmabuf_sg_nents(struct udmabuf *ubuf) +{
- unsigned int nents = 0;
- pgoff_t i;
- for (i = 0; i < ubuf->pagecount; i += udmabuf_folio_nr_pages(ubuf,
i))
nents++;- return nents;
+}
static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf, enum dma_data_direction direction) { struct udmabuf *ubuf = buf->priv;
- struct sg_table *sg; struct scatterlist *sgl;
- unsigned int i = 0;
struct sg_table *sg;
pgoff_t i, run;
unsigned int nents; int ret;
nents = udmabuf_sg_nents(ubuf);
sg = kzalloc_obj(*sg); if (!sg) return ERR_PTR(-ENOMEM);
- ret = sg_alloc_table(sg, ubuf->pagecount, GFP_KERNEL);
- ret = sg_alloc_table(sg, nents, GFP_KERNEL); if (ret < 0) goto err_alloc;
- for_each_sg(sg->sgl, sgl, ubuf->pagecount, i)
sg_set_folio(sgl, ubuf->folios[i], PAGE_SIZE,
sgl = sg->sgl;
for (i = 0; i < ubuf->pagecount; i += run) {
run = udmabuf_folio_nr_pages(ubuf, i);sg_set_folio(sgl, ubuf->folios[i], run << PAGE_SHIFT, ubuf->offsets[i]);sgl = sg_next(sgl);}
ret = dma_map_sgtable(dev, sg, direction, 0); if (ret < 0)
-- 2.53.0-Meta