[PATCH v5] libcamera: debayer_cpu: Sync DMABUFs

[Cheng-Hao Yang]

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.

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240923/5b68cb1a/attachment.htm>