* [PATCH v5 02/38] scatterlist: add generic wrappers for iterating over sgtable objects
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 03/38] iommu: add generic helper for mapping " Marek Szyprowski
` (35 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter
struct sg_table is a common structure used for describing a memory
buffer. It consists of a scatterlist with memory pages and DMA addresses
(sgl entry), as well as the number of scatterlist entries: CPU pages
(orig_nents entry) and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling the scatterlist iterating functions with a wrong number
of the entries.
To avoid such issues, lets introduce a common wrappers operating directly
on the struct sg_table objects, which take care of the proper use of
the nents and orig_nents entries.
While touching this, lets clarify some ambiguities in the comments for
the existing for_each helpers.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
include/linux/scatterlist.h | 50 ++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 47 insertions(+), 3 deletions(-)
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 6eec50f..4f922af 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -151,6 +151,20 @@ static inline void sg_set_buf(struct scatterlist *sg, const void *buf,
#define for_each_sg(sglist, sg, nr, __i) \
for (__i = 0, sg = (sglist); __i < (nr); __i++, sg = sg_next(sg))
+/*
+ * Loop over each sg element in the given sg_table object.
+ */
+#define for_each_sgtable_sg(sgt, sg, i) \
+ for_each_sg(sgt->sgl, sg, sgt->orig_nents, i)
+
+/*
+ * Loop over each sg element in the given *DMA mapped* sg_table object.
+ * Please use sg_dma_address(sg) and sg_dma_len(sg) to extract DMA addresses
+ * of the each element.
+ */
+#define for_each_sgtable_dma_sg(sgt, sg, i) \
+ for_each_sg(sgt->sgl, sg, sgt->nents, i)
+
/**
* sg_chain - Chain two sglists together
* @prv: First scatterlist
@@ -401,9 +415,10 @@ static inline struct page *sg_page_iter_page(struct sg_page_iter *piter)
* @sglist: sglist to iterate over
* @piter: page iterator to hold current page, sg, sg_pgoffset
* @nents: maximum number of sg entries to iterate over
- * @pgoffset: starting page offset
+ * @pgoffset: starting page offset (in pages)
*
* Callers may use sg_page_iter_page() to get each page pointer.
+ * In each loop it operates on PAGE_SIZE unit.
*/
#define for_each_sg_page(sglist, piter, nents, pgoffset) \
for (__sg_page_iter_start((piter), (sglist), (nents), (pgoffset)); \
@@ -412,18 +427,47 @@ static inline struct page *sg_page_iter_page(struct sg_page_iter *piter)
/**
* for_each_sg_dma_page - iterate over the pages of the given sg list
* @sglist: sglist to iterate over
- * @dma_iter: page iterator to hold current page
+ * @dma_iter: DMA page iterator to hold current page
* @dma_nents: maximum number of sg entries to iterate over, this is the value
* returned from dma_map_sg
- * @pgoffset: starting page offset
+ * @pgoffset: starting page offset (in pages)
*
* Callers may use sg_page_iter_dma_address() to get each page's DMA address.
+ * In each loop it operates on PAGE_SIZE unit.
*/
#define for_each_sg_dma_page(sglist, dma_iter, dma_nents, pgoffset) \
for (__sg_page_iter_start(&(dma_iter)->base, sglist, dma_nents, \
pgoffset); \
__sg_page_iter_dma_next(dma_iter);)
+/**
+ * for_each_sgtable_page - iterate over all pages in the sg_table object
+ * @sgt: sg_table object to iterate over
+ * @piter: page iterator to hold current page
+ * @pgoffset: starting page offset (in pages)
+ *
+ * Iterates over the all memory pages in the buffer described by
+ * a scatterlist stored in the given sg_table object.
+ * See also for_each_sg_page(). In each loop it operates on PAGE_SIZE unit.
+ */
+#define for_each_sgtable_page(sgt, piter, pgoffset) \
+ for_each_sg_page(sgt->sgl, piter, sgt->orig_nents, pgoffset)
+
+/**
+ * for_each_sgtable_dma_page - iterate over the DMA mapped sg_table object
+ * @sgt: sg_table object to iterate over
+ * @dma_iter: DMA page iterator to hold current page
+ * @pgoffset: starting page offset (in pages)
+ *
+ * Iterates over the all DMA mapped pages in the buffer described by
+ * a scatterlist stored in the given sg_table object.
+ * See also for_each_sg_dma_page(). In each loop it operates on PAGE_SIZE
+ * unit.
+ */
+#define for_each_sgtable_dma_page(sgt, dma_iter, pgoffset) \
+ for_each_sg_dma_page(sgt->sgl, dma_iter, sgt->nents, pgoffset)
+
+
/*
* Mapping sg iterator
*
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 03/38] iommu: add generic helper for mapping sgtable objects
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 02/38] scatterlist: add generic wrappers for iterating over " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 04/38] drm: prime: add common helper to check scatterlist contiguity Marek Szyprowski
` (34 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Joerg Roedel
struct sg_table is a common structure used for describing a memory
buffer. It consists of a scatterlist with memory pages and DMA addresses
(sgl entry), as well as the number of scatterlist entries: CPU pages
(orig_nents entry) and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling mapping functions with a wrong number of entries.
To avoid such issues, lets introduce a common wrapper operating directly
on the struct sg_table objects, which take care of the proper use of
the nents and orig_nents entries.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Joerg Roedel <jroedel@suse.de>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
include/linux/iommu.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7cfd2dd..5bed18a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -478,6 +478,22 @@ extern size_t iommu_map_sg_atomic(struct iommu_domain *domain,
extern void iommu_set_fault_handler(struct iommu_domain *domain,
iommu_fault_handler_t handler, void *token);
+/**
+ * iommu_map_sgtable - Map the given buffer to the IOMMU domain
+ * @domain: The IOMMU domain to perform the mapping
+ * @iova: The start address to map the buffer
+ * @sgt: The sg_table object describing the buffer
+ * @prot: IOMMU protection bits
+ *
+ * Creates a mapping at @iova for the buffer described by a scatterlist
+ * stored in the given sg_table object in the provided IOMMU domain.
+ */
+static inline size_t iommu_map_sgtable(struct iommu_domain *domain,
+ unsigned long iova, struct sg_table *sgt, int prot)
+{
+ return iommu_map_sg(domain, iova, sgt->sgl, sgt->orig_nents, prot);
+}
+
extern void iommu_get_resv_regions(struct device *dev, struct list_head *list);
extern void iommu_put_resv_regions(struct device *dev, struct list_head *list);
extern void generic_iommu_put_resv_regions(struct device *dev,
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 04/38] drm: prime: add common helper to check scatterlist contiguity
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 02/38] scatterlist: add generic wrappers for iterating over " Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 03/38] iommu: add generic helper for mapping " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 05/38] drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays() Marek Szyprowski
` (33 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
It is a common operation done by DRM drivers to check the contiguity
of the DMA-mapped buffer described by a scatterlist in the
sg_table object. Let's add a common helper for this operation.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/drm_gem_cma_helper.c | 23 +++--------------------
drivers/gpu/drm/drm_prime.c | 26 ++++++++++++++++++++++++++
include/drm/drm_prime.h | 2 ++
3 files changed, 31 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 12e98fb..9f2d13e 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -471,26 +471,9 @@ struct drm_gem_object *
{
struct drm_gem_cma_object *cma_obj;
- if (sgt->nents != 1) {
- /* check if the entries in the sg_table are contiguous */
- dma_addr_t next_addr = sg_dma_address(sgt->sgl);
- struct scatterlist *s;
- unsigned int i;
-
- for_each_sg(sgt->sgl, s, sgt->nents, i) {
- /*
- * sg_dma_address(s) is only valid for entries
- * that have sg_dma_len(s) != 0
- */
- if (!sg_dma_len(s))
- continue;
-
- if (sg_dma_address(s) != next_addr)
- return ERR_PTR(-EINVAL);
-
- next_addr = sg_dma_address(s) + sg_dma_len(s);
- }
- }
+ /* check if the entries in the sg_table are contiguous */
+ if (drm_prime_get_contiguous_size(sgt) < attach->dmabuf->size)
+ return ERR_PTR(-EINVAL);
/* Create a CMA GEM buffer. */
cma_obj = __drm_gem_cma_create(dev, attach->dmabuf->size);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 282774e..1d2e5fe 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -826,6 +826,32 @@ struct sg_table *drm_prime_pages_to_sg(struct page **pages, unsigned int nr_page
EXPORT_SYMBOL(drm_prime_pages_to_sg);
/**
+ * drm_prime_get_contiguous_size - returns the contiguous size of the buffer
+ * @sgt: sg_table describing the buffer to check
+ *
+ * This helper calculates the contiguous size in the DMA address space
+ * of the the buffer described by the provided sg_table.
+ *
+ * This is useful for implementing
+ * &drm_gem_object_funcs.gem_prime_import_sg_table.
+ */
+unsigned long drm_prime_get_contiguous_size(struct sg_table *sgt)
+{
+ dma_addr_t expected = sg_dma_address(sgt->sgl);
+ struct sg_dma_page_iter dma_iter;
+ unsigned long size = 0;
+
+ for_each_sgtable_dma_page(sgt, &dma_iter, 0) {
+ if (sg_page_iter_dma_address(&dma_iter) != expected)
+ break;
+ expected += PAGE_SIZE;
+ size += PAGE_SIZE;
+ }
+ return size;
+}
+EXPORT_SYMBOL(drm_prime_get_contiguous_size);
+
+/**
* drm_gem_prime_export - helper library implementation of the export callback
* @obj: GEM object to export
* @flags: flags like DRM_CLOEXEC and DRM_RDWR
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index 9af7422..47ef116 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -92,6 +92,8 @@ void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj,
int flags);
+unsigned long drm_prime_get_contiguous_size(struct sg_table *sgt);
+
/* helper functions for importing */
struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
struct dma_buf *dma_buf,
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 05/38] drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays()
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (2 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 04/38] drm: prime: add common helper to check scatterlist contiguity Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-09-21 23:15 ` Alex Goins
2020-05-13 13:32 ` [PATCH v5 06/38] drm: core: fix common struct sg_table related issues Marek Szyprowski
` (32 subsequent siblings)
36 siblings, 1 reply; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Replace the current hand-crafted code for extracting pages and DMA
addresses from the given scatterlist by the much more robust
code based on the generic scatterlist iterators and recently
introduced sg_table-based wrappers. The resulting code is simple and
easy to understand, so the comment describing the old code is no
longer needed.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/drm_prime.c | 47 ++++++++++++++-------------------------------
1 file changed, 14 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 1d2e5fe..dfdf4d4 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -985,45 +985,26 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
dma_addr_t *addrs, int max_entries)
{
- unsigned count;
- struct scatterlist *sg;
- struct page *page;
- u32 page_len, page_index;
- dma_addr_t addr;
- u32 dma_len, dma_index;
+ struct sg_dma_page_iter dma_iter;
+ struct sg_page_iter page_iter;
+ struct page **p = pages;
+ dma_addr_t *a = addrs;
- /*
- * 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).
- */
- page_index = 0;
- dma_index = 0;
- for_each_sg(sgt->sgl, sg, sgt->nents, count) {
- page_len = sg->length;
- page = sg_page(sg);
- dma_len = sg_dma_len(sg);
- addr = sg_dma_address(sg);
-
- while (pages && page_len > 0) {
- if (WARN_ON(page_index >= max_entries))
+ if (pages) {
+ for_each_sgtable_page(sgt, &page_iter, 0) {
+ if (p - pages >= max_entries)
return -1;
- pages[page_index] = page;
- page++;
- page_len -= PAGE_SIZE;
- page_index++;
+ *p++ = sg_page_iter_page(&page_iter);
}
- while (addrs && dma_len > 0) {
- if (WARN_ON(dma_index >= max_entries))
+ }
+ if (addrs) {
+ for_each_sgtable_dma_page(sgt, &dma_iter, 0) {
+ if (a - addrs >= max_entries)
return -1;
- addrs[dma_index] = addr;
- addr += PAGE_SIZE;
- dma_len -= PAGE_SIZE;
- dma_index++;
+ *a++ = sg_page_iter_dma_address(&dma_iter);
}
}
+
return 0;
}
EXPORT_SYMBOL(drm_prime_sg_to_page_addr_arrays);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v5 05/38] drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays()
2020-05-13 13:32 ` [PATCH v5 05/38] drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays() Marek Szyprowski
@ 2020-09-21 23:15 ` Alex Goins
2020-09-22 6:27 ` Marek Szyprowski
0 siblings, 1 reply; 56+ messages in thread
From: Alex Goins @ 2020-09-21 23:15 UTC (permalink / raw)
To: Marek Szyprowski
Cc: dri-devel, iommu, linaro-mm-sig, linux-kernel, Thomas Zimmermann,
Bartlomiej Zolnierkiewicz, David Airlie, Robin Murphy,
Christoph Hellwig, linux-arm-kernel
Tested-by: Alex Goins <agoins@nvidia.com>
This change fixes a regression with drm_prime_sg_to_page_addr_arrays() and
AMDGPU in v5.9.
Commit 39913934 similarly revamped AMDGPU to use sgtable helper functions. When
it changed from dma_map_sg_attrs() to dma_map_sgtable(), as a side effect it
started correctly updating sgt->nents to the return value of dma_map_sg_attrs().
However, drm_prime_sg_to_page_addr_arrays() incorrectly uses sgt->nents to
iterate over pages, rather than sgt->orig_nents, resulting in it now returning
the incorrect number of pages on AMDGPU.
I had written a patch that changes drm_prime_sg_to_page_addr_arrays() to use
for_each_sgtable_sg() instead of for_each_sg(), iterating using sgt->orig_nents:
- for_each_sg(sgt->sgl, sg, sgt->nents, count) {
+ for_each_sgtable_sg(sgt, sg, count) {
This patch takes it further, but still has the effect of fixing the number of
pages that drm_prime_sg_to_page_addr_arrays() returns. Something like this
should be included in v5.9 to prevent a regression with AMDGPU.
Thanks,
Alex
On Wed, 13 May 2020, Marek Szyprowski wrote:
> Replace the current hand-crafted code for extracting pages and DMA
> addresses from the given scatterlist by the much more robust
> code based on the generic scatterlist iterators and recently
> introduced sg_table-based wrappers. The resulting code is simple and
> easy to understand, so the comment describing the old code is no
> longer needed.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
> vs. orig_nents misuse' thread:
> https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
> ---
> drivers/gpu/drm/drm_prime.c | 47 ++++++++++++++-------------------------------
> 1 file changed, 14 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 1d2e5fe..dfdf4d4 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -985,45 +985,26 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
> int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
> dma_addr_t *addrs, int max_entries)
> {
> - unsigned count;
> - struct scatterlist *sg;
> - struct page *page;
> - u32 page_len, page_index;
> - dma_addr_t addr;
> - u32 dma_len, dma_index;
> + struct sg_dma_page_iter dma_iter;
> + struct sg_page_iter page_iter;
> + struct page **p = pages;
> + dma_addr_t *a = addrs;
>
> - /*
> - * 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).
> - */
> - page_index = 0;
> - dma_index = 0;
> - for_each_sg(sgt->sgl, sg, sgt->nents, count) {
> - page_len = sg->length;
> - page = sg_page(sg);
> - dma_len = sg_dma_len(sg);
> - addr = sg_dma_address(sg);
> -
> - while (pages && page_len > 0) {
> - if (WARN_ON(page_index >= max_entries))
> + if (pages) {
> + for_each_sgtable_page(sgt, &page_iter, 0) {
> + if (p - pages >= max_entries)
> return -1;
> - pages[page_index] = page;
> - page++;
> - page_len -= PAGE_SIZE;
> - page_index++;
> + *p++ = sg_page_iter_page(&page_iter);
> }
> - while (addrs && dma_len > 0) {
> - if (WARN_ON(dma_index >= max_entries))
> + }
> + if (addrs) {
> + for_each_sgtable_dma_page(sgt, &dma_iter, 0) {
> + if (a - addrs >= max_entries)
> return -1;
> - addrs[dma_index] = addr;
> - addr += PAGE_SIZE;
> - dma_len -= PAGE_SIZE;
> - dma_index++;
> + *a++ = sg_page_iter_dma_address(&dma_iter);
> }
> }
> +
> return 0;
> }
> EXPORT_SYMBOL(drm_prime_sg_to_page_addr_arrays);
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v5 05/38] drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays()
2020-09-21 23:15 ` Alex Goins
@ 2020-09-22 6:27 ` Marek Szyprowski
2020-09-22 21:12 ` Alex Goins
2020-09-25 21:23 ` [Linaro-mm-sig] " Alex Deucher
0 siblings, 2 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-09-22 6:27 UTC (permalink / raw)
To: Alex Goins
Cc: dri-devel, iommu, linaro-mm-sig, linux-kernel, Thomas Zimmermann,
Bartlomiej Zolnierkiewicz, David Airlie, Robin Murphy,
Christoph Hellwig, linux-arm-kernel
Hi Alex,
On 22.09.2020 01:15, Alex Goins wrote:
> Tested-by: Alex Goins <agoins@nvidia.com>
>
> This change fixes a regression with drm_prime_sg_to_page_addr_arrays() and
> AMDGPU in v5.9.
Thanks for testing!
> Commit 39913934 similarly revamped AMDGPU to use sgtable helper functions. When
> it changed from dma_map_sg_attrs() to dma_map_sgtable(), as a side effect it
> started correctly updating sgt->nents to the return value of dma_map_sg_attrs().
> However, drm_prime_sg_to_page_addr_arrays() incorrectly uses sgt->nents to
> iterate over pages, rather than sgt->orig_nents, resulting in it now returning
> the incorrect number of pages on AMDGPU.
>
> I had written a patch that changes drm_prime_sg_to_page_addr_arrays() to use
> for_each_sgtable_sg() instead of for_each_sg(), iterating using sgt->orig_nents:
>
> - for_each_sg(sgt->sgl, sg, sgt->nents, count) {
> + for_each_sgtable_sg(sgt, sg, count) {
>
> This patch takes it further, but still has the effect of fixing the number of
> pages that drm_prime_sg_to_page_addr_arrays() returns. Something like this
> should be included in v5.9 to prevent a regression with AMDGPU.
Probably the easiest way to handle a fix for v5.9 would be to simply
merge the latest version of this patch also to v5.9-rcX:
https://lore.kernel.org/dri-devel/20200904131711.12950-3-m.szyprowski@samsung.com/
This way we would get it fixed and avoid possible conflict in the -next.
Do you have any AMDGPU fixes for v5.9 in the queue? Maybe you can add
that patch to the queue? Dave: would it be okay that way?
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v5 05/38] drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays()
2020-09-22 6:27 ` Marek Szyprowski
@ 2020-09-22 21:12 ` Alex Goins
2020-09-25 21:23 ` [Linaro-mm-sig] " Alex Deucher
1 sibling, 0 replies; 56+ messages in thread
From: Alex Goins @ 2020-09-22 21:12 UTC (permalink / raw)
To: Marek Szyprowski
Cc: dri-devel, iommu, linaro-mm-sig, linux-kernel, Thomas Zimmermann,
Bartlomiej Zolnierkiewicz, David Airlie, Robin Murphy,
Christoph Hellwig, linux-arm-kernel
Hi Marek,
On Tue, 22 Sep 2020, Marek Szyprowski wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Alex,
>
> On 22.09.2020 01:15, Alex Goins wrote:
> > Tested-by: Alex Goins <agoins@nvidia.com>
> >
> > This change fixes a regression with drm_prime_sg_to_page_addr_arrays() and
> > AMDGPU in v5.9.
>
> Thanks for testing!
>
> > Commit 39913934 similarly revamped AMDGPU to use sgtable helper functions. When
> > it changed from dma_map_sg_attrs() to dma_map_sgtable(), as a side effect it
> > started correctly updating sgt->nents to the return value of dma_map_sg_attrs().
> > However, drm_prime_sg_to_page_addr_arrays() incorrectly uses sgt->nents to
> > iterate over pages, rather than sgt->orig_nents, resulting in it now returning
> > the incorrect number of pages on AMDGPU.
> >
> > I had written a patch that changes drm_prime_sg_to_page_addr_arrays() to use
> > for_each_sgtable_sg() instead of for_each_sg(), iterating using sgt->orig_nents:
> >
> > - for_each_sg(sgt->sgl, sg, sgt->nents, count) {
> > + for_each_sgtable_sg(sgt, sg, count) {
> >
> > This patch takes it further, but still has the effect of fixing the number of
> > pages that drm_prime_sg_to_page_addr_arrays() returns. Something like this
> > should be included in v5.9 to prevent a regression with AMDGPU.
>
> Probably the easiest way to handle a fix for v5.9 would be to simply
> merge the latest version of this patch also to v5.9-rcX:
> https://lore.kernel.org/dri-devel/20200904131711.12950-3-m.szyprowski@samsung.com/
Tested-by: Alex Goins <agoins@nvidia.com> that version too.
>
> This way we would get it fixed and avoid possible conflict in the -next.
> Do you have any AMDGPU fixes for v5.9 in the queue? Maybe you can add that
> patch to the queue?
I don't have any more AMDGPU fixes, just want to ensure that this makes it in.
Thanks,
Alex
> Dave: would it be okay that way?
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Linaro-mm-sig] [PATCH v5 05/38] drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays()
2020-09-22 6:27 ` Marek Szyprowski
2020-09-22 21:12 ` Alex Goins
@ 2020-09-25 21:23 ` Alex Deucher
2020-09-30 7:15 ` Marek Szyprowski
1 sibling, 1 reply; 56+ messages in thread
From: Alex Deucher @ 2020-09-25 21:23 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Alex Goins, Bartlomiej Zolnierkiewicz, David Airlie, LKML,
Maling list - DRI developers,
moderated list:DMA BUFFER SHARING FRAMEWORK,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,,
Thomas Zimmermann, Robin Murphy, Christoph Hellwig,
linux-arm-kernel
On Tue, Sep 22, 2020 at 2:28 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Alex,
>
> On 22.09.2020 01:15, Alex Goins wrote:
> > Tested-by: Alex Goins <agoins@nvidia.com>
> >
> > This change fixes a regression with drm_prime_sg_to_page_addr_arrays() and
> > AMDGPU in v5.9.
>
> Thanks for testing!
>
> > Commit 39913934 similarly revamped AMDGPU to use sgtable helper functions. When
> > it changed from dma_map_sg_attrs() to dma_map_sgtable(), as a side effect it
> > started correctly updating sgt->nents to the return value of dma_map_sg_attrs().
> > However, drm_prime_sg_to_page_addr_arrays() incorrectly uses sgt->nents to
> > iterate over pages, rather than sgt->orig_nents, resulting in it now returning
> > the incorrect number of pages on AMDGPU.
> >
> > I had written a patch that changes drm_prime_sg_to_page_addr_arrays() to use
> > for_each_sgtable_sg() instead of for_each_sg(), iterating using sgt->orig_nents:
> >
> > - for_each_sg(sgt->sgl, sg, sgt->nents, count) {
> > + for_each_sgtable_sg(sgt, sg, count) {
> >
> > This patch takes it further, but still has the effect of fixing the number of
> > pages that drm_prime_sg_to_page_addr_arrays() returns. Something like this
> > should be included in v5.9 to prevent a regression with AMDGPU.
>
> Probably the easiest way to handle a fix for v5.9 would be to simply
> merge the latest version of this patch also to v5.9-rcX:
> https://lore.kernel.org/dri-devel/20200904131711.12950-3-m.szyprowski@samsung.com/
>
>
> This way we would get it fixed and avoid possible conflict in the -next.
> Do you have any AMDGPU fixes for v5.9 in the queue? Maybe you can add
> that patch to the queue? Dave: would it be okay that way?
I think this should go into drm-misc for 5.9 since it's an update to
drm_prime.c. Is that patch ready to merge?
Acked-by: Alex Deucher <alexander.deucher@amd.com>
Alex
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
> _______________________________________________
> Linaro-mm-sig mailing list
> Linaro-mm-sig@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/linaro-mm-sig
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Linaro-mm-sig] [PATCH v5 05/38] drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays()
2020-09-25 21:23 ` [Linaro-mm-sig] " Alex Deucher
@ 2020-09-30 7:15 ` Marek Szyprowski
0 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-09-30 7:15 UTC (permalink / raw)
To: Alex Deucher, Thomas Zimmermann, Maxime Ripard, Maarten Lankhorst
Cc: Alex Goins, Bartlomiej Zolnierkiewicz, David Airlie, LKML,
Maling list - DRI developers,
moderated list:DMA BUFFER SHARING FRAMEWORK,
list@263.net:IOMMU DRIVERS, Joerg Roedel, iommu, Robin Murphy,
Christoph Hellwig, linux-arm-kernel
Hi All,
On 25.09.2020 23:23, Alex Deucher wrote:
> On Tue, Sep 22, 2020 at 2:28 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 22.09.2020 01:15, Alex Goins wrote:
>>> Tested-by: Alex Goins <agoins@nvidia.com>
>>>
>>> This change fixes a regression with drm_prime_sg_to_page_addr_arrays() and
>>> AMDGPU in v5.9.
>> Thanks for testing!
>>
>>> Commit 39913934 similarly revamped AMDGPU to use sgtable helper functions. When
>>> it changed from dma_map_sg_attrs() to dma_map_sgtable(), as a side effect it
>>> started correctly updating sgt->nents to the return value of dma_map_sg_attrs().
>>> However, drm_prime_sg_to_page_addr_arrays() incorrectly uses sgt->nents to
>>> iterate over pages, rather than sgt->orig_nents, resulting in it now returning
>>> the incorrect number of pages on AMDGPU.
>>>
>>> I had written a patch that changes drm_prime_sg_to_page_addr_arrays() to use
>>> for_each_sgtable_sg() instead of for_each_sg(), iterating using sgt->orig_nents:
>>>
>>> - for_each_sg(sgt->sgl, sg, sgt->nents, count) {
>>> + for_each_sgtable_sg(sgt, sg, count) {
>>>
>>> This patch takes it further, but still has the effect of fixing the number of
>>> pages that drm_prime_sg_to_page_addr_arrays() returns. Something like this
>>> should be included in v5.9 to prevent a regression with AMDGPU.
>> Probably the easiest way to handle a fix for v5.9 would be to simply
>> merge the latest version of this patch also to v5.9-rcX:
>> https://lore.kernel.org/dri-devel/20200904131711.12950-3-m.szyprowski@samsung.com/
>>
>>
>> This way we would get it fixed and avoid possible conflict in the -next.
>> Do you have any AMDGPU fixes for v5.9 in the queue? Maybe you can add
>> that patch to the queue? Dave: would it be okay that way?
> I think this should go into drm-misc for 5.9 since it's an update to
> drm_prime.c. Is that patch ready to merge?
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
Maarten, Maxime or Thomas: could you take this one:
https://lore.kernel.org/dri-devel/20200904131711.12950-3-m.szyprowski@samsung.com/
also to drm-misc-fixes for v5.9-rc?
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v5 06/38] drm: core: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (3 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 05/38] drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays() Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 07/38] drm: amdgpu: " Marek Szyprowski
` (31 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/drm_cache.c | 2 +-
drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++++-----
drivers/gpu/drm/drm_prime.c | 11 ++++++-----
3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
index 03e01b0..0fe3c49 100644
--- a/drivers/gpu/drm/drm_cache.c
+++ b/drivers/gpu/drm/drm_cache.c
@@ -127,7 +127,7 @@ static void drm_cache_flush_clflush(struct page *pages[],
struct sg_page_iter sg_iter;
mb(); /*CLFLUSH is ordered only by using memory barriers*/
- for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
+ for_each_sgtable_page(st, &sg_iter, 0)
drm_clflush_page(sg_page_iter_page(&sg_iter));
mb(); /*Make sure that all cache line entry is flushed*/
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index df31e57..00a43e8 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -117,8 +117,8 @@ void drm_gem_shmem_free_object(struct drm_gem_object *obj)
kvfree(shmem->pages);
} else {
if (shmem->sgt) {
- dma_unmap_sg(obj->dev->dev, shmem->sgt->sgl,
- shmem->sgt->nents, DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
+ DMA_BIDIRECTIONAL, 0);
sg_free_table(shmem->sgt);
kfree(shmem->sgt);
}
@@ -395,8 +395,7 @@ void drm_gem_shmem_purge_locked(struct drm_gem_object *obj)
WARN_ON(!drm_gem_shmem_is_purgeable(shmem));
- dma_unmap_sg(obj->dev->dev, shmem->sgt->sgl,
- shmem->sgt->nents, DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(obj->dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0);
sg_free_table(shmem->sgt);
kfree(shmem->sgt);
shmem->sgt = NULL;
@@ -623,12 +622,17 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_object *obj)
goto err_put_pages;
}
/* Map the pages for use by the h/w. */
- dma_map_sg(obj->dev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL);
+ ret = dma_map_sgtable(obj->dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
+ if (ret)
+ goto err_free_sgt;
shmem->sgt = sgt;
return sgt;
+err_free_sgt:
+ sg_free_table(sgt);
+ kfree(sgt);
err_put_pages:
drm_gem_shmem_put_pages(shmem);
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index dfdf4d4..5ed22dd 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -617,6 +617,7 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
{
struct drm_gem_object *obj = attach->dmabuf->priv;
struct sg_table *sgt;
+ int ret;
if (WARN_ON(dir == DMA_NONE))
return ERR_PTR(-EINVAL);
@@ -626,11 +627,12 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
else
sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
- if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
- DMA_ATTR_SKIP_CPU_SYNC)) {
+ ret = dma_map_sgtable(attach->dev, sgt, dir,
+ DMA_ATTR_SKIP_CPU_SYNC);
+ if (ret) {
sg_free_table(sgt);
kfree(sgt);
- sgt = ERR_PTR(-ENOMEM);
+ sgt = ERR_PTR(ret);
}
return sgt;
@@ -652,8 +654,7 @@ void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
if (!sgt)
return;
- dma_unmap_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
- DMA_ATTR_SKIP_CPU_SYNC);
+ dma_unmap_sgtable(attach->dev, sgt, dir, DMA_ATTR_SKIP_CPU_SYNC);
sg_free_table(sgt);
kfree(sgt);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 07/38] drm: amdgpu: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (4 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 06/38] drm: core: fix common struct sg_table related issues Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 08/38] drm: armada: " Marek Szyprowski
` (30 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Alex Deucher, Christian König, amd-gfx
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 9 +++------
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 8 ++++----
3 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 43d8ed7..519ce44 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -307,8 +307,8 @@ static struct sg_table *amdgpu_dma_buf_map(struct dma_buf_attachment *attach,
if (IS_ERR(sgt))
return sgt;
- if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
- DMA_ATTR_SKIP_CPU_SYNC))
+ if (dma_map_sgtable(attach->dev, sgt, dir,
+ DMA_ATTR_SKIP_CPU_SYNC))
goto error_free;
break;
@@ -349,7 +349,7 @@ static void amdgpu_dma_buf_unmap(struct dma_buf_attachment *attach,
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
if (sgt->sgl->page_link) {
- dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+ dma_unmap_sgtable(attach->dev, sgt, dir, 0);
sg_free_table(sgt);
kfree(sgt);
} else {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 9cbecd5..57a5d56 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1024,7 +1024,6 @@ static int amdgpu_ttm_tt_pin_userptr(struct ttm_tt *ttm)
{
struct amdgpu_device *adev = amdgpu_ttm_adev(ttm->bdev);
struct amdgpu_ttm_tt *gtt = (void *)ttm;
- unsigned nents;
int r;
int write = !(gtt->userflags & AMDGPU_GEM_USERPTR_READONLY);
@@ -1039,9 +1038,8 @@ static int amdgpu_ttm_tt_pin_userptr(struct ttm_tt *ttm)
goto release_sg;
/* Map SG to device */
- r = -ENOMEM;
- nents = dma_map_sg(adev->dev, ttm->sg->sgl, ttm->sg->nents, direction);
- if (nents == 0)
+ r = dma_map_sgtable(adev->dev, ttm->sg, direction, 0);
+ if (r)
goto release_sg;
/* convert SG to linear array of pages and dma addresses */
@@ -1072,8 +1070,7 @@ static void amdgpu_ttm_tt_unpin_userptr(struct ttm_tt *ttm)
return;
/* unmap the pages mapped to the device */
- dma_unmap_sg(adev->dev, ttm->sg->sgl, ttm->sg->nents, direction);
-
+ dma_unmap_sgtable(adev->dev, ttm->sg, direction, 0);
sg_free_table(ttm->sg);
#if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index d399e58..75495a7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -477,11 +477,11 @@ int amdgpu_vram_mgr_alloc_sgt(struct amdgpu_device *adev,
if (r)
goto error_free;
- for_each_sg((*sgt)->sgl, sg, num_entries, i)
+ for_each_sgtable_sg(*sgt, sg, i)
sg->length = 0;
node = mem->mm_node;
- for_each_sg((*sgt)->sgl, sg, num_entries, i) {
+ for_each_sgtable_sg(*sgt, sg, i) {
phys_addr_t phys = (node->start << PAGE_SHIFT) +
adev->gmc.aper_base;
size_t size = node->size << PAGE_SHIFT;
@@ -501,7 +501,7 @@ int amdgpu_vram_mgr_alloc_sgt(struct amdgpu_device *adev,
return 0;
error_unmap:
- for_each_sg((*sgt)->sgl, sg, num_entries, i) {
+ for_each_sgtable_sg(*sgt, sg, i) {
if (!sg->length)
continue;
@@ -532,7 +532,7 @@ void amdgpu_vram_mgr_free_sgt(struct amdgpu_device *adev,
struct scatterlist *sg;
int i;
- for_each_sg(sgt->sgl, sg, sgt->nents, i)
+ for_each_sgtable_sg(sgt, sg, i)
dma_unmap_resource(dev, sg->dma_address,
sg->length, dir,
DMA_ATTR_SKIP_CPU_SYNC);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 08/38] drm: armada: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (5 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 07/38] drm: amdgpu: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 09/38] drm: etnaviv: " Marek Szyprowski
` (29 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Russell King
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/armada/armada_gem.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index 976685f..0d61da5 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -395,7 +395,7 @@ int armada_gem_pwrite_ioctl(struct drm_device *dev, void *data,
mapping = dobj->obj.filp->f_mapping;
- for_each_sg(sgt->sgl, sg, count, i) {
+ for_each_sgtable_sg(sgt, sg, i) {
struct page *page;
page = shmem_read_mapping_page(mapping, i);
@@ -407,8 +407,8 @@ int armada_gem_pwrite_ioctl(struct drm_device *dev, void *data,
sg_set_page(sg, page, PAGE_SIZE, 0);
}
- if (dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir) == 0) {
- num = sgt->nents;
+ if (dma_map_sgtable(attach->dev, sgt, dir, 0)) {
+ num = count;
goto release;
}
} else if (dobj->page) {
@@ -418,7 +418,7 @@ int armada_gem_pwrite_ioctl(struct drm_device *dev, void *data,
sg_set_page(sgt->sgl, dobj->page, dobj->obj.size, 0);
- if (dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir) == 0)
+ if (dma_map_sgtable(attach->dev, sgt, dir, 0))
goto free_table;
} else if (dobj->linear) {
/* Single contiguous physical region - no struct page */
@@ -449,11 +449,11 @@ static void armada_gem_prime_unmap_dma_buf(struct dma_buf_attachment *attach,
int i;
if (!dobj->linear)
- dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+ dma_unmap_sgtable(attach->dev, sgt, dir, 0);
if (dobj->obj.filp) {
struct scatterlist *sg;
- for_each_sg(sgt->sgl, sg, sgt->nents, i)
+ for_each_sgtable_sg(sgt, sg, i)
put_page(sg_page(sg));
}
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 09/38] drm: etnaviv: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (6 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 08/38] drm: armada: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 10/38] drm: exynos: use common helper for a scatterlist contiguity check Marek Szyprowski
` (28 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Lucas Stach, etnaviv
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 12 +++++-------
drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 13 +++----------
2 files changed, 8 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index dc9ef30..cc50c7b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -27,7 +27,7 @@ static void etnaviv_gem_scatter_map(struct etnaviv_gem_object *etnaviv_obj)
* because display controller, GPU, etc. are not coherent.
*/
if (etnaviv_obj->flags & ETNA_BO_CACHE_MASK)
- dma_map_sg(dev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL);
+ dma_map_sgtable(dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
}
static void etnaviv_gem_scatterlist_unmap(struct etnaviv_gem_object *etnaviv_obj)
@@ -51,7 +51,7 @@ static void etnaviv_gem_scatterlist_unmap(struct etnaviv_gem_object *etnaviv_obj
* discard those writes.
*/
if (etnaviv_obj->flags & ETNA_BO_CACHE_MASK)
- dma_unmap_sg(dev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
}
/* called with etnaviv_obj->lock held */
@@ -404,9 +404,8 @@ int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op,
}
if (etnaviv_obj->flags & ETNA_BO_CACHED) {
- dma_sync_sg_for_cpu(dev->dev, etnaviv_obj->sgt->sgl,
- etnaviv_obj->sgt->nents,
- etnaviv_op_to_dma_dir(op));
+ dma_sync_sgtable_for_cpu(dev->dev, etnaviv_obj->sgt,
+ etnaviv_op_to_dma_dir(op));
etnaviv_obj->last_cpu_prep_op = op;
}
@@ -421,8 +420,7 @@ int etnaviv_gem_cpu_fini(struct drm_gem_object *obj)
if (etnaviv_obj->flags & ETNA_BO_CACHED) {
/* fini without a prep is almost certainly a userspace error */
WARN_ON(etnaviv_obj->last_cpu_prep_op == 0);
- dma_sync_sg_for_device(dev->dev, etnaviv_obj->sgt->sgl,
- etnaviv_obj->sgt->nents,
+ dma_sync_sgtable_for_device(dev->dev, etnaviv_obj->sgt,
etnaviv_op_to_dma_dir(etnaviv_obj->last_cpu_prep_op));
etnaviv_obj->last_cpu_prep_op = 0;
}
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index 3607d34..13b1005 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -79,7 +79,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
if (!context || !sgt)
return -EINVAL;
- for_each_sg(sgt->sgl, sg, sgt->nents, i) {
+ for_each_sgtable_dma_sg(sgt, sg, i) {
u32 pa = sg_dma_address(sg) - sg->offset;
size_t bytes = sg_dma_len(sg) + sg->offset;
@@ -95,14 +95,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
return 0;
fail:
- da = iova;
-
- for_each_sg(sgt->sgl, sg, i, j) {
- size_t bytes = sg_dma_len(sg) + sg->offset;
-
- etnaviv_context_unmap(context, da, bytes);
- da += bytes;
- }
+ etnaviv_context_unmap(context, iova, da - iova);
return ret;
}
@@ -113,7 +106,7 @@ static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
unsigned int da = iova;
int i;
- for_each_sg(sgt->sgl, sg, sgt->nents, i) {
+ for_each_sgtable_dma_sg(sgt, sg, i) {
size_t bytes = sg_dma_len(sg) + sg->offset;
etnaviv_context_unmap(context, da, bytes);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 10/38] drm: exynos: use common helper for a scatterlist contiguity check
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (7 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 09/38] drm: etnaviv: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 11/38] drm: exynos: fix common struct sg_table related issues Marek Szyprowski
` (27 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Inki Dae, Seung-Woo Kim, linux-samsung-soc
Use common helper for checking the contiguity of the imported dma-buf.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/exynos/exynos_drm_gem.c | 23 +++--------------------
1 file changed, 3 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 0df57ee..a49a8ea 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -431,27 +431,10 @@ struct drm_gem_object *
{
struct exynos_drm_gem *exynos_gem;
- if (sgt->nents < 1)
+ /* check if the entries in the sg_table are contiguous */
+ if (drm_prime_get_contiguous_size(sgt) < attach->dmabuf->size) {
+ DRM_ERROR("buffer chunks must be mapped contiguously");
return ERR_PTR(-EINVAL);
-
- /*
- * Check if the provided buffer has been mapped as contiguous
- * into DMA address space.
- */
- if (sgt->nents > 1) {
- dma_addr_t next_addr = sg_dma_address(sgt->sgl);
- struct scatterlist *s;
- unsigned int i;
-
- for_each_sg(sgt->sgl, s, sgt->nents, i) {
- if (!sg_dma_len(s))
- break;
- if (sg_dma_address(s) != next_addr) {
- DRM_ERROR("buffer chunks must be mapped contiguously");
- return ERR_PTR(-EINVAL);
- }
- next_addr = sg_dma_address(s) + sg_dma_len(s);
- }
}
exynos_gem = exynos_drm_gem_init(dev, attach->dmabuf->size);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 11/38] drm: exynos: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (8 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 10/38] drm: exynos: use common helper for a scatterlist contiguity check Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 12/38] drm: i915: " Marek Szyprowski
` (26 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Inki Dae, Seung-Woo Kim, linux-samsung-soc
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/exynos/exynos_drm_g2d.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
index fcee33a..7014a8c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
@@ -395,8 +395,8 @@ static void g2d_userptr_put_dma_addr(struct g2d_data *g2d,
return;
out:
- dma_unmap_sg(to_dma_dev(g2d->drm_dev), g2d_userptr->sgt->sgl,
- g2d_userptr->sgt->nents, DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(to_dma_dev(g2d->drm_dev), g2d_userptr->sgt,
+ DMA_BIDIRECTIONAL, 0);
pages = frame_vector_pages(g2d_userptr->vec);
if (!IS_ERR(pages)) {
@@ -511,10 +511,10 @@ static dma_addr_t *g2d_userptr_get_dma_addr(struct g2d_data *g2d,
g2d_userptr->sgt = sgt;
- if (!dma_map_sg(to_dma_dev(g2d->drm_dev), sgt->sgl, sgt->nents,
- DMA_BIDIRECTIONAL)) {
+ ret = dma_map_sgtable(to_dma_dev(g2d->drm_dev), sgt,
+ DMA_BIDIRECTIONAL, 0);
+ if (ret) {
DRM_DEV_ERROR(g2d->dev, "failed to map sgt with dma region.\n");
- ret = -ENOMEM;
goto err_sg_free_table;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 12/38] drm: i915: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (9 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 11/38] drm: exynos: fix common struct sg_table related issues Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 13/38] drm: lima: " Marek Szyprowski
` (25 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
intel-gfx
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
This driver creatively uses sg_table->orig_nents to store the size of the
allocated scatterlist and ignores the number of the entries returned by
dma_map_sg function. The sg_table->orig_nents is (mis)used to properly
free the (over)allocated scatterlist.
This patch only introduces the common DMA-mapping wrappers operating
directly on the struct sg_table objects to the dmabuf related functions,
so the other drivers, which might share buffers with i915 could rely on
the properly set nents and orig_nents values.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 11 +++--------
drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 7 +++----
2 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index 7db5a79..6c67810 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -48,12 +48,9 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme
src = sg_next(src);
}
- if (!dma_map_sg_attrs(attachment->dev,
- st->sgl, st->nents, dir,
- DMA_ATTR_SKIP_CPU_SYNC)) {
- ret = -ENOMEM;
+ ret = dma_map_sgtable(attachment->dev, st, dir, DMA_ATTR_SKIP_CPU_SYNC);
+ if (ret)
goto err_free_sg;
- }
return st;
@@ -73,9 +70,7 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
{
struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf);
- dma_unmap_sg_attrs(attachment->dev,
- sg->sgl, sg->nents, dir,
- DMA_ATTR_SKIP_CPU_SYNC);
+ dma_unmap_sgtable(attachment->dev, sg, dir, DMA_ATTR_SKIP_CPU_SYNC);
sg_free_table(sg);
kfree(sg);
diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c
index debaf7b..be30b27 100644
--- a/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c
@@ -28,10 +28,9 @@ static struct sg_table *mock_map_dma_buf(struct dma_buf_attachment *attachment,
sg = sg_next(sg);
}
- if (!dma_map_sg(attachment->dev, st->sgl, st->nents, dir)) {
- err = -ENOMEM;
+ err = dma_map_sgtable(attachment->dev, st, dir, 0);
+ if (err)
goto err_st;
- }
return st;
@@ -46,7 +45,7 @@ static void mock_unmap_dma_buf(struct dma_buf_attachment *attachment,
struct sg_table *st,
enum dma_data_direction dir)
{
- dma_unmap_sg(attachment->dev, st->sgl, st->nents, dir);
+ dma_unmap_sgtable(attachment->dev, st, dir, 0);
sg_free_table(st);
kfree(st);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 13/38] drm: lima: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (10 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 12/38] drm: i915: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-18 1:27 ` Qiang Yu
2020-05-13 13:32 ` [PATCH v5 14/38] drm: mediatek: use common helper for a scatterlist contiguity check Marek Szyprowski
` (24 subsequent siblings)
36 siblings, 1 reply; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Qiang Yu, lima
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/lima/lima_gem.c | 11 ++++++++---
drivers/gpu/drm/lima/lima_vm.c | 5 ++---
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
index 5404e0d..cda43f6 100644
--- a/drivers/gpu/drm/lima/lima_gem.c
+++ b/drivers/gpu/drm/lima/lima_gem.c
@@ -69,8 +69,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
return ret;
if (bo->base.sgt) {
- dma_unmap_sg(dev, bo->base.sgt->sgl,
- bo->base.sgt->nents, DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(dev, bo->base.sgt, DMA_BIDIRECTIONAL, 0);
sg_free_table(bo->base.sgt);
} else {
bo->base.sgt = kmalloc(sizeof(*bo->base.sgt), GFP_KERNEL);
@@ -80,7 +79,13 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
}
}
- dma_map_sg(dev, sgt.sgl, sgt.nents, DMA_BIDIRECTIONAL);
+ ret = dma_map_sgtable(dev, &sgt, DMA_BIDIRECTIONAL, 0);
+ if (ret) {
+ sg_free_table(&sgt);
+ kfree(bo->base.sgt);
+ bo->base.sgt = NULL;
+ return ret;
+ }
*bo->base.sgt = sgt;
diff --git a/drivers/gpu/drm/lima/lima_vm.c b/drivers/gpu/drm/lima/lima_vm.c
index 5b92fb8..2b2739a 100644
--- a/drivers/gpu/drm/lima/lima_vm.c
+++ b/drivers/gpu/drm/lima/lima_vm.c
@@ -124,7 +124,7 @@ int lima_vm_bo_add(struct lima_vm *vm, struct lima_bo *bo, bool create)
if (err)
goto err_out1;
- for_each_sg_dma_page(bo->base.sgt->sgl, &sg_iter, bo->base.sgt->nents, 0) {
+ for_each_sgtable_dma_page(bo->base.sgt, &sg_iter, 0) {
err = lima_vm_map_page(vm, sg_page_iter_dma_address(&sg_iter),
bo_va->node.start + offset);
if (err)
@@ -298,8 +298,7 @@ int lima_vm_map_bo(struct lima_vm *vm, struct lima_bo *bo, int pageoff)
mutex_lock(&vm->lock);
base = bo_va->node.start + (pageoff << PAGE_SHIFT);
- for_each_sg_dma_page(bo->base.sgt->sgl, &sg_iter,
- bo->base.sgt->nents, pageoff) {
+ for_each_sgtable_dma_page(bo->base.sgt, &sg_iter, pageoff) {
err = lima_vm_map_page(vm, sg_page_iter_dma_address(&sg_iter),
base + offset);
if (err)
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v5 13/38] drm: lima: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 13/38] drm: lima: " Marek Szyprowski
@ 2020-05-18 1:27 ` Qiang Yu
0 siblings, 0 replies; 56+ messages in thread
From: Qiang Yu @ 2020-05-18 1:27 UTC (permalink / raw)
To: Marek Szyprowski
Cc: dri-devel, iommu, linaro-mm-sig, Linux Kernel Mailing List,
Christoph Hellwig, Robin Murphy, Bartlomiej Zolnierkiewicz,
linux-arm-kernel, David Airlie, Daniel Vetter, lima
Looks good for me, patch is:
Reviewed-by: Qiang Yu <yuq825@gmail.com>
Regards,
Qiang
On Wed, May 13, 2020 at 9:33 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
> returns the number of the created entries in the DMA address space.
> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
> dma_unmap_sg must be called with the original number of the entries
> passed to the dma_map_sg().
>
> struct sg_table is a common structure used for describing a non-contiguous
> memory buffer, used commonly in the DRM and graphics subsystems. It
> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
> and DMA mapped pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, calling DMA-mapping functions with a wrong number of entries or
> ignoring the number of mapped entries returned by the dma_map_sg()
> function.
>
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
> vs. orig_nents misuse' thread:
> https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
> ---
> drivers/gpu/drm/lima/lima_gem.c | 11 ++++++++---
> drivers/gpu/drm/lima/lima_vm.c | 5 ++---
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
> index 5404e0d..cda43f6 100644
> --- a/drivers/gpu/drm/lima/lima_gem.c
> +++ b/drivers/gpu/drm/lima/lima_gem.c
> @@ -69,8 +69,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
> return ret;
>
> if (bo->base.sgt) {
> - dma_unmap_sg(dev, bo->base.sgt->sgl,
> - bo->base.sgt->nents, DMA_BIDIRECTIONAL);
> + dma_unmap_sgtable(dev, bo->base.sgt, DMA_BIDIRECTIONAL, 0);
> sg_free_table(bo->base.sgt);
> } else {
> bo->base.sgt = kmalloc(sizeof(*bo->base.sgt), GFP_KERNEL);
> @@ -80,7 +79,13 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
> }
> }
>
> - dma_map_sg(dev, sgt.sgl, sgt.nents, DMA_BIDIRECTIONAL);
> + ret = dma_map_sgtable(dev, &sgt, DMA_BIDIRECTIONAL, 0);
> + if (ret) {
> + sg_free_table(&sgt);
> + kfree(bo->base.sgt);
> + bo->base.sgt = NULL;
> + return ret;
> + }
>
> *bo->base.sgt = sgt;
>
> diff --git a/drivers/gpu/drm/lima/lima_vm.c b/drivers/gpu/drm/lima/lima_vm.c
> index 5b92fb8..2b2739a 100644
> --- a/drivers/gpu/drm/lima/lima_vm.c
> +++ b/drivers/gpu/drm/lima/lima_vm.c
> @@ -124,7 +124,7 @@ int lima_vm_bo_add(struct lima_vm *vm, struct lima_bo *bo, bool create)
> if (err)
> goto err_out1;
>
> - for_each_sg_dma_page(bo->base.sgt->sgl, &sg_iter, bo->base.sgt->nents, 0) {
> + for_each_sgtable_dma_page(bo->base.sgt, &sg_iter, 0) {
> err = lima_vm_map_page(vm, sg_page_iter_dma_address(&sg_iter),
> bo_va->node.start + offset);
> if (err)
> @@ -298,8 +298,7 @@ int lima_vm_map_bo(struct lima_vm *vm, struct lima_bo *bo, int pageoff)
> mutex_lock(&vm->lock);
>
> base = bo_va->node.start + (pageoff << PAGE_SHIFT);
> - for_each_sg_dma_page(bo->base.sgt->sgl, &sg_iter,
> - bo->base.sgt->nents, pageoff) {
> + for_each_sgtable_dma_page(bo->base.sgt, &sg_iter, pageoff) {
> err = lima_vm_map_page(vm, sg_page_iter_dma_address(&sg_iter),
> base + offset);
> if (err)
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v5 14/38] drm: mediatek: use common helper for a scatterlist contiguity check
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (11 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 13/38] drm: lima: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 15/38] drm: mediatek: use common helper for extracting pages array Marek Szyprowski
` (23 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Matthias Brugger, Chun-Kuang Hu, Philipp Zabel,
linux-mediatek
Use common helper for checking the contiguity of the imported dma-buf and
do this check before allocating resources, so the error path is simpler.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/mediatek/mtk_drm_gem.c | 25 ++++++-------------------
1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
index b04a3c2..6c34c06 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
@@ -212,34 +212,21 @@ struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device *dev,
struct dma_buf_attachment *attach, struct sg_table *sg)
{
struct mtk_drm_gem_obj *mtk_gem;
- int ret;
- struct scatterlist *s;
- unsigned int i;
- dma_addr_t expected;
- mtk_gem = mtk_drm_gem_init(dev, attach->dmabuf->size);
+ /* check if the entries in the sg_table are contiguous */
+ if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) {
+ DRM_ERROR("sg_table is not contiguous");
+ return ERR_PTR(-EINVAL);
+ }
+ mtk_gem = mtk_drm_gem_init(dev, attach->dmabuf->size);
if (IS_ERR(mtk_gem))
return ERR_CAST(mtk_gem);
- expected = sg_dma_address(sg->sgl);
- for_each_sg(sg->sgl, s, sg->nents, i) {
- if (sg_dma_address(s) != expected) {
- DRM_ERROR("sg_table is not contiguous");
- ret = -EINVAL;
- goto err_gem_free;
- }
- expected = sg_dma_address(s) + sg_dma_len(s);
- }
-
mtk_gem->dma_addr = sg_dma_address(sg->sgl);
mtk_gem->sg = sg;
return &mtk_gem->base;
-
-err_gem_free:
- kfree(mtk_gem);
- return ERR_PTR(ret);
}
void *mtk_drm_gem_prime_vmap(struct drm_gem_object *obj)
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 15/38] drm: mediatek: use common helper for extracting pages array
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (12 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 14/38] drm: mediatek: use common helper for a scatterlist contiguity check Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 16/38] drm: msm: fix common struct sg_table related issues Marek Szyprowski
` (22 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Matthias Brugger, Chun-Kuang Hu, Philipp Zabel,
linux-mediatek
Use common helper for converting a sg_table object into struct
page pointer array.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/mediatek/mtk_drm_gem.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
index 6c34c06..14fcd48 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
@@ -233,9 +233,7 @@ void *mtk_drm_gem_prime_vmap(struct drm_gem_object *obj)
{
struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
struct sg_table *sgt;
- struct sg_page_iter iter;
unsigned int npages;
- unsigned int i = 0;
if (mtk_gem->kvaddr)
return mtk_gem->kvaddr;
@@ -249,11 +247,8 @@ void *mtk_drm_gem_prime_vmap(struct drm_gem_object *obj)
if (!mtk_gem->pages)
goto out;
- for_each_sg_page(sgt->sgl, &iter, sgt->orig_nents, 0) {
- mtk_gem->pages[i++] = sg_page_iter_page(&iter);
- if (i > npages)
- break;
- }
+ drm_prime_sg_to_page_addr_arrays(sgt, mtk_gem->pages, NULL, npages);
+
mtk_gem->kvaddr = vmap(mtk_gem->pages, npages, VM_MAP,
pgprot_writecombine(PAGE_KERNEL));
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 16/38] drm: msm: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (13 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 15/38] drm: mediatek: use common helper for extracting pages array Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 17/38] drm: omapdrm: use common helper for extracting pages array Marek Szyprowski
` (21 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Rob Clark, Sean Paul, linux-arm-msm, freedreno
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/msm/msm_gem.c | 13 +++++--------
drivers/gpu/drm/msm/msm_gpummu.c | 14 ++++++--------
drivers/gpu/drm/msm/msm_iommu.c | 2 +-
3 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 5a6a79f..6318c20 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -53,11 +53,10 @@ static void sync_for_device(struct msm_gem_object *msm_obj)
struct device *dev = msm_obj->base.dev->dev;
if (get_dma_ops(dev) && IS_ENABLED(CONFIG_ARM64)) {
- dma_sync_sg_for_device(dev, msm_obj->sgt->sgl,
- msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
+ dma_sync_sgtable_for_device(dev, msm_obj->sgt,
+ DMA_BIDIRECTIONAL);
} else {
- dma_map_sg(dev, msm_obj->sgt->sgl,
- msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
+ dma_map_sgtable(dev, msm_obj->sgt, DMA_BIDIRECTIONAL, 0);
}
}
@@ -66,11 +65,9 @@ static void sync_for_cpu(struct msm_gem_object *msm_obj)
struct device *dev = msm_obj->base.dev->dev;
if (get_dma_ops(dev) && IS_ENABLED(CONFIG_ARM64)) {
- dma_sync_sg_for_cpu(dev, msm_obj->sgt->sgl,
- msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
+ dma_sync_sgtable_for_cpu(dev, msm_obj->sgt, DMA_BIDIRECTIONAL);
} else {
- dma_unmap_sg(dev, msm_obj->sgt->sgl,
- msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(dev, msm_obj->sgt, DMA_BIDIRECTIONAL, 0);
}
}
diff --git a/drivers/gpu/drm/msm/msm_gpummu.c b/drivers/gpu/drm/msm/msm_gpummu.c
index 34980d8..5d8da4d 100644
--- a/drivers/gpu/drm/msm/msm_gpummu.c
+++ b/drivers/gpu/drm/msm/msm_gpummu.c
@@ -35,21 +35,19 @@ static int msm_gpummu_map(struct msm_mmu *mmu, uint64_t iova,
{
struct msm_gpummu *gpummu = to_msm_gpummu(mmu);
unsigned idx = (iova - GPUMMU_VA_START) / GPUMMU_PAGE_SIZE;
- struct scatterlist *sg;
+ struct sg_dma_page_iter dma_iter;
unsigned prot_bits = 0;
- unsigned i, j;
if (prot & IOMMU_WRITE)
prot_bits |= 1;
if (prot & IOMMU_READ)
prot_bits |= 2;
- for_each_sg(sgt->sgl, sg, sgt->nents, i) {
- dma_addr_t addr = sg->dma_address;
- for (j = 0; j < sg->length / GPUMMU_PAGE_SIZE; j++, idx++) {
- gpummu->table[idx] = addr | prot_bits;
- addr += GPUMMU_PAGE_SIZE;
- }
+ for_each_sgtable_dma_page(sgt, &dma_iter, 0) {
+ dma_addr_t addr = sg_page_iter_dma_address(&dma_iter);
+
+ BUILD_BUG_ON(GPUMMU_PAGE_SIZE != PAGE_SIZE);
+ gpummu->table[idx++] = addr | prot_bits;
}
/* we can improve by deferring flush for multiple map() */
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index ad58cfe..d322b39 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -43,7 +43,7 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
struct msm_iommu *iommu = to_msm_iommu(mmu);
size_t ret;
- ret = iommu_map_sg(iommu->domain, iova, sgt->sgl, sgt->nents, prot);
+ ret = iommu_map_sgtable(iommu->domain, iova, sgt, prot);
WARN_ON(!ret);
return (ret == len) ? 0 : -EINVAL;
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 17/38] drm: omapdrm: use common helper for extracting pages array
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (14 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 16/38] drm: msm: fix common struct sg_table related issues Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 18/38] drm: omapdrm: fix common struct sg_table related issues Marek Szyprowski
` (20 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Tomi Valkeinen
Use common helper for converting a sg_table object into struct
page pointer array.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/omapdrm/omap_gem.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index d08ae95..c259411 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -1297,10 +1297,9 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,
omap_obj->dma_addr = sg_dma_address(sgt->sgl);
} else {
/* Create pages list from sgt */
- struct sg_page_iter iter;
struct page **pages;
unsigned int npages;
- unsigned int i = 0;
+ unsigned int ret;
npages = DIV_ROUND_UP(size, PAGE_SIZE);
pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL);
@@ -1311,14 +1310,9 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,
}
omap_obj->pages = pages;
-
- for_each_sg_page(sgt->sgl, &iter, sgt->orig_nents, 0) {
- pages[i++] = sg_page_iter_page(&iter);
- if (i > npages)
- break;
- }
-
- if (WARN_ON(i != npages)) {
+ ret = drm_prime_sg_to_page_addr_arrays(sgt, pages, NULL,
+ npages);
+ if (WARN_ON(ret)) {
omap_gem_free_object(obj);
obj = ERR_PTR(-ENOMEM);
goto done;
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 18/38] drm: omapdrm: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (15 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 17/38] drm: omapdrm: use common helper for extracting pages array Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 19/38] drm: panfrost: " Marek Szyprowski
` (19 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Tomi Valkeinen
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
Fix the code to refer to proper nents or orig_nents entries. This driver
checks for a buffer contiguity in DMA address space, so it should test
sg_table->nents entry.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/omapdrm/omap_gem.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index c259411..cbcb361 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -48,7 +48,7 @@ struct omap_gem_object {
* OMAP_BO_MEM_DMA_API flag set)
*
* - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag set)
- * if they are physically contiguous (when sgt->orig_nents == 1)
+ * if they are physically contiguous (when sgt->nents == 1)
*
* - buffers mapped through the TILER when dma_addr_cnt is not zero, in
* which case the DMA address points to the TILER aperture
@@ -1279,7 +1279,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,
union omap_gem_size gsize;
/* Without a DMM only physically contiguous buffers can be supported. */
- if (sgt->orig_nents != 1 && !priv->has_dmm)
+ if (sgt->nents != 1 && !priv->has_dmm)
return ERR_PTR(-EINVAL);
gsize.bytes = PAGE_ALIGN(size);
@@ -1293,7 +1293,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,
omap_obj->sgt = sgt;
- if (sgt->orig_nents == 1) {
+ if (sgt->nents == 1) {
omap_obj->dma_addr = sg_dma_address(sgt->sgl);
} else {
/* Create pages list from sgt */
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 19/38] drm: panfrost: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (16 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 18/38] drm: omapdrm: fix common struct sg_table related issues Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-14 13:51 ` Rob Herring
2020-05-13 13:32 ` [PATCH v5 20/38] drm: radeon: " Marek Szyprowski
` (18 subsequent siblings)
36 siblings, 1 reply; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Rob Herring, Tomeu Vizoso
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Steven Price <steven.price@arm.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/panfrost/panfrost_gem.c | 4 ++--
drivers/gpu/drm/panfrost/panfrost_mmu.c | 7 +++----
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 17b654e..6696304 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -41,8 +41,8 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
for (i = 0; i < n_sgt; i++) {
if (bo->sgts[i].sgl) {
- dma_unmap_sg(pfdev->dev, bo->sgts[i].sgl,
- bo->sgts[i].nents, DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(pfdev->dev, &bo->sgts[i],
+ DMA_BIDIRECTIONAL, 0);
sg_free_table(&bo->sgts[i]);
}
}
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index ed28aeb..eb2550e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -253,7 +253,7 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
struct io_pgtable_ops *ops = mmu->pgtbl_ops;
u64 start_iova = iova;
- for_each_sg(sgt->sgl, sgl, sgt->nents, count) {
+ for_each_sgtable_dma_sg(sgt, sgl, count) {
unsigned long paddr = sg_dma_address(sgl);
size_t len = sg_dma_len(sgl);
@@ -517,10 +517,9 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
if (ret)
goto err_pages;
- if (!dma_map_sg(pfdev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL)) {
- ret = -EINVAL;
+ ret = dma_map_sgtable(pfdev->dev, sgt, DMA_BIDIRECTIONAL, 0);
+ if (ret)
goto err_map;
- }
mmu_map_sg(pfdev, bomapping->mmu, addr,
IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v5 19/38] drm: panfrost: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 19/38] drm: panfrost: " Marek Szyprowski
@ 2020-05-14 13:51 ` Rob Herring
0 siblings, 0 replies; 56+ messages in thread
From: Rob Herring @ 2020-05-14 13:51 UTC (permalink / raw)
To: Marek Szyprowski
Cc: dri-devel, Linux IOMMU, linaro-mm-sig,
linux-kernel@vger.kernel.org, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
David Airlie, Daniel Vetter, Tomeu Vizoso
On Wed, May 13, 2020 at 8:33 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
> returns the number of the created entries in the DMA address space.
> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
> dma_unmap_sg must be called with the original number of the entries
> passed to the dma_map_sg().
>
> struct sg_table is a common structure used for describing a non-contiguous
> memory buffer, used commonly in the DRM and graphics subsystems. It
> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
> and DMA mapped pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, calling DMA-mapping functions with a wrong number of entries or
> ignoring the number of mapped entries returned by the dma_map_sg()
> function.
>
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Steven Price <steven.price@arm.com>
> ---
> For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
> vs. orig_nents misuse' thread:
> https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
> ---
> drivers/gpu/drm/panfrost/panfrost_gem.c | 4 ++--
> drivers/gpu/drm/panfrost/panfrost_mmu.c | 7 +++----
> 2 files changed, 5 insertions(+), 6 deletions(-)
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v5 20/38] drm: radeon: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (17 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 19/38] drm: panfrost: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 21/38] drm: rockchip: use common helper for a scatterlist contiguity check Marek Szyprowski
` (17 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Alex Deucher, Christian König, amd-gfx
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/radeon/radeon_ttm.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 5d50c9e..0e3eb0d 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -481,7 +481,7 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt *ttm)
{
struct radeon_device *rdev = radeon_get_rdev(ttm->bdev);
struct radeon_ttm_tt *gtt = (void *)ttm;
- unsigned pinned = 0, nents;
+ unsigned pinned = 0;
int r;
int write = !(gtt->userflags & RADEON_GEM_USERPTR_READONLY);
@@ -521,9 +521,8 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt *ttm)
if (r)
goto release_sg;
- r = -ENOMEM;
- nents = dma_map_sg(rdev->dev, ttm->sg->sgl, ttm->sg->nents, direction);
- if (nents == 0)
+ r = dma_map_sgtable(rdev->dev, ttm->sg, direction, 0);
+ if (r)
goto release_sg;
drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
@@ -554,9 +553,9 @@ static void radeon_ttm_tt_unpin_userptr(struct ttm_tt *ttm)
return;
/* free the sg table and pages again */
- dma_unmap_sg(rdev->dev, ttm->sg->sgl, ttm->sg->nents, direction);
+ dma_unmap_sgtable(rdev->dev, ttm->sg, direction, 0);
- for_each_sg_page(ttm->sg->sgl, &sg_iter, ttm->sg->nents, 0) {
+ for_each_sgtable_page(ttm->sg, &sg_iter, 0) {
struct page *page = sg_page_iter_page(&sg_iter);
if (!(gtt->userflags & RADEON_GEM_USERPTR_READONLY))
set_page_dirty(page);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 21/38] drm: rockchip: use common helper for a scatterlist contiguity check
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (18 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 20/38] drm: radeon: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 22/38] drm: rockchip: fix common struct sg_table related issues Marek Szyprowski
` (16 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Sandy Huang, Heiko Stübner, linux-rockchip
Use common helper for checking the contiguity of the imported dma-buf.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 19 +------------------
1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index 0d18846..21f8cb2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -460,23 +460,6 @@ struct sg_table *rockchip_gem_prime_get_sg_table(struct drm_gem_object *obj)
return sgt;
}
-static unsigned long rockchip_sg_get_contiguous_size(struct sg_table *sgt,
- int count)
-{
- struct scatterlist *s;
- dma_addr_t expected = sg_dma_address(sgt->sgl);
- unsigned int i;
- unsigned long size = 0;
-
- for_each_sg(sgt->sgl, s, count, i) {
- if (sg_dma_address(s) != expected)
- break;
- expected = sg_dma_address(s) + sg_dma_len(s);
- size += sg_dma_len(s);
- }
- return size;
-}
-
static int
rockchip_gem_iommu_map_sg(struct drm_device *drm,
struct dma_buf_attachment *attach,
@@ -498,7 +481,7 @@ static unsigned long rockchip_sg_get_contiguous_size(struct sg_table *sgt,
if (!count)
return -EINVAL;
- if (rockchip_sg_get_contiguous_size(sg, count) < attach->dmabuf->size) {
+ if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) {
DRM_ERROR("failed to map sg_table to contiguous linear address.\n");
dma_unmap_sg(drm->dev, sg->sgl, sg->nents,
DMA_BIDIRECTIONAL);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 22/38] drm: rockchip: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (19 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 21/38] drm: rockchip: use common helper for a scatterlist contiguity check Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 23/38] drm: tegra: " Marek Szyprowski
` (15 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Sandy Huang, Heiko Stübner, linux-rockchip
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index 21f8cb2..566557b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -36,8 +36,8 @@ static int rockchip_gem_iommu_map(struct rockchip_gem_object *rk_obj)
rk_obj->dma_addr = rk_obj->mm.start;
- ret = iommu_map_sg(private->domain, rk_obj->dma_addr, rk_obj->sgt->sgl,
- rk_obj->sgt->nents, prot);
+ ret = iommu_map_sgtable(private->domain, rk_obj->dma_addr, rk_obj->sgt,
+ prot);
if (ret < rk_obj->base.size) {
DRM_ERROR("failed to map buffer: size=%zd request_size=%zd\n",
ret, rk_obj->base.size);
@@ -98,11 +98,10 @@ static int rockchip_gem_get_pages(struct rockchip_gem_object *rk_obj)
* TODO: Replace this by drm_clflush_sg() once it can be implemented
* without relying on symbols that are not exported.
*/
- for_each_sg(rk_obj->sgt->sgl, s, rk_obj->sgt->nents, i)
+ for_each_sgtable_sg(rk_obj->sgt, s, i)
sg_dma_address(s) = sg_phys(s);
- dma_sync_sg_for_device(drm->dev, rk_obj->sgt->sgl, rk_obj->sgt->nents,
- DMA_TO_DEVICE);
+ dma_sync_sgtable_for_device(drm->dev, rk_obj->sgt, DMA_TO_DEVICE);
return 0;
@@ -350,8 +349,8 @@ void rockchip_gem_free_object(struct drm_gem_object *obj)
if (private->domain) {
rockchip_gem_iommu_unmap(rk_obj);
} else {
- dma_unmap_sg(drm->dev, rk_obj->sgt->sgl,
- rk_obj->sgt->nents, DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(drm->dev, rk_obj->sgt,
+ DMA_BIDIRECTIONAL, 0);
}
drm_prime_gem_destroy(obj, rk_obj->sgt);
} else {
@@ -476,15 +475,13 @@ struct sg_table *rockchip_gem_prime_get_sg_table(struct drm_gem_object *obj)
struct sg_table *sg,
struct rockchip_gem_object *rk_obj)
{
- int count = dma_map_sg(drm->dev, sg->sgl, sg->nents,
- DMA_BIDIRECTIONAL);
- if (!count)
- return -EINVAL;
+ int err = dma_map_sgtable(drm->dev, sg, DMA_BIDIRECTIONAL, 0);
+ if (err)
+ return err;
if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) {
DRM_ERROR("failed to map sg_table to contiguous linear address.\n");
- dma_unmap_sg(drm->dev, sg->sgl, sg->nents,
- DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(drm->dev, sg, DMA_BIDIRECTIONAL, 0);
return -EINVAL;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 23/38] drm: tegra: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (20 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 22/38] drm: rockchip: fix common struct sg_table related issues Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 24/38] drm: v3d: " Marek Szyprowski
` (14 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Thierry Reding, Jonathan Hunter, linux-tegra
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/tegra/gem.c | 27 ++++++++++-----------------
drivers/gpu/drm/tegra/plane.c | 15 +++++----------
2 files changed, 15 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index 6237681..2169130 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -98,8 +98,8 @@ static struct sg_table *tegra_bo_pin(struct device *dev, struct host1x_bo *bo,
* the SG table needs to be copied to avoid overwriting any
* other potential users of the original SG table.
*/
- err = sg_alloc_table_from_sg(sgt, obj->sgt->sgl, obj->sgt->nents,
- GFP_KERNEL);
+ err = sg_alloc_table_from_sg(sgt, obj->sgt->sgl,
+ obj->sgt->orig_nents, GFP_KERNEL);
if (err < 0)
goto free;
} else {
@@ -196,8 +196,7 @@ static int tegra_bo_iommu_map(struct tegra_drm *tegra, struct tegra_bo *bo)
bo->iova = bo->mm->start;
- bo->size = iommu_map_sg(tegra->domain, bo->iova, bo->sgt->sgl,
- bo->sgt->nents, prot);
+ bo->size = iommu_map_sgtable(tegra->domain, bo->iova, bo->sgt, prot);
if (!bo->size) {
dev_err(tegra->drm->dev, "failed to map buffer\n");
err = -ENOMEM;
@@ -264,8 +263,7 @@ static struct tegra_bo *tegra_bo_alloc_object(struct drm_device *drm,
static void tegra_bo_free(struct drm_device *drm, struct tegra_bo *bo)
{
if (bo->pages) {
- dma_unmap_sg(drm->dev, bo->sgt->sgl, bo->sgt->nents,
- DMA_FROM_DEVICE);
+ dma_unmap_sgtable(drm->dev, bo->sgt, DMA_FROM_DEVICE, 0);
drm_gem_put_pages(&bo->gem, bo->pages, true, true);
sg_free_table(bo->sgt);
kfree(bo->sgt);
@@ -290,12 +288,9 @@ static int tegra_bo_get_pages(struct drm_device *drm, struct tegra_bo *bo)
goto put_pages;
}
- err = dma_map_sg(drm->dev, bo->sgt->sgl, bo->sgt->nents,
- DMA_FROM_DEVICE);
- if (err == 0) {
- err = -EFAULT;
+ err = dma_map_sgtable(drm->dev, bo->sgt, DMA_FROM_DEVICE, 0);
+ if (err)
goto free_sgt;
- }
return 0;
@@ -571,7 +566,7 @@ int tegra_drm_mmap(struct file *file, struct vm_area_struct *vma)
goto free;
}
- if (dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir) == 0)
+ if (dma_map_sgtable(attach->dev, sgt, dir, 0))
goto free;
return sgt;
@@ -590,7 +585,7 @@ static void tegra_gem_prime_unmap_dma_buf(struct dma_buf_attachment *attach,
struct tegra_bo *bo = to_tegra_bo(gem);
if (bo->pages)
- dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+ dma_unmap_sgtable(attach->dev, sgt, dir, 0);
sg_free_table(sgt);
kfree(sgt);
@@ -609,8 +604,7 @@ static int tegra_gem_prime_begin_cpu_access(struct dma_buf *buf,
struct drm_device *drm = gem->dev;
if (bo->pages)
- dma_sync_sg_for_cpu(drm->dev, bo->sgt->sgl, bo->sgt->nents,
- DMA_FROM_DEVICE);
+ dma_sync_sgtable_for_cpu(drm->dev, bo->sgt, DMA_FROM_DEVICE);
return 0;
}
@@ -623,8 +617,7 @@ static int tegra_gem_prime_end_cpu_access(struct dma_buf *buf,
struct drm_device *drm = gem->dev;
if (bo->pages)
- dma_sync_sg_for_device(drm->dev, bo->sgt->sgl, bo->sgt->nents,
- DMA_TO_DEVICE);
+ dma_sync_sgtable_for_device(drm->dev, bo->sgt, DMA_TO_DEVICE);
return 0;
}
diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c
index 9ccfb56..0d2ef16 100644
--- a/drivers/gpu/drm/tegra/plane.c
+++ b/drivers/gpu/drm/tegra/plane.c
@@ -130,12 +130,9 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state)
}
if (sgt) {
- err = dma_map_sg(dc->dev, sgt->sgl, sgt->nents,
- DMA_TO_DEVICE);
- if (err == 0) {
- err = -ENOMEM;
+ err = dma_map_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
+ if (err)
goto unpin;
- }
/*
* The display controller needs contiguous memory, so
@@ -143,7 +140,7 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state)
* map its SG table to a single contiguous chunk of
* I/O virtual memory.
*/
- if (err > 1) {
+ if (sgt->nents > 1) {
err = -EINVAL;
goto unpin;
}
@@ -165,8 +162,7 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state)
struct sg_table *sgt = state->sgt[i];
if (sgt)
- dma_unmap_sg(dc->dev, sgt->sgl, sgt->nents,
- DMA_TO_DEVICE);
+ dma_unmap_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
host1x_bo_unpin(dc->dev, &bo->base, sgt);
state->iova[i] = DMA_MAPPING_ERROR;
@@ -185,8 +181,7 @@ static void tegra_dc_unpin(struct tegra_dc *dc, struct tegra_plane_state *state)
struct sg_table *sgt = state->sgt[i];
if (sgt)
- dma_unmap_sg(dc->dev, sgt->sgl, sgt->nents,
- DMA_TO_DEVICE);
+ dma_unmap_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
host1x_bo_unpin(dc->dev, &bo->base, sgt);
state->iova[i] = DMA_MAPPING_ERROR;
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 24/38] drm: v3d: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (21 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 23/38] drm: tegra: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 21:51 ` Eric Anholt
2020-05-13 13:32 ` [PATCH v5 25/38] drm: virtio: " Marek Szyprowski
` (13 subsequent siblings)
36 siblings, 1 reply; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Eric Anholt
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/v3d/v3d_mmu.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_mmu.c b/drivers/gpu/drm/v3d/v3d_mmu.c
index 3b81ea2..175c257 100644
--- a/drivers/gpu/drm/v3d/v3d_mmu.c
+++ b/drivers/gpu/drm/v3d/v3d_mmu.c
@@ -90,19 +90,16 @@ void v3d_mmu_insert_ptes(struct v3d_bo *bo)
struct v3d_dev *v3d = to_v3d_dev(shmem_obj->base.dev);
u32 page = bo->node.start;
u32 page_prot = V3D_PTE_WRITEABLE | V3D_PTE_VALID;
- unsigned int count;
- struct scatterlist *sgl;
+ struct sg_dma_page_iter dma_iter;
- for_each_sg(shmem_obj->sgt->sgl, sgl, shmem_obj->sgt->nents, count) {
- u32 page_address = sg_dma_address(sgl) >> V3D_MMU_PAGE_SHIFT;
+ for_each_sgtable_dma_page(shmem_obj->sgt, &dma_iter, 0) {
+ dma_addr_t dma_addr = sg_page_iter_dma_address(&dma_iter);
+ u32 page_address = dma_addr >> V3D_MMU_PAGE_SHIFT;
u32 pte = page_prot | page_address;
- u32 i;
- BUG_ON(page_address + (sg_dma_len(sgl) >> V3D_MMU_PAGE_SHIFT) >=
- BIT(24));
-
- for (i = 0; i < sg_dma_len(sgl) >> V3D_MMU_PAGE_SHIFT; i++)
- v3d->pt[page++] = pte + i;
+ BUILD_BUG_ON(V3D_MMU_PAGE_SHIFT != PAGE_SIZE);
+ BUG_ON(page_address + 1 >= BIT(24));
+ v3d->pt[page++] = pte;
}
WARN_ON_ONCE(page - bo->node.start !=
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v5 24/38] drm: v3d: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 24/38] drm: v3d: " Marek Szyprowski
@ 2020-05-13 21:51 ` Eric Anholt
0 siblings, 0 replies; 56+ messages in thread
From: Eric Anholt @ 2020-05-13 21:51 UTC (permalink / raw)
To: Marek Szyprowski
Cc: DRI Development, iommu, linaro-mm-sig, linux-kernel,
Christoph Hellwig, Robin Murphy, Bartlomiej Zolnierkiewicz,
linux-arm-kernel, David Airlie, Daniel Vetter
On Wed, May 13, 2020 at 6:33 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
> returns the number of the created entries in the DMA address space.
> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
> dma_unmap_sg must be called with the original number of the entries
> passed to the dma_map_sg().
>
> struct sg_table is a common structure used for describing a non-contiguous
> memory buffer, used commonly in the DRM and graphics subsystems. It
> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
> and DMA mapped pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, calling DMA-mapping functions with a wrong number of entries or
> ignoring the number of mapped entries returned by the dma_map_sg()
> function.
>
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v5 25/38] drm: virtio: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (22 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 24/38] drm: v3d: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-15 10:12 ` Gerd Hoffmann
2020-05-13 13:32 ` [PATCH v5 26/38] drm: vmwgfx: " Marek Szyprowski
` (12 subsequent siblings)
36 siblings, 1 reply; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Gerd Hoffmann, virtualization
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/virtio/virtgpu_object.c | 36 +++++++++++++++++++--------------
drivers/gpu/drm/virtio/virtgpu_vq.c | 12 +++++------
2 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index 6ccbd01..399556f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -72,9 +72,8 @@ void virtio_gpu_cleanup_object(struct virtio_gpu_object *bo)
if (shmem->pages) {
if (shmem->mapped) {
- dma_unmap_sg(vgdev->vdev->dev.parent,
- shmem->pages->sgl, shmem->mapped,
- DMA_TO_DEVICE);
+ dma_unmap_sgtable(vgdev->vdev->dev.parent,
+ shmem->pages, DMA_TO_DEVICE, 0);
shmem->mapped = 0;
}
@@ -157,13 +156,13 @@ static int virtio_gpu_object_shmem_init(struct virtio_gpu_device *vgdev,
}
if (use_dma_api) {
- shmem->mapped = dma_map_sg(vgdev->vdev->dev.parent,
- shmem->pages->sgl,
- shmem->pages->nents,
- DMA_TO_DEVICE);
- *nents = shmem->mapped;
+ ret = dma_map_sgtable(vgdev->vdev->dev.parent,
+ shmem->pages, DMA_TO_DEVICE, 0);
+ if (ret)
+ return ret;
+ *nents = shmem->mapped = shmem->pages->nents;
} else {
- *nents = shmem->pages->nents;
+ *nents = shmem->pages->orig_nents;
}
*ents = kmalloc_array(*nents, sizeof(struct virtio_gpu_mem_entry),
@@ -173,13 +172,20 @@ static int virtio_gpu_object_shmem_init(struct virtio_gpu_device *vgdev,
return -ENOMEM;
}
- for_each_sg(shmem->pages->sgl, sg, *nents, si) {
- (*ents)[si].addr = cpu_to_le64(use_dma_api
- ? sg_dma_address(sg)
- : sg_phys(sg));
- (*ents)[si].length = cpu_to_le32(sg->length);
- (*ents)[si].padding = 0;
+ if (use_dma_api) {
+ for_each_sgtable_dma_sg(shmem->pages, sg, si) {
+ (*ents)[si].addr = cpu_to_le64(sg_dma_address(sg));
+ (*ents)[si].length = cpu_to_le32(sg_dma_len(sg));
+ (*ents)[si].padding = 0;
+ }
+ } else {
+ for_each_sgtable_sg(shmem->pages, sg, si) {
+ (*ents)[si].addr = cpu_to_le64(sg_phys(sg));
+ (*ents)[si].length = cpu_to_le32(sg->length);
+ (*ents)[si].padding = 0;
+ }
}
+
return 0;
}
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 9e663a5..e5765db 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -302,7 +302,7 @@ static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size, int *sg_ents)
return NULL;
}
- for_each_sg(sgt->sgl, sg, *sg_ents, i) {
+ for_each_sgtable_sg(sgt, sg, i) {
pg = vmalloc_to_page(data);
if (!pg) {
sg_free_table(sgt);
@@ -603,9 +603,8 @@ void virtio_gpu_cmd_transfer_to_host_2d(struct virtio_gpu_device *vgdev,
struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);
if (use_dma_api)
- dma_sync_sg_for_device(vgdev->vdev->dev.parent,
- shmem->pages->sgl, shmem->pages->nents,
- DMA_TO_DEVICE);
+ dma_sync_sgtable_for_device(vgdev->vdev->dev.parent,
+ shmem->pages, DMA_TO_DEVICE);
cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
memset(cmd_p, 0, sizeof(*cmd_p));
@@ -1019,9 +1018,8 @@ void virtio_gpu_cmd_transfer_to_host_3d(struct virtio_gpu_device *vgdev,
struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);
if (use_dma_api)
- dma_sync_sg_for_device(vgdev->vdev->dev.parent,
- shmem->pages->sgl, shmem->pages->nents,
- DMA_TO_DEVICE);
+ dma_sync_sgtable_for_device(vgdev->vdev->dev.parent,
+ shmem->pages, DMA_TO_DEVICE);
cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
memset(cmd_p, 0, sizeof(*cmd_p));
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v5 25/38] drm: virtio: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 25/38] drm: virtio: " Marek Szyprowski
@ 2020-05-15 10:12 ` Gerd Hoffmann
0 siblings, 0 replies; 56+ messages in thread
From: Gerd Hoffmann @ 2020-05-15 10:12 UTC (permalink / raw)
To: Marek Szyprowski
Cc: dri-devel, iommu, linaro-mm-sig, linux-kernel, Christoph Hellwig,
Robin Murphy, Bartlomiej Zolnierkiewicz, linux-arm-kernel,
David Airlie, Daniel Vetter, virtualization
On Wed, May 13, 2020 at 03:32:32PM +0200, Marek Szyprowski wrote:
> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
> returns the number of the created entries in the DMA address space.
> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
> dma_unmap_sg must be called with the original number of the entries
> passed to the dma_map_sg().
>
> struct sg_table is a common structure used for describing a non-contiguous
> memory buffer, used commonly in the DRM and graphics subsystems. It
> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
> and DMA mapped pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, calling DMA-mapping functions with a wrong number of entries or
> ignoring the number of mapped entries returned by the dma_map_sg()
> function.
>
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
Looks all sane.
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
take care,
Gerd
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v5 26/38] drm: vmwgfx: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (23 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 25/38] drm: virtio: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 27/38] xen: gntdev: " Marek Szyprowski
` (11 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, VMware Graphics, Thomas Hellstrom
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Roland Scheidegger <sroland@vmware.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
index bf0bc46..49ed6ad 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
@@ -362,8 +362,7 @@ static void vmw_ttm_unmap_from_dma(struct vmw_ttm_tt *vmw_tt)
{
struct device *dev = vmw_tt->dev_priv->dev->dev;
- dma_unmap_sg(dev, vmw_tt->sgt.sgl, vmw_tt->sgt.nents,
- DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(dev, vmw_tt->sgt, DMA_BIDIRECTIONAL, 0);
vmw_tt->sgt.nents = vmw_tt->sgt.orig_nents;
}
@@ -383,16 +382,8 @@ static void vmw_ttm_unmap_from_dma(struct vmw_ttm_tt *vmw_tt)
static int vmw_ttm_map_for_dma(struct vmw_ttm_tt *vmw_tt)
{
struct device *dev = vmw_tt->dev_priv->dev->dev;
- int ret;
-
- ret = dma_map_sg(dev, vmw_tt->sgt.sgl, vmw_tt->sgt.orig_nents,
- DMA_BIDIRECTIONAL);
- if (unlikely(ret == 0))
- return -ENOMEM;
- vmw_tt->sgt.nents = ret;
-
- return 0;
+ return dma_map_sgtable(dev, vmw_tt->sgt, DMA_BIDIRECTIONAL, 0);
}
/**
@@ -449,10 +440,10 @@ static int vmw_ttm_map_dma(struct vmw_ttm_tt *vmw_tt)
if (unlikely(ret != 0))
goto out_sg_alloc_fail;
- if (vsgt->num_pages > vmw_tt->sgt.nents) {
+ if (vsgt->num_pages > vmw_tt->sgt.orig_nents) {
uint64_t over_alloc =
sgl_size * (vsgt->num_pages -
- vmw_tt->sgt.nents);
+ vmw_tt->sgt.orig_nents);
ttm_mem_global_free(glob, over_alloc);
vmw_tt->sg_alloc_size -= over_alloc;
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 27/38] xen: gntdev: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (24 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 26/38] drm: vmwgfx: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-18 5:17 ` Jürgen Groß
2020-05-13 13:32 ` [PATCH v5 28/38] drm: host1x: " Marek Szyprowski
` (10 subsequent siblings)
36 siblings, 1 reply; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Boris Ostrovsky, Juergen Gross, xen-devel
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/xen/gntdev-dmabuf.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index 75d3bb9..ba6cad8 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -247,10 +247,9 @@ static void dmabuf_exp_ops_detach(struct dma_buf *dma_buf,
if (sgt) {
if (gntdev_dmabuf_attach->dir != DMA_NONE)
- dma_unmap_sg_attrs(attach->dev, sgt->sgl,
- sgt->nents,
- gntdev_dmabuf_attach->dir,
- DMA_ATTR_SKIP_CPU_SYNC);
+ dma_unmap_sgtable(attach->dev, sgt,
+ gntdev_dmabuf_attach->dir,
+ DMA_ATTR_SKIP_CPU_SYNC);
sg_free_table(sgt);
}
@@ -288,8 +287,8 @@ static void dmabuf_exp_ops_detach(struct dma_buf *dma_buf,
sgt = dmabuf_pages_to_sgt(gntdev_dmabuf->pages,
gntdev_dmabuf->nr_pages);
if (!IS_ERR(sgt)) {
- if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
- DMA_ATTR_SKIP_CPU_SYNC)) {
+ if (dma_map_sgtable(attach->dev, sgt, dir,
+ DMA_ATTR_SKIP_CPU_SYNC)) {
sg_free_table(sgt);
kfree(sgt);
sgt = ERR_PTR(-ENOMEM);
@@ -625,7 +624,7 @@ static struct gntdev_dmabuf *dmabuf_imp_alloc_storage(int count)
/* Now convert sgt to array of pages and check for page validity. */
i = 0;
- for_each_sg_page(sgt->sgl, &sg_iter, sgt->nents, 0) {
+ for_each_sgtable_page(sgt, &sg_iter, 0) {
struct page *page = sg_page_iter_page(&sg_iter);
/*
* Check if page is valid: this can happen if we are given
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v5 27/38] xen: gntdev: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 27/38] xen: gntdev: " Marek Szyprowski
@ 2020-05-18 5:17 ` Jürgen Groß
0 siblings, 0 replies; 56+ messages in thread
From: Jürgen Groß @ 2020-05-18 5:17 UTC (permalink / raw)
To: Marek Szyprowski, dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Christoph Hellwig, Robin Murphy, Bartlomiej Zolnierkiewicz,
linux-arm-kernel, David Airlie, Daniel Vetter, Boris Ostrovsky,
xen-devel
On 13.05.20 15:32, Marek Szyprowski wrote:
> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
> returns the number of the created entries in the DMA address space.
> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
> dma_unmap_sg must be called with the original number of the entries
> passed to the dma_map_sg().
>
> struct sg_table is a common structure used for describing a non-contiguous
> memory buffer, used commonly in the DRM and graphics subsystems. It
> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
> and DMA mapped pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, calling DMA-mapping functions with a wrong number of entries or
> ignoring the number of mapped entries returned by the dma_map_sg()
> function.
>
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Juergen Gross <jgross@suse.com>
Juergen
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v5 28/38] drm: host1x: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (25 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 27/38] xen: gntdev: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 29/38] drm: rcar-du: " Marek Szyprowski
` (9 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Thierry Reding, linux-tegra
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/host1x/job.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index a10643a..4832b57 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -166,11 +166,9 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
goto unpin;
}
- err = dma_map_sg(dev, sgt->sgl, sgt->nents, dir);
- if (!err) {
- err = -ENOMEM;
+ err = dma_map_sgtable(dev, sgt, dir, 0);
+ if (err)
goto unpin;
- }
job->unpins[job->num_unpins].dev = dev;
job->unpins[job->num_unpins].dir = dir;
@@ -217,7 +215,7 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
}
if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && host->domain) {
- for_each_sg(sgt->sgl, sg, sgt->nents, j)
+ for_each_sgtable_sg(sgt, sg, j)
gather_size += sg->length;
gather_size = iova_align(&host->iova, gather_size);
@@ -229,9 +227,9 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
goto unpin;
}
- err = iommu_map_sg(host->domain,
+ err = iommu_map_sgtable(host->domain,
iova_dma_addr(&host->iova, alloc),
- sgt->sgl, sgt->nents, IOMMU_READ);
+ sgt, IOMMU_READ);
if (err == 0) {
__free_iova(&host->iova, alloc);
err = -EINVAL;
@@ -241,12 +239,9 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
job->unpins[job->num_unpins].size = gather_size;
phys_addr = iova_dma_addr(&host->iova, alloc);
} else if (sgt) {
- err = dma_map_sg(host->dev, sgt->sgl, sgt->nents,
- DMA_TO_DEVICE);
- if (!err) {
- err = -ENOMEM;
+ err = dma_map_sgtable(host->dev, sgt, DMA_TO_DEVICE, 0);
+ if (err)
goto unpin;
- }
job->unpins[job->num_unpins].dir = DMA_TO_DEVICE;
job->unpins[job->num_unpins].dev = host->dev;
@@ -647,8 +642,7 @@ void host1x_job_unpin(struct host1x_job *job)
}
if (unpin->dev && sgt)
- dma_unmap_sg(unpin->dev, sgt->sgl, sgt->nents,
- unpin->dir);
+ dma_unmap_sgtable(unpin->dev, sgt, unpin->dir, 0);
host1x_bo_unpin(dev, unpin->bo, sgt);
host1x_bo_put(unpin->bo);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 29/38] drm: rcar-du: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (26 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 28/38] drm: host1x: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 14:23 ` Laurent Pinchart
2020-05-13 13:32 ` [PATCH v5 30/38] dmabuf: " Marek Szyprowski
` (8 subsequent siblings)
36 siblings, 1 reply; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Mauro Carvalho Chehab, Laurent Pinchart,
Kieran Bingham, linux-renesas-soc, linux-media
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
dma_map_sgtable() function returns zero or an error code, so adjust the
return value check for the vsp1_du_map_sg() function.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 +--
drivers/media/platform/vsp1/vsp1_drm.c | 8 ++++----
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 5e4faf2..2fc1816 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -197,9 +197,8 @@ int rcar_du_vsp_map_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
goto fail;
ret = vsp1_du_map_sg(vsp->vsp, sgt);
- if (!ret) {
+ if (ret) {
sg_free_table(sgt);
- ret = -ENOMEM;
goto fail;
}
}
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index a4a45d6..86d5e3f 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -912,8 +912,8 @@ int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt)
* skip cache sync. This will need to be revisited when support for
* non-coherent buffers will be added to the DU driver.
*/
- return dma_map_sg_attrs(vsp1->bus_master, sgt->sgl, sgt->nents,
- DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
+ return dma_map_sgtable(vsp1->bus_master, sgt, DMA_TO_DEVICE,
+ DMA_ATTR_SKIP_CPU_SYNC);
}
EXPORT_SYMBOL_GPL(vsp1_du_map_sg);
@@ -921,8 +921,8 @@ void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt)
{
struct vsp1_device *vsp1 = dev_get_drvdata(dev);
- dma_unmap_sg_attrs(vsp1->bus_master, sgt->sgl, sgt->nents,
- DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
+ dma_unmap_sgtable(vsp1->bus_master, sgt, DMA_TO_DEVICE,
+ DMA_ATTR_SKIP_CPU_SYNC);
}
EXPORT_SYMBOL_GPL(vsp1_du_unmap_sg);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v5 29/38] drm: rcar-du: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 29/38] drm: rcar-du: " Marek Szyprowski
@ 2020-05-13 14:23 ` Laurent Pinchart
0 siblings, 0 replies; 56+ messages in thread
From: Laurent Pinchart @ 2020-05-13 14:23 UTC (permalink / raw)
To: Marek Szyprowski
Cc: dri-devel, iommu, linaro-mm-sig, linux-kernel, Christoph Hellwig,
Robin Murphy, Bartlomiej Zolnierkiewicz, linux-arm-kernel,
David Airlie, Daniel Vetter, Mauro Carvalho Chehab,
Kieran Bingham, linux-renesas-soc, linux-media
Hi Marek,
Thank you for the patch.
On Wed, May 13, 2020 at 03:32:36PM +0200, Marek Szyprowski wrote:
> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
> returns the number of the created entries in the DMA address space.
> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
> dma_unmap_sg must be called with the original number of the entries
> passed to the dma_map_sg().
>
> struct sg_table is a common structure used for describing a non-contiguous
> memory buffer, used commonly in the DRM and graphics subsystems. It
> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
> and DMA mapped pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, calling DMA-mapping functions with a wrong number of entries or
> ignoring the number of mapped entries returned by the dma_map_sg()
> function.
>
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
>
> dma_map_sgtable() function returns zero or an error code, so adjust the
> return value check for the vsp1_du_map_sg() function.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
This is a very nice simplification, I've always foudn the dma_map_sg API
cumbersome to use.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
I assume you will get the whole series merged in one go. If I need to
pick the patch up at any point, please let me know. Otherwise I'll wait
until I see it upstream, no reply needed :-)
> ---
> For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
> vs. orig_nents misuse' thread:
> https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
> ---
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 +--
> drivers/media/platform/vsp1/vsp1_drm.c | 8 ++++----
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 5e4faf2..2fc1816 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -197,9 +197,8 @@ int rcar_du_vsp_map_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
> goto fail;
>
> ret = vsp1_du_map_sg(vsp->vsp, sgt);
> - if (!ret) {
> + if (ret) {
> sg_free_table(sgt);
> - ret = -ENOMEM;
> goto fail;
> }
> }
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index a4a45d6..86d5e3f 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -912,8 +912,8 @@ int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt)
> * skip cache sync. This will need to be revisited when support for
> * non-coherent buffers will be added to the DU driver.
> */
> - return dma_map_sg_attrs(vsp1->bus_master, sgt->sgl, sgt->nents,
> - DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> + return dma_map_sgtable(vsp1->bus_master, sgt, DMA_TO_DEVICE,
> + DMA_ATTR_SKIP_CPU_SYNC);
> }
> EXPORT_SYMBOL_GPL(vsp1_du_map_sg);
>
> @@ -921,8 +921,8 @@ void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt)
> {
> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>
> - dma_unmap_sg_attrs(vsp1->bus_master, sgt->sgl, sgt->nents,
> - DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> + dma_unmap_sgtable(vsp1->bus_master, sgt, DMA_TO_DEVICE,
> + DMA_ATTR_SKIP_CPU_SYNC);
> }
> EXPORT_SYMBOL_GPL(vsp1_du_unmap_sg);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v5 30/38] dmabuf: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (27 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 29/38] drm: rcar-du: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-15 10:13 ` Gerd Hoffmann
2020-05-13 13:32 ` [PATCH v5 31/38] staging: ion: remove dead code Marek Szyprowski
` (7 subsequent siblings)
36 siblings, 1 reply; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Sumit Semwal, Gerd Hoffmann, linux-media
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/dma-buf/heaps/heap-helpers.c | 13 ++++++-------
drivers/dma-buf/udmabuf.c | 7 +++----
2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/dma-buf/heaps/heap-helpers.c b/drivers/dma-buf/heaps/heap-helpers.c
index 9f964ca..d0696cf 100644
--- a/drivers/dma-buf/heaps/heap-helpers.c
+++ b/drivers/dma-buf/heaps/heap-helpers.c
@@ -140,13 +140,12 @@ struct sg_table *dma_heap_map_dma_buf(struct dma_buf_attachment *attachment,
enum dma_data_direction direction)
{
struct dma_heaps_attachment *a = attachment->priv;
- struct sg_table *table;
-
- table = &a->table;
+ struct sg_table *table = &a->table;
+ int ret;
- if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
- direction))
- table = ERR_PTR(-ENOMEM);
+ ret = dma_map_sgtable(attachment->dev, table, direction, 0);
+ if (ret)
+ table = ERR_PTR(ret);
return table;
}
@@ -154,7 +153,7 @@ static void dma_heap_unmap_dma_buf(struct dma_buf_attachment *attachment,
struct sg_table *table,
enum dma_data_direction direction)
{
- dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
+ dma_unmap_sgtable(attachment->dev, table, direction, 0);
}
static vm_fault_t dma_heap_vm_fault(struct vm_fault *vmf)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index acb26c6..89e293b 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -63,10 +63,9 @@ static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
GFP_KERNEL);
if (ret < 0)
goto err;
- if (!dma_map_sg(dev, sg->sgl, sg->nents, direction)) {
- ret = -EINVAL;
+ ret = dma_map_sgtable(dev, sg, direction, 0);
+ if (ret < 0)
goto err;
- }
return sg;
err:
@@ -78,7 +77,7 @@ static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
static void put_sg_table(struct device *dev, struct sg_table *sg,
enum dma_data_direction direction)
{
- dma_unmap_sg(dev, sg->sgl, sg->nents, direction);
+ dma_unmap_sgtable(dev, sg, direction, 0);
sg_free_table(sg);
kfree(sg);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v5 30/38] dmabuf: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 30/38] dmabuf: " Marek Szyprowski
@ 2020-05-15 10:13 ` Gerd Hoffmann
0 siblings, 0 replies; 56+ messages in thread
From: Gerd Hoffmann @ 2020-05-15 10:13 UTC (permalink / raw)
To: Marek Szyprowski
Cc: dri-devel, iommu, linaro-mm-sig, linux-kernel, Christoph Hellwig,
Robin Murphy, Bartlomiej Zolnierkiewicz, linux-arm-kernel,
David Airlie, Daniel Vetter, Sumit Semwal, linux-media
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index acb26c6..89e293b 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -63,10 +63,9 @@ static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
> GFP_KERNEL);
> if (ret < 0)
> goto err;
> - if (!dma_map_sg(dev, sg->sgl, sg->nents, direction)) {
> - ret = -EINVAL;
> + ret = dma_map_sgtable(dev, sg, direction, 0);
> + if (ret < 0)
> goto err;
> - }
> return sg;
>
> err:
> @@ -78,7 +77,7 @@ static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
> static void put_sg_table(struct device *dev, struct sg_table *sg,
> enum dma_data_direction direction)
> {
> - dma_unmap_sg(dev, sg->sgl, sg->nents, direction);
> + dma_unmap_sgtable(dev, sg, direction, 0);
> sg_free_table(sg);
> kfree(sg);
> }
Easy straightforward conversation.
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
take care,
Gerd
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v5 31/38] staging: ion: remove dead code
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (28 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 30/38] dmabuf: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 32/38] staging: ion: fix common struct sg_table related issues Marek Szyprowski
` (6 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Laura Abbott, Sumit Semwal, Greg Kroah-Hartman,
Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
Joel Fernandes, Christian Brauner, devel
ion_heap_pages_zero() function is not used at all, so remove it to
simplify the ion_heap_sglist_zero() function later.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/staging/android/ion/ion.h | 1 -
drivers/staging/android/ion/ion_heap.c | 9 ---------
2 files changed, 10 deletions(-)
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index 74914a2..c199e88 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -177,7 +177,6 @@ struct ion_heap {
int ion_heap_map_user(struct ion_heap *heap, struct ion_buffer *buffer,
struct vm_area_struct *vma);
int ion_heap_buffer_zero(struct ion_buffer *buffer);
-int ion_heap_pages_zero(struct page *page, size_t size, pgprot_t pgprot);
/**
* ion_heap_init_shrinker
diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c
index 0755b11..9c23b23 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -145,15 +145,6 @@ int ion_heap_buffer_zero(struct ion_buffer *buffer)
return ion_heap_sglist_zero(table->sgl, table->nents, pgprot);
}
-int ion_heap_pages_zero(struct page *page, size_t size, pgprot_t pgprot)
-{
- struct scatterlist sg;
-
- sg_init_table(&sg, 1);
- sg_set_page(&sg, page, size, 0);
- return ion_heap_sglist_zero(&sg, 1, pgprot);
-}
-
void ion_heap_freelist_add(struct ion_heap *heap, struct ion_buffer *buffer)
{
spin_lock(&heap->free_lock);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 32/38] staging: ion: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (29 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 31/38] staging: ion: remove dead code Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 33/38] staging: tegra-vde: " Marek Szyprowski
` (5 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Laura Abbott, Sumit Semwal, Greg Kroah-Hartman,
Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
Joel Fernandes, Christian Brauner, devel
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/staging/android/ion/ion.c | 25 +++++++--------
drivers/staging/android/ion/ion_heap.c | 44 ++++++++-------------------
drivers/staging/android/ion/ion_system_heap.c | 2 +-
3 files changed, 25 insertions(+), 46 deletions(-)
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 38b51ea..3c9f095 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -147,14 +147,14 @@ static struct sg_table *dup_sg_table(struct sg_table *table)
if (!new_table)
return ERR_PTR(-ENOMEM);
- ret = sg_alloc_table(new_table, table->nents, GFP_KERNEL);
+ ret = sg_alloc_table(new_table, table->orig_nents, GFP_KERNEL);
if (ret) {
kfree(new_table);
return ERR_PTR(-ENOMEM);
}
new_sg = new_table->sgl;
- for_each_sg(table->sgl, sg, table->nents, i) {
+ for_each_sgtable_sg(table, sg, i) {
memcpy(new_sg, sg, sizeof(*sg));
new_sg->dma_address = 0;
new_sg = sg_next(new_sg);
@@ -224,12 +224,13 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
{
struct ion_dma_buf_attachment *a = attachment->priv;
struct sg_table *table;
+ int ret;
table = a->table;
- if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
- direction))
- return ERR_PTR(-ENOMEM);
+ ret = dma_map_sgtable(attachment->dev, table, direction, 0);
+ if (ret)
+ return ERR_PTR(ret);
return table;
}
@@ -238,7 +239,7 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
struct sg_table *table,
enum dma_data_direction direction)
{
- dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
+ dma_unmap_sgtable(attachment->dev, table, direction, 0);
}
static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
@@ -296,10 +297,8 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
}
mutex_lock(&buffer->lock);
- list_for_each_entry(a, &buffer->attachments, list) {
- dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
- direction);
- }
+ list_for_each_entry(a, &buffer->attachments, list)
+ dma_sync_sgtable_for_cpu(a->dev, a->table, direction);
unlock:
mutex_unlock(&buffer->lock);
@@ -319,10 +318,8 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
}
mutex_lock(&buffer->lock);
- list_for_each_entry(a, &buffer->attachments, list) {
- dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents,
- direction);
- }
+ list_for_each_entry(a, &buffer->attachments, list)
+ dma_sync_sgtable_for_device(a->dev, a->table, direction);
mutex_unlock(&buffer->lock);
return 0;
diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c
index 9c23b23..79f2794 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -20,8 +20,7 @@
void *ion_heap_map_kernel(struct ion_heap *heap,
struct ion_buffer *buffer)
{
- struct scatterlist *sg;
- int i, j;
+ struct sg_page_iter piter;
void *vaddr;
pgprot_t pgprot;
struct sg_table *table = buffer->sg_table;
@@ -38,14 +37,11 @@ void *ion_heap_map_kernel(struct ion_heap *heap,
else
pgprot = pgprot_writecombine(PAGE_KERNEL);
- for_each_sg(table->sgl, sg, table->nents, i) {
- int npages_this_entry = PAGE_ALIGN(sg->length) / PAGE_SIZE;
- struct page *page = sg_page(sg);
-
- BUG_ON(i >= npages);
- for (j = 0; j < npages_this_entry; j++)
- *(tmp++) = page++;
+ for_each_sgtable_page(table, &piter, 0) {
+ BUG_ON(tmp - pages >= npages);
+ *tmp++ = sg_page_iter_page(&piter);
}
+
vaddr = vmap(pages, npages, VM_MAP, pgprot);
vfree(pages);
@@ -64,32 +60,19 @@ void ion_heap_unmap_kernel(struct ion_heap *heap,
int ion_heap_map_user(struct ion_heap *heap, struct ion_buffer *buffer,
struct vm_area_struct *vma)
{
+ struct sg_page_iter piter;
struct sg_table *table = buffer->sg_table;
unsigned long addr = vma->vm_start;
- unsigned long offset = vma->vm_pgoff * PAGE_SIZE;
- struct scatterlist *sg;
- int i;
int ret;
- for_each_sg(table->sgl, sg, table->nents, i) {
- struct page *page = sg_page(sg);
- unsigned long remainder = vma->vm_end - addr;
- unsigned long len = sg->length;
+ for_each_sgtable_page(table, &piter, vma->vm_pgoff) {
+ struct page *page = sg_page_iter_page(&piter);
- if (offset >= sg->length) {
- offset -= sg->length;
- continue;
- } else if (offset) {
- page += offset / PAGE_SIZE;
- len = sg->length - offset;
- offset = 0;
- }
- len = min(len, remainder);
- ret = remap_pfn_range(vma, addr, page_to_pfn(page), len,
+ ret = remap_pfn_range(vma, addr, page_to_pfn(page), PAGE_SIZE,
vma->vm_page_prot);
if (ret)
return ret;
- addr += len;
+ addr += PAGE_SIZE;
if (addr >= vma->vm_end)
return 0;
}
@@ -109,15 +92,14 @@ static int ion_heap_clear_pages(struct page **pages, int num, pgprot_t pgprot)
return 0;
}
-static int ion_heap_sglist_zero(struct scatterlist *sgl, unsigned int nents,
- pgprot_t pgprot)
+static int ion_heap_sglist_zero(struct sg_table *sgt, pgprot_t pgprot)
{
int p = 0;
int ret = 0;
struct sg_page_iter piter;
struct page *pages[32];
- for_each_sg_page(sgl, &piter, nents, 0) {
+ for_each_sgtable_page(sgt, &piter, 0) {
pages[p++] = sg_page_iter_page(&piter);
if (p == ARRAY_SIZE(pages)) {
ret = ion_heap_clear_pages(pages, p, pgprot);
@@ -142,7 +124,7 @@ int ion_heap_buffer_zero(struct ion_buffer *buffer)
else
pgprot = pgprot_writecombine(PAGE_KERNEL);
- return ion_heap_sglist_zero(table->sgl, table->nents, pgprot);
+ return ion_heap_sglist_zero(table, pgprot);
}
void ion_heap_freelist_add(struct ion_heap *heap, struct ion_buffer *buffer)
diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
index b83a1d1..eac0632 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -162,7 +162,7 @@ static void ion_system_heap_free(struct ion_buffer *buffer)
if (!(buffer->private_flags & ION_PRIV_FLAG_SHRINKER_FREE))
ion_heap_buffer_zero(buffer);
- for_each_sg(table->sgl, sg, table->nents, i)
+ for_each_sgtable_sg(table, sg, i)
free_buffer_page(sys_heap, buffer, sg_page(sg));
sg_free_table(table);
kfree(table);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 33/38] staging: tegra-vde: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (30 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 32/38] staging: ion: fix common struct sg_table related issues Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-14 20:07 ` Dmitry Osipenko
2020-05-13 13:32 ` [PATCH v5 34/38] misc: fastrpc: " Marek Szyprowski
` (4 subsequent siblings)
36 siblings, 1 reply; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Dmitry Osipenko, Mauro Carvalho Chehab,
Greg Kroah-Hartman, Thierry Reding, Jonathan Hunter, linux-media,
linux-tegra, devel
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/staging/media/tegra-vde/iommu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/tegra-vde/iommu.c b/drivers/staging/media/tegra-vde/iommu.c
index 6af863d..adf8dc7 100644
--- a/drivers/staging/media/tegra-vde/iommu.c
+++ b/drivers/staging/media/tegra-vde/iommu.c
@@ -36,8 +36,8 @@ int tegra_vde_iommu_map(struct tegra_vde *vde,
addr = iova_dma_addr(&vde->iova, iova);
- size = iommu_map_sg(vde->domain, addr, sgt->sgl, sgt->nents,
- IOMMU_READ | IOMMU_WRITE);
+ size = iommu_map_sgtable(vde->domain, addr, sgt,
+ IOMMU_READ | IOMMU_WRITE);
if (!size) {
__free_iova(&vde->iova, iova);
return -ENXIO;
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v5 33/38] staging: tegra-vde: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 33/38] staging: tegra-vde: " Marek Szyprowski
@ 2020-05-14 20:07 ` Dmitry Osipenko
0 siblings, 0 replies; 56+ messages in thread
From: Dmitry Osipenko @ 2020-05-14 20:07 UTC (permalink / raw)
To: Marek Szyprowski, dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Christoph Hellwig, Robin Murphy, Bartlomiej Zolnierkiewicz,
linux-arm-kernel, David Airlie, Daniel Vetter,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Thierry Reding,
Jonathan Hunter, linux-media, linux-tegra, devel
13.05.2020 16:32, Marek Szyprowski пишет:
> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
> returns the number of the created entries in the DMA address space.
> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
> dma_unmap_sg must be called with the original number of the entries
> passed to the dma_map_sg().
>
> struct sg_table is a common structure used for describing a non-contiguous
> memory buffer, used commonly in the DRM and graphics subsystems. It
> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
> and DMA mapped pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, calling DMA-mapping functions with a wrong number of entries or
> ignoring the number of mapped entries returned by the dma_map_sg()
> function.
>
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
> vs. orig_nents misuse' thread:
> https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
> ---
> drivers/staging/media/tegra-vde/iommu.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/tegra-vde/iommu.c b/drivers/staging/media/tegra-vde/iommu.c
> index 6af863d..adf8dc7 100644
> --- a/drivers/staging/media/tegra-vde/iommu.c
> +++ b/drivers/staging/media/tegra-vde/iommu.c
> @@ -36,8 +36,8 @@ int tegra_vde_iommu_map(struct tegra_vde *vde,
>
> addr = iova_dma_addr(&vde->iova, iova);
>
> - size = iommu_map_sg(vde->domain, addr, sgt->sgl, sgt->nents,
> - IOMMU_READ | IOMMU_WRITE);
> + size = iommu_map_sgtable(vde->domain, addr, sgt,
> + IOMMU_READ | IOMMU_WRITE);
> if (!size) {
> __free_iova(&vde->iova, iova);
> return -ENXIO;
>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v5 34/38] misc: fastrpc: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (31 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 33/38] staging: tegra-vde: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 35/38] rapidio: " Marek Szyprowski
` (3 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Arnd Bergmann, Greg Kroah-Hartman
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/misc/fastrpc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index e3e085e..0b5b927 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -518,7 +518,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
table = &a->sgt;
- if (!dma_map_sg(attachment->dev, table->sgl, table->nents, dir))
+ if (!dma_map_sgtable(attachment->dev, table, dir, 0))
return ERR_PTR(-ENOMEM);
return table;
@@ -528,7 +528,7 @@ static void fastrpc_unmap_dma_buf(struct dma_buf_attachment *attach,
struct sg_table *table,
enum dma_data_direction dir)
{
- dma_unmap_sg(attach->dev, table->sgl, table->nents, dir);
+ dma_unmap_sgtable(attach->dev, table, dir, 0);
}
static void fastrpc_release(struct dma_buf *dmabuf)
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 35/38] rapidio: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (32 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 34/38] misc: fastrpc: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 36/38] samples: vfio-mdev/mbochs: " Marek Szyprowski
` (2 subsequent siblings)
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Matt Porter, Alexandre Bounine
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/rapidio/devices/rio_mport_cdev.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
index 4029637..92d8742 100644
--- a/drivers/rapidio/devices/rio_mport_cdev.c
+++ b/drivers/rapidio/devices/rio_mport_cdev.c
@@ -574,8 +574,7 @@ static void dma_req_free(struct kref *ref)
struct mport_cdev_priv *priv = req->priv;
unsigned int i;
- dma_unmap_sg(req->dmach->device->dev,
- req->sgt.sgl, req->sgt.nents, req->dir);
+ dma_unmap_sgtable(req->dmach->device->dev, req->sgt, req->dir, 0);
sg_free_table(&req->sgt);
if (req->page_list) {
for (i = 0; i < req->nr_pages; i++)
@@ -927,9 +926,8 @@ static int do_dma_request(struct mport_dma_req *req,
xfer->offset, xfer->length);
}
- nents = dma_map_sg(chan->device->dev,
- req->sgt.sgl, req->sgt.nents, dir);
- if (nents == 0) {
+ ret = dma_map_sgtable(chan->device->dev, req->sgt, dir, 0);
+ if (ret) {
rmcd_error("Failed to map SG list");
ret = -EFAULT;
goto err_pg;
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 36/38] samples: vfio-mdev/mbochs: fix common struct sg_table related issues
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (33 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 35/38] rapidio: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 37/38] media: pci: fix common ALSA DMA-mapping related codes Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 38/38] videobuf2: use sgtable-based scatterlist wrappers Marek Szyprowski
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Kirti Wankhede, kvm
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
returns the number of the created entries in the DMA address space.
However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
dma_unmap_sg must be called with the original number of the entries
passed to the dma_map_sg().
struct sg_table is a common structure used for describing a non-contiguous
memory buffer, used commonly in the DRM and graphics subsystems. It
consists of a scatterlist with memory pages and DMA addresses (sgl entry),
as well as the number of scatterlist entries: CPU pages (orig_nents entry)
and DMA mapped pages (nents entry).
It turned out that it was a common mistake to misuse nents and orig_nents
entries, calling DMA-mapping functions with a wrong number of entries or
ignoring the number of mapped entries returned by the dma_map_sg()
function.
To avoid such issues, lets use a common dma-mapping wrappers operating
directly on the struct sg_table objects and use scatterlist page
iterators where possible. This, almost always, hides references to the
nents and orig_nents entries, making the code robust, easier to follow
and copy/paste safe.
While touching this code, also add missing call to dma_unmap_sgtable.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
samples/vfio-mdev/mbochs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index 3cc5e59..e030689 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -846,7 +846,7 @@ static struct sg_table *mbochs_map_dmabuf(struct dma_buf_attachment *at,
if (sg_alloc_table_from_pages(sg, dmabuf->pages, dmabuf->pagecount,
0, dmabuf->mode.size, GFP_KERNEL) < 0)
goto err2;
- if (!dma_map_sg(at->dev, sg->sgl, sg->nents, direction))
+ if (dma_map_sgtable(at->dev, sg, direction, 0))
goto err3;
return sg;
@@ -868,6 +868,7 @@ static void mbochs_unmap_dmabuf(struct dma_buf_attachment *at,
dev_dbg(dev, "%s: %d\n", __func__, dmabuf->id);
+ dma_unmap_sgtable(at->dev, sg, direction, 0);
sg_free_table(sg);
kfree(sg);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 37/38] media: pci: fix common ALSA DMA-mapping related codes
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (34 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 36/38] samples: vfio-mdev/mbochs: " Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
2020-05-13 13:32 ` [PATCH v5 38/38] videobuf2: use sgtable-based scatterlist wrappers Marek Szyprowski
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Mauro Carvalho Chehab, linux-media
The Documentation/DMA-API-HOWTO.txt states that dma_map_sg returns the
numer of the created entries in the DMA address space. However the
subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be
called with the original number of entries passed to dma_map_sg. The
sg_table->nents in turn holds the result of the dma_map_sg call as stated
in include/linux/scatterlist.h. Adapt the code to obey those rules.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
drivers/media/pci/cx23885/cx23885-alsa.c | 2 +-
drivers/media/pci/cx25821/cx25821-alsa.c | 2 +-
drivers/media/pci/cx88/cx88-alsa.c | 2 +-
drivers/media/pci/saa7134/saa7134-alsa.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/pci/cx23885/cx23885-alsa.c b/drivers/media/pci/cx23885/cx23885-alsa.c
index df44ed7..3f366e4 100644
--- a/drivers/media/pci/cx23885/cx23885-alsa.c
+++ b/drivers/media/pci/cx23885/cx23885-alsa.c
@@ -129,7 +129,7 @@ static int cx23885_alsa_dma_unmap(struct cx23885_audio_dev *dev)
if (!buf->sglen)
return 0;
- dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->sglen, PCI_DMA_FROMDEVICE);
+ dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->nr_pages, PCI_DMA_FROMDEVICE);
buf->sglen = 0;
return 0;
}
diff --git a/drivers/media/pci/cx25821/cx25821-alsa.c b/drivers/media/pci/cx25821/cx25821-alsa.c
index 3016164..c40304d 100644
--- a/drivers/media/pci/cx25821/cx25821-alsa.c
+++ b/drivers/media/pci/cx25821/cx25821-alsa.c
@@ -193,7 +193,7 @@ static int cx25821_alsa_dma_unmap(struct cx25821_audio_dev *dev)
if (!buf->sglen)
return 0;
- dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->sglen, PCI_DMA_FROMDEVICE);
+ dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->nr_pages, PCI_DMA_FROMDEVICE);
buf->sglen = 0;
return 0;
}
diff --git a/drivers/media/pci/cx88/cx88-alsa.c b/drivers/media/pci/cx88/cx88-alsa.c
index 7d7acee..3c6fe6c 100644
--- a/drivers/media/pci/cx88/cx88-alsa.c
+++ b/drivers/media/pci/cx88/cx88-alsa.c
@@ -332,7 +332,7 @@ static int cx88_alsa_dma_unmap(struct cx88_audio_dev *dev)
if (!buf->sglen)
return 0;
- dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->sglen,
+ dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->nr_pages,
PCI_DMA_FROMDEVICE);
buf->sglen = 0;
return 0;
diff --git a/drivers/media/pci/saa7134/saa7134-alsa.c b/drivers/media/pci/saa7134/saa7134-alsa.c
index 544ca57..398c47f 100644
--- a/drivers/media/pci/saa7134/saa7134-alsa.c
+++ b/drivers/media/pci/saa7134/saa7134-alsa.c
@@ -313,7 +313,7 @@ static int saa7134_alsa_dma_unmap(struct saa7134_dev *dev)
if (!dma->sglen)
return 0;
- dma_unmap_sg(&dev->pci->dev, dma->sglist, dma->sglen, PCI_DMA_FROMDEVICE);
+ dma_unmap_sg(&dev->pci->dev, dma->sglist, dma->nr_pages, PCI_DMA_FROMDEVICE);
dma->sglen = 0;
return 0;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH v5 38/38] videobuf2: use sgtable-based scatterlist wrappers
2020-05-13 13:32 ` [PATCH v5 01/38] dma-mapping: add generic helpers for mapping sgtable objects Marek Szyprowski
` (35 preceding siblings ...)
2020-05-13 13:32 ` [PATCH v5 37/38] media: pci: fix common ALSA DMA-mapping related codes Marek Szyprowski
@ 2020-05-13 13:32 ` Marek Szyprowski
36 siblings, 0 replies; 56+ messages in thread
From: Marek Szyprowski @ 2020-05-13 13:32 UTC (permalink / raw)
To: dri-devel, iommu, linaro-mm-sig, linux-kernel
Cc: Marek Szyprowski, Christoph Hellwig, Robin Murphy,
Bartlomiej Zolnierkiewicz, linux-arm-kernel, David Airlie,
Daniel Vetter, Pawel Osciak, Kyungmin Park, Mauro Carvalho Chehab,
linux-media
Use recently introduced common wrappers operating directly on the struct
sg_table objects and scatterlist page iterators to make the code a bit
more compact, robust, easier to follow and copy/paste safe.
No functional change, because the code already properly did all the
scaterlist related calls.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
For more information, see '[PATCH v5 00/38] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread:
https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
---
.../media/common/videobuf2/videobuf2-dma-contig.c | 41 ++++++++++------------
drivers/media/common/videobuf2/videobuf2-dma-sg.c | 32 +++++++----------
drivers/media/common/videobuf2/videobuf2-vmalloc.c | 12 +++----
3 files changed, 34 insertions(+), 51 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index d3a3ee5..bf31a9d 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -48,16 +48,15 @@ struct vb2_dc_buf {
static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)
{
- struct scatterlist *s;
dma_addr_t expected = sg_dma_address(sgt->sgl);
- unsigned int i;
+ struct sg_dma_page_iter dma_iter;
unsigned long size = 0;
- for_each_sg(sgt->sgl, s, sgt->nents, i) {
- if (sg_dma_address(s) != expected)
+ for_each_sgtable_dma_page(sgt, &dma_iter, 0) {
+ if (sg_page_iter_dma_address(&dma_iter) != expected)
break;
- expected = sg_dma_address(s) + sg_dma_len(s);
- size += sg_dma_len(s);
+ expected += PAGE_SIZE;
+ size += PAGE_SIZE;
}
return size;
}
@@ -99,8 +98,7 @@ static void vb2_dc_prepare(void *buf_priv)
if (!sgt || buf->db_attach)
return;
- dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,
- buf->dma_dir);
+ dma_sync_sgtable_for_device(buf->dev, sgt, buf->dma_dir);
}
static void vb2_dc_finish(void *buf_priv)
@@ -112,7 +110,7 @@ static void vb2_dc_finish(void *buf_priv)
if (!sgt || buf->db_attach)
return;
- dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
+ dma_sync_sgtable_for_cpu(buf->dev, sgt, buf->dma_dir);
}
/*********************************************/
@@ -273,8 +271,8 @@ static void vb2_dc_dmabuf_ops_detach(struct dma_buf *dbuf,
* memory locations do not require any explicit cache
* maintenance prior or after being used by the device.
*/
- dma_unmap_sg_attrs(db_attach->dev, sgt->sgl, sgt->orig_nents,
- attach->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
+ dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir,
+ DMA_ATTR_SKIP_CPU_SYNC);
sg_free_table(sgt);
kfree(attach);
db_attach->priv = NULL;
@@ -299,8 +297,8 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
/* release any previous cache */
if (attach->dma_dir != DMA_NONE) {
- dma_unmap_sg_attrs(db_attach->dev, sgt->sgl, sgt->orig_nents,
- attach->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
+ dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir,
+ DMA_ATTR_SKIP_CPU_SYNC);
attach->dma_dir = DMA_NONE;
}
@@ -308,9 +306,8 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
* mapping to the client with new direction, no cache sync
* required see comment in vb2_dc_dmabuf_ops_detach()
*/
- sgt->nents = dma_map_sg_attrs(db_attach->dev, sgt->sgl, sgt->orig_nents,
- dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
- if (!sgt->nents) {
+ if (dma_map_sgtable(db_attach->dev, sgt, dma_dir,
+ DMA_ATTR_SKIP_CPU_SYNC)) {
pr_err("failed to map scatterlist\n");
mutex_unlock(lock);
return ERR_PTR(-EIO);
@@ -423,8 +420,8 @@ static void vb2_dc_put_userptr(void *buf_priv)
* No need to sync to CPU, it's already synced to the CPU
* since the finish() memop will have been called before this.
*/
- dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
- buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
+ dma_unmap_sgtable(buf->dev, sgt, buf->dma_dir,
+ DMA_ATTR_SKIP_CPU_SYNC);
pages = frame_vector_pages(buf->vec);
/* sgt should exist only if vector contains pages... */
BUG_ON(IS_ERR(pages));
@@ -521,9 +518,8 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
* No need to sync to the device, this will happen later when the
* prepare() memop is called.
*/
- sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
- buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
- if (sgt->nents <= 0) {
+ if (dma_map_sgtable(buf->dev, sgt, buf->dma_dir,
+ DMA_ATTR_SKIP_CPU_SYNC)) {
pr_err("failed to map scatterlist\n");
ret = -EIO;
goto fail_sgt_init;
@@ -545,8 +541,7 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
return buf;
fail_map_sg:
- dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
- buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
+ dma_unmap_sgtable(buf->dev, sgt, buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
fail_sgt_init:
sg_free_table(sgt);
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 92072a0..6ddf953 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -142,9 +142,8 @@ static void *vb2_dma_sg_alloc(struct device *dev, unsigned long dma_attrs,
* No need to sync to the device, this will happen later when the
* prepare() memop is called.
*/
- sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
- buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
- if (!sgt->nents)
+ if (dma_map_sgtable(buf->dev, sgt, buf->dma_dir,
+ DMA_ATTR_SKIP_CPU_SYNC)) {
goto fail_map;
buf->handler.refcount = &buf->refcount;
@@ -180,8 +179,8 @@ static void vb2_dma_sg_put(void *buf_priv)
if (refcount_dec_and_test(&buf->refcount)) {
dprintk(1, "%s: Freeing buffer of %d pages\n", __func__,
buf->num_pages);
- dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
- buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
+ dma_unmap_sgtable(buf->dev, sgt, buf->dma_dir,
+ DMA_ATTR_SKIP_CPU_SYNC);
if (buf->vaddr)
vm_unmap_ram(buf->vaddr, buf->num_pages);
sg_free_table(buf->dma_sgt);
@@ -202,8 +201,7 @@ static void vb2_dma_sg_prepare(void *buf_priv)
if (buf->db_attach)
return;
- dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,
- buf->dma_dir);
+ dma_sync_sgtable_for_device(buf->dev, sgt, buf->dma_dir);
}
static void vb2_dma_sg_finish(void *buf_priv)
@@ -215,7 +213,7 @@ static void vb2_dma_sg_finish(void *buf_priv)
if (buf->db_attach)
return;
- dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
+ dma_sync_sgtable_for_cpu(buf->dev, sgt, buf->dma_dir);
}
static void *vb2_dma_sg_get_userptr(struct device *dev, unsigned long vaddr,
@@ -258,9 +256,8 @@ static void *vb2_dma_sg_get_userptr(struct device *dev, unsigned long vaddr,
* No need to sync to the device, this will happen later when the
* prepare() memop is called.
*/
- sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
- buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
- if (!sgt->nents)
+ if (dma_map_sgtable(buf->dev, sgt, buf->dma_dir,
+ DMA_ATTR_SKIP_CPU_SYNC)) {
goto userptr_fail_map;
return buf;
@@ -286,8 +283,7 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
dprintk(1, "%s: Releasing userspace buffer of %d pages\n",
__func__, buf->num_pages);
- dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir,
- DMA_ATTR_SKIP_CPU_SYNC);
+ dma_unmap_sgtable(buf->dev, sgt, buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
if (buf->vaddr)
vm_unmap_ram(buf->vaddr, buf->num_pages);
sg_free_table(buf->dma_sgt);
@@ -410,8 +406,7 @@ static void vb2_dma_sg_dmabuf_ops_detach(struct dma_buf *dbuf,
/* release the scatterlist cache */
if (attach->dma_dir != DMA_NONE)
- dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
- attach->dma_dir);
+ dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir);
sg_free_table(sgt);
kfree(attach);
db_attach->priv = NULL;
@@ -436,15 +431,12 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
/* release any previous cache */
if (attach->dma_dir != DMA_NONE) {
- dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
- attach->dma_dir);
+ dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir);
attach->dma_dir = DMA_NONE;
}
/* mapping to the client with new direction */
- sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
- dma_dir);
- if (!sgt->nents) {
+ if (dma_map_sgtable(db_attach->dev, sgt, dma_dir, 0)) {
pr_err("failed to map scatterlist\n");
mutex_unlock(lock);
return ERR_PTR(-EIO);
diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
index c66fda4..bf5ac63 100644
--- a/drivers/media/common/videobuf2/videobuf2-vmalloc.c
+++ b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
@@ -229,7 +229,7 @@ static int vb2_vmalloc_dmabuf_ops_attach(struct dma_buf *dbuf,
kfree(attach);
return ret;
}
- for_each_sg(sgt->sgl, sg, sgt->nents, i) {
+ for_each_sgtable_sg(sgt, sg, i) {
struct page *page = vmalloc_to_page(vaddr);
if (!page) {
@@ -259,8 +259,7 @@ static void vb2_vmalloc_dmabuf_ops_detach(struct dma_buf *dbuf,
/* release the scatterlist cache */
if (attach->dma_dir != DMA_NONE)
- dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
- attach->dma_dir);
+ dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir, 0);
sg_free_table(sgt);
kfree(attach);
db_attach->priv = NULL;
@@ -285,15 +284,12 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(
/* release any previous cache */
if (attach->dma_dir != DMA_NONE) {
- dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
- attach->dma_dir);
+ dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir, 0);
attach->dma_dir = DMA_NONE;
}
/* mapping to the client with new direction */
- sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
- dma_dir);
- if (!sgt->nents) {
+ if (dma_map_sgtable(db_attach->dev, sgt, dma_dir, 0)) {
pr_err("failed to map scatterlist\n");
mutex_unlock(lock);
return ERR_PTR(-EIO);
--
1.9.1
^ permalink raw reply related [flat|nested] 56+ messages in thread