[PATCH v4] libcamera: debayer_cpu: Sync DMABUFs

[Milan Zamazal]

Kieran Bingham <kieran.bingham at ideasonboard.com> writes:

> Quoting Milan Zamazal (2024-09-18 10:55:47)
>> Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
>> 
>
>> > Quoting Robert Mader (2024-09-16 17:12:31)
>> >> 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 v4:
>> >>  - Fixed errno handling
>> >>  - Added sync flags to error message
>> >>  - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace
>> >> 
>> >> Changes in v3:
>> >>  - add syncBufferForCPU() helper function
>> >> 
>> >> 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 | 30 ++++++++++++++++++++++
>> >>  1 file changed, 30 insertions(+)
>> >> 
>> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> >> index 077f7f4b..96cf2c71 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)
>> >>         }
>> >>  }
>> >>  
>> >> +namespace {
>> >> +
>> >> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)
>> >
>> > Now these are in an annoymous namespace, they don't need to be static -
>> > but that can be fixed while applying perhaps or ignored for now if no
>> > further version is required otherwise.
>> >
>> > I'm happy to merge, but under software_isp/* I think we should have an
>> > Acked-by/Reviewed-by by one of the SoftISP developers.
>> >
>> > Milan, Hans ? Are you happy with this patch? Is there any impact with
>> > the Intel devices for instance?
>> 
>> I'm fine with the patch (with the exception below) and it works fine on
>> Debix (with only ~0,5% performance penalty).  It would be nice if Hans
>> tested it on Intel.
>> 
>> > --
>> > Kieran
>> >
>> >
>> >
>> >
>> >> +{
>> >> +       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);
>> 
>> I think `ret' and not `-ret' should be used here.  And I would drop
>> `ret = errno' above and use errno here directly.  I'm not sure why
>> Kieran suggested exactly this -- using strerror is right but changing
>> the sign seems wrong (tested in my environment).
>
> This is a pattern we've used throughout libcamera. It's not advised to
> use errno directly in a stream operation ( << ) because other parts of
> the stream or calls, or even the construction of the LOG() class could
> do something that also sets errno.
>
> So we take a copy of errno as soon as the error is detected.

Ah, right, thanks for explanation.

> Yes - I'm wrong on the -ret in that instance. Sorry - I got confused
> because we would only return a negative error value - but in this
> instance we are in a void function that doesn't return anything.
> (there's no point I think).
>
> But yes I inverted the value - sorry. As long as 'errno' is not directly
> used in the LOG() << errno; statements it's fine.
>
> --
> Kieran
>
>> 
>> >> +               }
>> >> +       }
>> >> +}
>> >> +
>> >>  static 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) {
>> >> -- 
>> >> 2.46.0
>> >>
>>