[PATCH v2] libcamera: debayer_cpu: Sync DMABUFs

[Kieran Bingham]

Quoting Laurent Pinchart (2024-09-13 13:38:44)
> 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.

How about in FrameBuffer::Private::sync() then ?

Or worst case - and to progress here, at the very least - just a private
DebayerCpu::syncBuffer() for

	syncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);

which would at least bump any documentation requirements until there are
more users.

--
Kieran

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