[PATCH v4] libcamera: debayer_cpu: Sync DMABUFs
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Sep 19 12:15:50 CEST 2024
Quoting Hans de Goede (2024-09-18 20:03:47)
> 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
Checking the stats is a great idea and I've just realised I can also do
this ;-)
On a Lenovo X13s with OV5675 2584x1944-ABGR8888 output pipeline:
Without the patch: 67446 (average of below)
[0:53:27.924908852] [12249] INFO Debayer debayer_cpu.cpp:772 Processed 30 frames in 2026886us, 67562 us/frame
[0:54:04.343961479] [12316] INFO Debayer debayer_cpu.cpp:772 Processed 30 frames in 2023919us, 67463 us/frame
[0:56:43.206420524] [13810] INFO Debayer debayer_cpu.cpp:772 Processed 30 frames in 2021544us, 67384 us/frame
[1:03:39.259725236] [15813] INFO Debayer debayer_cpu.cpp:772 Processed 30 frames in 2021270us, 67375 us/frame
With the patch: 67620 (average of below)
[0:59:29.918112330] [14793] INFO Debayer debayer_cpu.cpp:802 Processed 30 frames in 2026329us, 67544 us/frame
[1:00:04.847115778] [14834] INFO Debayer debayer_cpu.cpp:802 Processed 30 frames in 2033635us, 67787 us/frame
[1:00:33.564276945] [15161] INFO Debayer debayer_cpu.cpp:802 Processed 30 frames in 2028140us, 67604 us/frame
[1:01:26.971334924] [15200] INFO Debayer debayer_cpu.cpp:802 Processed 30 frames in 2026393us, 67546 us/frame
I think that's a ... 0.25% slowdown? I think I can live with that if
it's providing cache handling guarantees ;-)
Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com> # Lenovo X13s + OV5675
--
Kieran
>
> 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
> >>>
> >
>
More information about the libcamera-devel
mailing list