| Message ID | 1349880405-26049-24-git-send-email-t.stanislaws@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) ?
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);
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(+)