[PATCH v5] libcamera: debayer_cpu: Sync DMABUFs

[Nicolas Dufresne]

Hi,

Le vendredi 20 septembre 2024 à 12:49 +0100, Kieran Bingham a écrit :
> From: Robert Mader <robert.mader at collabora.com>
> 
> 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.
> 
> While the new helper is added to an annoymous namespace, add
> timeDiff to the same namespace and remove the static definition as a
> drive by.
> 
> Signed-off-by: Robert Mader <robert.mader at collabora.com>
> Tested-by: Milan Zamazal <mzamazal at redhat.com> # Debix
> Tested-by: Hans de Goede <hdegoede at redhat.com> # IPU6 + ov2740
> Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com> # Lenovo X13s + OV5675
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

If we generalize this in the future, a more C++ friendly implementation would be
nice. I'd see something similar to the mutex locker, something you can't forget
to close.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>

> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> ---
> v5: Kieran:
>  - Remove static
>  - Fix ret negation that I suggested incorrectly.
> 
>  src/libcamera/software_isp/debayer_cpu.cpp | 32 +++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 077f7f4bc45b..8a757fe9e02d 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"
> @@ -718,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)
>  	}
>  }
>  
> -static inline int64_t timeDiff(timespec &after, timespec &before)
> +namespace {
> +
> +void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)
> +{
> +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> +		const int fd = plane.fd.get();
> +		struct dma_buf_sync sync = { syncFlags };
> +		int ret;
> +
> +		ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);
> +		if (ret < 0) {
> +			ret = errno;
> +			LOG(Debayer, Error)
> +				<< "Syncing buffer FD " << fd << " with flags "
> +				<< syncFlags << " failed: " << strerror(ret);
> +		}
> +	}
> +}
> +
> +inline int64_t timeDiff(timespec &after, timespec &before)
>  {
>  	return (after.tv_sec - before.tv_sec) * 1000000000LL +
>  	       (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;
>  }
>  
> +} /* namespace */
> +
>  void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)
>  {
>  	timespec frameStartTime;
> @@ -733,6 +757,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>  		clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
>  	}
>  
> +	syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);
> +	syncBufferForCPU(output, 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 +787,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>  
>  	metadata.planes()[0].bytesused = out.planes()[0].size();
>  
> +	syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);
> +	syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);
> +
>  	/* Measure before emitting signals */
>  	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
>  	    ++measuredFrames_ > DebayerCpu::kFramesToSkip) {