[PATCH v4] libcamera: debayer_cpu: Sync DMABUFs

[Milan Zamazal]

Robert Mader <robert.mader at collabora.com> writes:

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

I would prefer tracking it in bugzilla.  I consider the software ISP
TODO being a snapshot of the implementation requirements at the given
moment, which should be cleared ASAP and not used anymore.

> 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) {