[PATCH v2] libcamera: debayer_cpu: Sync DMABUFs

[Nicolas Dufresne]

Hi,

Le vendredi 13 septembre 2024 à 17:34 +0300, Laurent Pinchart a écrit :
> On Fri, Sep 13, 2024 at 02:06:37PM +0100, Kieran Bingham wrote:
> > 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 ?
> 
> I was thinking about the same, I think
> 
> 	input->_d()->sync()
> 
> would work.

For our future sanity, I may suggest to call this sync_for_cpu() or something
like this. Just being a little more precise make its clear what this is doing.

> 
> > 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.
> > 
> > > > > +       }
> > > > > +
> > > > >         /* Measure before emitting signals */
> > > > >         if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
> > > > >             ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
>