[PATCH v2] libcamera: debayer_cpu: Sync DMABUFs

[Laurent Pinchart]

On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote:
> Quoting Robert Mader (2024-09-13 13:04:16)
> > 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 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 | 35 ++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> > index 077f7f4b..a3b4851c 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"
> > @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
> >                 clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
> >         }
> >  
> > +       for (const FrameBuffer::Plane &plane : input->planes()) {
> > +               const int fd = plane.fd.get();
> > +               struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ };
> > +
> > +               if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
> > +                       LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
> 
> We can't use 'errno' directly on our error messages because the LOG()
> implementations or other processing of the log statement (not in this
> case, but in others it could be possible) could affect errno.
> 
> So we use the style 
> 
> src/libcamera/dma_buf_allocator.cpp-    if (ret < 0) {
> src/libcamera/dma_buf_allocator.cpp:            ret = errno;
> src/libcamera/dma_buf_allocator.cpp-            LOG(DmaBufAllocator, Error)
> src/libcamera/dma_buf_allocator.cpp-                    << "Failed to create dma buf for " << name
> src/libcamera/dma_buf_allocator.cpp-                    << ": " << strerror(ret);
> src/libcamera/dma_buf_allocator.cpp-            return {};
> src/libcamera/dma_buf_allocator.cpp-    }
> 
> Where we store the errno immediately and then use it in the log. It's
> also helpful to use strerror too.
> 
> > +       }
> > +
> > +       for (const FrameBuffer::Plane &plane : output->planes()) {
> > +               const int fd = plane.fd.get();
> > +               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;
> > +       }
> 
> (jumping back up from below) And this likewise could then be:
> 
> 	intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);
>  	output->sync(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 +779,22 @@ 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 | DMA_BUF_SYNC_WRITE };
> > +
> > +               if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
> > +                       LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
> > +       }
> > +
> > +       for (const FrameBuffer::Plane &plane : input->planes()) {
> > +               const int fd = plane.fd.get();
> > +               struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ };
> > +
> > +               if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
> > +                       LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
> 
> That's four iterations of very similar code. I think we could add a
> helper directly into FrameBuffer directly as a preceeding patch, that
> would allow something more like:
> 
> 	output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);
> 	input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);
> 
> We could probably do that using our Flags class too ... but the above
> would be a good start.

I'd like a helper too, but I don't think it should be part of the public
API, so the FrameBuffer class isn't the right place.

> > +       }
> > +
> >         /* Measure before emitting signals */
> >         if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
> >             ++measuredFrames_ > DebayerCpu::kFramesToSkip) {

-- 
Regards,

Laurent Pinchart