Messages in this thread | | | Subject | Re: [Intel-gfx] [PATCH v9 08/32] drm: i915: fix common struct sg_table related issues | From | Robin Murphy <> | Date | Tue, 1 Sep 2020 20:54:12 +0100 |
| |
On 2020-09-01 20:38, Ruhl, Michael J wrote: >> -----Original Message----- >> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of >> Marek Szyprowski >> Sent: Wednesday, August 26, 2020 2:33 AM >> To: dri-devel@lists.freedesktop.org; iommu@lists.linux-foundation.org; >> linaro-mm-sig@lists.linaro.org; linux-kernel@vger.kernel.org >> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>; David Airlie >> <airlied@linux.ie>; intel-gfx@lists.freedesktop.org; Robin Murphy >> <robin.murphy@arm.com>; Christoph Hellwig <hch@lst.de>; linux-arm- >> kernel@lists.infradead.org; Marek Szyprowski >> <m.szyprowski@samsung.com> >> 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 <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(-) >> >> 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 >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
| |