| Message ID | 20240913120416.96828-1-robert.mader@collabora.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Robert, Quoting Robert Mader (2024-09-13 13:04:16) > Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure > correct output. Not doing so currently results in occasional tearing > and/or backlashes in GL/VK clients that use the buffers directly for > rendering. > > An alternative approach to have the sync code in `MappedFrameBuffer` was > considered but rejected for now, in order to allow clients more > flexibility. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > --- > > Changes in v2: > - sync input buffer as well > - update commit title accordingly > - small changes to the commit message > - linting fixes, should pass now > --- > src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index 077f7f4b..a3b4851c 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -12,8 +12,11 @@ > #include "debayer_cpu.h" > > #include <stdlib.h> > +#include <sys/ioctl.h> > #include <time.h> > > +#include <linux/dma-buf.h> > + > #include <libcamera/formats.h> > > #include "libcamera/internal/bayer_format.h" > @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > } > > + for (const FrameBuffer::Plane &plane : input->planes()) { > + const int fd = plane.fd.get(); > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ }; > + > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; We can't use 'errno' directly on our error messages because the LOG() implementations or other processing of the log statement (not in this case, but in others it could be possible) could affect errno. So we use the style src/libcamera/dma_buf_allocator.cpp- if (ret < 0) { src/libcamera/dma_buf_allocator.cpp: ret = errno; src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error) src/libcamera/dma_buf_allocator.cpp- << "Failed to create dma buf for " << name src/libcamera/dma_buf_allocator.cpp- << ": " << strerror(ret); src/libcamera/dma_buf_allocator.cpp- return {}; src/libcamera/dma_buf_allocator.cpp- } Where we store the errno immediately and then use it in the log. It's also helpful to use strerror too. > + } > + > + for (const FrameBuffer::Plane &plane : output->planes()) { > + const int fd = plane.fd.get(); > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE }; > + > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > + } (jumping back up from below) And this likewise could then be: intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); > + > green_ = params.green; > red_ = swapRedBlueGains_ ? params.blue : params.red; > blue_ = swapRedBlueGains_ ? params.red : params.blue; > @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > > metadata.planes()[0].bytesused = out.planes()[0].size(); > > + for (const FrameBuffer::Plane &plane : output->planes()) { > + const int fd = plane.fd.get(); > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE }; > + > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > + } > + > + for (const FrameBuffer::Plane &plane : input->planes()) { > + const int fd = plane.fd.get(); > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ }; > + > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; That's four iterations of very similar code. I think we could add a helper directly into FrameBuffer directly as a preceeding patch, that would allow something more like: output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); We could probably do that using our Flags class too ... but the above would be a good start. > + } > + > /* Measure before emitting signals */ > if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && > ++measuredFrames_ > DebayerCpu::kFramesToSkip) { > -- > 2.46.0 >
On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote: > Quoting Robert Mader (2024-09-13 13:04:16) > > Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure > > correct output. Not doing so currently results in occasional tearing > > and/or backlashes in GL/VK clients that use the buffers directly for > > rendering. > > > > An alternative approach to have the sync code in `MappedFrameBuffer` was > > considered but rejected for now, in order to allow clients more > > flexibility. > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > > > --- > > > > Changes in v2: > > - sync input buffer as well > > - update commit title accordingly > > - small changes to the commit message > > - linting fixes, should pass now > > --- > > src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > > index 077f7f4b..a3b4851c 100644 > > --- a/src/libcamera/software_isp/debayer_cpu.cpp > > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > > @@ -12,8 +12,11 @@ > > #include "debayer_cpu.h" > > > > #include <stdlib.h> > > +#include <sys/ioctl.h> > > #include <time.h> > > > > +#include <linux/dma-buf.h> > > + > > #include <libcamera/formats.h> > > > > #include "libcamera/internal/bayer_format.h" > > @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > > } > > > > + for (const FrameBuffer::Plane &plane : input->planes()) { > > + const int fd = plane.fd.get(); > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ }; > > + > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > We can't use 'errno' directly on our error messages because the LOG() > implementations or other processing of the log statement (not in this > case, but in others it could be possible) could affect errno. > > So we use the style > > src/libcamera/dma_buf_allocator.cpp- if (ret < 0) { > src/libcamera/dma_buf_allocator.cpp: ret = errno; > src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error) > src/libcamera/dma_buf_allocator.cpp- << "Failed to create dma buf for " << name > src/libcamera/dma_buf_allocator.cpp- << ": " << strerror(ret); > src/libcamera/dma_buf_allocator.cpp- return {}; > src/libcamera/dma_buf_allocator.cpp- } > > Where we store the errno immediately and then use it in the log. It's > also helpful to use strerror too. > > > + } > > + > > + for (const FrameBuffer::Plane &plane : output->planes()) { > > + const int fd = plane.fd.get(); > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE }; > > + > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > + } > > (jumping back up from below) And this likewise could then be: > > intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); > output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); > > > + > > green_ = params.green; > > red_ = swapRedBlueGains_ ? params.blue : params.red; > > blue_ = swapRedBlueGains_ ? params.red : params.blue; > > @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > > > > metadata.planes()[0].bytesused = out.planes()[0].size(); > > > > + for (const FrameBuffer::Plane &plane : output->planes()) { > > + const int fd = plane.fd.get(); > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE }; > > + > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > + } > > + > > + for (const FrameBuffer::Plane &plane : input->planes()) { > > + const int fd = plane.fd.get(); > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ }; > > + > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > That's four iterations of very similar code. I think we could add a > helper directly into FrameBuffer directly as a preceeding patch, that > would allow something more like: > > output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); > input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); > > We could probably do that using our Flags class too ... but the above > would be a good start. I'd like a helper too, but I don't think it should be part of the public API, so the FrameBuffer class isn't the right place. > > + } > > + > > /* Measure before emitting signals */ > > if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && > > ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
Quoting Laurent Pinchart (2024-09-13 13:38:44) > On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote: > > Quoting Robert Mader (2024-09-13 13:04:16) > > > Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure > > > correct output. Not doing so currently results in occasional tearing > > > and/or backlashes in GL/VK clients that use the buffers directly for > > > rendering. > > > > > > An alternative approach to have the sync code in `MappedFrameBuffer` was > > > considered but rejected for now, in order to allow clients more > > > flexibility. > > > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > > > > > --- > > > > > > Changes in v2: > > > - sync input buffer as well > > > - update commit title accordingly > > > - small changes to the commit message > > > - linting fixes, should pass now > > > --- > > > src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++ > > > 1 file changed, 35 insertions(+) > > > > > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > > > index 077f7f4b..a3b4851c 100644 > > > --- a/src/libcamera/software_isp/debayer_cpu.cpp > > > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > > > @@ -12,8 +12,11 @@ > > > #include "debayer_cpu.h" > > > > > > #include <stdlib.h> > > > +#include <sys/ioctl.h> > > > #include <time.h> > > > > > > +#include <linux/dma-buf.h> > > > + > > > #include <libcamera/formats.h> > > > > > > #include "libcamera/internal/bayer_format.h" > > > @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > > > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > > > } > > > > > > + for (const FrameBuffer::Plane &plane : input->planes()) { > > > + const int fd = plane.fd.get(); > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ }; > > > + > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > > We can't use 'errno' directly on our error messages because the LOG() > > implementations or other processing of the log statement (not in this > > case, but in others it could be possible) could affect errno. > > > > So we use the style > > > > src/libcamera/dma_buf_allocator.cpp- if (ret < 0) { > > src/libcamera/dma_buf_allocator.cpp: ret = errno; > > src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error) > > src/libcamera/dma_buf_allocator.cpp- << "Failed to create dma buf for " << name > > src/libcamera/dma_buf_allocator.cpp- << ": " << strerror(ret); > > src/libcamera/dma_buf_allocator.cpp- return {}; > > src/libcamera/dma_buf_allocator.cpp- } > > > > Where we store the errno immediately and then use it in the log. It's > > also helpful to use strerror too. > > > > > + } > > > + > > > + for (const FrameBuffer::Plane &plane : output->planes()) { > > > + const int fd = plane.fd.get(); > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE }; > > > + > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > + } > > > > (jumping back up from below) And this likewise could then be: > > > > intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); > > output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); > > > > > + > > > green_ = params.green; > > > red_ = swapRedBlueGains_ ? params.blue : params.red; > > > blue_ = swapRedBlueGains_ ? params.red : params.blue; > > > @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > > > > > > metadata.planes()[0].bytesused = out.planes()[0].size(); > > > > > > + for (const FrameBuffer::Plane &plane : output->planes()) { > > > + const int fd = plane.fd.get(); > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE }; > > > + > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > + } > > > + > > > + for (const FrameBuffer::Plane &plane : input->planes()) { > > > + const int fd = plane.fd.get(); > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ }; > > > + > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > > That's four iterations of very similar code. I think we could add a > > helper directly into FrameBuffer directly as a preceeding patch, that > > would allow something more like: > > > > output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); > > input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); > > > > We could probably do that using our Flags class too ... but the above > > would be a good start. > > I'd like a helper too, but I don't think it should be part of the public > API, so the FrameBuffer class isn't the right place. How about in FrameBuffer::Private::sync() then ? Or worst case - and to progress here, at the very least - just a private DebayerCpu::syncBuffer() for syncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); which would at least bump any documentation requirements until there are more users. -- Kieran > > > > + } > > > + > > > /* Measure before emitting signals */ > > > if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && > > > ++measuredFrames_ > DebayerCpu::kFramesToSkip) { > > -- > Regards, > > Laurent Pinchart
On Fri, Sep 13, 2024 at 02:06:37PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2024-09-13 13:38:44) > > On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote: > > > Quoting Robert Mader (2024-09-13 13:04:16) > > > > Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure > > > > correct output. Not doing so currently results in occasional tearing > > > > and/or backlashes in GL/VK clients that use the buffers directly for > > > > rendering. > > > > > > > > An alternative approach to have the sync code in `MappedFrameBuffer` was > > > > considered but rejected for now, in order to allow clients more > > > > flexibility. > > > > > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > > > > > > > --- > > > > > > > > Changes in v2: > > > > - sync input buffer as well > > > > - update commit title accordingly > > > > - small changes to the commit message > > > > - linting fixes, should pass now > > > > --- > > > > src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++ > > > > 1 file changed, 35 insertions(+) > > > > > > > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > > > > index 077f7f4b..a3b4851c 100644 > > > > --- a/src/libcamera/software_isp/debayer_cpu.cpp > > > > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > > > > @@ -12,8 +12,11 @@ > > > > #include "debayer_cpu.h" > > > > > > > > #include <stdlib.h> > > > > +#include <sys/ioctl.h> > > > > #include <time.h> > > > > > > > > +#include <linux/dma-buf.h> > > > > + > > > > #include <libcamera/formats.h> > > > > > > > > #include "libcamera/internal/bayer_format.h" > > > > @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > > > > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > > > > } > > > > > > > > + for (const FrameBuffer::Plane &plane : input->planes()) { > > > > + const int fd = plane.fd.get(); > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ }; > > > > + > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > > > > We can't use 'errno' directly on our error messages because the LOG() > > > implementations or other processing of the log statement (not in this > > > case, but in others it could be possible) could affect errno. > > > > > > So we use the style > > > > > > src/libcamera/dma_buf_allocator.cpp- if (ret < 0) { > > > src/libcamera/dma_buf_allocator.cpp: ret = errno; > > > src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error) > > > src/libcamera/dma_buf_allocator.cpp- << "Failed to create dma buf for " << name > > > src/libcamera/dma_buf_allocator.cpp- << ": " << strerror(ret); > > > src/libcamera/dma_buf_allocator.cpp- return {}; > > > src/libcamera/dma_buf_allocator.cpp- } > > > > > > Where we store the errno immediately and then use it in the log. It's > > > also helpful to use strerror too. > > > > > > > + } > > > > + > > > > + for (const FrameBuffer::Plane &plane : output->planes()) { > > > > + const int fd = plane.fd.get(); > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE }; > > > > + > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > > + } > > > > > > (jumping back up from below) And this likewise could then be: > > > > > > intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); > > > output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); > > > > > > > + > > > > green_ = params.green; > > > > red_ = swapRedBlueGains_ ? params.blue : params.red; > > > > blue_ = swapRedBlueGains_ ? params.red : params.blue; > > > > @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > > > > > > > > metadata.planes()[0].bytesused = out.planes()[0].size(); > > > > > > > > + for (const FrameBuffer::Plane &plane : output->planes()) { > > > > + const int fd = plane.fd.get(); > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE }; > > > > + > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > > + } > > > > + > > > > + for (const FrameBuffer::Plane &plane : input->planes()) { > > > > + const int fd = plane.fd.get(); > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ }; > > > > + > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > > > > That's four iterations of very similar code. I think we could add a > > > helper directly into FrameBuffer directly as a preceeding patch, that > > > would allow something more like: > > > > > > output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); > > > input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); > > > > > > We could probably do that using our Flags class too ... but the above > > > would be a good start. > > > > I'd like a helper too, but I don't think it should be part of the public > > API, so the FrameBuffer class isn't the right place. > > How about in FrameBuffer::Private::sync() then ? I was thinking about the same, I think input->_d()->sync() would work. > Or worst case - and to progress here, at the very least - just a private > DebayerCpu::syncBuffer() for > > syncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); > > which would at least bump any documentation requirements until there are > more users. > > > > > + } > > > > + > > > > /* Measure before emitting signals */ > > > > if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && > > > > ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
Hi, Le vendredi 13 septembre 2024 à 17:34 +0300, Laurent Pinchart a écrit : > On Fri, Sep 13, 2024 at 02:06:37PM +0100, Kieran Bingham wrote: > > Quoting Laurent Pinchart (2024-09-13 13:38:44) > > > On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote: > > > > Quoting Robert Mader (2024-09-13 13:04:16) > > > > > Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure > > > > > correct output. Not doing so currently results in occasional tearing > > > > > and/or backlashes in GL/VK clients that use the buffers directly for > > > > > rendering. > > > > > > > > > > An alternative approach to have the sync code in `MappedFrameBuffer` was > > > > > considered but rejected for now, in order to allow clients more > > > > > flexibility. > > > > > > > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > > > > > > > > > --- > > > > > > > > > > Changes in v2: > > > > > - sync input buffer as well > > > > > - update commit title accordingly > > > > > - small changes to the commit message > > > > > - linting fixes, should pass now > > > > > --- > > > > > src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++ > > > > > 1 file changed, 35 insertions(+) > > > > > > > > > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > > > > > index 077f7f4b..a3b4851c 100644 > > > > > --- a/src/libcamera/software_isp/debayer_cpu.cpp > > > > > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > > > > > @@ -12,8 +12,11 @@ > > > > > #include "debayer_cpu.h" > > > > > > > > > > #include <stdlib.h> > > > > > +#include <sys/ioctl.h> > > > > > #include <time.h> > > > > > > > > > > +#include <linux/dma-buf.h> > > > > > + > > > > > #include <libcamera/formats.h> > > > > > > > > > > #include "libcamera/internal/bayer_format.h" > > > > > @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > > > > > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > > > > > } > > > > > > > > > > + for (const FrameBuffer::Plane &plane : input->planes()) { > > > > > + const int fd = plane.fd.get(); > > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ }; > > > > > + > > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > > > > > > We can't use 'errno' directly on our error messages because the LOG() > > > > implementations or other processing of the log statement (not in this > > > > case, but in others it could be possible) could affect errno. > > > > > > > > So we use the style > > > > > > > > src/libcamera/dma_buf_allocator.cpp- if (ret < 0) { > > > > src/libcamera/dma_buf_allocator.cpp: ret = errno; > > > > src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error) > > > > src/libcamera/dma_buf_allocator.cpp- << "Failed to create dma buf for " << name > > > > src/libcamera/dma_buf_allocator.cpp- << ": " << strerror(ret); > > > > src/libcamera/dma_buf_allocator.cpp- return {}; > > > > src/libcamera/dma_buf_allocator.cpp- } > > > > > > > > Where we store the errno immediately and then use it in the log. It's > > > > also helpful to use strerror too. > > > > > > > > > + } > > > > > + > > > > > + for (const FrameBuffer::Plane &plane : output->planes()) { > > > > > + const int fd = plane.fd.get(); > > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE }; > > > > > + > > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > > > + } > > > > > > > > (jumping back up from below) And this likewise could then be: > > > > > > > > intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); > > > > output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); > > > > > > > > > + > > > > > green_ = params.green; > > > > > red_ = swapRedBlueGains_ ? params.blue : params.red; > > > > > blue_ = swapRedBlueGains_ ? params.red : params.blue; > > > > > @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > > > > > > > > > > metadata.planes()[0].bytesused = out.planes()[0].size(); > > > > > > > > > > + for (const FrameBuffer::Plane &plane : output->planes()) { > > > > > + const int fd = plane.fd.get(); > > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE }; > > > > > + > > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > > > + } > > > > > + > > > > > + for (const FrameBuffer::Plane &plane : input->planes()) { > > > > > + const int fd = plane.fd.get(); > > > > > + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ }; > > > > > + > > > > > + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > > > > > + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > > > > > > > > That's four iterations of very similar code. I think we could add a > > > > helper directly into FrameBuffer directly as a preceeding patch, that > > > > would allow something more like: > > > > > > > > output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); > > > > input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); > > > > > > > > We could probably do that using our Flags class too ... but the above > > > > would be a good start. > > > > > > I'd like a helper too, but I don't think it should be part of the public > > > API, so the FrameBuffer class isn't the right place. > > > > How about in FrameBuffer::Private::sync() then ? > > I was thinking about the same, I think > > input->_d()->sync() > > would work. For our future sanity, I may suggest to call this sync_for_cpu() or something like this. Just being a little more precise make its clear what this is doing. > > > Or worst case - and to progress here, at the very least - just a private > > DebayerCpu::syncBuffer() for > > > > syncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); > > > > which would at least bump any documentation requirements until there are > > more users. > > > > > > > + } > > > > > + > > > > > /* Measure before emitting signals */ > > > > > if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && > > > > > ++measuredFrames_ > DebayerCpu::kFramesToSkip) { >
Any resistance against a local syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); then? On 13.09.24 16:50, Nicolas Dufresne wrote: > Hi, > > Le vendredi 13 septembre 2024 à 17:34 +0300, Laurent Pinchart a écrit : >> On Fri, Sep 13, 2024 at 02:06:37PM +0100, Kieran Bingham wrote: >>> Quoting Laurent Pinchart (2024-09-13 13:38:44) >>>> On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote: >>>>> Quoting Robert Mader (2024-09-13 13:04:16) >>>>>> Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure >>>>>> correct output. Not doing so currently results in occasional tearing >>>>>> and/or backlashes in GL/VK clients that use the buffers directly for >>>>>> rendering. >>>>>> >>>>>> An alternative approach to have the sync code in `MappedFrameBuffer` was >>>>>> considered but rejected for now, in order to allow clients more >>>>>> flexibility. >>>>>> >>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> >>>>>> >>>>>> --- >>>>>> >>>>>> Changes in v2: >>>>>> - sync input buffer as well >>>>>> - update commit title accordingly >>>>>> - small changes to the commit message >>>>>> - linting fixes, should pass now >>>>>> --- >>>>>> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++ >>>>>> 1 file changed, 35 insertions(+) >>>>>> >>>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >>>>>> index 077f7f4b..a3b4851c 100644 >>>>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp >>>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >>>>>> @@ -12,8 +12,11 @@ >>>>>> #include "debayer_cpu.h" >>>>>> >>>>>> #include <stdlib.h> >>>>>> +#include <sys/ioctl.h> >>>>>> #include <time.h> >>>>>> >>>>>> +#include <linux/dma-buf.h> >>>>>> + >>>>>> #include <libcamera/formats.h> >>>>>> >>>>>> #include "libcamera/internal/bayer_format.h" >>>>>> @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams >>>>>> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); >>>>>> } >>>>>> >>>>>> + for (const FrameBuffer::Plane &plane : input->planes()) { >>>>>> + const int fd = plane.fd.get(); >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ }; >>>>>> + >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) >>>>>> + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; >>>>> We can't use 'errno' directly on our error messages because the LOG() >>>>> implementations or other processing of the log statement (not in this >>>>> case, but in others it could be possible) could affect errno. >>>>> >>>>> So we use the style >>>>> >>>>> src/libcamera/dma_buf_allocator.cpp- if (ret < 0) { >>>>> src/libcamera/dma_buf_allocator.cpp: ret = errno; >>>>> src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error) >>>>> src/libcamera/dma_buf_allocator.cpp- << "Failed to create dma buf for " << name >>>>> src/libcamera/dma_buf_allocator.cpp- << ": " << strerror(ret); >>>>> src/libcamera/dma_buf_allocator.cpp- return {}; >>>>> src/libcamera/dma_buf_allocator.cpp- } >>>>> >>>>> Where we store the errno immediately and then use it in the log. It's >>>>> also helpful to use strerror too. >>>>> >>>>>> + } >>>>>> + >>>>>> + for (const FrameBuffer::Plane &plane : output->planes()) { >>>>>> + const int fd = plane.fd.get(); >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE }; >>>>>> + >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) >>>>>> + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; >>>>>> + } >>>>> (jumping back up from below) And this likewise could then be: >>>>> >>>>> intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); >>>>> output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); >>>>> >>>>>> + >>>>>> green_ = params.green; >>>>>> red_ = swapRedBlueGains_ ? params.blue : params.red; >>>>>> blue_ = swapRedBlueGains_ ? params.red : params.blue; >>>>>> @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams >>>>>> >>>>>> metadata.planes()[0].bytesused = out.planes()[0].size(); >>>>>> >>>>>> + for (const FrameBuffer::Plane &plane : output->planes()) { >>>>>> + const int fd = plane.fd.get(); >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE }; >>>>>> + >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) >>>>>> + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; >>>>>> + } >>>>>> + >>>>>> + for (const FrameBuffer::Plane &plane : input->planes()) { >>>>>> + const int fd = plane.fd.get(); >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ }; >>>>>> + >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) >>>>>> + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; >>>>> That's four iterations of very similar code. I think we could add a >>>>> helper directly into FrameBuffer directly as a preceeding patch, that >>>>> would allow something more like: >>>>> >>>>> output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); >>>>> input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); >>>>> >>>>> We could probably do that using our Flags class too ... but the above >>>>> would be a good start. >>>> I'd like a helper too, but I don't think it should be part of the public >>>> API, so the FrameBuffer class isn't the right place. >>> How about in FrameBuffer::Private::sync() then ? >> I was thinking about the same, I think >> >> input->_d()->sync() >> >> would work. > For our future sanity, I may suggest to call this sync_for_cpu() or something > like this. Just being a little more precise make its clear what this is doing. > >>> Or worst case - and to progress here, at the very least - just a private >>> DebayerCpu::syncBuffer() for >>> >>> syncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); >>> >>> which would at least bump any documentation requirements until there are >>> more users. >>> >>>>>> + } >>>>>> + >>>>>> /* Measure before emitting signals */ >>>>>> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && >>>>>> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
Quoting Robert Mader (2024-09-16 11:45:30) > Any resistance against a local > > syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); > > then? If this is the first/only place to add these so far then I think that's fine, as long as the duplication below is wrapped into a simple helper. It can be local, and then if/when we determine a second place to make use of it we can move to the FrameBuffer::Private::syncForCpu() and handle documentation and flags conversions that might be more desireable if this is used more widely. -- Kieran > > On 13.09.24 16:50, Nicolas Dufresne wrote: > > Hi, > > > > Le vendredi 13 septembre 2024 à 17:34 +0300, Laurent Pinchart a écrit : > >> On Fri, Sep 13, 2024 at 02:06:37PM +0100, Kieran Bingham wrote: > >>> Quoting Laurent Pinchart (2024-09-13 13:38:44) > >>>> On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote: > >>>>> Quoting Robert Mader (2024-09-13 13:04:16) > >>>>>> Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure > >>>>>> correct output. Not doing so currently results in occasional tearing > >>>>>> and/or backlashes in GL/VK clients that use the buffers directly for > >>>>>> rendering. > >>>>>> > >>>>>> An alternative approach to have the sync code in `MappedFrameBuffer` was > >>>>>> considered but rejected for now, in order to allow clients more > >>>>>> flexibility. > >>>>>> > >>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> > >>>>>> > >>>>>> --- > >>>>>> > >>>>>> Changes in v2: > >>>>>> - sync input buffer as well > >>>>>> - update commit title accordingly > >>>>>> - small changes to the commit message > >>>>>> - linting fixes, should pass now > >>>>>> --- > >>>>>> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++ > >>>>>> 1 file changed, 35 insertions(+) > >>>>>> > >>>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > >>>>>> index 077f7f4b..a3b4851c 100644 > >>>>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp > >>>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp > >>>>>> @@ -12,8 +12,11 @@ > >>>>>> #include "debayer_cpu.h" > >>>>>> > >>>>>> #include <stdlib.h> > >>>>>> +#include <sys/ioctl.h> > >>>>>> #include <time.h> > >>>>>> > >>>>>> +#include <linux/dma-buf.h> > >>>>>> + > >>>>>> #include <libcamera/formats.h> > >>>>>> > >>>>>> #include "libcamera/internal/bayer_format.h" > >>>>>> @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > >>>>>> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > >>>>>> } > >>>>>> > >>>>>> + for (const FrameBuffer::Plane &plane : input->planes()) { > >>>>>> + const int fd = plane.fd.get(); > >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ }; > >>>>>> + > >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > >>>>>> + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > >>>>> We can't use 'errno' directly on our error messages because the LOG() > >>>>> implementations or other processing of the log statement (not in this > >>>>> case, but in others it could be possible) could affect errno. > >>>>> > >>>>> So we use the style > >>>>> > >>>>> src/libcamera/dma_buf_allocator.cpp- if (ret < 0) { > >>>>> src/libcamera/dma_buf_allocator.cpp: ret = errno; > >>>>> src/libcamera/dma_buf_allocator.cpp- LOG(DmaBufAllocator, Error) > >>>>> src/libcamera/dma_buf_allocator.cpp- << "Failed to create dma buf for " << name > >>>>> src/libcamera/dma_buf_allocator.cpp- << ": " << strerror(ret); > >>>>> src/libcamera/dma_buf_allocator.cpp- return {}; > >>>>> src/libcamera/dma_buf_allocator.cpp- } > >>>>> > >>>>> Where we store the errno immediately and then use it in the log. It's > >>>>> also helpful to use strerror too. > >>>>> > >>>>>> + } > >>>>>> + > >>>>>> + for (const FrameBuffer::Plane &plane : output->planes()) { > >>>>>> + const int fd = plane.fd.get(); > >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE }; > >>>>>> + > >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > >>>>>> + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > >>>>>> + } > >>>>> (jumping back up from below) And this likewise could then be: > >>>>> > >>>>> intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); > >>>>> output->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); > >>>>> > >>>>>> + > >>>>>> green_ = params.green; > >>>>>> red_ = swapRedBlueGains_ ? params.blue : params.red; > >>>>>> blue_ = swapRedBlueGains_ ? params.red : params.blue; > >>>>>> @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > >>>>>> > >>>>>> metadata.planes()[0].bytesused = out.planes()[0].size(); > >>>>>> > >>>>>> + for (const FrameBuffer::Plane &plane : output->planes()) { > >>>>>> + const int fd = plane.fd.get(); > >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE }; > >>>>>> + > >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > >>>>>> + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > >>>>>> + } > >>>>>> + > >>>>>> + for (const FrameBuffer::Plane &plane : input->planes()) { > >>>>>> + const int fd = plane.fd.get(); > >>>>>> + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ }; > >>>>>> + > >>>>>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) > >>>>>> + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; > >>>>> That's four iterations of very similar code. I think we could add a > >>>>> helper directly into FrameBuffer directly as a preceeding patch, that > >>>>> would allow something more like: > >>>>> > >>>>> output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); > >>>>> input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); > >>>>> > >>>>> We could probably do that using our Flags class too ... but the above > >>>>> would be a good start. > >>>> I'd like a helper too, but I don't think it should be part of the public > >>>> API, so the FrameBuffer class isn't the right place. > >>> How about in FrameBuffer::Private::sync() then ? > >> I was thinking about the same, I think > >> > >> input->_d()->sync() > >> > >> would work. > > For our future sanity, I may suggest to call this sync_for_cpu() or something > > like this. Just being a little more precise make its clear what this is doing. > > > >>> Or worst case - and to progress here, at the very least - just a private > >>> DebayerCpu::syncBuffer() for > >>> > >>> syncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE); > >>> > >>> which would at least bump any documentation requirements until there are > >>> more users. > >>> > >>>>>> + } > >>>>>> + > >>>>>> /* Measure before emitting signals */ > >>>>>> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && > >>>>>> ++measuredFrames_ > DebayerCpu::kFramesToSkip) { > > -- > Robert Mader > Consultant Software Developer > > Collabora Ltd. > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > Registered in England & Wales, no. 5513718 >
diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index 077f7f4b..a3b4851c 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -12,8 +12,11 @@ #include "debayer_cpu.h" #include <stdlib.h> +#include <sys/ioctl.h> #include <time.h> +#include <linux/dma-buf.h> + #include <libcamera/formats.h> #include "libcamera/internal/bayer_format.h" @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); } + for (const FrameBuffer::Plane &plane : input->planes()) { + const int fd = plane.fd.get(); + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ }; + + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; + } + + for (const FrameBuffer::Plane &plane : output->planes()) { + const int fd = plane.fd.get(); + struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE }; + + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; + } + green_ = params.green; red_ = swapRedBlueGains_ ? params.blue : params.red; blue_ = swapRedBlueGains_ ? params.red : params.blue; @@ -760,6 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams metadata.planes()[0].bytesused = out.planes()[0].size(); + for (const FrameBuffer::Plane &plane : output->planes()) { + const int fd = plane.fd.get(); + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE }; + + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; + } + + for (const FrameBuffer::Plane &plane : input->planes()) { + const int fd = plane.fd.get(); + struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ }; + + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0) + LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno; + } + /* Measure before emitting signals */ if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure && ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure correct output. Not doing so currently results in occasional tearing and/or backlashes in GL/VK clients that use the buffers directly for rendering. An alternative approach to have the sync code in `MappedFrameBuffer` was considered but rejected for now, in order to allow clients more flexibility. Signed-off-by: Robert Mader <robert.mader@collabora.com> --- Changes in v2: - sync input buffer as well - update commit title accordingly - small changes to the commit message - linting fixes, should pass now --- src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++++++++++++ 1 file changed, 35 insertions(+)