[PATCH v5] libcamera: debayer_cpu: Sync DMABUFs

[Kieran Bingham]

Quoting Cheng-Hao Yang (2024-09-23 08:52:32)
> Hi Robert and developers,
> 
> Sorry to be late to the party, as I saw the v5 patch is already accepted
> [1].
> 
> However, I'm wondering if it makes sense to add the DMABUF
> synchronization as a generic function in the DmaBufAllocator,
> like CrOS had done [2]. CrOS mtkisp7 needs the sync as well,
> and maybe we can save some duplicated code this way.
> 
> Whether the input is a single fd or a FrameBuffer (that we sync
> all fds in all planes) can be discussed.
> 
> I'd be happy to add the new patch, and update the usages in SoftwareISP
> if this is the way to go.

Working on a clean generic helper to save duplications is worthwhile. We
did discuss that in the patches, but went for the simple/local merged
version as it only currently affected the soft-isp, and we can't put it
in a code-path that is used by all other pipelines all the time.

I don't know where it should go yet though. Will the allocator be the
right place for it ?  What happens if the buffers come from an external
source ? (Or maybe in that case, the provider is responsible?)

--
Kieran


> 
> Thanks!
> Harvey
> 
> [1]: https://patchwork.libcamera.org/patch/21290/
> [2]:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/4714763/21/src/libcamera/dma_heaps.cpp#79
> 
> On Sat, Sep 21, 2024 at 12:52 AM Nicolas Dufresne <nicolas at ndufresne.ca>
> wrote:
> 
> > Hi,
> >
> > Le vendredi 20 septembre 2024 à 12:49 +0100, Kieran Bingham a écrit :
> > > From: Robert Mader <robert.mader at collabora.com>
> > >
> > > 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.
> > >
> > > While the new helper is added to an annoymous namespace, add
> > > timeDiff to the same namespace and remove the static definition as a
> > > drive by.
> > >
> > > Signed-off-by: Robert Mader <robert.mader at collabora.com>
> > > Tested-by: Milan Zamazal <mzamazal at redhat.com> # Debix
> > > Tested-by: Hans de Goede <hdegoede at redhat.com> # IPU6 + ov2740
> > > Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com> # Lenovo
> > X13s + OV5675
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > If we generalize this in the future, a more C++ friendly implementation
> > would be
> > nice. I'd see something similar to the mutex locker, something you can't
> > forget
> > to close.
> >
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> >
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > ---
> > > v5: Kieran:
> > >  - Remove static
> > >  - Fix ret negation that I suggested incorrectly.
> > >
> > >  src/libcamera/software_isp/debayer_cpu.cpp | 32 +++++++++++++++++++++-
> > >  1 file changed, 31 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp
> > b/src/libcamera/software_isp/debayer_cpu.cpp
> > > index 077f7f4bc45b..8a757fe9e02d 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)
> > >       }
> > >  }
> > >
> > > -static inline int64_t timeDiff(timespec &after, timespec &before)
> > > +namespace {
> > > +
> > > +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);
> > > +             }
> > > +     }
> > > +}
> > > +
> > > +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) {
> >
> >
> 
> -- 
> BR,
> Harvey Yang