[PATCH v2] libcamera: debayer_cpu: Sync DMABUFs

[Robert Mader]

Any resistance against a local

syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);

then?

On 13.09.24 16:50, Nicolas Dufresne wrote:
> Hi,
>
> Le vendredi 13 septembre 2024 à 17:34 +0300, Laurent Pinchart a écrit :
>> On Fri, Sep 13, 2024 at 02:06:37PM +0100, Kieran Bingham wrote:
>>> Quoting Laurent Pinchart (2024-09-13 13:38:44)
>>>> On Fri, Sep 13, 2024 at 01:31:11PM +0100, Kieran Bingham wrote:
>>>>> Quoting Robert Mader (2024-09-13 13:04:16)
>>>>>> 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 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 | 35 ++++++++++++++++++++++
>>>>>>   1 file changed, 35 insertions(+)
>>>>>>
>>>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>>>>>> index 077f7f4b..a3b4851c 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"
>>>>>> @@ -733,6 +736,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>>>>>>                  clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
>>>>>>          }
>>>>>>   
>>>>>> +       for (const FrameBuffer::Plane &plane : input->planes()) {
>>>>>> +               const int fd = plane.fd.get();
>>>>>> +               struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ };
>>>>>> +
>>>>>> +               if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
>>>>>> +                       LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
>>>>> We can't use 'errno' directly on our error messages because the LOG()
>>>>> implementations or other processing of the log statement (not in this
>>>>> case, but in others it could be possible) could affect errno.
>>>>>
>>>>> So we use the style
>>>>>
>>>>> src/libcamera/dma_buf_allocator.cpp-    if (ret < 0) {
>>>>> src/libcamera/dma_buf_allocator.cpp:            ret = errno;
>>>>> src/libcamera/dma_buf_allocator.cpp-            LOG(DmaBufAllocator, Error)
>>>>> src/libcamera/dma_buf_allocator.cpp-                    << "Failed to create dma buf for " << name
>>>>> src/libcamera/dma_buf_allocator.cpp-                    << ": " << strerror(ret);
>>>>> src/libcamera/dma_buf_allocator.cpp-            return {};
>>>>> src/libcamera/dma_buf_allocator.cpp-    }
>>>>>
>>>>> Where we store the errno immediately and then use it in the log. It's
>>>>> also helpful to use strerror too.
>>>>>
>>>>>> +       }
>>>>>> +
>>>>>> +       for (const FrameBuffer::Plane &plane : output->planes()) {
>>>>>> +               const int fd = plane.fd.get();
>>>>>> +               struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE };
>>>>>> +
>>>>>> +               if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
>>>>>> +                       LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
>>>>>> +       }
>>>>> (jumping back up from below) And this likewise could then be:
>>>>>
>>>>>        intput->sync(DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);
>>>>>        output->sync(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 +779,22 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>>>>>>   
>>>>>>          metadata.planes()[0].bytesused = out.planes()[0].size();
>>>>>>   
>>>>>> +       for (const FrameBuffer::Plane &plane : output->planes()) {
>>>>>> +               const int fd = plane.fd.get();
>>>>>> +               struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE };
>>>>>> +
>>>>>> +               if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
>>>>>> +                       LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
>>>>>> +       }
>>>>>> +
>>>>>> +       for (const FrameBuffer::Plane &plane : input->planes()) {
>>>>>> +               const int fd = plane.fd.get();
>>>>>> +               struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ };
>>>>>> +
>>>>>> +               if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
>>>>>> +                       LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
>>>>> That's four iterations of very similar code. I think we could add a
>>>>> helper directly into FrameBuffer directly as a preceeding patch, that
>>>>> would allow something more like:
>>>>>
>>>>>        output->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);
>>>>>        input->sync(DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);
>>>>>
>>>>> We could probably do that using our Flags class too ... but the above
>>>>> would be a good start.
>>>> I'd like a helper too, but I don't think it should be part of the public
>>>> API, so the FrameBuffer class isn't the right place.
>>> How about in FrameBuffer::Private::sync() then ?
>> I was thinking about the same, I think
>>
>> 	input->_d()->sync()
>>
>> would work.
> For our future sanity, I may suggest to call this sync_for_cpu() or something
> like this. Just being a little more precise make its clear what this is doing.
>
>>> Or worst case - and to progress here, at the very least - just a private
>>> DebayerCpu::syncBuffer() for
>>>
>>> 	syncBuffer(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);
>>>
>>> which would at least bump any documentation requirements until there are
>>> more users.
>>>
>>>>>> +       }
>>>>>> +
>>>>>>          /* Measure before emitting signals */
>>>>>>          if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
>>>>>>              ++measuredFrames_ > DebayerCpu::kFramesToSkip) {

-- 
Robert Mader
Consultant Software Developer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718