[PATCH v4] libcamera: debayer_cpu: Sync DMABUFs

[Robert Mader]

One thing I'd like to highlight again for the record: while the patch 
here reduces glitches considerably for me, I still occasionally see some 
when using GL clients like Snapshot.

Assuming that's not because of hardware/driver bugs (I see it on quite 
different platforms), that could be caused by us still waiting for 
implicit/explicit sync before reusing buffers.

 From https://docs.kernel.org/driver-api/dma-buf.html:

> The synchronization provided via DMA_BUF_IOCTL_SYNC only provides 
> cache coherency. It does not prevent other processes or devices from 
> accessing the memory at the same time. If synchronization with a GPU 
> or other device driver is required, it is the client’s responsibility 
> to wait for buffer to be ready for reading or writing before calling 
> this ioctl with DMA_BUF_SYNC_START. Likewise, the client must ensure 
> that follow-up work is not submitted to GPU or other device driver 
> until after this ioctl has been called with DMA_BUF_SYNC_END?
>
> If the driver or API with which the client is interacting uses 
> implicit synchronization, waiting for prior work to complete can be 
> done via poll() on the DMA buffer file descriptor. If the driver or 
> API requires explicit synchronization, the client may have to wait on 
> a sync_file or other synchronization primitive outside the scope of 
> the DMA buffer API.
>
I wonder if we should add a swISP TODO for this - or is opening a 
bugzilla issue for tracking enough?

P.S.: I'm not entirely sure if the udmabuf sync implementation really 
doesn't wait for implicit fences - but if it does, there's no guarantee 
that things stay that way.

On 16.09.24 18:12, Robert Mader wrote:
> 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 v4:
>   - Fixed errno handling
>   - Added sync flags to error message
>   - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace
>
> Changes in v3:
>   - add syncBufferForCPU() helper function
>
> 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 | 30 ++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
>
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 077f7f4b..96cf2c71 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)
>   	}
>   }
>   
> +namespace {
> +
> +static 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);
> +		}
> +	}
> +}
> +
>   static 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) {

-- 
Robert Mader
Consultant Software Developer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718