[PATCH v2] libcamera: debayer_cpu: Sync DMABUFs

[Kieran Bingham]

Quoting Robert Mader (2024-09-16 11:45:30)
> Any resistance against a local
> 
> syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);
> 
> then?

If this is the first/only place to add these so far then I think that's
fine, as long as the duplication below is wrapped into a simple helper.

It can be local, and then if/when we determine a second place to make
use of it we can move to the FrameBuffer::Private::syncForCpu() and
handle documentation and flags conversions that might be more desireable
if this is used more widely.

--
Kieran


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