[PATCH v4] libcamera: debayer_cpu: Sync DMABUFs

[Milan Zamazal]

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).

>> +               }
>> +       }
>> +}
>> +
>>  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
>>