Dear All,
During the Exynos DRM GEM rework and fixing the issues in the.
drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most
drivers in DRM framework incorrectly use nents and orig_nents entries of
the struct sg_table.
In case of the most DMA-mapping implementations exchanging those two
entries or using nents for all loops on the scatterlist is harmless,
because they both have the same value. There exists however a DMA-mapping
implementations, for which such incorrect usage breaks things. The nents
returned by dma_map_sg() might be lower than the nents passed as its
parameter and this is perfectly fine. DMA framework or IOMMU is allowed
to join consecutive chunks while mapping if such operation is supported
by the underlying HW (bus, bridge, IOMMU, etc). Example of the case
where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages
is described here [2]
The DMA-mapping framework documentation [3] 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 common pattern in DRM drivers were to assign the
dma_map_sg() return value to sg_table->nents and use that value for
the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also
the code iterated over nents times to access the pages stored in the
processed scatterlist, while it should use orig_nents as the numer of
the page entries.
I've tried to identify all such incorrect usage of sg_table->nents and
this is a result of my research. It looks that the incorrect pattern has
been copied over the many drivers mainly in the DRM subsystem. Too bad in
most cases it even worked correctly if the system used a simple, linear
DMA-mapping implementation, for which swapping nents and orig_nents
doesn't make any difference. To avoid similar issues in the future, I've
introduced a common wrappers for DMA-mapping calls, which operate directly
on the sg_table objects. I've also added wrappers for iterating over the
scatterlists stored in the sg_table objects and applied them where
possible. This, together with some common DRM prime helpers, allowed me
to almost get rid of all nents/orig_nents usage in the drivers. I hope
that such change makes the code robust, easier to follow and copy/paste
safe.
The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix
it fully. The driver creatively uses sg_table->orig_nents to store the
size of the allocate scatterlist and ignores the number of the entries
returned by dma_map_sg function. In this patchset I only fixed the
sg_table objects exported by dmabuf related functions. I hope that I
didn't break anything there.
Patches are based on top of Linux next-20200825. The required changes to
DMA-mapping framework has been already merged to v5.8-rc1.
I would like ask for merging of the 1-27 patches via DRM misc tree.
Best regards,
Marek Szyprowski
References:
[1] https://lkml.org/lkml/2020/3/27/555
[2] https://lkml.org/lkml/2020/3/29/65
[3] Documentation/DMA-API-HOWTO.txt
[4] https://lore.kernel.org/linux-iommu/[email protected]/T/#ma18c958a48c3b241d5409517fa7d192eef87459b
Changelog:
v9:
- rebased onto Linux next-20200825, which is based on v5.9-rc2; fixed conflicts
- dropped merged patches
v8:
- rapidio: fixed issues pointed by kbuilt test robot (use of uninitialized
variable
- vb2: rebased after recent changes in the code
v7: https://lore.kernel.org/linux-iommu/[email protected]/T/
- changed DMA page interators to standard DMA SG iterators in drm/prime and
videobuf2-dma-contig as suggested by Robin Murphy
- fixed build issues
v6: https://lore.kernel.org/linux-iommu/[email protected]/T/
- rebased onto Linux next-20200618, which is based on v5.8-rc1; fixed conflicts
v5: https://lore.kernel.org/linux-iommu/[email protected]/T/
- fixed some minor style issues and typos
- fixed lack of the attrs argument in ion, dmabuf, rapidio, fastrpc and
vfio patches
v4: https://lore.kernel.org/linux-iommu/[email protected]/T/
- added for_each_sgtable_* wrappers and applied where possible
- added drm_prime_get_contiguous_size() and applied where possible
- applied drm_prime_sg_to_page_addr_arrays() where possible to remove page
extraction from sg_table objects
- added documentation for the introduced wrappers
- improved patches description a bit
v3: https://lore.kernel.org/dri-devel/[email protected]/
- introduce dma_*_sgtable_* wrappers and use them in all patches
v2: https://lore.kernel.org/linux-iommu/[email protected]/T/
- dropped most of the changes to drm/i915
- added fixes for rcar-du, xen, media and ion
- fixed a few issues pointed by kbuild test robot
- added wide cc: list for each patch
v1: https://lore.kernel.org/linux-iommu/[email protected]/T/
- initial version
Patch summary:
Marek Szyprowski (32):
drm: prime: add common helper to check scatterlist contiguity
drm: prime: use sgtable iterators in
drm_prime_sg_to_page_addr_arrays()
drm: core: fix common struct sg_table related issues
drm: armada: fix common struct sg_table related issues
drm: etnaviv: fix common struct sg_table related issues
drm: exynos: use common helper for a scatterlist contiguity check
drm: exynos: fix common struct sg_table related issues
drm: i915: fix common struct sg_table related issues
drm: lima: fix common struct sg_table related issues
drm: mediatek: use common helper for a scatterlist contiguity check
drm: mediatek: use common helper for extracting pages array
drm: msm: fix common struct sg_table related issues
drm: omapdrm: use common helper for extracting pages array
drm: omapdrm: fix common struct sg_table related issues
drm: panfrost: fix common struct sg_table related issues
drm: rockchip: use common helper for a scatterlist contiguity check
drm: rockchip: fix common struct sg_table related issues
drm: tegra: fix common struct sg_table related issues
drm: v3d: fix common struct sg_table related issues
drm: virtio: fix common struct sg_table related issues
drm: vmwgfx: fix common struct sg_table related issues
drm: xen: fix common struct sg_table related issues
xen: gntdev: fix common struct sg_table related issues
drm: host1x: fix common struct sg_table related issues
drm: rcar-du: fix common struct sg_table related issues
dmabuf: fix common struct sg_table related issues
staging: tegra-vde: fix common struct sg_table related issues
misc: fastrpc: fix common struct sg_table related issues
rapidio: fix common struct sg_table related issues
samples: vfio-mdev/mbochs: fix common struct sg_table related issues
media: pci: fix common ALSA DMA-mapping related codes
videobuf2: use sgtable-based scatterlist wrappers
drivers/dma-buf/heaps/heap-helpers.c | 13 ++-
drivers/dma-buf/udmabuf.c | 7 +-
drivers/gpu/drm/armada/armada_gem.c | 12 +--
drivers/gpu/drm/drm_cache.c | 2 +-
drivers/gpu/drm/drm_gem_cma_helper.c | 23 +----
drivers/gpu/drm/drm_gem_shmem_helper.c | 14 ++-
drivers/gpu/drm/drm_prime.c | 91 +++++++++++--------
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 12 +--
drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 13 +--
drivers/gpu/drm/exynos/exynos_drm_g2d.c | 10 +-
drivers/gpu/drm/exynos/exynos_drm_gem.c | 23 +----
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 11 +--
.../gpu/drm/i915/gem/selftests/mock_dmabuf.c | 7 +-
drivers/gpu/drm/lima/lima_gem.c | 11 ++-
drivers/gpu/drm/lima/lima_vm.c | 5 +-
drivers/gpu/drm/mediatek/mtk_drm_gem.c | 37 ++------
drivers/gpu/drm/msm/msm_gem.c | 13 +--
drivers/gpu/drm/msm/msm_gpummu.c | 14 ++-
drivers/gpu/drm/msm/msm_iommu.c | 2 +-
drivers/gpu/drm/omapdrm/omap_gem.c | 20 ++--
drivers/gpu/drm/panfrost/panfrost_gem.c | 4 +-
drivers/gpu/drm/panfrost/panfrost_mmu.c | 7 +-
drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 +-
drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 42 +++------
drivers/gpu/drm/tegra/gem.c | 27 ++----
drivers/gpu/drm/tegra/plane.c | 15 +--
drivers/gpu/drm/v3d/v3d_mmu.c | 13 ++-
drivers/gpu/drm/virtio/virtgpu_object.c | 36 +++++---
drivers/gpu/drm/virtio/virtgpu_vq.c | 12 +--
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 17 +---
drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +-
drivers/gpu/host1x/job.c | 22 ++---
.../common/videobuf2/videobuf2-dma-contig.c | 34 +++----
.../media/common/videobuf2/videobuf2-dma-sg.c | 32 +++----
.../common/videobuf2/videobuf2-vmalloc.c | 12 +--
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 +-
drivers/media/platform/vsp1/vsp1_drm.c | 8 +-
drivers/misc/fastrpc.c | 4 +-
drivers/rapidio/devices/rio_mport_cdev.c | 11 +--
drivers/staging/media/tegra-vde/iommu.c | 4 +-
drivers/xen/gntdev-dmabuf.c | 13 ++-
include/drm/drm_prime.h | 2 +
samples/vfio-mdev/mbochs.c | 3 +-
46 files changed, 273 insertions(+), 398 deletions(-)
--
2.17.1
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 <[email protected]>
Acked-by: Roland Scheidegger <[email protected]>
---
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 ab524ab3b0b4..f2f2bff1eedf 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;
--
2.17.1
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 <[email protected]>
---
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 723df142a981..01d94befab11 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 @@ tegra_gem_prime_map_dma_buf(struct dma_buf_attachment *attach,
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 4cd0461cc508..539d14935728 100644
--- a/drivers/gpu/drm/tegra/plane.c
+++ b/drivers/gpu/drm/tegra/plane.c
@@ -131,12 +131,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
@@ -144,7 +141,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;
}
@@ -166,8 +163,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;
@@ -186,8 +182,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;
--
2.17.1
Use common helper for checking the contiguity of the imported dma-buf.
Signed-off-by: Marek Szyprowski <[email protected]>
---
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 b9275ba7c5a5..2970e534e2bb 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 @@ rockchip_gem_dma_map_sg(struct drm_device *drm,
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);
--
2.17.1
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 <[email protected]>
Reviewed-by: Dmitry Osipenko <[email protected]>
---
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 6af863d92123..adf8dc7ee25c 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;
--
2.17.1
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
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 f1a81c9b184d..a27bff999649 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 a4a45d68a6ef..86d5e3f4b1ff 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);
--
2.17.1
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 <[email protected]>
---
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 df44ed7393a0..3f366e4e4685 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 301616426d8a..c40304d33776 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 7d7aceecc985..3c6fe6ceb0b7 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 544ca57eee75..398c47ff473d 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;
}
--
2.17.1
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 <[email protected]>
---
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 3cc5e5921682..e03068917273 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);
}
--
2.17.1
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 <[email protected]>
---
.../common/videobuf2/videobuf2-dma-contig.c | 34 ++++++++-----------
.../media/common/videobuf2/videobuf2-dma-sg.c | 32 +++++++----------
.../common/videobuf2/videobuf2-vmalloc.c | 12 +++----
3 files changed, 31 insertions(+), 47 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index ec3446cc45b8..1b242d844dde 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -58,10 +58,10 @@ static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)
unsigned int i;
unsigned long size = 0;
- for_each_sg(sgt->sgl, s, sgt->nents, i) {
+ for_each_sgtable_dma_sg(sgt, s, i) {
if (sg_dma_address(s) != expected)
break;
- expected = sg_dma_address(s) + sg_dma_len(s);
+ expected += sg_dma_len(s);
size += sg_dma_len(s);
}
return size;
@@ -103,8 +103,7 @@ static void vb2_dc_prepare(void *buf_priv)
if (!sgt)
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)
@@ -115,7 +114,7 @@ static void vb2_dc_finish(void *buf_priv)
if (!sgt)
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);
}
/*********************************************/
@@ -275,8 +274,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;
@@ -301,8 +300,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;
}
@@ -310,9 +309,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);
@@ -455,8 +453,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));
@@ -553,9 +551,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;
@@ -577,8 +574,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 0a40e00f0d7e..0dd3b19025e0 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -148,9 +148,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;
@@ -186,8 +185,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);
@@ -204,8 +203,7 @@ static void vb2_dma_sg_prepare(void *buf_priv)
struct vb2_dma_sg_buf *buf = buf_priv;
struct sg_table *sgt = buf->dma_sgt;
- 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)
@@ -213,7 +211,7 @@ static void vb2_dma_sg_finish(void *buf_priv)
struct vb2_dma_sg_buf *buf = buf_priv;
struct sg_table *sgt = buf->dma_sgt;
- 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,
@@ -256,9 +254,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;
@@ -284,8 +281,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);
@@ -408,8 +404,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, 0);
sg_free_table(sgt);
kfree(attach);
db_attach->priv = NULL;
@@ -434,15 +429,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, 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);
diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
index c66fda4a65e4..bf5ac63a5742 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);
--
2.17.1
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 <[email protected]>
---
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 7939c55daceb..9d6867749316 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -518,7 +518,7 @@ fastrpc_map_dma_buf(struct dma_buf_attachment *attachment,
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)
--
2.17.1
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 <[email protected]>
---
drivers/rapidio/devices/rio_mport_cdev.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
index a30342942e26..89eb3d212652 100644
--- a/drivers/rapidio/devices/rio_mport_cdev.c
+++ b/drivers/rapidio/devices/rio_mport_cdev.c
@@ -573,8 +573,7 @@ static void dma_req_free(struct kref *ref)
refcount);
struct mport_cdev_priv *priv = req->priv;
- 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) {
unpin_user_pages(req->page_list, req->nr_pages);
@@ -814,7 +813,6 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
struct mport_dev *md = priv->md;
struct dma_chan *chan;
int ret;
- int nents;
if (xfer->length == 0)
return -EINVAL;
@@ -930,15 +928,14 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
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;
}
- ret = do_dma_request(req, xfer, sync, nents);
+ ret = do_dma_request(req, xfer, sync, req->sgt.nents);
if (ret >= 0) {
if (sync == RIO_TRANSFER_ASYNC)
--
2.17.1
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 <[email protected]>
Acked-by: Juergen Gross <[email protected]>
---
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 b1b6eebafd5d..4c13cbc99896 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 @@ dmabuf_exp_ops_map_dma_buf(struct dma_buf_attachment *attach,
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);
@@ -633,7 +632,7 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
/* 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
--
2.17.1
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
reports the number of the pages in the imported scatterlist, so it should
refer to sg_table->orig_nents entry.
Signed-off-by: Marek Szyprowski <[email protected]>
Acked-by: Oleksandr Andrushchenko <[email protected]>
---
drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 39ff95b75357..0e57c80058b2 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -216,7 +216,7 @@ xen_drm_front_gem_import_sg_table(struct drm_device *dev,
return ERR_PTR(ret);
DRM_DEBUG("Imported buffer of size %zu with nents %u\n",
- size, sgt->nents);
+ size, sgt->orig_nents);
return &xen_obj->base;
}
--
2.17.1
Use common helper for converting a sg_table object into struct
page pointer array.
Signed-off-by: Marek Szyprowski <[email protected]>
---
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 d0d12d5dd76c..ff0c4b0c3fd0 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;
--
2.17.1
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 <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
---
drivers/gpu/drm/drm_prime.c | 49 ++++++++++++-------------------------
1 file changed, 15 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 4ed5ed1f078c..5d181bf60a44 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -990,45 +990,26 @@ EXPORT_SYMBOL(drm_gem_prime_import);
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;
-
- /*
- * 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))
+ struct sg_dma_page_iter dma_iter;
+ struct sg_page_iter page_iter;
+ struct page **p = pages;
+ dma_addr_t *a = addrs;
+
+ 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);
--
2.17.1
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 <[email protected]>
Acked-by: Gerd Hoffmann <[email protected]>
---
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 9f964ca3f59c..d0696cf937af 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 acb26c627d27..89e293bd9252 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);
}
--
2.17.1
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 <[email protected]>
Acked-by: Gerd Hoffmann <[email protected]>
---
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 e83651b7747d..a0559d3ed362 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;
}
@@ -158,13 +157,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),
@@ -174,13 +173,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 53af60d484a4..7947b1047bd0 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));
--
2.17.1
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 <[email protected]>
---
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 2679380159fc..8a988592715b 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 debaf7b18ab5..be30b27e2926 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);
}
--
2.17.1
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 <[email protected]>
Reviewed-by: Qiang Yu <[email protected]>
---
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 155f2b4b4030..11223fe348df 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 5b92fb82674a..2b2739adc7f5 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)
--
2.17.1
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 <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
Acked-by : Inki Dae <[email protected]>
---
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 03be31427181..967a5cdc120e 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;
}
--
2.17.1
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 <[email protected]>
---
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 f06e19e7be04..eaf1949bc2e4 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 3607d348c298..13b100553a0b 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);
--
2.17.1
Use common helper for checking the contiguity of the imported dma-buf.
Signed-off-by: Marek Szyprowski <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
Acked-by : Inki Dae <[email protected]>
---
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 efa476858db5..1716a023bca0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -431,27 +431,10 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
{
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);
--
2.17.1
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 <[email protected]>
Reviewed-by: Steven Price <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
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 33355dd302f1..1a6cea0e0bd7 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 e8f7b11352d2..776448c527ea 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);
--
2.17.1
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 <[email protected]>
Acked-by: Rob Clark <[email protected]>
---
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 b2f49152b4d4..8c7ae812b813 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 310a31b05faa..319f06c28235 100644
--- a/drivers/gpu/drm/msm/msm_gpummu.c
+++ b/drivers/gpu/drm/msm/msm_gpummu.c
@@ -30,21 +30,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 3a381a9674c9..6c31e65834c6 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -36,7 +36,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;
--
2.17.1
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 <[email protected]>
Reviewed-by: Eric Anholt <[email protected]>
---
drivers/gpu/drm/v3d/v3d_mmu.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_mmu.c b/drivers/gpu/drm/v3d/v3d_mmu.c
index 3b81ea28c0bb..5a453532901f 100644
--- a/drivers/gpu/drm/v3d/v3d_mmu.c
+++ b/drivers/gpu/drm/v3d/v3d_mmu.c
@@ -90,18 +90,17 @@ 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) >=
+ BUG_ON(page_address + (PAGE_SIZE >> V3D_MMU_PAGE_SHIFT) >=
BIT(24));
-
- for (i = 0; i < sg_dma_len(sgl) >> V3D_MMU_PAGE_SHIFT; i++)
+ for (i = 0; i < PAGE_SIZE >> V3D_MMU_PAGE_SHIFT; i++)
v3d->pt[page++] = pte + i;
}
--
2.17.1
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 <[email protected]>
---
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 89b6c14b7392..82d0a60ba3f7 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -170,11 +170,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;
@@ -228,7 +226,7 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
}
if (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);
@@ -240,9 +238,9 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
goto put;
}
- 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;
@@ -252,12 +250,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 put;
- }
job->unpins[job->num_unpins].dir = DMA_TO_DEVICE;
job->unpins[job->num_unpins].dev = host->dev;
@@ -660,8 +655,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);
--
2.17.1
Use common helper for converting a sg_table object into struct
page pointer array.
Signed-off-by: Marek Szyprowski <[email protected]>
---
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 3654ec732029..0583e557ad37 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));
--
2.17.1
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 <[email protected]>
---
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 2970e534e2bb..cb50f2ba2e46 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 @@ rockchip_gem_dma_map_sg(struct drm_device *drm,
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;
}
--
2.17.1
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 <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_drm_gem.c | 28 ++++++--------------------
1 file changed, 6 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
index 6190cc3b7b0d..3654ec732029 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
@@ -212,37 +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_len(s))
- break;
-
- 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)
--
2.17.1
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 <[email protected]>
---
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 ff0c4b0c3fd0..a7a9a0afe2b6 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 */
--
2.17.1
On 2020-08-26 07:32, 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.
Is removing the WARN_ON()s intentional? It certainly seems like it would
be a genuine driver bug if the caller asked for addresses but didn't
allocate appropriately-sized arrays. Might be worth noting either way.
I'm also assuming this isn't called in performance-critical paths with
massive lists such that the two separate iterations might have a
noticeable impact.
Nits aside,
Reviewed-by: Robin Murphy <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
> Reviewed-by: Andrzej Hajda <[email protected]>
> ---
> drivers/gpu/drm/drm_prime.c | 49 ++++++++++++-------------------------
> 1 file changed, 15 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 4ed5ed1f078c..5d181bf60a44 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -990,45 +990,26 @@ EXPORT_SYMBOL(drm_gem_prime_import);
> 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;
> -
> - /*
> - * 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))
> + struct sg_dma_page_iter dma_iter;
> + struct sg_page_iter page_iter;
> + struct page **p = pages;
> + dma_addr_t *a = addrs;
> +
> + 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);
>
On 2020-08-26 07: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 <[email protected]>
> ---
> 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 f06e19e7be04..eaf1949bc2e4 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 3607d348c298..13b100553a0b 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);
I had to take a closer look to figure this out, but AFAICS it does
indeed work out as a simpler way of achieving the exact same result, and
in fact neatly mirrors how etnaviv_context_map() itself cleans up.
Reviewed-by: Robin Murphy <[email protected]>
> 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);
>
On 2020-08-26 07:32, Marek Szyprowski wrote:
> 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.
Reviewed-by: Robin Murphy <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_gem.c | 28 ++++++--------------------
> 1 file changed, 6 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> index 6190cc3b7b0d..3654ec732029 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> @@ -212,37 +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_len(s))
> - break;
> -
> - 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)
>
On 2020-08-26 07: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.
>
> 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.
This one looks mechanical enough :)
Reviewed-by: Robin Murphy <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> 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 2679380159fc..8a988592715b 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 debaf7b18ab5..be30b27e2926 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);
> }
>
On 2020-08-26 07:32, Marek Szyprowski wrote:
> Use common helper for converting a sg_table object into struct
> page pointer array.
Reviewed-by: Robin Murphy <[email protected]>
Side note: is mtk_drm_gem_prime_vmap() missing a call to
sg_free_table(sgt) before its kfree(sgt)?
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> 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 3654ec732029..0583e557ad37 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));
>
>
On 2020-08-26 07: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 <[email protected]>
> Acked-by: Rob Clark <[email protected]>
> ---
> 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 b2f49152b4d4..8c7ae812b813 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 310a31b05faa..319f06c28235 100644
> --- a/drivers/gpu/drm/msm/msm_gpummu.c
> +++ b/drivers/gpu/drm/msm/msm_gpummu.c
> @@ -30,21 +30,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;
Given that the BUILD_BUG_ON might prevent valid arm64 configs from
building, how about a simple tweak like:
for (i = 0; i < PAGE_SIZE; i += GPUMMU_PAGE_SIZE)
gpummu->table[idx++] = i + addr | prot_bits;
?
Or alternatively perhaps some more aggressive #ifdefs or makefile tweaks
to prevent the GPUMMU code building for arm64 at all if it's only
relevant to 32-bit platforms (which I believe might be the case).
Robin.
> }
>
> /* 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 3a381a9674c9..6c31e65834c6 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -36,7 +36,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;
>
On 2020-08-26 07:32, Marek Szyprowski wrote:
> Use common helper for converting a sg_table object into struct
> page pointer array.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> 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 d0d12d5dd76c..ff0c4b0c3fd0 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)) {
Again, I'm inclined to think the WARN_ON should remain in
drm_prime_sg_to_page_addr_arrays() itself such that it could be removed
here, but either way,
Reviewed-by: Robin Murphy <[email protected]>
> omap_gem_free_object(obj);
> obj = ERR_PTR(-ENOMEM);
> goto done;
>
On 2020-08-26 07: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.
>
> 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 <[email protected]>
> ---
> 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 ff0c4b0c3fd0..a7a9a0afe2b6 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)
Hmm, if this really does mean *physically* contiguous - i.e. if buffers
might be shared between DMA-translatable and non-DMA-translatable
devices - then these changes might not be appropriate. If not and it
only actually means DMA-contiguous, then it would be good to clarify the
comments to that effect.
Can anyone familiar with omapdrm clarify what exactly the case is here?
I know that IOMMUs might be involved to some degree, and I've skimmed
the interconnect chapters of enough OMAP TRMs to be scared by the
reference to the tiler aperture in the context below :)
Robin.
> *
> * - 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 */
>
On 2020-08-26 07:33, Marek Szyprowski wrote:
> Use common helper for checking the contiguity of the imported dma-buf.
Reviewed-by: Robin Murphy <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> 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 b9275ba7c5a5..2970e534e2bb 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 @@ rockchip_gem_dma_map_sg(struct drm_device *drm,
> 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);
>
>-----Original Message-----
>From: Intel-gfx <[email protected]> On Behalf Of
>Marek Szyprowski
>Sent: Wednesday, August 26, 2020 2:33 AM
>To: [email protected]; [email protected];
>[email protected]; [email protected]
>Cc: Bartlomiej Zolnierkiewicz <[email protected]>; David Airlie
><[email protected]>; [email protected]; Robin Murphy
><[email protected]>; Christoph Hellwig <[email protected]>; linux-arm-
>[email protected]; Marek Szyprowski
><[email protected]>
>Subject: [Intel-gfx] [PATCH v9 08/32] drm: i915: fix common struct sg_table
>related issues
>
>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 <[email protected]>
>---
> 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 2679380159fc..8a988592715b 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;
You have dropped this error value.
Do you now if this is a benign loss?
M
>+ 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 debaf7b18ab5..be30b27e2926 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);
> }
>--
>2.17.1
>
>_______________________________________________
>Intel-gfx mailing list
>[email protected]
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 2020-09-01 20:38, Ruhl, Michael J wrote:
>> -----Original Message-----
>> From: Intel-gfx <[email protected]> On Behalf Of
>> Marek Szyprowski
>> Sent: Wednesday, August 26, 2020 2:33 AM
>> To: [email protected]; [email protected];
>> [email protected]; [email protected]
>> Cc: Bartlomiej Zolnierkiewicz <[email protected]>; David Airlie
>> <[email protected]>; [email protected]; Robin Murphy
>> <[email protected]>; Christoph Hellwig <[email protected]>; linux-arm-
>> [email protected]; Marek Szyprowski
>> <[email protected]>
>> Subject: [Intel-gfx] [PATCH v9 08/32] drm: i915: fix common struct sg_table
>> related issues
>>
>> 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 <[email protected]>
>> ---
>> 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 2679380159fc..8a988592715b 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;
>
> You have dropped this error value.
>
> Do you now if this is a benign loss?
True, dma_map_sgtable() will return -EINVAL rather than -ENOMEM for
failure. A quick look through other .map_dma_buf callbacks suggests
they're returning a motley mix of error values and NULL for failure
cases, so I'd imagine that importers shouldn't be too sensitive to the
exact value.
Robin.
>
> M
>
>> + 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 debaf7b18ab5..be30b27e2926 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);
>> }
>> --
>> 2.17.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 2020-08-26 07:33, 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.
Reviewed-by: Robin Murphy <[email protected]>
(Until now I hadn't noticed the crimes against the API that
rockchip_gem_get_pages() is committing, but it's not this patch's
fault... I'll have to take a closer look at that)
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> 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 2970e534e2bb..cb50f2ba2e46 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 @@ rockchip_gem_dma_map_sg(struct drm_device *drm,
> 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;
> }
>
>
On 2020-08-26 07:33, 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.
Reviewed-by: Robin Murphy <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> 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 723df142a981..01d94befab11 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 @@ tegra_gem_prime_map_dma_buf(struct dma_buf_attachment *attach,
> 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 4cd0461cc508..539d14935728 100644
> --- a/drivers/gpu/drm/tegra/plane.c
> +++ b/drivers/gpu/drm/tegra/plane.c
> @@ -131,12 +131,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
> @@ -144,7 +141,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;
> }
> @@ -166,8 +163,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;
> @@ -186,8 +182,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;
>
On 2020-08-26 07:33, 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.
Reviewed-by: Robin Murphy <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> 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 89b6c14b7392..82d0a60ba3f7 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -170,11 +170,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;
> @@ -228,7 +226,7 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
> }
>
> if (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);
>
> @@ -240,9 +238,9 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
> goto put;
> }
>
> - 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;
> @@ -252,12 +250,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 put;
> - }
>
> job->unpins[job->num_unpins].dir = DMA_TO_DEVICE;
> job->unpins[job->num_unpins].dev = host->dev;
> @@ -660,8 +655,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);
>
On 2020-08-26 07:33, 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.
Reviewed-by: Robin Murphy <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> 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 7939c55daceb..9d6867749316 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -518,7 +518,7 @@ fastrpc_map_dma_buf(struct dma_buf_attachment *attachment,
>
> 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)
>
On 2020-08-26 07:33, 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.
Reviewed-by: Robin Murphy <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/rapidio/devices/rio_mport_cdev.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
> index a30342942e26..89eb3d212652 100644
> --- a/drivers/rapidio/devices/rio_mport_cdev.c
> +++ b/drivers/rapidio/devices/rio_mport_cdev.c
> @@ -573,8 +573,7 @@ static void dma_req_free(struct kref *ref)
> refcount);
> struct mport_cdev_priv *priv = req->priv;
>
> - 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) {
> unpin_user_pages(req->page_list, req->nr_pages);
> @@ -814,7 +813,6 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
> struct mport_dev *md = priv->md;
> struct dma_chan *chan;
> int ret;
> - int nents;
>
> if (xfer->length == 0)
> return -EINVAL;
> @@ -930,15 +928,14 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
> 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;
> }
>
> - ret = do_dma_request(req, xfer, sync, nents);
> + ret = do_dma_request(req, xfer, sync, req->sgt.nents);
>
> if (ret >= 0) {
> if (sync == RIO_TRANSFER_ASYNC)
>
On 2020-08-26 07:33, 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.
>
> While touching this code, also add missing call to dma_unmap_sgtable.
Reviewed-by: Robin Murphy <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> 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 3cc5e5921682..e03068917273 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);
> }
>
On 2020-08-26 07:33, Marek Szyprowski wrote:
> 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.
^^ typo
Otherwise,
Reviewed-by: Robin Murphy <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> .../common/videobuf2/videobuf2-dma-contig.c | 34 ++++++++-----------
> .../media/common/videobuf2/videobuf2-dma-sg.c | 32 +++++++----------
> .../common/videobuf2/videobuf2-vmalloc.c | 12 +++----
> 3 files changed, 31 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index ec3446cc45b8..1b242d844dde 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> @@ -58,10 +58,10 @@ static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)
> unsigned int i;
> unsigned long size = 0;
>
> - for_each_sg(sgt->sgl, s, sgt->nents, i) {
> + for_each_sgtable_dma_sg(sgt, s, i) {
> if (sg_dma_address(s) != expected)
> break;
> - expected = sg_dma_address(s) + sg_dma_len(s);
> + expected += sg_dma_len(s);
> size += sg_dma_len(s);
> }
> return size;
> @@ -103,8 +103,7 @@ static void vb2_dc_prepare(void *buf_priv)
> if (!sgt)
> 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)
> @@ -115,7 +114,7 @@ static void vb2_dc_finish(void *buf_priv)
> if (!sgt)
> 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);
> }
>
> /*********************************************/
> @@ -275,8 +274,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;
> @@ -301,8 +300,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;
> }
>
> @@ -310,9 +309,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);
> @@ -455,8 +453,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));
> @@ -553,9 +551,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;
> @@ -577,8 +574,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 0a40e00f0d7e..0dd3b19025e0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -148,9 +148,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;
> @@ -186,8 +185,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);
> @@ -204,8 +203,7 @@ static void vb2_dma_sg_prepare(void *buf_priv)
> struct vb2_dma_sg_buf *buf = buf_priv;
> struct sg_table *sgt = buf->dma_sgt;
>
> - 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)
> @@ -213,7 +211,7 @@ static void vb2_dma_sg_finish(void *buf_priv)
> struct vb2_dma_sg_buf *buf = buf_priv;
> struct sg_table *sgt = buf->dma_sgt;
>
> - 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,
> @@ -256,9 +254,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;
> @@ -284,8 +281,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);
> @@ -408,8 +404,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, 0);
> sg_free_table(sgt);
> kfree(attach);
> db_attach->priv = NULL;
> @@ -434,15 +429,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, 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);
> diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
> index c66fda4a65e4..bf5ac63a5742 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);
>
On 2020-08-26 07:33, Marek Szyprowski wrote:
> 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 <[email protected]>
> ---
> 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 df44ed7393a0..3f366e4e4685 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);
If we're touching these lines anyway, we should update them to use the
modern DMA_FROM_DEVICE definitions too.
Robin.
> buf->sglen = 0;
> return 0;
> }
> diff --git a/drivers/media/pci/cx25821/cx25821-alsa.c b/drivers/media/pci/cx25821/cx25821-alsa.c
> index 301616426d8a..c40304d33776 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 7d7aceecc985..3c6fe6ceb0b7 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 544ca57eee75..398c47ff473d 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;
> }
>
>-----Original Message-----
>From: Robin Murphy <[email protected]>
>Sent: Tuesday, September 1, 2020 3:54 PM
>To: Ruhl, Michael J <[email protected]>; Marek Szyprowski
><[email protected]>; [email protected];
>[email protected]; [email protected]; linux-
>[email protected]
>Cc: Bartlomiej Zolnierkiewicz <[email protected]>; David Airlie
><[email protected]>; [email protected]; Christoph Hellwig
><[email protected]>; [email protected]
>Subject: Re: [Intel-gfx] [PATCH v9 08/32] drm: i915: fix common struct
>sg_table related issues
>
>On 2020-09-01 20:38, Ruhl, Michael J wrote:
>>> -----Original Message-----
>>> From: Intel-gfx <[email protected]> On Behalf Of
>>> Marek Szyprowski
>>> Sent: Wednesday, August 26, 2020 2:33 AM
>>> To: [email protected]; [email protected];
>>> [email protected]; [email protected]
>>> Cc: Bartlomiej Zolnierkiewicz <[email protected]>; David Airlie
>>> <[email protected]>; [email protected]; Robin Murphy
>>> <[email protected]>; Christoph Hellwig <[email protected]>; linux-arm-
>>> [email protected]; Marek Szyprowski
>>> <[email protected]>
>>> Subject: [Intel-gfx] [PATCH v9 08/32] drm: i915: fix common struct sg_table
>>> related issues
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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 2679380159fc..8a988592715b 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;
>>
>> You have dropped this error value.
>>
>> Do you now if this is a benign loss?
>
>True, dma_map_sgtable() will return -EINVAL rather than -ENOMEM for
>failure. A quick look through other .map_dma_buf callbacks suggests
>they're returning a motley mix of error values and NULL for failure
>cases, so I'd imagine that importers shouldn't be too sensitive to the
>exact value.
I followed some of our code through to see if anyone is checking for -ENOMEM...
I have found in some test paths... However, it is not clear to me if we can get
to those paths from here.
Anyways,
Reviewed-by: Michael J. Ruhl <[email protected]>
Mike
>Robin.
>
>>
>> M
>>
>>> + 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 debaf7b18ab5..be30b27e2926 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);
>>> }
>>> --
>>> 2.17.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> [email protected]
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi, Marek:
Marek Szyprowski <[email protected]> 於 2020年8月26日 週三 下午2:35寫道：
>
> 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.
>
Acked-by: Chun-Kuang Hu <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_gem.c | 28 ++++++--------------------
> 1 file changed, 6 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> index 6190cc3b7b0d..3654ec732029 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> @@ -212,37 +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_len(s))
> - break;
> -
> - 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)
> --
> 2.17.1
>
Robin Murphy <[email protected]> 於 2020年9月2日 週三 上午2:55寫道：
>
> On 2020-08-26 07:32, Marek Szyprowski wrote:
> > Use common helper for converting a sg_table object into struct
> > page pointer array.
>
> Reviewed-by: Robin Murphy <[email protected]>
>
> Side note: is mtk_drm_gem_prime_vmap() missing a call to
> sg_free_table(sgt) before its kfree(sgt)?
Yes, we need another patch to fix that bug, But for this patch,
Acked-by: Chun-Kuang Hu <[email protected]>
>
> > Signed-off-by: Marek Szyprowski <[email protected]>
> > ---
> > 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 3654ec732029..0583e557ad37 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));
> >
> >
On Tue, Sep 1, 2020 at 12:14 PM Robin Murphy <[email protected]> wrote:
>
> On 2020-08-26 07: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 <[email protected]>
> > Acked-by: Rob Clark <[email protected]>
> > ---
> > 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 b2f49152b4d4..8c7ae812b813 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 310a31b05faa..319f06c28235 100644
> > --- a/drivers/gpu/drm/msm/msm_gpummu.c
> > +++ b/drivers/gpu/drm/msm/msm_gpummu.c
> > @@ -30,21 +30,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;
>
> Given that the BUILD_BUG_ON might prevent valid arm64 configs from
> building, how about a simple tweak like:
>
> for (i = 0; i < PAGE_SIZE; i += GPUMMU_PAGE_SIZE)
> gpummu->table[idx++] = i + addr | prot_bits;
> ?
>
> Or alternatively perhaps some more aggressive #ifdefs or makefile tweaks
> to prevent the GPUMMU code building for arm64 at all if it's only
> relevant to 32-bit platforms (which I believe might be the case).
yes, the gpummu path is only used on older armv7 snapdragon and imx5
platforms.. I suppose maybe the easy thing would be to add a stub for
msm_gpummu_new() and msm_gpummu_params() and then only build it on
arm64?
BR,
-R
> Robin.
>
> > }
> >
> > /* 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 3a381a9674c9..6c31e65834c6 100644
> > --- a/drivers/gpu/drm/msm/msm_iommu.c
> > +++ b/drivers/gpu/drm/msm/msm_iommu.c
> > @@ -36,7 +36,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;
> >
On 01/09/2020 22:33, Robin Murphy wrote:
> On 2020-08-26 07: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.
>>
>> 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 <[email protected]>
>> ---
>> 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 ff0c4b0c3fd0..a7a9a0afe2b6 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)
>
> Hmm, if this really does mean *physically* contiguous - i.e. if buffers might be shared between
> DMA-translatable and non-DMA-translatable devices - then these changes might not be appropriate. If
> not and it only actually means DMA-contiguous, then it would be good to clarify the comments to that
> effect.
>
> Can anyone familiar with omapdrm clarify what exactly the case is here? I know that IOMMUs might be
> involved to some degree, and I've skimmed the interconnect chapters of enough OMAP TRMs to be scared
> by the reference to the tiler aperture in the context below :)
DSS (like many other IPs in OMAP) does not have any MMU/PAT, and can only use contiguous buffers
(contiguous in the RAM).
There's a special case with TILER (which is not part of DSS but of the memory subsystem, but it's
still handled internally by the omapdrm driver), which has a PAT. PAT can create a contiguous view
of scattered pages, and DSS can then use this contiguous view ("tiler aperture", which to DSS looks
just like normal contiguous memory).
Note that omapdrm does not use dma_map_sg() & co. mentioned in the patch description.
If there's no MMU/PAT, is orig_nents always the same as nents? Or can we have multiple physically
contiguous pages listed separately in the sgt (so orig_nents > 1) but as the pages form one big
contiguous area, nents == 1?
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Tomi,
On 02.09.2020 10:00, Tomi Valkeinen wrote:
> On 01/09/2020 22:33, Robin Murphy wrote:
>> On 2020-08-26 07: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.
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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 ff0c4b0c3fd0..a7a9a0afe2b6 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)
>> Hmm, if this really does mean *physically* contiguous - i.e. if buffers might be shared between
>> DMA-translatable and non-DMA-translatable devices - then these changes might not be appropriate. If
>> not and it only actually means DMA-contiguous, then it would be good to clarify the comments to that
>> effect.
>>
>> Can anyone familiar with omapdrm clarify what exactly the case is here? I know that IOMMUs might be
>> involved to some degree, and I've skimmed the interconnect chapters of enough OMAP TRMs to be scared
>> by the reference to the tiler aperture in the context below :)
> DSS (like many other IPs in OMAP) does not have any MMU/PAT, and can only use contiguous buffers
> (contiguous in the RAM).
>
> There's a special case with TILER (which is not part of DSS but of the memory subsystem, but it's
> still handled internally by the omapdrm driver), which has a PAT. PAT can create a contiguous view
> of scattered pages, and DSS can then use this contiguous view ("tiler aperture", which to DSS looks
> just like normal contiguous memory).
>
> Note that omapdrm does not use dma_map_sg() & co. mentioned in the patch description.
>
> If there's no MMU/PAT, is orig_nents always the same as nents? Or can we have multiple physically
> contiguous pages listed separately in the sgt (so orig_nents > 1) but as the pages form one big
> contiguous area, nents == 1?
Well, when DMA-mapping API is properly used, the difference between
nents and orig_nents is only when the scatterlist have been mapped for DMA.
For the mentioned case, even if the creator of the buffer used only the
pages that are consecutive in the physical memory, he is free to chose
either to set nents/orig_nents to 1 and length to n*PAGE_SIZE or set
nents/orig_nents to n and length to PAGE_SIZE for each. Then the buffer
chunks might be merged, but this is done by the DMA-mapping code. For
your case, without any call to DMA-mapping, you can only assume that the
buffer is contiguous in physical memory if orig_nents is 1.
I've changed the use of nents to orig_nents to make things consistent -
this code operates only on the unmapped buffers. I want to ensure that
anyone who will potentially copy this code, won't make the
nents/orig_nents mistake in the future.
If you don't like it, we can drop this patch, because it won't change
the way the driver works.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Hi again,
On 04.09.2020 14:06, Marek Szyprowski wrote:
> Hi Tomi,
>
> On 02.09.2020 10:00, Tomi Valkeinen wrote:
>> On 01/09/2020 22:33, Robin Murphy wrote:
>>> On 2020-08-26 07: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.
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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 ff0c4b0c3fd0..a7a9a0afe2b6 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)
>>> Hmm, if this really does mean *physically* contiguous - i.e. if
>>> buffers might be shared between
>>> DMA-translatable and non-DMA-translatable devices - then these
>>> changes might not be appropriate. If
>>> not and it only actually means DMA-contiguous, then it would be good
>>> to clarify the comments to that
>>> effect.
>>>
>>> Can anyone familiar with omapdrm clarify what exactly the case is
>>> here? I know that IOMMUs might be
>>> involved to some degree, and I've skimmed the interconnect chapters
>>> of enough OMAP TRMs to be scared
>>> by the reference to the tiler aperture in the context below :)
>> DSS (like many other IPs in OMAP) does not have any MMU/PAT, and can
>> only use contiguous buffers
>> (contiguous in the RAM).
>>
>> There's a special case with TILER (which is not part of DSS but of
>> the memory subsystem, but it's
>> still handled internally by the omapdrm driver), which has a PAT. PAT
>> can create a contiguous view
>> of scattered pages, and DSS can then use this contiguous view ("tiler
>> aperture", which to DSS looks
>> just like normal contiguous memory).
>>
>> Note that omapdrm does not use dma_map_sg() & co. mentioned in the
>> patch description.
>>
>> If there's no MMU/PAT, is orig_nents always the same as nents? Or can
>> we have multiple physically
>> contiguous pages listed separately in the sgt (so orig_nents > 1) but
>> as the pages form one big
>> contiguous area, nents == 1?
>
> Well, when DMA-mapping API is properly used, the difference between
> nents and orig_nents is only when the scatterlist have been mapped for
> DMA.
>
> For the mentioned case, even if the creator of the buffer used only
> the pages that are consecutive in the physical memory, he is free to
> chose either to set nents/orig_nents to 1 and length to n*PAGE_SIZE or
> set nents/orig_nents to n and length to PAGE_SIZE for each. Then the
> buffer chunks might be merged, but this is done by the DMA-mapping
> code. For your case, without any call to DMA-mapping, you can only
> assume that the buffer is contiguous in physical memory if orig_nents
> is 1.
>
> I've changed the use of nents to orig_nents to make things consistent
> - this code operates only on the unmapped buffers. I want to ensure
> that anyone who will potentially copy this code, won't make the
> nents/orig_nents mistake in the future.
I've just noticed that I've read my patch (the diff) in the reverse
order, I'm sorry. The omapdrm code is right, this patch should be dropped.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland