[PATCH v5] libcamera: debayer_cpu: Sync DMABUFs
Cheng-Hao Yang
chenghaoyang at google.com
Mon Sep 23 12:07:58 CEST 2024
Hi Kieran,
On Mon, Sep 23, 2024 at 5:03 PM Kieran Bingham <
kieran.bingham at 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 at ndufresne.ca>
> > wrote:
> >
> > > Hi,
> > >
> > > Le vendredi 20 septembre 2024 à 12:49 +0100, Kieran Bingham a écrit :
> > > > From: Robert Mader <robert.mader at 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 at collabora.com>
> > > > Tested-by: Milan Zamazal <mzamazal at redhat.com> # Debix
> > > > Tested-by: Hans de Goede <hdegoede at redhat.com> # IPU6 + ov2740
> > > > Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com> # Lenovo
> > > X13s + OV5675
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at 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 at collabora.com>
> > >
> > > > Signed-off-by: Kieran Bingham <kieran.bingham at 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
>
--
BR,
Harvey Yang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240923/94974c12/attachment.htm>
More information about the libcamera-devel
mailing list