Hi Michael,

On 12.05.2020 19:52, Ruhl, Michael J wrote:

...

-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Marek Szyprowski Sent: Tuesday, May 12, 2020 5:01 AM To: dri-devel@lists.freedesktop.org; iommu@lists.linux-foundation.org; linaro-mm-sig@lists.linaro.org; linux-kernel@vger.kernel.org Cc: Pawel Osciak pawel@osciak.com; Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com; David Airlie airlied@linux.ie; linux- media@vger.kernel.org; Hans Verkuil hverkuil-cisco@xs4all.nl; Mauro Carvalho Chehab mchehab@kernel.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: [PATCH v4 38/38] videobuf2: use sgtable-based scatterlist wrappers

Use recently introduced common wrappers operating directly on the struct sg_table objects and scatterlist page iterators to make the code a bit more compact, robust, easier to follow and copy/paste safe.

No functional change, because the code already properly did all the scaterlist related calls.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

For more information, see '[PATCH v4 00/38] DRM: fix struct sg_table nents vs. orig_nents misuse' thread: https://lore.kernel.org/dri-devel/20200512085710.14688-1- m.szyprowski@samsung.com/T/

---

.../media/common/videobuf2/videobuf2-dma-contig.c | 41 ++++++++++----

drivers/media/common/videobuf2/videobuf2-dma-sg.c | 32 +++++++--------

drivers/media/common/videobuf2/videobuf2-vmalloc.c | 12 +++---- 3 files changed, 34 insertions(+), 51 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c index d3a3ee5..bf31a9d 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c @@ -48,16 +48,15 @@ struct vb2_dc_buf {

static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt) {

• struct scatterlist *s; dma_addr_t expected = sg_dma_address(sgt->sgl);
• unsigned int i;

• struct sg_dma_page_iter dma_iter; unsigned long size = 0;

• for_each_sg(sgt->sgl, s, sgt->nents, i) {

• if (sg_dma_address(s) != expected)
  

• for_each_sgtable_dma_page(sgt, &dma_iter, 0) {

• if (sg_page_iter_dma_address(&dma_iter) != expected)
  break;
  

• expected = sg_dma_address(s) + sg_dma_len(s);
  

• size += sg_dma_len(s);
  

• expected += PAGE_SIZE;
  

• size += PAGE_SIZE;
  

This code in drm_prime_t_contiguous_size and here. I seem to remember seeing the same pattern in other drivers.

Would it worthwhile to make this a helper as well?

I think I've identified such patterns in all DRM drivers and replaced with a common helper. So far I have no idea where to put such helper to make it available for media/videobuf2, so those a few lines are indeed duplicated here.

Also, isn't the sg_dma_len() the actual length of the chunk we are looking at?

If its I not PAGE_SIZE (ie. dma chunk is 4 * PAGE_SIZE?), does your loop/calculation still work?

scaterlist page iterators (for_each_sg_page/for_each_sg_dma_page and their sgtable variants) always operates on PAGE_SIZE units. They correctly handle larger sg_dma_len().

Best regards