# Re: \[PATCH v4 38/38\] videobuf2: use sgtable-based scatterlist wrappers - From: Marek Szyprowski \ - Date: 2020-05-12 20:33:59 - List: linux-arm-kernel - Page: https://ratatoskr.run/linux-arm-kernel/2020/05/5962818 - Thread: https://ratatoskr.run/dri-devel/2020/05/5962025/t.md ```text Hi Michael, On 12.05.2020 19:52, Ruhl, Michael J wrote: >> -----Original Message----- >> From: dri-devel On Behalf Of >> Marek Szyprowski >> Sent: Tuesday, May 12, 2020 5:01 AM >> To: ; ; >> linaro-mm-sig@lists.linaro.org; linux-kernel@vger.kernel.org >> Cc: Pawel Osciak ; Bartlomiej Zolnierkiewicz >> [ref]; David Airlie [ref]; linux- >> media@vger.kernel.org; Hans Verkuil [ref]; Mauro >> Carvalho Chehab [ref]; Robin Murphy >> [ref]; Christoph Hellwig [ref]; linux-arm- >> kernel@lists.infradead.org; Marek Szyprowski >> [ref] >> 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 >> --- >> 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 -- Marek Szyprowski, PhD Samsung R&D Institute Poland _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ```