[2/2] libcamera: dma_buf_allocator: Retry ::ioctl on EINTR

20748 diff mbox series

V4L userspace API documentation clearly mentions the handling of
EINTR on ioctl(). Align with the xioctl() reference provided in [1].

Retry the ::ioctl() if failed with errno == EINTR. Use a do..while{}
to check the return value of ::ioctl() and errno value. If the return
value is found to be -1, and errno is set with EINTR, the ioctl() call
should be retried.

[1]: https://docs.kernel.org/userspace-api/media/v4l/capture.c.html

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/libcamera/dma_buf_allocator.cpp | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Quoting Umang Jain (2024-08-02 08:24:16)
> V4L userspace API documentation clearly mentions the handling of
> EINTR on ioctl(). Align with the xioctl() reference provided in [1].

The DMA Buf allocator isn't part of V4L2 though ... so I think this
commit message needs to be updated.

> Retry the ::ioctl() if failed with errno == EINTR. Use a do..while{}
> to check the return value of ::ioctl() and errno value. If the return
> value is found to be -1, and errno is set with EINTR, the ioctl() call
> should be retried.
> 
> [1]: https://docs.kernel.org/userspace-api/media/v4l/capture.c.html
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/libcamera/dma_buf_allocator.cpp | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index be6efb89..5b469b19 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -128,6 +128,8 @@ DmaBufAllocator::~DmaBufAllocator() = default;
>   */
>  UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
>  {
> +       int ret;
> +
>         /* Size must be a multiple of the page size. Round it up. */
>         std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
>         size = (size + pageMask) & ~pageMask;
> @@ -144,7 +146,10 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
>         create.offset = 0;
>         create.size = size;
>  
> -       int ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);
> +       do {
> +               ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);
> +       } while (ret == -1 && errno == EINTR);
> +
>         if (ret < 0) {
>                 ret = errno;
>                 LOG(DmaBufAllocator, Error)
> @@ -165,7 +170,10 @@ UniqueFD DmaBufAllocator::allocFromHeap(const char *name, std::size_t size)
>         alloc.len = size;
>         alloc.fd_flags = O_CLOEXEC | O_RDWR;
>  
> -       ret = ::ioctl(providerHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);
> +       do {
> +               ret = ::ioctl(providerHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);
> +       } while (ret == -1 && errno == EINTR);
> +
>         if (ret < 0) {
>                 LOG(DmaBufAllocator, Error)
>                         << "dma-heap allocation failure for " << name;
> @@ -173,7 +181,11 @@ UniqueFD DmaBufAllocator::allocFromHeap(const char *name, std::size_t size)
>         }
>  
>         UniqueFD allocFd(alloc.fd);
> -       ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);
> +
> +       do {
> +               ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);
> +       } while (ret == -1 && errno == EINTR);
> +

I think anytime we repeat the same pattern three times - a helper is
worthwhile...

I almost wonder if we should just have a central ioctl helper in
libcamera to wrap this in a single place throughout though ... I guess
it depends if there are any incidences where we would prefer /not/ to
retry or if we can guarantee the code path of that ioctl is not
interruptible - but I suspect it's safer just to wrap!

I'll await more discussion here ;-)

--
Kieran


>         if (ret < 0) {
>                 LOG(DmaBufAllocator, Error)
>                         << "dma-heap naming failure for " << name;
> -- 
> 2.45.2
>

On Fri, Aug 02, 2024 at 08:44:16AM +0100, Kieran Bingham wrote:
> Quoting Umang Jain (2024-08-02 08:24:16)
> > V4L userspace API documentation clearly mentions the handling of
> > EINTR on ioctl(). Align with the xioctl() reference provided in [1].
> 
> The DMA Buf allocator isn't part of V4L2 though ... so I think this
> commit message needs to be updated.
> 
> > Retry the ::ioctl() if failed with errno == EINTR. Use a do..while{}
> > to check the return value of ::ioctl() and errno value. If the return
> > value is found to be -1, and errno is set with EINTR, the ioctl() call
> > should be retried.
> > 
> > [1]: https://docs.kernel.org/userspace-api/media/v4l/capture.c.html
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  src/libcamera/dma_buf_allocator.cpp | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> > index be6efb89..5b469b19 100644
> > --- a/src/libcamera/dma_buf_allocator.cpp
> > +++ b/src/libcamera/dma_buf_allocator.cpp
> > @@ -128,6 +128,8 @@ DmaBufAllocator::~DmaBufAllocator() = default;
> >   */
> >  UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
> >  {
> > +       int ret;
> > +
> >         /* Size must be a multiple of the page size. Round it up. */
> >         std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> >         size = (size + pageMask) & ~pageMask;
> > @@ -144,7 +146,10 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
> >         create.offset = 0;
> >         create.size = size;
> >  
> > -       int ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);
> > +       do {
> > +               ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);
> > +       } while (ret == -1 && errno == EINTR);
> > +
> >         if (ret < 0) {
> >                 ret = errno;
> >                 LOG(DmaBufAllocator, Error)
> > @@ -165,7 +170,10 @@ UniqueFD DmaBufAllocator::allocFromHeap(const char *name, std::size_t size)
> >         alloc.len = size;
> >         alloc.fd_flags = O_CLOEXEC | O_RDWR;
> >  
> > -       ret = ::ioctl(providerHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);
> > +       do {
> > +               ret = ::ioctl(providerHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);
> > +       } while (ret == -1 && errno == EINTR);
> > +
> >         if (ret < 0) {
> >                 LOG(DmaBufAllocator, Error)
> >                         << "dma-heap allocation failure for " << name;
> > @@ -173,7 +181,11 @@ UniqueFD DmaBufAllocator::allocFromHeap(const char *name, std::size_t size)
> >         }
> >  
> >         UniqueFD allocFd(alloc.fd);
> > -       ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);
> > +
> > +       do {
> > +               ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);
> > +       } while (ret == -1 && errno == EINTR);
> > +
> 
> I think anytime we repeat the same pattern three times - a helper is
> worthwhile...
> 
> I almost wonder if we should just have a central ioctl helper in
> libcamera to wrap this in a single place throughout though ... I guess
> it depends if there are any incidences where we would prefer /not/ to
> retry or if we can guarantee the code path of that ioctl is not
> interruptible - but I suspect it's safer just to wrap!
> 
> I'll await more discussion here ;-)

Extending the File class may be an option. That would require a bit of
work, as currently it models a file with a path name, and that's not
very applicable when operating on a dmabuf FD. It should be possible to
make the name optional. The class would also need to support wrapping an
already open fd.

> >         if (ret < 0) {
> >                 LOG(DmaBufAllocator, Error)
> >                         << "dma-heap naming failure for " << name;