[PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable

[Cheng-Hao Yang]

Hi Milan,

On Mon, Jan 13, 2025 at 3:44 PM Milan Zamazal <mzamazal at redhat.com> wrote:
>
> Hi,
>
> Harvey Yang <chenghaoyang at chromium.org> writes:
>
> > As DmaSyncer does sync start/end in the c'tor/d'tor, copying a DmaSyncer
> > instance would trigger sync end earlier than expected. This patch makes
> > it non-copyable to avoid the issue.
>
> After this patch, software ISP still works but reports the following
> error on each frame:

May I know what the environment is? (Unit tests or specific tests
you're running?

>
>   ERROR DmaBufAllocator dma_buf_allocator.cpp:344 Unable to sync dma fd: -1, err: Bad file descriptor, flags: 5

Based on the log, `fd_.get()` is -1. Could you confirm that
`plane.fd.get()` below is never `-1`?

BR,
Harvey

>
> DmaSyncer is used only in debayer_cpu.cpp:
>
>   std::vector<DmaSyncer> dmaSyncers;
>   for (const FrameBuffer::Plane &plane : input->planes())
>         dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);
>
>   for (const FrameBuffer::Plane &plane : output->planes())
>         dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);
>
>   green_ = params.green;
>   red_ = swapRedBlueGains_ ? params.blue : params.red;
>   blue_ = swapRedBlueGains_ ? params.red : params.blue;
>
>   /* Copy metadata from the input buffer */
>   FrameMetadata &metadata = output->_d()->metadata();
>   metadata.status = input->metadata().status;
>   metadata.sequence = input->metadata().sequence;
>   metadata.timestamp = input->metadata().timestamp;
>
>   MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);
>   MappedFrameBuffer out(output, MappedFrameBuffer::MapFlag::Write);
>   if (!in.isValid() || !out.isValid()) {
>         LOG(Debayer, Error) << "mmap-ing buffer(s) failed";
>         metadata.status = FrameMetadata::FrameError;
>         return;
>   }
>
>   stats_->startFrame();
>
>   if (inputConfig_.patternSize.height == 2)
>         process2(in.planes()[0].data(), out.planes()[0].data());
>   else
>         process4(in.planes()[0].data(), out.planes()[0].data());
>
>   metadata.planes()[0].bytesused = out.planes()[0].size();
>
>   dmaSyncers.clear();
>
> Any idea what could be wrong?
>
> > Fixes: 39482d59fe71 ("DmaBufAllocator: Add Dma Buffer synchronization function & helper class")
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/internal/dma_buf_allocator.h |  5 +++++
> >  src/libcamera/dma_buf_allocator.cpp            | 12 ++++++++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h
> > index fc5de2c13edd..d26f8a74f4c6 100644
> > --- a/include/libcamera/internal/dma_buf_allocator.h
> > +++ b/include/libcamera/internal/dma_buf_allocator.h
> > @@ -60,9 +60,14 @@ public:
> >
> >       explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite);
> >
> > +     DmaSyncer(DmaSyncer &&other) = default;
> > +     DmaSyncer &operator=(DmaSyncer &&other) = default;
> > +
> >       ~DmaSyncer();
> >
> >  private:
> > +     LIBCAMERA_DISABLE_COPY(DmaSyncer)
> > +
> >       void sync(uint64_t step);
> >
> >       SharedFD fd_;
> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> > index 3cc52f9686b0..a014c3b4263c 100644
> > --- a/src/libcamera/dma_buf_allocator.cpp
> > +++ b/src/libcamera/dma_buf_allocator.cpp
> > @@ -311,6 +311,18 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)
> >       sync(DMA_BUF_SYNC_START);
> >  }
> >
> > +/**
> > + * \fn DmaSyncer::DmaSyncer(DmaSyncer &&other);
> > + * \param[in] other The other instance
> > + * \brief Enable move on class DmaSyncer
> > + */
> > +
> > +/**
> > + * \fn DmaSyncer::operator=(DmaSyncer &&other);
> > + * \param[in] other The other instance
> > + * \brief Enable move on class DmaSyncer
> > + */
> > +
> >  DmaSyncer::~DmaSyncer()
> >  {
> >       sync(DMA_BUF_SYNC_END);
>