[PATCH v4] libcamera: debayer_cpu: Sync DMABUFs

[Hans de Goede]

Hi,

On 18-Sep-24 11:55 AM, Milan Zamazal wrote:
> 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.

I have tested this on a ThinkPad X1 Yoga gen 8 with IPU6 + ov2740
sensor running at 1920x1080. Things still work and the builtin
bench function shows an increase of the frame-processing time
(including the syncing) from ~6.1 ms/frame to ~6.3 ms/frame which
I consider an acceptable slowdown:

Tested-by: Hans de Goede <hdegoede at redhat.com> # IPU6 + ov2740

Regards,

Hans






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