[v5] libcamera: debayer_cpu: Sync DMABUFs
diff mbox series

Message ID 20240920114915.32728-1-kieran.bingham@ideasonboard.com
State Accepted
Commit cbfe04f77ca6ee46fcb6635294c8dbe9be26c5d1
Headers show
Series
  • [v5] libcamera: debayer_cpu: Sync DMABUFs
Related show

Commit Message

Kieran Bingham Sept. 20, 2024, 11:49 a.m. UTC
From: Robert Mader <robert.mader@collabora.com>

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.

While the new helper is added to an annoymous namespace, add
timeDiff to the same namespace and remove the static definition as a
drive by.

Signed-off-by: Robert Mader <robert.mader@collabora.com>
Tested-by: Milan Zamazal <mzamazal@redhat.com> # Debix
Tested-by: Hans de Goede <hdegoede@redhat.com> # IPU6 + ov2740
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo X13s + OV5675
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
v5: Kieran:
 - Remove static
 - Fix ret negation that I suggested incorrectly.

 src/libcamera/software_isp/debayer_cpu.cpp | 32 +++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

Comments

Nicolas Dufresne Sept. 20, 2024, 4:52 p.m. UTC | #1
Hi,

Le vendredi 20 septembre 2024 à 12:49 +0100, Kieran Bingham a écrit :
> From: Robert Mader <robert.mader@collabora.com>
> 
> 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.
> 
> While the new helper is added to an annoymous namespace, add
> timeDiff to the same namespace and remove the static definition as a
> drive by.
> 
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> Tested-by: Milan Zamazal <mzamazal@redhat.com> # Debix
> Tested-by: Hans de Goede <hdegoede@redhat.com> # IPU6 + ov2740
> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo X13s + OV5675
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

If we generalize this in the future, a more C++ friendly implementation would be
nice. I'd see something similar to the mutex locker, something you can't forget
to close.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> v5: Kieran:
>  - Remove static
>  - Fix ret negation that I suggested incorrectly.
> 
>  src/libcamera/software_isp/debayer_cpu.cpp | 32 +++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 077f7f4bc45b..8a757fe9e02d 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)
>  	}
>  }
>  
> -static inline int64_t timeDiff(timespec &after, timespec &before)
> +namespace {
> +
> +void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)
> +{
> +	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);
> +		}
> +	}
> +}
> +
> +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) {
Cheng-Hao Yang Sept. 23, 2024, 7:52 a.m. UTC | #2
Hi Robert and developers,

Sorry to be late to the party, as I saw the v5 patch is already accepted
[1].

However, I'm wondering if it makes sense to add the DMABUF
synchronization as a generic function in the DmaBufAllocator,
like CrOS had done [2]. CrOS mtkisp7 needs the sync as well,
and maybe we can save some duplicated code this way.

Whether the input is a single fd or a FrameBuffer (that we sync
all fds in all planes) can be discussed.

I'd be happy to add the new patch, and update the usages in SoftwareISP
if this is the way to go.

Thanks!
Harvey

[1]: https://patchwork.libcamera.org/patch/21290/
[2]:
https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/4714763/21/src/libcamera/dma_heaps.cpp#79

On Sat, Sep 21, 2024 at 12:52 AM Nicolas Dufresne <nicolas@ndufresne.ca>
wrote:

> Hi,
>
> Le vendredi 20 septembre 2024 à 12:49 +0100, Kieran Bingham a écrit :
> > From: Robert Mader <robert.mader@collabora.com>
> >
> > 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.
> >
> > While the new helper is added to an annoymous namespace, add
> > timeDiff to the same namespace and remove the static definition as a
> > drive by.
> >
> > Signed-off-by: Robert Mader <robert.mader@collabora.com>
> > Tested-by: Milan Zamazal <mzamazal@redhat.com> # Debix
> > Tested-by: Hans de Goede <hdegoede@redhat.com> # IPU6 + ov2740
> > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo
> X13s + OV5675
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> If we generalize this in the future, a more C++ friendly implementation
> would be
> nice. I'd see something similar to the mutex locker, something you can't
> forget
> to close.
>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > ---
> > v5: Kieran:
> >  - Remove static
> >  - Fix ret negation that I suggested incorrectly.
> >
> >  src/libcamera/software_isp/debayer_cpu.cpp | 32 +++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp
> b/src/libcamera/software_isp/debayer_cpu.cpp
> > index 077f7f4bc45b..8a757fe9e02d 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)
> >       }
> >  }
> >
> > -static inline int64_t timeDiff(timespec &after, timespec &before)
> > +namespace {
> > +
> > +void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)
> > +{
> > +     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);
> > +             }
> > +     }
> > +}
> > +
> > +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) {
>
>
Kieran Bingham Sept. 23, 2024, 9:03 a.m. UTC | #3
Quoting Cheng-Hao Yang (2024-09-23 08:52:32)
> Hi Robert and developers,
> 
> Sorry to be late to the party, as I saw the v5 patch is already accepted
> [1].
> 
> However, I'm wondering if it makes sense to add the DMABUF
> synchronization as a generic function in the DmaBufAllocator,
> like CrOS had done [2]. CrOS mtkisp7 needs the sync as well,
> and maybe we can save some duplicated code this way.
> 
> Whether the input is a single fd or a FrameBuffer (that we sync
> all fds in all planes) can be discussed.
> 
> I'd be happy to add the new patch, and update the usages in SoftwareISP
> if this is the way to go.

Working on a clean generic helper to save duplications is worthwhile. We
did discuss that in the patches, but went for the simple/local merged
version as it only currently affected the soft-isp, and we can't put it
in a code-path that is used by all other pipelines all the time.

I don't know where it should go yet though. Will the allocator be the
right place for it ?  What happens if the buffers come from an external
source ? (Or maybe in that case, the provider is responsible?)

--
Kieran


> 
> Thanks!
> Harvey
> 
> [1]: https://patchwork.libcamera.org/patch/21290/
> [2]:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/4714763/21/src/libcamera/dma_heaps.cpp#79
> 
> On Sat, Sep 21, 2024 at 12:52 AM Nicolas Dufresne <nicolas@ndufresne.ca>
> wrote:
> 
> > Hi,
> >
> > Le vendredi 20 septembre 2024 à 12:49 +0100, Kieran Bingham a écrit :
> > > From: Robert Mader <robert.mader@collabora.com>
> > >
> > > 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.
> > >
> > > While the new helper is added to an annoymous namespace, add
> > > timeDiff to the same namespace and remove the static definition as a
> > > drive by.
> > >
> > > Signed-off-by: Robert Mader <robert.mader@collabora.com>
> > > Tested-by: Milan Zamazal <mzamazal@redhat.com> # Debix
> > > Tested-by: Hans de Goede <hdegoede@redhat.com> # IPU6 + ov2740
> > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo
> > X13s + OV5675
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > If we generalize this in the future, a more C++ friendly implementation
> > would be
> > nice. I'd see something similar to the mutex locker, something you can't
> > forget
> > to close.
> >
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> >
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > ---
> > > v5: Kieran:
> > >  - Remove static
> > >  - Fix ret negation that I suggested incorrectly.
> > >
> > >  src/libcamera/software_isp/debayer_cpu.cpp | 32 +++++++++++++++++++++-
> > >  1 file changed, 31 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp
> > b/src/libcamera/software_isp/debayer_cpu.cpp
> > > index 077f7f4bc45b..8a757fe9e02d 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)
> > >       }
> > >  }
> > >
> > > -static inline int64_t timeDiff(timespec &after, timespec &before)
> > > +namespace {
> > > +
> > > +void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)
> > > +{
> > > +     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);
> > > +             }
> > > +     }
> > > +}
> > > +
> > > +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) {
> >
> >
> 
> -- 
> BR,
> Harvey Yang
Cheng-Hao Yang Sept. 23, 2024, 10:07 a.m. UTC | #4
Hi Kieran,

On Mon, Sep 23, 2024 at 5:03 PM Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Cheng-Hao Yang (2024-09-23 08:52:32)
> > Hi Robert and developers,
> >
> > Sorry to be late to the party, as I saw the v5 patch is already accepted
> > [1].
> >
> > However, I'm wondering if it makes sense to add the DMABUF
> > synchronization as a generic function in the DmaBufAllocator,
> > like CrOS had done [2]. CrOS mtkisp7 needs the sync as well,
> > and maybe we can save some duplicated code this way.
> >
> > Whether the input is a single fd or a FrameBuffer (that we sync
> > all fds in all planes) can be discussed.
> >
> > I'd be happy to add the new patch, and update the usages in SoftwareISP
> > if this is the way to go.
>
> Working on a clean generic helper to save duplications is worthwhile. We
> did discuss that in the patches, but went for the simple/local merged
> version as it only currently affected the soft-isp, and we can't put it
> in a code-path that is used by all other pipelines all the time.


> I don't know where it should go yet though. Will the allocator be the
> right place for it ?  What happens if the buffers come from an external
> source ? (Or maybe in that case, the provider is responsible?)
>

Yeah that's a question that we need to figure out indeed.

Maybe an option is to keep the information that the fd is allocated by
DmaBufAllocator in UniqueFD/SharedFD, and the sync helper
function/class would abort if the input is not from the allocator?
We may also be able to make it a function in SharedFD in this way.

I'm also fine with the idea that the provider is responsible though.

BTW, CrOS mtkisp7 also has a class DmaSyncer [1], which manages
the sync with the instance's lifetime. I think it's worth a thought as well.

[1]:
https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674883

BR,
Harvey


> --
> Kieran
>
>
> >
> > Thanks!
> > Harvey
> >
> > [1]: https://patchwork.libcamera.org/patch/21290/
> > [2]:
> >
> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/4714763/21/src/libcamera/dma_heaps.cpp#79
> >
> > On Sat, Sep 21, 2024 at 12:52 AM Nicolas Dufresne <nicolas@ndufresne.ca>
> > wrote:
> >
> > > Hi,
> > >
> > > Le vendredi 20 septembre 2024 à 12:49 +0100, Kieran Bingham a écrit :
> > > > From: Robert Mader <robert.mader@collabora.com>
> > > >
> > > > 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.
> > > >
> > > > While the new helper is added to an annoymous namespace, add
> > > > timeDiff to the same namespace and remove the static definition as a
> > > > drive by.
> > > >
> > > > Signed-off-by: Robert Mader <robert.mader@collabora.com>
> > > > Tested-by: Milan Zamazal <mzamazal@redhat.com> # Debix
> > > > Tested-by: Hans de Goede <hdegoede@redhat.com> # IPU6 + ov2740
> > > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo
> > > X13s + OV5675
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > If we generalize this in the future, a more C++ friendly implementation
> > > would be
> > > nice. I'd see something similar to the mutex locker, something you
> can't
> > > forget
> > > to close.
> > >
> > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > >
> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > >
> > > > ---
> > > > v5: Kieran:
> > > >  - Remove static
> > > >  - Fix ret negation that I suggested incorrectly.
> > > >
> > > >  src/libcamera/software_isp/debayer_cpu.cpp | 32
> +++++++++++++++++++++-
> > > >  1 file changed, 31 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp
> > > b/src/libcamera/software_isp/debayer_cpu.cpp
> > > > index 077f7f4bc45b..8a757fe9e02d 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)
> > > >       }
> > > >  }
> > > >
> > > > -static inline int64_t timeDiff(timespec &after, timespec &before)
> > > > +namespace {
> > > > +
> > > > +void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)
> > > > +{
> > > > +     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);
> > > > +             }
> > > > +     }
> > > > +}
> > > > +
> > > > +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) {
> > >
> > >
> >
> > --
> > BR,
> > Harvey Yang
>

Patch
diff mbox series

diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index 077f7f4bc45b..8a757fe9e02d 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)
 	}
 }
 
-static inline int64_t timeDiff(timespec &after, timespec &before)
+namespace {
+
+void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)
+{
+	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);
+		}
+	}
+}
+
+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) {