[PATCH v2] libcamera: debayer_cpu: Sync DMABUFs

[Kieran Bingham]

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 at 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
>