[PATCH v4] libcamera: debayer_cpu: Sync DMABUFs

[Laurent Pinchart]

On Wed, Sep 18, 2024 at 01:26:41PM +0200, Milan Zamazal wrote:
> 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.

Agreed.

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

-- 
Regards,

Laurent Pinchart