[PATCH v2] drm/prime: fix extracting of the DMA addresses from a scatterlist

[Ruhl, Michael J]

>-----Original Message-----
>From: Marek Szyprowski <m.szyprowski at samsung.com>
>Sent: Friday, March 27, 2020 12:21 PM
>To: dri-devel at lists.freedesktop.org; linux-samsung-soc at vger.kernel.org;
>linux-kernel at vger.kernel.org
>Cc: Marek Szyprowski <m.szyprowski at samsung.com>;
>stable at vger.kernel.org; Bartlomiej Zolnierkiewicz
><b.zolnierkie at samsung.com>; Maarten Lankhorst
><maarten.lankhorst at linux.intel.com>; Maxime Ripard
><mripard at kernel.org>; Thomas Zimmermann <tzimmermann at suse.de>;
>David Airlie <airlied at linux.ie>; Daniel Vetter <daniel at ffwll.ch>; Alex Deucher
><alexander.deucher at amd.com>; Shane Francis <bigbeeshane at gmail.com>;
>Ruhl, Michael J <michael.j.ruhl at intel.com>
>Subject: [PATCH v2] drm/prime: fix extracting of the DMA addresses from a
>scatterlist
>
>Scatterlist elements contains both pages and DMA addresses, but one
>should not assume 1:1 relation between them. The sg->length is the size
>of the physical memory chunk described by the sg->page, while
>sg_dma_len(sg) is the size of the DMA (IO virtual) chunk described by
>the sg_dma_address(sg).
>
>The proper way of extracting both: pages and DMA addresses of the whole
>buffer described by a scatterlist it to iterate independently over the
>sg->pages/sg->length and sg_dma_address(sg)/sg_dma_len(sg) entries.
>
>Fixes: 42e67b479eab ("drm/prime: use dma length macro when mapping sg")
>Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
>Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>---
> drivers/gpu/drm/drm_prime.c | 37 +++++++++++++++++++++++++-----------
>-
> 1 file changed, 25 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>index 1de2cde2277c..282774e469ac 100644
>--- a/drivers/gpu/drm/drm_prime.c
>+++ b/drivers/gpu/drm/drm_prime.c
>@@ -962,27 +962,40 @@ int drm_prime_sg_to_page_addr_arrays(struct
>sg_table *sgt, struct page **pages,
> 	unsigned count;
> 	struct scatterlist *sg;
> 	struct page *page;
>-	u32 len, index;
>+	u32 page_len, page_index;
> 	dma_addr_t addr;
>+	u32 dma_len, dma_index;
>
>-	index = 0;
>+	/*
>+	 * Scatterlist elements contains both pages and DMA addresses, but
>+	 * one shoud not assume 1:1 relation between them. The sg->length
>is
>+	 * the size of the physical memory chunk described by the sg->page,
>+	 * while sg_dma_len(sg) is the size of the DMA (IO virtual) chunk
>+	 * described by the sg_dma_address(sg).
>+	 */

Is there an example of what the scatterlist would look like in this case?

Does each SG entry always have the page and dma info? or could you have
entries that have page information only, and entries that have dma info only?

If the same entry has different size info (page_len = PAGE_SIZE,
dma_len = 4 * PAGE_SIZE?), are we guaranteed that the arrays (page and addrs) have
been sized correctly?

Just trying to get my head wrapped around this.

Thanks,

Mike

>+	page_index = 0;
>+	dma_index = 0;
> 	for_each_sg(sgt->sgl, sg, sgt->nents, count) {
>-		len = sg_dma_len(sg);
>+		page_len = sg->length;
> 		page = sg_page(sg);
>+		dma_len = sg_dma_len(sg);
> 		addr = sg_dma_address(sg);
>
>-		while (len > 0) {
>-			if (WARN_ON(index >= max_entries))
>+		while (pages && page_len > 0) {
>+			if (WARN_ON(page_index >= max_entries))
> 				return -1;
>-			if (pages)
>-				pages[index] = page;
>-			if (addrs)
>-				addrs[index] = addr;
>-
>+			pages[page_index] = page;
> 			page++;
>+			page_len -= PAGE_SIZE;
>+			page_index++;
>+		}
>+		while (addrs && dma_len > 0) {
>+			if (WARN_ON(dma_index >= max_entries))
>+				return -1;
>+			addrs[dma_index] = addr;
> 			addr += PAGE_SIZE;
>-			len -= PAGE_SIZE;
>-			index++;
>+			dma_len -= PAGE_SIZE;
>+			dma_index++;
> 		}
> 	}
> 	return 0;
>--
>2.17.1