[PATCH] libcamera: debayer_cpu: Sync output buffer

[Hans de Goede]

Hi Robert,

On 8/31/24 9:05 PM, Robert Mader wrote:
> Using `DMA_BUF_IOCTL_SYNC` is required for dmabufs in order to ensure
> correct results. Not doing so currently results in occasional tearing
> and/or backlashes in GL/VK clients that use the buffers directly for
> rendering.
> 
> Signed-off-by: Robert Mader <robert.mader at collabora.com>

Thank you for your patch.

I'm not a dmabuf expert, with that said the suggested change looks
reasonable to me.

One small code-style remark inline below.

> ---
>  src/libcamera/software_isp/debayer_cpu.cpp | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 077f7f4b..6c953b03 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -11,7 +11,9 @@
>  
>  #include "debayer_cpu.h"
>  
> +#include <linux/dma-buf.h>
>  #include <stdlib.h>
> +#include <sys/ioctl.h>
>  #include <time.h>
>  
>  #include <libcamera/formats.h>
> @@ -733,6 +735,16 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>  		clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
>  	}
>  
> +	for (const FrameBuffer::Plane &plane : output->planes()) {
> +		const int fd = plane.fd.get();
> +		struct dma_buf_sync sync = { DMA_BUF_SYNC_START };
> +
> +		sync.flags |= DMA_BUF_SYNC_WRITE;

These 2 lines are a weird way to write:

		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 +772,16 @@ 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 };
> +
> +		sync.flags |= DMA_BUF_SYNC_WRITE;

Same here, why not use:

		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;
> +	}
> +
>  	/* Measure before emitting signals */
>  	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
>  	    ++measuredFrames_ > DebayerCpu::kFramesToSkip) {

Regards,

Hans