[{"id":30542,"web_url":"https://patchwork.libcamera.org/comment/30542/","msgid":"<172258465607.2725865.13849911406748174638@ping.linuxembedded.co.uk>","date":"2024-08-02T07:44:16","subject":"Re: [PATCH 2/2] libcamera: dma_buf_allocator: Retry ::ioctl on EINTR","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2024-08-02 08:24:16)\n> V4L userspace API documentation clearly mentions the handling of\n> EINTR on ioctl(). Align with the xioctl() reference provided in [1].\n\nThe DMA Buf allocator isn't part of V4L2 though ... so I think this\ncommit message needs to be updated.\n\n> Retry the ::ioctl() if failed with errno == EINTR. Use a do..while{}\n> to check the return value of ::ioctl() and errno value. If the return\n> value is found to be -1, and errno is set with EINTR, the ioctl() call\n> should be retried.\n> \n> [1]: https://docs.kernel.org/userspace-api/media/v4l/capture.c.html\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/libcamera/dma_buf_allocator.cpp | 18 +++++++++++++++---\n> 1 file changed, 15 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n> index be6efb89..5b469b19 100644\n> --- a/src/libcamera/dma_buf_allocator.cpp\n> +++ b/src/libcamera/dma_buf_allocator.cpp\n> @@ -128,6 +128,8 @@ DmaBufAllocator::~DmaBufAllocator() = default;\n> */\n> UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)\n> {\n> + int ret;\n> +\n> /* Size must be a multiple of the page size. Round it up. */\n> std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;\n> size = (size + pageMask) & ~pageMask;\n> @@ -144,7 +146,10 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)\n> create.offset = 0;\n> create.size = size;\n> \n> - int ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);\n> + do {\n> + ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);\n> + } while (ret == -1 && errno == EINTR);\n> +\n> if (ret < 0) {\n> ret = errno;\n> LOG(DmaBufAllocator, Error)\n> @@ -165,7 +170,10 @@ UniqueFD DmaBufAllocator::allocFromHeap(const char *name, std::size_t size)\n> alloc.len = size;\n> alloc.fd_flags = O_CLOEXEC | O_RDWR;\n> \n> - ret = ::ioctl(providerHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> + do {\n> + ret = ::ioctl(providerHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> + } while (ret == -1 && errno == EINTR);\n> +\n> if (ret < 0) {\n> LOG(DmaBufAllocator, Error)\n> << \"dma-heap allocation failure for \" << name;\n> @@ -173,7 +181,11 @@ UniqueFD DmaBufAllocator::allocFromHeap(const char *name, std::size_t size)\n> }\n> \n> UniqueFD allocFd(alloc.fd);\n> - ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> +\n> + do {\n> + ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> + } while (ret == -1 && errno == EINTR);\n> +\n\nI think anytime we repeat the same pattern three times - a helper is\nworthwhile...\n\nI almost wonder if we should just have a central ioctl helper in\nlibcamera to wrap this in a single place throughout though ... I guess\nit depends if there are any incidences where we would prefer /not/ to\nretry or if we can guarantee the code path of that ioctl is not\ninterruptible - but I suspect it's safer just to wrap!\n\nI'll await more discussion here ;-)\n\n--\nKieran\n\n\n> if (ret < 0) {\n> LOG(DmaBufAllocator, Error)\n> << \"dma-heap naming failure for \" << name;\n> -- \n> 2.45.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4CB95BDC71\n\tfor ;\n\tFri, 2 Aug 2024 07:44:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C8DD63383;\n\tFri, 2 Aug 2024 09:44:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD3276337F\n\tfor ;\n\tFri, 2 Aug 2024 09:44:18 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9BFFC7F3;\n\tFri, 2 Aug 2024 09:43:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CZpVadZD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722584609;\n\tbh=yzrdfTDPW4pgMC0Ft+eX3bbyOtbodfuZoQqPbGaE+DY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=CZpVadZDB9l3M2cnQwlbsBGTURQjCBxpen29awU3bP9YI02Y+gx8n4dPoffJF3uho\n\tP0XzyLXO7cisoAZmo3PuArihmEjmu3fuaJu+L7h/HCvLKBF0SxSvsIG9IZQC0w6ttX\n\tLl3KDhzhKqep00Cu9sxW4aScYNJ+wELRuDCK485c=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240802072416.25297-3-umang.jain@ideasonboard.com>","References":"<20240802072416.25297-1-umang.jain@ideasonboard.com>\n\t<20240802072416.25297-3-umang.jain@ideasonboard.com>","Subject":"Re: [PATCH 2/2] libcamera: dma_buf_allocator: Retry ::ioctl on EINTR","From":"Kieran Bingham ","Cc":"Umang Jain ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 02 Aug 2024 08:44:16 +0100","Message-ID":"<172258465607.2725865.13849911406748174638@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30544,"web_url":"https://patchwork.libcamera.org/comment/30544/","msgid":"<20240802130107.GF18732@pendragon.ideasonboard.com>","date":"2024-08-02T13:01:07","subject":"Re: [PATCH 2/2] libcamera: dma_buf_allocator: Retry ::ioctl on EINTR","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Aug 02, 2024 at 08:44:16AM +0100, Kieran Bingham wrote:\n> Quoting Umang Jain (2024-08-02 08:24:16)\n> > V4L userspace API documentation clearly mentions the handling of\n> > EINTR on ioctl(). Align with the xioctl() reference provided in [1].\n> \n> The DMA Buf allocator isn't part of V4L2 though ... so I think this\n> commit message needs to be updated.\n> \n> > Retry the ::ioctl() if failed with errno == EINTR. Use a do..while{}\n> > to check the return value of ::ioctl() and errno value. If the return\n> > value is found to be -1, and errno is set with EINTR, the ioctl() call\n> > should be retried.\n> > \n> > [1]: https://docs.kernel.org/userspace-api/media/v4l/capture.c.html\n> > \n> > Signed-off-by: Umang Jain \n> > ---\n> > src/libcamera/dma_buf_allocator.cpp | 18 +++++++++++++++---\n> > 1 file changed, 15 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n> > index be6efb89..5b469b19 100644\n> > --- a/src/libcamera/dma_buf_allocator.cpp\n> > +++ b/src/libcamera/dma_buf_allocator.cpp\n> > @@ -128,6 +128,8 @@ DmaBufAllocator::~DmaBufAllocator() = default;\n> > */\n> > UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)\n> > {\n> > + int ret;\n> > +\n> > /* Size must be a multiple of the page size. Round it up. */\n> > std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;\n> > size = (size + pageMask) & ~pageMask;\n> > @@ -144,7 +146,10 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)\n> > create.offset = 0;\n> > create.size = size;\n> > \n> > - int ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);\n> > + do {\n> > + ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);\n> > + } while (ret == -1 && errno == EINTR);\n> > +\n> > if (ret < 0) {\n> > ret = errno;\n> > LOG(DmaBufAllocator, Error)\n> > @@ -165,7 +170,10 @@ UniqueFD DmaBufAllocator::allocFromHeap(const char *name, std::size_t size)\n> > alloc.len = size;\n> > alloc.fd_flags = O_CLOEXEC | O_RDWR;\n> > \n> > - ret = ::ioctl(providerHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> > + do {\n> > + ret = ::ioctl(providerHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> > + } while (ret == -1 && errno == EINTR);\n> > +\n> > if (ret < 0) {\n> > LOG(DmaBufAllocator, Error)\n> > << \"dma-heap allocation failure for \" << name;\n> > @@ -173,7 +181,11 @@ UniqueFD DmaBufAllocator::allocFromHeap(const char *name, std::size_t size)\n> > }\n> > \n> > UniqueFD allocFd(alloc.fd);\n> > - ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> > +\n> > + do {\n> > + ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> > + } while (ret == -1 && errno == EINTR);\n> > +\n> \n> I think anytime we repeat the same pattern three times - a helper is\n> worthwhile...\n> \n> I almost wonder if we should just have a central ioctl helper in\n> libcamera to wrap this in a single place throughout though ... I guess\n> it depends if there are any incidences where we would prefer /not/ to\n> retry or if we can guarantee the code path of that ioctl is not\n> interruptible - but I suspect it's safer just to wrap!\n> \n> I'll await more discussion here ;-)\n\nExtending the File class may be an option. That would require a bit of\nwork, as currently it models a file with a path name, and that's not\nvery applicable when operating on a dmabuf FD. It should be possible to\nmake the name optional. The class would also need to support wrapping an\nalready open fd.\n\n> > if (ret < 0) {\n> > LOG(DmaBufAllocator, Error)\n> > << \"dma-heap naming failure for \" << name;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 246BEBDC71\n\tfor ;\n\tFri, 2 Aug 2024 13:01:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2EF4C63370;\n\tFri, 2 Aug 2024 15:01:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 44C726336B\n\tfor ;\n\tFri, 2 Aug 2024 15:01:28 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A74FE7F3;\n\tFri, 2 Aug 2024 15:00:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"o+xmUb17\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722603638;\n\tbh=L9dmZGj8DY1g39wcpk3ZVlhRw6hhug5hVboDHXb4AWg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=o+xmUb17v2zkbI+FFKqtgtgcGXSoh8NZgxi2sH2VCegDNwvPbT85siyxLxUAsQVOb\n\tDq69W1NMfVxTH9rkae5dJtTQN6fgU/RuL1561k8l38A9Fs/i9lvS+NpdaIVyh4lNjC\n\tO25/hmzYr+HLlhJ5p0+JiSpi9BTHMuUdiMaI0lb4=","Date":"Fri, 2 Aug 2024 16:01:07 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] libcamera: dma_buf_allocator: Retry ::ioctl on EINTR","Message-ID":"<20240802130107.GF18732@pendragon.ideasonboard.com>","References":"<20240802072416.25297-1-umang.jain@ideasonboard.com>\n\t<20240802072416.25297-3-umang.jain@ideasonboard.com>\n\t<172258465607.2725865.13849911406748174638@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172258465607.2725865.13849911406748174638@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]