RE: [PATCH v4 38/38] videobuf2: use sgtable-based scatterlist wrappers

[Ruhl, Michael J]

>-----Original Message-----
>From: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>Sent: Tuesday, May 12, 2020 4:34 PM
>To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; dri-
>devel@xxxxxxxxxxxxxxxxxxxxx; iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; linaro-mm-
>sig@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>Cc: Pawel Osciak <pawel@xxxxxxxxxx>; Bartlomiej Zolnierkiewicz
><b.zolnierkie@xxxxxxxxxxx>; David Airlie <airlied@xxxxxxxx>; linux-
>media@xxxxxxxxxxxxxxx; Hans Verkuil <hverkuil-cisco@xxxxxxxxx>; Mauro
>Carvalho Chehab <mchehab@xxxxxxxxxx>; Robin Murphy
><robin.murphy@xxxxxxx>; Christoph Hellwig <hch@xxxxxx>; linux-arm-
>kernel@xxxxxxxxxxxxxxxxxxx
>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 <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
>>> Marek Szyprowski
>>> Sent: Tuesday, May 12, 2020 5:01 AM
>>> To: dri-devel@xxxxxxxxxxxxxxxxxxxxx; iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx;
>>> linaro-mm-sig@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>>> Cc: Pawel Osciak <pawel@xxxxxxxxxx>; Bartlomiej Zolnierkiewicz
>>> <b.zolnierkie@xxxxxxxxxxx>; David Airlie <airlied@xxxxxxxx>; linux-
>>> media@xxxxxxxxxxxxxxx; Hans Verkuil <hverkuil-cisco@xxxxxxxxx>; Mauro
>>> Carvalho Chehab <mchehab@xxxxxxxxxx>; Robin Murphy
>>> <robin.murphy@xxxxxxx>; Christoph Hellwig <hch@xxxxxx>; linux-arm-
>>> kernel@xxxxxxxxxxxxxxxxxxx; Marek Szyprowski
>>> <m.szyprowski@xxxxxxxxxxx>
>>> 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@xxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxx/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