Re: [PATCH v4 08/14] v4l: vb2-dma-contig: add support for scatterlist in userptr mode

[Tomasz Stanislawski <t.stanislaws@samsung.com>]

Hi Laurent,


On 04/17/2012 02:43 AM, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> Thanks for the patch.
> 
> On Friday 13 April 2012 17:47:50 Tomasz Stanislawski wrote:
>> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>>
>> This patch introduces usage of dma_map_sg to map memory behind
>> a userspace pointer to a device as dma-contiguous mapping.
>>
>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> 	[bugfixing]
>> Signed-off-by: Kamil Debski <k.debski@samsung.com>
>> 	[bugfixing]
>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
>> 	[add sglist subroutines/code refactoring]
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/media/video/videobuf2-dma-contig.c |  287 +++++++++++++++++++++++--
>>  1 files changed, 270 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/media/video/videobuf2-dma-contig.c
>> b/drivers/media/video/videobuf2-dma-contig.c index 476e536..3a1e314 100644
>> --- a/drivers/media/video/videobuf2-dma-contig.c
>> +++ b/drivers/media/video/videobuf2-dma-contig.c
> 
> [snip]
> 

I found out that the function below may be useful for DMABUF
exporters and importers. I found that its code in
'[PATCH 1/4] [RFC] drm/exynos: DMABUF: Added support for exporting non-contig buffers'
by Prathyush K.

Therefore I posted a patch on linux-media:
'[PATCH] scatterlist: add sg_alloc_table_by_pages function'.

For now I will keep the function below (with your fixes). If
the patch above gets merged then the function will be removed.

Regards,
Tomasz Stanislawski

>> +static struct sg_table *vb2_dc_pages_to_sgt(struct page **pages,
>> +	unsigned int n_pages, unsigned long offset, unsigned long size)
>> +{
>> +	struct sg_table *sgt;
>> +	unsigned int chunks;
>> +	unsigned int i;
>> +	unsigned int cur_page;
>> +	int ret;
>> +	struct scatterlist *s;
>> +	unsigned int offset_end = n_pages * PAGE_SIZE - size;
> 
> Shouldn't offset_end be equal to n_page * PAGE_SIZE - size - offset ?
> 
>> +	sgt = kzalloc(sizeof *sgt, GFP_KERNEL);
>> +	if (!sgt)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	/* compute number of chunks */
>> +	chunks = 1;
>> +	for (i = 1; i < n_pages; ++i)
>> +		if (pages[i] != pages[i - 1] + 1)
>> +			++chunks;
>> +
>> +	ret = sg_alloc_table(sgt, chunks, GFP_KERNEL);
>> +	if (ret) {
>> +		kfree(sgt);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	/* merging chunks and putting them into the scatterlist */
>> +	cur_page = 0;
>> +	for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
>> +		size_t size = PAGE_SIZE;
> 
> This will shadow the size parameter, it's a bit confusing. You could rename it 
> chunk_size.
> 
>> +		unsigned int j;
>> +
>> +		for (j = cur_page + 1; j < n_pages; ++j) {
>> +			if (pages[j] != pages[j - 1] + 1)
>> +				break;
>> +			size += PAGE_SIZE;
>> +		}
> 
>> +		/* cut offset if chunk starts at the first page */
>> +		if (cur_page == 0)
>> +			size -= offset;
>> +		/* cut offset_end if chunk ends at the last page */
>> +		if (j == n_pages)
>> +			size -= offset_end;
>> +
>> +		sg_set_page(s, pages[cur_page], size, offset);
>> +		offset = 0;
> 
> What about just
> 
> 		chunk_size -= offset;
> 		sg_set_page(s, pages[cur_page], min(size, chunk_size), offset);
> 		size -= chunk_size;
> 		offset = 0;
> 
> You could then remove the offset_end calculation above.
> 
>> +		cur_page = j;
>> +	}
>> +
>> +	return sgt;
>> +}
>