>-----Original Message----- >From: Marek Szyprowski <[email protected]> >Sent: Tuesday, May 12, 2020 4:34 PM >To: Ruhl, Michael J <[email protected]>; dri- >[email protected]; [email protected]; linaro-mm- >[email protected]; [email protected] >Cc: Pawel Osciak <[email protected]>; Bartlomiej Zolnierkiewicz ><[email protected]>; David Airlie <[email protected]>; linux- >[email protected]; Hans Verkuil <[email protected]>; Mauro >Carvalho Chehab <[email protected]>; Robin Murphy ><[email protected]>; Christoph Hellwig <[email protected]>; linux-arm- >[email protected] >Subject: Re: [PATCH v4 38/38] videobuf2: use sgtable-based scatterlist >wrappers > >Hi Michael, > >On 12.05.2020 19:52, Ruhl, Michael J wrote: >>> -----Original Message----- >>> From: dri-devel <[email protected]> On Behalf Of >>> Marek Szyprowski >>> Sent: Tuesday, May 12, 2020 5:01 AM >>> To: [email protected]; [email protected]; >>> [email protected]; [email protected] >>> Cc: Pawel Osciak <[email protected]>; Bartlomiej Zolnierkiewicz >>> <[email protected]>; David Airlie <[email protected]>; linux- >>> [email protected]; Hans Verkuil <[email protected]>; Mauro >>> Carvalho Chehab <[email protected]>; Robin Murphy >>> <[email protected]>; Christoph Hellwig <[email protected]>; linux-arm- >>> [email protected]; Marek Szyprowski >>> <[email protected]> >>> 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 <[email protected]> >>> --- >>> 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- >>> [email protected]/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.
I was thinking of drivers outside of DRM/media. Specifically RDMA. However, looking at that code, I see that my memory was a little off. It is working with continuous pages, but not finding the size. >> 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(). Ahh, ok, I see. Thank you! Mike > >Best regards >-- >Marek Szyprowski, PhD >Samsung R&D Institute Poland
