| Message ID | 20240625112425.37172-1-fusibrandon13@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,v4l-utils] common: Use posix_memalign for allocating userptr buffers | expand |
Hi Brandon, On 25/06/2024 13:24, Brandon Cheo Fusi wrote: > When dealing with a userptr pointing to a buffer in userspace, > videobuf2 swaps the corresponding physical pages with other pages > so we have a contiguous area of memory for DMA. This only works if > the userptr is page aligned. > > The current way of allocating user buffers using malloc only > guarantees alignment up to `alignof(max_align_t)`, which is usually > 16. So replace malloc with posix_memalign to ensure the returned > pointer is on a page boundary. With which driver is this tested? USERPTR is really supposed to work with malloc()ed memory. There have been drivers in the past that used contiguous DMA but still allowed USERPTR, but that was a hack and these days you would use DMABUF. Regards, Hans > > Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com> > --- > utils/common/v4l-helpers.h | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h > index cf0e92d..aa9ee26 100644 > --- a/utils/common/v4l-helpers.h > +++ b/utils/common/v4l-helpers.h > @@ -1661,10 +1661,11 @@ static inline int v4l_queue_alloc_bufs(struct v4l_fd *f, > return 0; > for (b = from; b < v4l_queue_g_buffers(q); b++) { > for (p = 0; p < v4l_queue_g_num_planes(q); p++) { > - void *m = malloc(v4l_queue_g_length(q, p)); > - > - if (m == NULL) > - return errno; > + void *m; > + int ret = posix_memalign(&m, getpagesize(), > + v4l_queue_g_length(q, p)); > + if (ret) > + return ret; > v4l_queue_s_userptr(q, b, p, m); > } > }
On Thu, Oct 17, 2024 at 7:51 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > Hi Brandon, > > On 25/06/2024 13:24, Brandon Cheo Fusi wrote: > > When dealing with a userptr pointing to a buffer in userspace, > > videobuf2 swaps the corresponding physical pages with other pages > > so we have a contiguous area of memory for DMA. This only works if > > the userptr is page aligned. > > > > The current way of allocating user buffers using malloc only > > guarantees alignment up to `alignof(max_align_t)`, which is usually > > 16. So replace malloc with posix_memalign to ensure the returned > > pointer is on a page boundary. > > With which driver is this tested? USERPTR is really supposed to work > with malloc()ed memory. In theory yes, but the entire idea of USERPTR is shaky enough that some hardware may not work if the pointer is not aligned enough. Another aspect is security - IOMMUs can only work on page granularity, so if the first page of the buffer is preceded (or last page followed) by some other data, that data would be leaked to the hardware. That said, that doesn't seem to be what the commit description refers to and actually it's kind of wrong - videobuf2 doesn't do anything to make the userptr memory contiguous. It's all down to the DMA mapping ops of the device, and if it's behind an IOMMU, it should just work regardless of the alignment, minus the user data leak caveat I mentioned. Besides that, any idea why this function actually even uses userptr? (I assume this is libv4l?) > > There have been drivers in the past that used contiguous DMA but still > allowed USERPTR, but that was a hack and these days you would use DMABUF. > > Regards, > > Hans > > > > > Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com> > > --- > > utils/common/v4l-helpers.h | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h > > index cf0e92d..aa9ee26 100644 > > --- a/utils/common/v4l-helpers.h > > +++ b/utils/common/v4l-helpers.h > > @@ -1661,10 +1661,11 @@ static inline int v4l_queue_alloc_bufs(struct v4l_fd *f, > > return 0; > > for (b = from; b < v4l_queue_g_buffers(q); b++) { > > for (p = 0; p < v4l_queue_g_num_planes(q); p++) { > > - void *m = malloc(v4l_queue_g_length(q, p)); > > - > > - if (m == NULL) > > - return errno; > > + void *m; > > + int ret = posix_memalign(&m, getpagesize(), > > + v4l_queue_g_length(q, p)); > > + if (ret) > > + return ret; > > v4l_queue_s_userptr(q, b, p, m); > > } > > } >
diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h index cf0e92d..aa9ee26 100644 --- a/utils/common/v4l-helpers.h +++ b/utils/common/v4l-helpers.h @@ -1661,10 +1661,11 @@ static inline int v4l_queue_alloc_bufs(struct v4l_fd *f, return 0; for (b = from; b < v4l_queue_g_buffers(q); b++) { for (p = 0; p < v4l_queue_g_num_planes(q); p++) { - void *m = malloc(v4l_queue_g_length(q, p)); - - if (m == NULL) - return errno; + void *m; + int ret = posix_memalign(&m, getpagesize(), + v4l_queue_g_length(q, p)); + if (ret) + return ret; v4l_queue_s_userptr(q, b, p, m); } }
When dealing with a userptr pointing to a buffer in userspace, videobuf2 swaps the corresponding physical pages with other pages so we have a contiguous area of memory for DMA. This only works if the userptr is page aligned. The current way of allocating user buffers using malloc only guarantees alignment up to `alignof(max_align_t)`, which is usually 16. So replace malloc with posix_memalign to ensure the returned pointer is on a page boundary. Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com> --- utils/common/v4l-helpers.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)