libcamera: dma_buf_allocator: Work around lack of memfd_create() in uClibc

20203 diff mbox series

uClibc doesn't provide a memfd_create() implementation. Fix it by using
a direct syscall when the function isn't available.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
I should have bundled this in a series with "[PATCH] libcamera:
dma_buf_allocator: Work around lack of file seals in uClibc", sorry
about that.
---
 src/libcamera/dma_buf_allocator.cpp | 5 +++++
 1 file changed, 5 insertions(+)


base-commit: 98071d3109c131820439f61d9380c0bd4cd2119a

On Wed, Jun 05, 2024 at 11:37:57AM +0300, Laurent Pinchart wrote:
> uClibc doesn't provide a memfd_create() implementation. Fix it by using
> a direct syscall when the function isn't available.
> 

Fixes: ea4baaacc325 ("libcamera: DmaBufAllocator: Support allocating from /dev/udmabuf")

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> I should have bundled this in a series with "[PATCH] libcamera:
> dma_buf_allocator: Work around lack of file seals in uClibc", sorry
> about that.
> ---
>  src/libcamera/dma_buf_allocator.cpp | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index 5ec29949c66a..d7d08e188a62 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -13,6 +13,7 @@
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
>  #include <sys/stat.h>
> +#include <sys/syscall.h>
>  #include <sys/types.h>
>  #include <unistd.h>
>  
> @@ -132,7 +133,11 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
>  	std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
>  	size = (size + pageMask) & ~pageMask;
>  
> +#if HAVE_MEMFD_CREATE
>  	int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> +#else
> +	int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> +#endif
>  	if (ret < 0) {
>  		ret = errno;
>  		LOG(DmaBufAllocator, Error)
> 
> base-commit: 98071d3109c131820439f61d9380c0bd4cd2119a

Quoting Laurent Pinchart (2024-06-05 09:37:57)
> uClibc doesn't provide a memfd_create() implementation. Fix it by using
> a direct syscall when the function isn't available.

I presume the fact these are only coming up now means we don't yet have
uClibc in CI?


> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> I should have bundled this in a series with "[PATCH] libcamera:
> dma_buf_allocator: Work around lack of file seals in uClibc", sorry
> about that.
> ---
>  src/libcamera/dma_buf_allocator.cpp | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index 5ec29949c66a..d7d08e188a62 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -13,6 +13,7 @@
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
>  #include <sys/stat.h>
> +#include <sys/syscall.h>
>  #include <sys/types.h>
>  #include <unistd.h>
>  
> @@ -132,7 +133,11 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
>         std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
>         size = (size + pageMask) & ~pageMask;
>  
> +#if HAVE_MEMFD_CREATE
>         int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> +#else
> +       int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> +#endif

Is there any benefit to going through memfd_create at all in that case?

But this is fine otherwise.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         if (ret < 0) {
>                 ret = errno;
>                 LOG(DmaBufAllocator, Error)
> 
> base-commit: 98071d3109c131820439f61d9380c0bd4cd2119a
> -- 
> Regards,
> 
> Laurent Pinchart
>

On Wed, Jun 05, 2024 at 09:49:26AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-06-05 09:37:57)
> > uClibc doesn't provide a memfd_create() implementation. Fix it by using
> > a direct syscall when the function isn't available.
> 
> I presume the fact these are only coming up now means we don't yet have
> uClibc in CI?

That's right. musl would be relatively easy to add with an Alpine
container, but uClibc would be harder.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > I should have bundled this in a series with "[PATCH] libcamera:
> > dma_buf_allocator: Work around lack of file seals in uClibc", sorry
> > about that.
> > ---
> >  src/libcamera/dma_buf_allocator.cpp | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> > index 5ec29949c66a..d7d08e188a62 100644
> > --- a/src/libcamera/dma_buf_allocator.cpp
> > +++ b/src/libcamera/dma_buf_allocator.cpp
> > @@ -13,6 +13,7 @@
> >  #include <sys/ioctl.h>
> >  #include <sys/mman.h>
> >  #include <sys/stat.h>
> > +#include <sys/syscall.h>
> >  #include <sys/types.h>
> >  #include <unistd.h>
> >  
> > @@ -132,7 +133,11 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
> >         std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> >         size = (size + pageMask) & ~pageMask;
> >  
> > +#if HAVE_MEMFD_CREATE
> >         int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> > +#else
> > +       int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> > +#endif
> 
> Is there any benefit to going through memfd_create at all in that case?

I think it's best to use what the libc provides, that lowers the risk of
breakages in the future.

We already have a similar construct in
src/libcamera/shared_mem_object.cpp by the way, see commit acf61456cc55
("libcamera: shared_mem_object: Fix compilation with uClibc").

> But this is fine otherwise.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >         if (ret < 0) {
> >                 ret = errno;
> >                 LOG(DmaBufAllocator, Error)
> > 
> > base-commit: 98071d3109c131820439f61d9380c0bd4cd2119a

Hi,

On 6/5/24 10:37 AM, Laurent Pinchart wrote:
> uClibc doesn't provide a memfd_create() implementation. Fix it by using
> a direct syscall when the function isn't available.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
> I should have bundled this in a series with "[PATCH] libcamera:
> dma_buf_allocator: Work around lack of file seals in uClibc", sorry
> about that.
> ---
>  src/libcamera/dma_buf_allocator.cpp | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index 5ec29949c66a..d7d08e188a62 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -13,6 +13,7 @@
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
>  #include <sys/stat.h>
> +#include <sys/syscall.h>
>  #include <sys/types.h>
>  #include <unistd.h>
>  
> @@ -132,7 +133,11 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
>  	std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
>  	size = (size + pageMask) & ~pageMask;
>  
> +#if HAVE_MEMFD_CREATE
>  	int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> +#else
> +	int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> +#endif
>  	if (ret < 0) {
>  		ret = errno;
>  		LOG(DmaBufAllocator, Error)
> 
> base-commit: 98071d3109c131820439f61d9380c0bd4cd2119a

I know this is merged already, but for future style improvements ....

Le mercredi 05 juin 2024 à 11:37 +0300, Laurent Pinchart a écrit :
> uClibc doesn't provide a memfd_create() implementation. Fix it by using
> a direct syscall when the function isn't available.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> I should have bundled this in a series with "[PATCH] libcamera:
> dma_buf_allocator: Work around lack of file seals in uClibc", sorry
> about that.
> ---
>  src/libcamera/dma_buf_allocator.cpp | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index 5ec29949c66a..d7d08e188a62 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -13,6 +13,7 @@
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
>  #include <sys/stat.h>
> +#include <sys/syscall.h>
>  #include <sys/types.h>
>  #include <unistd.h>
>  
> @@ -132,7 +133,11 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
>  	std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
>  	size = (size + pageMask) & ~pageMask;
>  
> +#if HAVE_MEMFD_CREATE
>  	int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> +#else
> +	int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> +#endif

Instead of polluting code with ifdefs, a strongly suggest using a compat header
(or C++ file):

  #ifndef HAVE_MEMFD_CREATE
  inline int memfd_create(...)
  {
  }
  #endif

Nicolas

>  	if (ret < 0) {
>  		ret = errno;
>  		LOG(DmaBufAllocator, Error)
> 
> base-commit: 98071d3109c131820439f61d9380c0bd4cd2119a

Hi Nicolas,

On Wed, Jun 05, 2024 at 01:53:47PM -0400, Nicolas Dufresne wrote:
> I know this is merged already, but for future style improvements ....

We can still fix or improve code that has been merged already :-)

> Le mercredi 05 juin 2024 à 11:37 +0300, Laurent Pinchart a écrit :
> > uClibc doesn't provide a memfd_create() implementation. Fix it by using
> > a direct syscall when the function isn't available.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > I should have bundled this in a series with "[PATCH] libcamera:
> > dma_buf_allocator: Work around lack of file seals in uClibc", sorry
> > about that.
> > ---
> >  src/libcamera/dma_buf_allocator.cpp | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> > index 5ec29949c66a..d7d08e188a62 100644
> > --- a/src/libcamera/dma_buf_allocator.cpp
> > +++ b/src/libcamera/dma_buf_allocator.cpp
> > @@ -13,6 +13,7 @@
> >  #include <sys/ioctl.h>
> >  #include <sys/mman.h>
> >  #include <sys/stat.h>
> > +#include <sys/syscall.h>
> >  #include <sys/types.h>
> >  #include <unistd.h>
> >  
> > @@ -132,7 +133,11 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
> >  	std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> >  	size = (size + pageMask) & ~pageMask;
> >  
> > +#if HAVE_MEMFD_CREATE
> >  	int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> > +#else
> > +	int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> > +#endif
> 
> Instead of polluting code with ifdefs, a strongly suggest using a compat header
> (or C++ file):
> 
>   #ifndef HAVE_MEMFD_CREATE
>   inline int memfd_create(...)
>   {
>   }
>   #endif

That's a good idea. I'll test it out.

> >  	if (ret < 0) {
> >  		ret = errno;
> >  		LOG(DmaBufAllocator, Error)
> > 
> > base-commit: 98071d3109c131820439f61d9380c0bd4cd2119a