| Message ID | 20200619103636.11974-10-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Hi On 19.06.2020 12:36, 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. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> May I ask for a review/ack/comment for this patch? > --- > 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); > } Best regards
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); }
The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg(). struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry). It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function. This driver creatively uses sg_table->orig_nents to store the size of the allocated scatterlist and ignores the number of the entries returned by dma_map_sg function. The sg_table->orig_nents is (mis)used to properly free the (over)allocated scatterlist. This patch only introduces the common DMA-mapping wrappers operating directly on the struct sg_table objects to the dmabuf related functions, so the other drivers, which might share buffers with i915 could rely on the properly set nents and orig_nents values. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- 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(-)