[v5] libcamera: debayer_cpu: Sync DMABUFs

21290 diff mbox series

From: Robert Mader <robert.mader@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@collabora.com>
Tested-by: Milan Zamazal <mzamazal@redhat.com> # Debix
Tested-by: Hans de Goede <hdegoede@redhat.com> # IPU6 + ov2740
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo X13s + OV5675
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@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(-)

Hi,

Le vendredi 20 septembre 2024 à 12:49 +0100, Kieran Bingham a écrit :
> From: Robert Mader <robert.mader@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@collabora.com>
> Tested-by: Milan Zamazal <mzamazal@redhat.com> # Debix
> Tested-by: Hans de Goede <hdegoede@redhat.com> # IPU6 + ov2740
> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo X13s + OV5675
> Reviewed-by: Kieran Bingham <kieran.bingham@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@collabora.com>

> Signed-off-by: Kieran Bingham <kieran.bingham@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) {