[{"id":31220,"web_url":"https://patchwork.libcamera.org/comment/31220/","msgid":"<172623067173.3474483.767935304398814078@ping.linuxembedded.co.uk>","date":"2024-09-13T12:31:11","subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Robert,\n\nQuoting Robert Mader (2024-09-13 13:04:16)\n> Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure\n> correct output. Not doing so currently results in occasional tearing\n> and/or backlashes in GL/VK clients that use the buffers directly for\n> rendering.\n> \n> An alternative approach to have the sync code in `MappedFrameBuffer` was\n> considered but rejected for now, in order to allow clients more\n> flexibility.\n> \n> Signed-off-by: Robert Mader \n> \n> ---\n> \n> Changes in v2:\n> - sync input buffer as well\n> - update commit title accordingly\n> - small changes to the commit message\n> - linting fixes, should pass now\n> ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++\n> 1 file changed, 35 insertions(+)\n> \n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 077f7f4b..a3b4851c 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -12,8 +12,11 @@\n> #include \"debayer_cpu.h\"\n> \n> #include \n> +#include \n> #include \n> \n> +#include \n> +\n> #include \n> \n> #include \"libcamera/internal/bayer_format.h\"\n> @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> }\n> \n> + for (const FrameBuffer::Plane &plane : input->planes()) {\n> + const int fd = plane.fd.get();\n> + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ };\n> +\n> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n\nWe can't use 'errno' directly on our error messages because the LOG()\nimplementations or other processing of the log statement (not in this\ncase, but in others it could be possible) could affect errno.\n\nSo we use the style \n\nsrc/libcamera/dma_buf_allocator.cpp- if (ret < 0) {\nsrc/libcamera/dma_buf_allocator.cpp: ret = errno;\nsrc/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error)\nsrc/libcamera/dma_buf_allocator.cpp- << \"Failed to create dma buf for \" << name\nsrc/libcamera/dma_buf_allocator.cpp- << \": \" << strerror(ret);\nsrc/libcamera/dma_buf_allocator.cpp- return {};\nsrc/libcamera/dma_buf_allocator.cpp- }\n\nWhere we store the errno immediately and then use it in the log. It's\nalso helpful to use strerror too.\n\n> + }\n> +\n> + for (const FrameBuffer::Plane &plane : output->planes()) {\n> + const int fd = plane.fd.get();\n> + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE };\n> +\n> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> + }\n\n(jumping back up from below) And this likewise could then be:\n\n\tintput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n \toutput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n\n> +\n> green_ = params.green;\n> red_ = swapRedBlueGains_ ? params.blue : params.red;\n> blue_ = swapRedBlueGains_ ? params.red : params.blue;\n> @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> \n> metadata.planes()[0].bytesused = out.planes()[0].size();\n> \n> + for (const FrameBuffer::Plane &plane : output->planes()) {\n> + const int fd = plane.fd.get();\n> + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE };\n> +\n> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> + }\n> +\n> + for (const FrameBuffer::Plane &plane : input->planes()) {\n> + const int fd = plane.fd.get();\n> + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ };\n> +\n> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n\nThat's four iterations of very similar code. I think we could add a\nhelper directly into FrameBuffer directly as a preceeding patch, that\nwould allow something more like:\n\n\toutput->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n\tinput->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n\n\nWe could probably do that using our Flags class too ... but the above\nwould be a good start.\n\n\n\n> + }\n> +\n> /* Measure before emitting signals */\n> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> -- \n> 2.46.0\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 061A6C3257\n\tfor ;\n\tFri, 13 Sep 2024 12:31:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 090A9634F8;\n\tFri, 13 Sep 2024 14:31:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46468634E3\n\tfor ;\n\tFri, 13 Sep 2024 14:31:14 +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 387D474C;\n\tFri, 13 Sep 2024 14:29:55 +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=\"UByt1CNe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726230595;\n\tbh=RazpYIPjPwp9hPr4SN6H0D+qMCaH0aGQPs8jkCsQhSw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=UByt1CNe+VbU49weHEiE0qLpkRfp+3dx0JTzZJc3G8unpPidmjlNIYtZzVR4aF/Pk\n\tIHODv+ZLb75Ez3KgE9PkFaX6uQVbmM0s1tNHlPVQuYVzPuiumLWt2gIyFkcH9uH88k\n\tbmX7589g/FdD71ldwjttvZEfRv0O1DoM8Yt0HzUE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240913120416.96828-1-robert.mader@collabora.com>","References":"<20240913120416.96828-1-robert.mader@collabora.com>","Subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","From":"Kieran Bingham ","Cc":"Robert Mader ","To":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 13 Sep 2024 13:31:11 +0100","Message-ID":"<172623067173.3474483.767935304398814078@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":31221,"web_url":"https://patchwork.libcamera.org/comment/31221/","msgid":"<20240913123844.GG21327@pendragon.ideasonboard.com>","date":"2024-09-13T12:38:44","subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote:\n> Quoting Robert Mader (2024-09-13 13:04:16)\n> > Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure\n> > correct output. Not doing so currently results in occasional tearing\n> > and/or backlashes in GL/VK clients that use the buffers directly for\n> > rendering.\n> > \n> > An alternative approach to have the sync code in `MappedFrameBuffer` was\n> > considered but rejected for now, in order to allow clients more\n> > flexibility.\n> > \n> > Signed-off-by: Robert Mader \n> > \n> > ---\n> > \n> > Changes in v2:\n> > - sync input buffer as well\n> > - update commit title accordingly\n> > - small changes to the commit message\n> > - linting fixes, should pass now\n> > ---\n> > src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++\n> > 1 file changed, 35 insertions(+)\n> > \n> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> > index 077f7f4b..a3b4851c 100644\n> > --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> > @@ -12,8 +12,11 @@\n> > #include \"debayer_cpu.h\"\n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > +#include \n> > +\n> > #include \n> > \n> > #include \"libcamera/internal/bayer_format.h\"\n> > @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> > }\n> > \n> > + for (const FrameBuffer::Plane &plane : input->planes()) {\n> > + const int fd = plane.fd.get();\n> > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ };\n> > +\n> > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> \n> We can't use 'errno' directly on our error messages because the LOG()\n> implementations or other processing of the log statement (not in this\n> case, but in others it could be possible) could affect errno.\n> \n> So we use the style \n> \n> src/libcamera/dma_buf_allocator.cpp- if (ret < 0) {\n> src/libcamera/dma_buf_allocator.cpp: ret = errno;\n> src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error)\n> src/libcamera/dma_buf_allocator.cpp- << \"Failed to create dma buf for \" << name\n> src/libcamera/dma_buf_allocator.cpp- << \": \" << strerror(ret);\n> src/libcamera/dma_buf_allocator.cpp- return {};\n> src/libcamera/dma_buf_allocator.cpp- }\n> \n> Where we store the errno immediately and then use it in the log. It's\n> also helpful to use strerror too.\n> \n> > + }\n> > +\n> > + for (const FrameBuffer::Plane &plane : output->planes()) {\n> > + const int fd = plane.fd.get();\n> > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE };\n> > +\n> > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > + }\n> \n> (jumping back up from below) And this likewise could then be:\n> \n> \tintput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> \toutput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> \n> > +\n> > green_ = params.green;\n> > red_ = swapRedBlueGains_ ? params.blue : params.red;\n> > blue_ = swapRedBlueGains_ ? params.red : params.blue;\n> > @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> > \n> > metadata.planes()[0].bytesused = out.planes()[0].size();\n> > \n> > + for (const FrameBuffer::Plane &plane : output->planes()) {\n> > + const int fd = plane.fd.get();\n> > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE };\n> > +\n> > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > + }\n> > +\n> > + for (const FrameBuffer::Plane &plane : input->planes()) {\n> > + const int fd = plane.fd.get();\n> > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ };\n> > +\n> > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> \n> That's four iterations of very similar code. I think we could add a\n> helper directly into FrameBuffer directly as a preceeding patch, that\n> would allow something more like:\n> \n> \toutput->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> \tinput->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> \n> We could probably do that using our Flags class too ... but the above\n> would be a good start.\n\nI'd like a helper too, but I don't think it should be part of the public\nAPI, so the FrameBuffer class isn't the right place.\n\n> > + }\n> > +\n> > /* Measure before emitting signals */\n> > if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> > ++measuredFrames_ > DebayerCpu::kFramesToSkip) {","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 48662C324C\n\tfor ;\n\tFri, 13 Sep 2024 12:39:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B328634F4;\n\tFri, 13 Sep 2024 14:39:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C8FD634E3\n\tfor ;\n\tFri, 13 Sep 2024 14:39:20 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(213-229-8-243.static.upcbusiness.at [213.229.8.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8CA8574C;\n\tFri, 13 Sep 2024 14:38:00 +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=\"nHKUJcv3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726231081;\n\tbh=z8GcwVCqDNeqT0p54zTkYjQ3HT6dFspc9rQ/bm1MiT4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nHKUJcv3Ns/X53IPjL0l5FgZdKSzOBSMKE+vMdhFnKnaHw65YsVZKzEFUgD1+Enfq\n\tiBmsrnStqHOVcsrDyy6j10/PZ6m54LlRehrV8K8ZDwQJu1eZAnDpzLzYqGgJY3Xk/k\n\tjlqXJ5EqLq9P5VxGde22tD++ynFmc2E8cDpM422g=","Date":"Fri, 13 Sep 2024 15:38:44 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","Message-ID":"<20240913123844.GG21327@pendragon.ideasonboard.com>","References":"<20240913120416.96828-1-robert.mader@collabora.com>\n\t<172623067173.3474483.767935304398814078@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172623067173.3474483.767935304398814078@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\" "}},{"id":31223,"web_url":"https://patchwork.libcamera.org/comment/31223/","msgid":"<172623279791.3474483.1263179436181482490@ping.linuxembedded.co.uk>","date":"2024-09-13T13:06:37","subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-09-13 13:38:44)\n> On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote:\n> > Quoting Robert Mader (2024-09-13 13:04:16)\n> > > Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure\n> > > correct output. Not doing so currently results in occasional tearing\n> > > and/or backlashes in GL/VK clients that use the buffers directly for\n> > > rendering.\n> > > \n> > > An alternative approach to have the sync code in `MappedFrameBuffer` was\n> > > considered but rejected for now, in order to allow clients more\n> > > flexibility.\n> > > \n> > > Signed-off-by: Robert Mader \n> > > \n> > > ---\n> > > \n> > > Changes in v2:\n> > > - sync input buffer as well\n> > > - update commit title accordingly\n> > > - small changes to the commit message\n> > > - linting fixes, should pass now\n> > > ---\n> > > src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++\n> > > 1 file changed, 35 insertions(+)\n> > > \n> > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> > > index 077f7f4b..a3b4851c 100644\n> > > --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> > > +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> > > @@ -12,8 +12,11 @@\n> > > #include \"debayer_cpu.h\"\n> > > \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > +#include \n> > > +\n> > > #include \n> > > \n> > > #include \"libcamera/internal/bayer_format.h\"\n> > > @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> > > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> > > }\n> > > \n> > > + for (const FrameBuffer::Plane &plane : input->planes()) {\n> > > + const int fd = plane.fd.get();\n> > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ };\n> > > +\n> > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > \n> > We can't use 'errno' directly on our error messages because the LOG()\n> > implementations or other processing of the log statement (not in this\n> > case, but in others it could be possible) could affect errno.\n> > \n> > So we use the style \n> > \n> > src/libcamera/dma_buf_allocator.cpp- if (ret < 0) {\n> > src/libcamera/dma_buf_allocator.cpp: ret = errno;\n> > src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error)\n> > src/libcamera/dma_buf_allocator.cpp- << \"Failed to create dma buf for \" << name\n> > src/libcamera/dma_buf_allocator.cpp- << \": \" << strerror(ret);\n> > src/libcamera/dma_buf_allocator.cpp- return {};\n> > src/libcamera/dma_buf_allocator.cpp- }\n> > \n> > Where we store the errno immediately and then use it in the log. It's\n> > also helpful to use strerror too.\n> > \n> > > + }\n> > > +\n> > > + for (const FrameBuffer::Plane &plane : output->planes()) {\n> > > + const int fd = plane.fd.get();\n> > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE };\n> > > +\n> > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > > + }\n> > \n> > (jumping back up from below) And this likewise could then be:\n> > \n> > intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> > output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> > \n> > > +\n> > > green_ = params.green;\n> > > red_ = swapRedBlueGains_ ? params.blue : params.red;\n> > > blue_ = swapRedBlueGains_ ? params.red : params.blue;\n> > > @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> > > \n> > > metadata.planes()[0].bytesused = out.planes()[0].size();\n> > > \n> > > + for (const FrameBuffer::Plane &plane : output->planes()) {\n> > > + const int fd = plane.fd.get();\n> > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE };\n> > > +\n> > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > > + }\n> > > +\n> > > + for (const FrameBuffer::Plane &plane : input->planes()) {\n> > > + const int fd = plane.fd.get();\n> > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ };\n> > > +\n> > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > \n> > That's four iterations of very similar code. I think we could add a\n> > helper directly into FrameBuffer directly as a preceeding patch, that\n> > would allow something more like:\n> > \n> > output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> > input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> > \n> > We could probably do that using our Flags class too ... but the above\n> > would be a good start.\n> \n> I'd like a helper too, but I don't think it should be part of the public\n> API, so the FrameBuffer class isn't the right place.\n\nHow about in FrameBuffer::Private::sync() then ?\n\nOr worst case - and to progress here, at the very least - just a private\nDebayerCpu::syncBuffer() for\n\n\tsyncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n\nwhich would at least bump any documentation requirements until there are\nmore users.\n\n--\nKieran\n\n> \n> > > + }\n> > > +\n> > > /* Measure before emitting signals */\n> > > if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> > > ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 599E7C324C\n\tfor ;\n\tFri, 13 Sep 2024 13:06:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32D38634F8;\n\tFri, 13 Sep 2024 15:06:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 44E39634E3\n\tfor ;\n\tFri, 13 Sep 2024 15:06:41 +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 47D4DE0D;\n\tFri, 13 Sep 2024 15:05:22 +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=\"EN4LypJm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726232722;\n\tbh=h3mes2CfOfyZ6USuvkHkX1HFiQO+Sh1LeL44Q3dKQ6M=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=EN4LypJmfIKF8wbXilniYGL9zQv7PY3zk7K3e7Aj4RZt7+rDuYx1UHIWei/a/Iujf\n\tiPxw3Vj+j+/Ih119+ZlTBUsD2fZZH64PUGme4iALZfEiLqj9QFkjNZtwYk9P5BLqIs\n\t5zBBBCC7Tgt9WDEUy4RvZcVGyPoixmHZN75LyRbU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240913123844.GG21327@pendragon.ideasonboard.com>","References":"<20240913120416.96828-1-robert.mader@collabora.com>\n\t<172623067173.3474483.767935304398814078@ping.linuxembedded.co.uk>\n\t<20240913123844.GG21327@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","From":"Kieran Bingham ","Cc":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ","Date":"Fri, 13 Sep 2024 14:06:37 +0100","Message-ID":"<172623279791.3474483.1263179436181482490@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":31224,"web_url":"https://patchwork.libcamera.org/comment/31224/","msgid":"<20240913143421.GI21327@pendragon.ideasonboard.com>","date":"2024-09-13T14:34:21","subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Sep 13, 2024 at 02:06:37PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2024-09-13 13:38:44)\n> > On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote:\n> > > Quoting Robert Mader (2024-09-13 13:04:16)\n> > > > Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure\n> > > > correct output. Not doing so currently results in occasional tearing\n> > > > and/or backlashes in GL/VK clients that use the buffers directly for\n> > > > rendering.\n> > > > \n> > > > An alternative approach to have the sync code in `MappedFrameBuffer` was\n> > > > considered but rejected for now, in order to allow clients more\n> > > > flexibility.\n> > > > \n> > > > Signed-off-by: Robert Mader \n> > > > \n> > > > ---\n> > > > \n> > > > Changes in v2:\n> > > > - sync input buffer as well\n> > > > - update commit title accordingly\n> > > > - small changes to the commit message\n> > > > - linting fixes, should pass now\n> > > > ---\n> > > > src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++\n> > > > 1 file changed, 35 insertions(+)\n> > > > \n> > > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> > > > index 077f7f4b..a3b4851c 100644\n> > > > --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> > > > +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> > > > @@ -12,8 +12,11 @@\n> > > > #include \"debayer_cpu.h\"\n> > > > \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > \n> > > > +#include \n> > > > +\n> > > > #include \n> > > > \n> > > > #include \"libcamera/internal/bayer_format.h\"\n> > > > @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> > > > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> > > > }\n> > > > \n> > > > + for (const FrameBuffer::Plane &plane : input->planes()) {\n> > > > + const int fd = plane.fd.get();\n> > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ };\n> > > > +\n> > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > > \n> > > We can't use 'errno' directly on our error messages because the LOG()\n> > > implementations or other processing of the log statement (not in this\n> > > case, but in others it could be possible) could affect errno.\n> > > \n> > > So we use the style \n> > > \n> > > src/libcamera/dma_buf_allocator.cpp- if (ret < 0) {\n> > > src/libcamera/dma_buf_allocator.cpp: ret = errno;\n> > > src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error)\n> > > src/libcamera/dma_buf_allocator.cpp- << \"Failed to create dma buf for \" << name\n> > > src/libcamera/dma_buf_allocator.cpp- << \": \" << strerror(ret);\n> > > src/libcamera/dma_buf_allocator.cpp- return {};\n> > > src/libcamera/dma_buf_allocator.cpp- }\n> > > \n> > > Where we store the errno immediately and then use it in the log. It's\n> > > also helpful to use strerror too.\n> > > \n> > > > + }\n> > > > +\n> > > > + for (const FrameBuffer::Plane &plane : output->planes()) {\n> > > > + const int fd = plane.fd.get();\n> > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE };\n> > > > +\n> > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > > > + }\n> > > \n> > > (jumping back up from below) And this likewise could then be:\n> > > \n> > > intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> > > output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> > > \n> > > > +\n> > > > green_ = params.green;\n> > > > red_ = swapRedBlueGains_ ? params.blue : params.red;\n> > > > blue_ = swapRedBlueGains_ ? params.red : params.blue;\n> > > > @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> > > > \n> > > > metadata.planes()[0].bytesused = out.planes()[0].size();\n> > > > \n> > > > + for (const FrameBuffer::Plane &plane : output->planes()) {\n> > > > + const int fd = plane.fd.get();\n> > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE };\n> > > > +\n> > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > > > + }\n> > > > +\n> > > > + for (const FrameBuffer::Plane &plane : input->planes()) {\n> > > > + const int fd = plane.fd.get();\n> > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ };\n> > > > +\n> > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > > \n> > > That's four iterations of very similar code. I think we could add a\n> > > helper directly into FrameBuffer directly as a preceeding patch, that\n> > > would allow something more like:\n> > > \n> > > output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> > > input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> > > \n> > > We could probably do that using our Flags class too ... but the above\n> > > would be a good start.\n> > \n> > I'd like a helper too, but I don't think it should be part of the public\n> > API, so the FrameBuffer class isn't the right place.\n> \n> How about in FrameBuffer::Private::sync() then ?\n\nI was thinking about the same, I think\n\n\tinput->_d()->sync()\n\nwould work.\n\n> Or worst case - and to progress here, at the very least - just a private\n> DebayerCpu::syncBuffer() for\n> \n> \tsyncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> \n> which would at least bump any documentation requirements until there are\n> more users.\n> \n> > > > + }\n> > > > +\n> > > > /* Measure before emitting signals */\n> > > > if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> > > > ++measuredFrames_ > DebayerCpu::kFramesToSkip) {","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 EC6C9C3257\n\tfor ;\n\tFri, 13 Sep 2024 14:34:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA2AA634F8;\n\tFri, 13 Sep 2024 16:34:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D15E634E3\n\tfor ;\n\tFri, 13 Sep 2024 16:34:56 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(213-229-8-243.static.upcbusiness.at [213.229.8.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EFBDC9FF;\n\tFri, 13 Sep 2024 16:33:36 +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=\"futcxaE/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726238017;\n\tbh=ZdeoX1QqybQeIi0p+9msaiSybcI4a2DlVIpXxXuds2c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=futcxaE/p2XAs21ZvOWeexzHEanGg6np6puNktM4OwK39y1ScYr0YhkgnnPtg8sxM\n\twl5obF6Z9msEFAFwaSvgzQwk984NHUpsQBbyF27KwW/uikR3SChc19p06E4z6Ee5DX\n\t0gnKsxu6ORO87f5ZE/Axc5E39axUT+sU3lkimjp8=","Date":"Fri, 13 Sep 2024 17:34:21 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","Message-ID":"<20240913143421.GI21327@pendragon.ideasonboard.com>","References":"<20240913120416.96828-1-robert.mader@collabora.com>\n\t<172623067173.3474483.767935304398814078@ping.linuxembedded.co.uk>\n\t<20240913123844.GG21327@pendragon.ideasonboard.com>\n\t<172623279791.3474483.1263179436181482490@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172623279791.3474483.1263179436181482490@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\" "}},{"id":31225,"web_url":"https://patchwork.libcamera.org/comment/31225/","msgid":"<481b1b5296f9bc49e1de5d2d22f6d066bfd14ed0.camel@ndufresne.ca>","date":"2024-09-13T14:50:05","subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Hi,\n\nLe vendredi 13 septembre 2024 à 17:34 +0300, Laurent Pinchart a écrit :\n> On Fri, Sep 13, 2024 at 02:06:37PM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2024-09-13 13:38:44)\n> > > On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote:\n> > > > Quoting Robert Mader (2024-09-13 13:04:16)\n> > > > > Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure\n> > > > > correct output. Not doing so currently results in occasional tearing\n> > > > > and/or backlashes in GL/VK clients that use the buffers directly for\n> > > > > rendering.\n> > > > > \n> > > > > An alternative approach to have the sync code in `MappedFrameBuffer` was\n> > > > > considered but rejected for now, in order to allow clients more\n> > > > > flexibility.\n> > > > > \n> > > > > Signed-off-by: Robert Mader \n> > > > > \n> > > > > ---\n> > > > > \n> > > > > Changes in v2:\n> > > > > - sync input buffer as well\n> > > > > - update commit title accordingly\n> > > > > - small changes to the commit message\n> > > > > - linting fixes, should pass now\n> > > > > ---\n> > > > > src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++\n> > > > > 1 file changed, 35 insertions(+)\n> > > > > \n> > > > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> > > > > index 077f7f4b..a3b4851c 100644\n> > > > > --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> > > > > +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> > > > > @@ -12,8 +12,11 @@\n> > > > > #include \"debayer_cpu.h\"\n> > > > > \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > \n> > > > > +#include \n> > > > > +\n> > > > > #include \n> > > > > \n> > > > > #include \"libcamera/internal/bayer_format.h\"\n> > > > > @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> > > > > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> > > > > }\n> > > > > \n> > > > > + for (const FrameBuffer::Plane &plane : input->planes()) {\n> > > > > + const int fd = plane.fd.get();\n> > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ };\n> > > > > +\n> > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > > > \n> > > > We can't use 'errno' directly on our error messages because the LOG()\n> > > > implementations or other processing of the log statement (not in this\n> > > > case, but in others it could be possible) could affect errno.\n> > > > \n> > > > So we use the style \n> > > > \n> > > > src/libcamera/dma_buf_allocator.cpp- if (ret < 0) {\n> > > > src/libcamera/dma_buf_allocator.cpp: ret = errno;\n> > > > src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error)\n> > > > src/libcamera/dma_buf_allocator.cpp- << \"Failed to create dma buf for \" << name\n> > > > src/libcamera/dma_buf_allocator.cpp- << \": \" << strerror(ret);\n> > > > src/libcamera/dma_buf_allocator.cpp- return {};\n> > > > src/libcamera/dma_buf_allocator.cpp- }\n> > > > \n> > > > Where we store the errno immediately and then use it in the log. It's\n> > > > also helpful to use strerror too.\n> > > > \n> > > > > + }\n> > > > > +\n> > > > > + for (const FrameBuffer::Plane &plane : output->planes()) {\n> > > > > + const int fd = plane.fd.get();\n> > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE };\n> > > > > +\n> > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > > > > + }\n> > > > \n> > > > (jumping back up from below) And this likewise could then be:\n> > > > \n> > > > intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> > > > output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> > > > \n> > > > > +\n> > > > > green_ = params.green;\n> > > > > red_ = swapRedBlueGains_ ? params.blue : params.red;\n> > > > > blue_ = swapRedBlueGains_ ? params.red : params.blue;\n> > > > > @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> > > > > \n> > > > > metadata.planes()[0].bytesused = out.planes()[0].size();\n> > > > > \n> > > > > + for (const FrameBuffer::Plane &plane : output->planes()) {\n> > > > > + const int fd = plane.fd.get();\n> > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE };\n> > > > > +\n> > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > > > > + }\n> > > > > +\n> > > > > + for (const FrameBuffer::Plane &plane : input->planes()) {\n> > > > > + const int fd = plane.fd.get();\n> > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ };\n> > > > > +\n> > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> > > > > + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> > > > \n> > > > That's four iterations of very similar code. I think we could add a\n> > > > helper directly into FrameBuffer directly as a preceeding patch, that\n> > > > would allow something more like:\n> > > > \n> > > > output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> > > > input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> > > > \n> > > > We could probably do that using our Flags class too ... but the above\n> > > > would be a good start.\n> > > \n> > > I'd like a helper too, but I don't think it should be part of the public\n> > > API, so the FrameBuffer class isn't the right place.\n> > \n> > How about in FrameBuffer::Private::sync() then ?\n> \n> I was thinking about the same, I think\n> \n> \tinput->_d()->sync()\n> \n> would work.\n\nFor our future sanity, I may suggest to call this sync_for_cpu() or something\nlike this. Just being a little more precise make its clear what this is doing.\n\n> \n> > Or worst case - and to progress here, at the very least - just a private\n> > DebayerCpu::syncBuffer() for\n> > \n> > \tsyncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> > \n> > which would at least bump any documentation requirements until there are\n> > more users.\n> > \n> > > > > + }\n> > > > > +\n> > > > > /* Measure before emitting signals */\n> > > > > if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> > > > > ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\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 E45D1C324C\n\tfor ;\n\tFri, 13 Sep 2024 14:50:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BEB30634F8;\n\tFri, 13 Sep 2024 16:50:10 +0200 (CEST)","from mail-qv1-xf34.google.com (mail-qv1-xf34.google.com\n\t[IPv6:2607:f8b0:4864:20::f34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F163634E3\n\tfor ;\n\tFri, 13 Sep 2024 16:50:08 +0200 (CEST)","by mail-qv1-xf34.google.com with SMTP id\n\t6a1803df08f44-6bf7ad1ec3aso15955426d6.0\n\tfor ;\n\tFri, 13 Sep 2024 07:50:08 -0700 (PDT)","from nicolas-tpx395.lan ([2606:6d00:17:9cac::580])\n\tby smtp.gmail.com with ESMTPSA id\n\t6a1803df08f44-6c534787c01sm66772836d6.144.2024.09.13.07.50.06\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 13 Sep 2024 07:50:06 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20230601.gappssmtp.com\n\theader.i=@ndufresne-ca.20230601.gappssmtp.com\n\theader.b=\"InkXh+1Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20230601.gappssmtp.com; s=20230601; t=1726239007;\n\tx=1726843807; darn=lists.libcamera.org; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=IIdk3F3lsAZkoYVNoq5pgcBY6qHhFQ1Hnnf4arS6xOo=;\n\tb=InkXh+1YbR243/KoYUPxHrLTri94azyEKDdNAWL1yQt0as+isFkMbcEH5lzVqo07FZ\n\t4AZHYjRF2XpcRQwp8wl0k2zMPfYl80GZ78GT5dL9GHKWl6qditG1Zj3aJld9tVAw3QEy\n\tDvuOQPcDrmALNJpEk+Qf0rTDRaOi04um3P4ztpGkoLizvN28+bt4TtmQAIQEmHWiSMbe\n\tyfa1q7x11/TY3HtqYYVfG2kIiS8VRo9hUcuVAQw1NoSyb0uFi8oI+VZHUWNs37CFZUzI\n\tK5VKZTcKqb7tHSUfjx4m3XlVQ2z2GDCyazL/8771oPsdJxs02S8EOUBzPksQlw8Eluzc\n\ta6yA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726239007; x=1726843807;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=IIdk3F3lsAZkoYVNoq5pgcBY6qHhFQ1Hnnf4arS6xOo=;\n\tb=FZtcbGdUl26crPJqdPl7YXOZZ6k69tW+Y6orn942VTS3ejwC/BFvBfNTXK7j4PjASp\n\tAmDxw7mjxboTimULwEm74ShVuaoJ/LkmUC/ekv1jondU1yovkM+JjG+/Z93QOxvnVpQP\n\tF9Zol0sL+PTAEBeCLEbHaHt11apApxoazumJS/KMNpRv8omOTxnivfBimYrR6WxgARZ1\n\tZx3s9EI/0TChPakNJh+GrLgX7+EIbICAA+kMz4uX8ajc6+0aXJ/J7KrlujKpkW08awJq\n\tJXqWPOgplXzNZADhnbOWaDA2UllOazzADU0PwsYJ2ybncC7TSRS+gB+doP1qQF1soKHb\n\tVlog==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCV16MQW+fATH+8/89cmaCB0RvEemqJcchYFasYvD7N05ZuTNJOdKl/ASItZKbddopMOeTJUQGjgY6QB/L2jwso=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YyZHOqZMCbp2IzvrMjHbr7CpNQzdstdQIwuvsyTRPWr+T6FcAXC\n\trCJ72OXW3aFkbO04e7c9yscGAs0RLpuNch/HFDDmQmX6HFpETOmynef7Vkp/i28=","X-Google-Smtp-Source":"AGHT+IHp6w/S3gsbdh5F0TqwsIbnRtC0JrYogflKlD6ayomQtRCSEdhBgSlNLJTwLRdAOo2ZK1PSoQ==","X-Received":"by 2002:a05:6214:3d0d:b0:6c1:6f9b:619a with SMTP id\n\t6a1803df08f44-6c57352ef9emr114774386d6.30.1726239007130; \n\tFri, 13 Sep 2024 07:50:07 -0700 (PDT)","Message-ID":"<481b1b5296f9bc49e1de5d2d22f6d066bfd14ed0.camel@ndufresne.ca>","Subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","From":"Nicolas Dufresne ","To":"Laurent Pinchart , Kieran Bingham\n\t","Cc":"Robert Mader , \n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 13 Sep 2024 10:50:05 -0400","In-Reply-To":"<20240913143421.GI21327@pendragon.ideasonboard.com>","References":"<20240913120416.96828-1-robert.mader@collabora.com>\n\t<172623067173.3474483.767935304398814078@ping.linuxembedded.co.uk>\n\t<20240913123844.GG21327@pendragon.ideasonboard.com>\n\t<172623279791.3474483.1263179436181482490@ping.linuxembedded.co.uk>\n\t<20240913143421.GI21327@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.52.4 (3.52.4-1.fc40) ","MIME-Version":"1.0","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":31240,"web_url":"https://patchwork.libcamera.org/comment/31240/","msgid":"","date":"2024-09-16T10:45:30","subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"Any resistance against a local\n\nsyncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n\nthen?\n\nOn 13.09.24 16:50, Nicolas Dufresne wrote:\n> Hi,\n>\n> Le vendredi 13 septembre 2024 à 17:34 +0300, Laurent Pinchart a écrit :\n>> On Fri, Sep 13, 2024 at 02:06:37PM +0100, Kieran Bingham wrote:\n>>> Quoting Laurent Pinchart (2024-09-13 13:38:44)\n>>>> On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote:\n>>>>> Quoting Robert Mader (2024-09-13 13:04:16)\n>>>>>> Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure\n>>>>>> correct output. Not doing so currently results in occasional tearing\n>>>>>> and/or backlashes in GL/VK clients that use the buffers directly for\n>>>>>> rendering.\n>>>>>>\n>>>>>> An alternative approach to have the sync code in `MappedFrameBuffer` was\n>>>>>> considered but rejected for now, in order to allow clients more\n>>>>>> flexibility.\n>>>>>>\n>>>>>> Signed-off-by: Robert Mader \n>>>>>>\n>>>>>> ---\n>>>>>>\n>>>>>> Changes in v2:\n>>>>>> - sync input buffer as well\n>>>>>> - update commit title accordingly\n>>>>>> - small changes to the commit message\n>>>>>> - linting fixes, should pass now\n>>>>>> ---\n>>>>>> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++\n>>>>>> 1 file changed, 35 insertions(+)\n>>>>>>\n>>>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>>>>>> index 077f7f4b..a3b4851c 100644\n>>>>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>>>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>>>>>> @@ -12,8 +12,11 @@\n>>>>>> #include \"debayer_cpu.h\"\n>>>>>> \n>>>>>> #include \n>>>>>> +#include \n>>>>>> #include \n>>>>>> \n>>>>>> +#include \n>>>>>> +\n>>>>>> #include \n>>>>>> \n>>>>>> #include \"libcamera/internal/bayer_format.h\"\n>>>>>> @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n>>>>>> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n>>>>>> }\n>>>>>> \n>>>>>> + for (const FrameBuffer::Plane &plane : input->planes()) {\n>>>>>> + const int fd = plane.fd.get();\n>>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ };\n>>>>>> +\n>>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n>>>>>> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n>>>>> We can't use 'errno' directly on our error messages because the LOG()\n>>>>> implementations or other processing of the log statement (not in this\n>>>>> case, but in others it could be possible) could affect errno.\n>>>>>\n>>>>> So we use the style\n>>>>>\n>>>>> src/libcamera/dma_buf_allocator.cpp- if (ret < 0) {\n>>>>> src/libcamera/dma_buf_allocator.cpp: ret = errno;\n>>>>> src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error)\n>>>>> src/libcamera/dma_buf_allocator.cpp- << \"Failed to create dma buf for \" << name\n>>>>> src/libcamera/dma_buf_allocator.cpp- << \": \" << strerror(ret);\n>>>>> src/libcamera/dma_buf_allocator.cpp- return {};\n>>>>> src/libcamera/dma_buf_allocator.cpp- }\n>>>>>\n>>>>> Where we store the errno immediately and then use it in the log. It's\n>>>>> also helpful to use strerror too.\n>>>>>\n>>>>>> + }\n>>>>>> +\n>>>>>> + for (const FrameBuffer::Plane &plane : output->planes()) {\n>>>>>> + const int fd = plane.fd.get();\n>>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE };\n>>>>>> +\n>>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n>>>>>> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n>>>>>> + }\n>>>>> (jumping back up from below) And this likewise could then be:\n>>>>>\n>>>>> intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n>>>>> output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n>>>>>\n>>>>>> +\n>>>>>> green_ = params.green;\n>>>>>> red_ = swapRedBlueGains_ ? params.blue : params.red;\n>>>>>> blue_ = swapRedBlueGains_ ? params.red : params.blue;\n>>>>>> @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n>>>>>> \n>>>>>> metadata.planes()[0].bytesused = out.planes()[0].size();\n>>>>>> \n>>>>>> + for (const FrameBuffer::Plane &plane : output->planes()) {\n>>>>>> + const int fd = plane.fd.get();\n>>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE };\n>>>>>> +\n>>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n>>>>>> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n>>>>>> + }\n>>>>>> +\n>>>>>> + for (const FrameBuffer::Plane &plane : input->planes()) {\n>>>>>> + const int fd = plane.fd.get();\n>>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ };\n>>>>>> +\n>>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n>>>>>> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n>>>>> That's four iterations of very similar code. I think we could add a\n>>>>> helper directly into FrameBuffer directly as a preceeding patch, that\n>>>>> would allow something more like:\n>>>>>\n>>>>> output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n>>>>> input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n>>>>>\n>>>>> We could probably do that using our Flags class too ... but the above\n>>>>> would be a good start.\n>>>> I'd like a helper too, but I don't think it should be part of the public\n>>>> API, so the FrameBuffer class isn't the right place.\n>>> How about in FrameBuffer::Private::sync() then ?\n>> I was thinking about the same, I think\n>>\n>> \tinput->_d()->sync()\n>>\n>> would work.\n> For our future sanity, I may suggest to call this sync_for_cpu() or something\n> like this. Just being a little more precise make its clear what this is doing.\n>\n>>> Or worst case - and to progress here, at the very least - just a private\n>>> DebayerCpu::syncBuffer() for\n>>>\n>>> \tsyncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n>>>\n>>> which would at least bump any documentation requirements until there are\n>>> more users.\n>>>\n>>>>>> + }\n>>>>>> +\n>>>>>> /* Measure before emitting signals */\n>>>>>> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>>>>>> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {","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 246C3C324C\n\tfor ;\n\tMon, 16 Sep 2024 10:45:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9265E634F8;\n\tMon, 16 Sep 2024 12:45:40 +0200 (CEST)","from sender4-pp-f112.zoho.com (sender4-pp-f112.zoho.com\n\t[136.143.188.112])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EC2AB634F5\n\tfor ;\n\tMon, 16 Sep 2024 12:45:37 +0200 (CEST)","by mx.zohomail.com with SMTPS id 1726483532684862.7072927095936;\n\tMon, 16 Sep 2024 03:45:32 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"coz1LXQh\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1726483534; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=bBZR5ZWi+qhY6+/im6GNvKMAjMQoc5TNVFDq6KqSnen0muVAH0TeTxp3qaW3DwCxYQetDctKDgYVKEvf+dYKPXXTCwXtk13bXRYVL4kr6LS0uMK8eDBAjZh94pWTomdvNmi0gBPVF4sCJuxVi/uVNyB3VaI7LCVOxAwOFePyg/Q=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1726483534;\n\th=Content-Type:Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To;\n\tbh=KAkrUqHEj55QDOqA/Ogeznjh0ZDKfgD0+Hswnz4wKvs=; \n\tb=Lt9WqgU6NKDkSep7gHqwyGZW1rYCBLkdaOSnF4AvUH3RsMhr3+rfFd936EBnC/fnmtPGzyDLU8lde/Q/5kSAChqzz2MEyFnZ2cvP/kkcmD6eq5IMJ+NcjwGnzGolxAPBeCYeON36j0pODXje3kzf0Bk+3qGwMPnwg++sw787v84=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1726483534;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:Cc:Cc:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To;\n\tbh=KAkrUqHEj55QDOqA/Ogeznjh0ZDKfgD0+Hswnz4wKvs=;\n\tb=coz1LXQhggQMLoWPcWFJcowKuBvxZl7m6dY2Loi0047Qx7lie5QzAunqkMmyFq6p\n\tiC7wSW6tq3TzQLjo7LJY1cbtxbt5pHShlZodPX5fRL3C2IKEQFsn0IIuh+xZG767Pwa\n\tqs5X0Q+HGoVG3zzXecdD1YxRAcg1ZBUBVfJWrbcA=","Message-ID":"","Date":"Mon, 16 Sep 2024 12:45:30 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","To":"Nicolas Dufresne ,\n\tLaurent Pinchart ,\n\tKieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240913120416.96828-1-robert.mader@collabora.com>\n\t<172623067173.3474483.767935304398814078@ping.linuxembedded.co.uk>\n\t<20240913123844.GG21327@pendragon.ideasonboard.com>\n\t<172623279791.3474483.1263179436181482490@ping.linuxembedded.co.uk>\n\t<20240913143421.GI21327@pendragon.ideasonboard.com>\n\t<481b1b5296f9bc49e1de5d2d22f6d066bfd14ed0.camel@ndufresne.ca>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<481b1b5296f9bc49e1de5d2d22f6d066bfd14ed0.camel@ndufresne.ca>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":31241,"web_url":"https://patchwork.libcamera.org/comment/31241/","msgid":"<172649048444.3421941.3267107478500707387@ping.linuxembedded.co.uk>","date":"2024-09-16T12:41:24","subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Robert Mader (2024-09-16 11:45:30)\n> Any resistance against a local\n> \n> syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> \n> then?\n\nIf this is the first/only place to add these so far then I think that's\nfine, as long as the duplication below is wrapped into a simple helper.\n\nIt can be local, and then if/when we determine a second place to make\nuse of it we can move to the FrameBuffer::Private::syncForCpu() and\nhandle documentation and flags conversions that might be more desireable\nif this is used more widely.\n\n--\nKieran\n\n\n> \n> On 13.09.24 16:50, Nicolas Dufresne wrote:\n> > Hi,\n> >\n> > Le vendredi 13 septembre 2024 à 17:34 +0300, Laurent Pinchart a écrit :\n> >> On Fri, Sep 13, 2024 at 02:06:37PM +0100, Kieran Bingham wrote:\n> >>> Quoting Laurent Pinchart (2024-09-13 13:38:44)\n> >>>> On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote:\n> >>>>> Quoting Robert Mader (2024-09-13 13:04:16)\n> >>>>>> Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure\n> >>>>>> correct output. Not doing so currently results in occasional tearing\n> >>>>>> and/or backlashes in GL/VK clients that use the buffers directly for\n> >>>>>> rendering.\n> >>>>>>\n> >>>>>> An alternative approach to have the sync code in `MappedFrameBuffer` was\n> >>>>>> considered but rejected for now, in order to allow clients more\n> >>>>>> flexibility.\n> >>>>>>\n> >>>>>> Signed-off-by: Robert Mader \n> >>>>>>\n> >>>>>> ---\n> >>>>>>\n> >>>>>> Changes in v2:\n> >>>>>> - sync input buffer as well\n> >>>>>> - update commit title accordingly\n> >>>>>> - small changes to the commit message\n> >>>>>> - linting fixes, should pass now\n> >>>>>> ---\n> >>>>>> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++\n> >>>>>> 1 file changed, 35 insertions(+)\n> >>>>>>\n> >>>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> >>>>>> index 077f7f4b..a3b4851c 100644\n> >>>>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> >>>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> >>>>>> @@ -12,8 +12,11 @@\n> >>>>>> #include \"debayer_cpu.h\"\n> >>>>>> \n> >>>>>> #include \n> >>>>>> +#include \n> >>>>>> #include \n> >>>>>> \n> >>>>>> +#include \n> >>>>>> +\n> >>>>>> #include \n> >>>>>> \n> >>>>>> #include \"libcamera/internal/bayer_format.h\"\n> >>>>>> @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> >>>>>> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> >>>>>> }\n> >>>>>> \n> >>>>>> + for (const FrameBuffer::Plane &plane : input->planes()) {\n> >>>>>> + const int fd = plane.fd.get();\n> >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ };\n> >>>>>> +\n> >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> >>>>>> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> >>>>> We can't use 'errno' directly on our error messages because the LOG()\n> >>>>> implementations or other processing of the log statement (not in this\n> >>>>> case, but in others it could be possible) could affect errno.\n> >>>>>\n> >>>>> So we use the style\n> >>>>>\n> >>>>> src/libcamera/dma_buf_allocator.cpp- if (ret < 0) {\n> >>>>> src/libcamera/dma_buf_allocator.cpp: ret = errno;\n> >>>>> src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error)\n> >>>>> src/libcamera/dma_buf_allocator.cpp- << \"Failed to create dma buf for \" << name\n> >>>>> src/libcamera/dma_buf_allocator.cpp- << \": \" << strerror(ret);\n> >>>>> src/libcamera/dma_buf_allocator.cpp- return {};\n> >>>>> src/libcamera/dma_buf_allocator.cpp- }\n> >>>>>\n> >>>>> Where we store the errno immediately and then use it in the log. It's\n> >>>>> also helpful to use strerror too.\n> >>>>>\n> >>>>>> + }\n> >>>>>> +\n> >>>>>> + for (const FrameBuffer::Plane &plane : output->planes()) {\n> >>>>>> + const int fd = plane.fd.get();\n> >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE };\n> >>>>>> +\n> >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> >>>>>> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> >>>>>> + }\n> >>>>> (jumping back up from below) And this likewise could then be:\n> >>>>>\n> >>>>> intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> >>>>> output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> >>>>>\n> >>>>>> +\n> >>>>>> green_ = params.green;\n> >>>>>> red_ = swapRedBlueGains_ ? params.blue : params.red;\n> >>>>>> blue_ = swapRedBlueGains_ ? params.red : params.blue;\n> >>>>>> @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> >>>>>> \n> >>>>>> metadata.planes()[0].bytesused = out.planes()[0].size();\n> >>>>>> \n> >>>>>> + for (const FrameBuffer::Plane &plane : output->planes()) {\n> >>>>>> + const int fd = plane.fd.get();\n> >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE };\n> >>>>>> +\n> >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> >>>>>> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> >>>>>> + }\n> >>>>>> +\n> >>>>>> + for (const FrameBuffer::Plane &plane : input->planes()) {\n> >>>>>> + const int fd = plane.fd.get();\n> >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ };\n> >>>>>> +\n> >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> >>>>>> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> >>>>> That's four iterations of very similar code. I think we could add a\n> >>>>> helper directly into FrameBuffer directly as a preceeding patch, that\n> >>>>> would allow something more like:\n> >>>>>\n> >>>>> output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> >>>>> input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> >>>>>\n> >>>>> We could probably do that using our Flags class too ... but the above\n> >>>>> would be a good start.\n> >>>> I'd like a helper too, but I don't think it should be part of the public\n> >>>> API, so the FrameBuffer class isn't the right place.\n> >>> How about in FrameBuffer::Private::sync() then ?\n> >> I was thinking about the same, I think\n> >>\n> >> input->_d()->sync()\n> >>\n> >> would work.\n> > For our future sanity, I may suggest to call this sync_for_cpu() or something\n> > like this. Just being a little more precise make its clear what this is doing.\n> >\n> >>> Or worst case - and to progress here, at the very least - just a private\n> >>> DebayerCpu::syncBuffer() for\n> >>>\n> >>> syncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> >>>\n> >>> which would at least bump any documentation requirements until there are\n> >>> more users.\n> >>>\n> >>>>>> + }\n> >>>>>> +\n> >>>>>> /* Measure before emitting signals */\n> >>>>>> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> >>>>>> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> \n> -- \n> Robert Mader\n> Consultant Software Developer\n> \n> Collabora Ltd.\n> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK\n> Registered in England & Wales, no. 5513718\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 A3037C3257\n\tfor ;\n\tMon, 16 Sep 2024 12:41:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3381C634F8;\n\tMon, 16 Sep 2024 14:41: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 1659E634F5\n\tfor ;\n\tMon, 16 Sep 2024 14:41:28 +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 93116496;\n\tMon, 16 Sep 2024 14:40:06 +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=\"SIgNutPX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726490406;\n\tbh=8meuHcSP6bNyFAc2omnkKtcsbpWp03JYMMII/k0Brf0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=SIgNutPXdhuV4AIqu1YK1ZAnHiTK6OXV6i9a8ej0QPUnyk4SdjZ9hOTwwyElS8NWh\n\tou/p8BBTKfIFLhh1twUmCPpW8UNvbhzDFEkQwBpL3aULOK2Agwc8H5M9Rl1UD6xCCa\n\tSAcGR3hG4N47j2yL34y38S83g9PzQGR/6NReRVaU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240913120416.96828-1-robert.mader@collabora.com>\n\t<172623067173.3474483.767935304398814078@ping.linuxembedded.co.uk>\n\t<20240913123844.GG21327@pendragon.ideasonboard.com>\n\t<172623279791.3474483.1263179436181482490@ping.linuxembedded.co.uk>\n\t<20240913143421.GI21327@pendragon.ideasonboard.com>\n\t<481b1b5296f9bc49e1de5d2d22f6d066bfd14ed0.camel@ndufresne.ca>\n\t","Subject":"Re: [PATCH v2] libcamera: debayer_cpu: Sync DMABUFs","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ,\n\tNicolas Dufresne ,\n\tRobert Mader ","Date":"Mon, 16 Sep 2024 13:41:24 +0100","Message-ID":"<172649048444.3421941.3267107478500707387@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\" "}}]