[PATCHv10,23/26] v4l: vb2-dma-contig: align buffer size to PAGE_SIZE

Most operations on DMA and DMABUF framework need page
aligned buffers. This fix guarantees this requirement
for vb2-dma-contig buffers.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c |    3 +++
 1 file changed, 3 insertions(+)

Hi Tomasz,

On Wednesday 10 October 2012 16:46:42 Tomasz Stanislawski wrote:
> Most operations on DMA and DMABUF framework need page
> aligned buffers.

The comment is a bit misleading, the buffer is already page-aligned (unless 
I'm mistaken dma_alloc_coherent() returns a page-aligned buffer) but its size 
isn't a multiple of the page size.

Do we really need a page size multiple ? Isn't it enough to make the size a 
multiple of the cache line size ?

> This fix guarantees this requirement
> for vb2-dma-contig buffers.
> 
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 571a919..002ee50
> 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -162,6 +162,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long
> size) if (!buf)
>  		return ERR_PTR(-ENOMEM);
> 
> +	/* align image size to PAGE_SIZE */
> +	size = PAGE_ALIGN(size);
> +
>  	buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL);
>  	if (!buf->vaddr) {
>  		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);

Hi Laurent,

On 10/11/2012 11:31 PM, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> On Wednesday 10 October 2012 16:46:42 Tomasz Stanislawski wrote:
>> Most operations on DMA and DMABUF framework need page
>> aligned buffers.
> 
> The comment is a bit misleading, the buffer is already page-aligned (unless 
> I'm mistaken dma_alloc_coherent() returns a page-aligned buffer) but its size 
> isn't a multiple of the page size.

Ok. I will update the commit message that only buffer size is going to be page aligned.

> 
> Do we really need a page size multiple ? Isn't it enough to make the size a 
> multiple of the cache line size ?
> 

Frankly, I strongly oppose forcing a size of a DMA buffer to be rounded up.

However, I discovered a problem while testing mmap() interface in dma-buf.
The test in dma_buf_mmap() will fail if the size is not a multiple of 4k.

Maybe the value from dma-buf.c:456 should be changed from:

dmabuf->size >> PAGE_SHIFT

to

PAGE_ALIGN(dmabuf->size) >> PAGE_SHIFT

However, I preferred to avoid any changes outside of the media tree
hoping that the patchset gets merged. Rounding the buffer size to
a page size was quick workaround for the issue with DMABUF mmap().

Regards,
Tomasz Stanislawski

>> This fix guarantees this requirement
>> for vb2-dma-contig buffers.
>>
>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-dma-contig.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 571a919..002ee50
>> 100644
>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> @@ -162,6 +162,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long
>> size) if (!buf)
>>  		return ERR_PTR(-ENOMEM);
>>
>> +	/* align image size to PAGE_SIZE */
>> +	size = PAGE_ALIGN(size);
>> +
>>  	buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL);
>>  	if (!buf->vaddr) {
>>  		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);

Hi Tomasz,

On Friday 12 October 2012 10:24:34 Tomasz Stanislawski wrote:
> On 10/11/2012 11:31 PM, Laurent Pinchart wrote:
> > On Wednesday 10 October 2012 16:46:42 Tomasz Stanislawski wrote:
> >> Most operations on DMA and DMABUF framework need page
> >> aligned buffers.
> > 
> > The comment is a bit misleading, the buffer is already page-aligned
> > (unless I'm mistaken dma_alloc_coherent() returns a page-aligned buffer)
> > but its size isn't a multiple of the page size.
> 
> Ok. I will update the commit message that only buffer size is going to be
> page aligned.
>
> > Do we really need a page size multiple ? Isn't it enough to make the size
> > a multiple of the cache line size ?
> 
> Frankly, I strongly oppose forcing a size of a DMA buffer to be rounded up.
> 
> However, I discovered a problem while testing mmap() interface in dma-buf.
> The test in dma_buf_mmap() will fail if the size is not a multiple of 4k.
> 
> Maybe the value from dma-buf.c:456 should be changed from:
> 
> dmabuf->size >> PAGE_SHIFT
> 
> to
> 
> PAGE_ALIGN(dmabuf->size) >> PAGE_SHIFT
> 
> However, I preferred to avoid any changes outside of the media tree hoping
> that the patchset gets merged. Rounding the buffer size to a page size was
> quick workaround for the issue with DMABUF mmap().

After some more thoughts I'm not sure whether this patch does the right thing. 
We have two sizes that we neeed to care about, the user usable buffer size and 
the allocated memory size.

When a user of a buffer requests buffer allocation with an explicit or 
implicit size we might need to allocate a larger buffer to fulfill usage 
requirements. We can thus end up with an allocated memory size larger than 
what the user requested. Such usage requirements include

- DMA and CPU access: when accessing the buffer both through DMA and directly 
by the CPU cache management comes into play, and the buffer address and size 
then need to be aligned to a cache line size boundary.

- Mapping to userspace: we can only map complete pages to userspace, the 
buffer address and size need to be aligned to a page size boundary to make 
sure that we won't leak unrelated data to userspace.

As the cache line size is smaller than a page fulfilling the second 
requirement always fulfills the first. There might be other requirements that 
escape my mind right now.

As we don't precisely know at allocation time how the buffer will be used (for 
instance whether it will eventually be mapped to userspace or not), we have 
two options:

- Align the buffer size to a page size boundary unconditionally.

- Let the user handle that requirement by specifying an allocation size 
aligned to a page size boundary.

Given the complexity associated with the second solution and the very small, 
if not inexistent, expected memory gain (when using the DMA allocation APIs we 
always get complete pages anyway, so there would be no gain in not aligning 
the allocation size to a page size boundary), I think the first solution 
should be preferred.

This leaves us with two questions related to buffer size: what size (between 
the requested size and the actually allocated size) do we report back to the 
user (in the v4l2_buffer length field for instance), and what size do we 
report to the dma-buf core (and thus to the other subsystems and other buffer 
users) ?