[PATCH] libcamera: debayer_cpu: Sync output buffer

[Laurent Pinchart]

On Sun, Sep 01, 2024 at 12:56:10PM +0200, Hans de Goede wrote:
> Hi Robert,
> 
> On 8/31/24 9:05 PM, Robert Mader wrote:
> > Using `DMA_BUF_IOCTL_SYNC` is required for dmabufs in order to ensure
> > correct results. Not doing so currently results in occasional tearing
> > and/or backlashes in GL/VK clients that use the buffers directly for
> > rendering.
> > 
> > Signed-off-by: Robert Mader <robert.mader at collabora.com>
> 
> Thank you for your patch.
> 
> I'm not a dmabuf expert, with that said the suggested change looks
> reasonable to me.

Hans, would you be able to test this on an IPU6-based device, and check
the performance impact ? I don't expect expensive cache management
operations on an x86 device.

Bryan, could you do the same with camss ?

> One small code-style remark inline below.
> 
> > ---
> >  src/libcamera/software_isp/debayer_cpu.cpp | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> > index 077f7f4b..6c953b03 100644
> > --- a/src/libcamera/software_isp/debayer_cpu.cpp
> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> > @@ -11,7 +11,9 @@
> >  
> >  #include "debayer_cpu.h"
> >  
> > +#include <linux/dma-buf.h>
> >  #include <stdlib.h>
> > +#include <sys/ioctl.h>
> >  #include <time.h>
> >  
> >  #include <libcamera/formats.h>
> > @@ -733,6 +735,16 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
> >  		clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
> >  	}
> >  
> > +	for (const FrameBuffer::Plane &plane : output->planes()) {
> > +		const int fd = plane.fd.get();
> > +		struct dma_buf_sync sync = { DMA_BUF_SYNC_START };
> > +
> > +		sync.flags |= DMA_BUF_SYNC_WRITE;
> 
> These 2 lines are a weird way to write:
> 
> 		struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE };
> 
> ?
> 
> > +
> > +		if (ioctl (fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
> > +			LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
> > +	}
> > +
> >  	green_ = params.green;
> >  	red_ = swapRedBlueGains_ ? params.blue : params.red;
> >  	blue_ = swapRedBlueGains_ ? params.red : params.blue;
> > @@ -760,6 +772,16 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
> >  
> >  	metadata.planes()[0].bytesused = out.planes()[0].size();
> >  
> > +	for (const FrameBuffer::Plane &plane : output->planes()) {
> > +		const int fd = plane.fd.get();
> > +		struct dma_buf_sync sync = { DMA_BUF_SYNC_END };
> > +
> > +		sync.flags |= DMA_BUF_SYNC_WRITE;
> 
> Same here, why not use:
> 
> 		struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE };
> 
> ?
> 
> > +
> > +		if (ioctl (fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
> > +			LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
> > +	}
> > +
> >  	/* Measure before emitting signals */
> >  	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
> >  	    ++measuredFrames_ > DebayerCpu::kFramesToSkip) {

-- 
Regards,

Laurent Pinchart