[PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable

[Milan Zamazal]

Hi Harvey,

Cheng-Hao Yang <chenghaoyang at chromium.org> writes:

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

No tests, running `cam' with software ISP.

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

Yes, they are never -1.

The problem occurs in the following line from the snippet below:

   dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);

It processes the input plane fine and calls DmaSyncer::sync with the
right fd, then it process the output plane and it first calls
DmaSyncer::sync with the right fd, followed by a call with fd -1.
Here is the backtrace of the corresponding call (note the
line numbers are a bit off due to debugging prints I inserted):

  #0  0x0000fffff7f0810c in libcamera::DmaSyncer::sync (this=this at entry=0xffffe4000bf0, step=step at entry=4) at ../src/libcamera/dma_buf_allocator.cpp:341
  #1  0x0000fffff7f08228 in libcamera::DmaSyncer::~DmaSyncer (this=this at entry=0xffffe4000bf0, __in_chrg=<optimized out>) at ../src/libcamera/dma_buf_allocator.cpp:329
  #2  0x0000fffff7f68fe4 in std::_Destroy<libcamera::DmaSyncer> (__pointer=0xffffe4000bf0) at /usr/include/c++/11/bits/stl_construct.h:151
  #3  std::_Destroy_aux<false>::__destroy<libcamera::DmaSyncer*> (__last=<optimized out>, __first=0xffffe4000bf0) at /usr/include/c++/11/bits/stl_construct.h:163
  #4  std::_Destroy<libcamera::DmaSyncer*> (__last=<optimized out>, __first=<optimized out>) at /usr/include/c++/11/bits/stl_construct.h:196
  #5  std::_Destroy<libcamera::DmaSyncer*, libcamera::DmaSyncer> (__last=0xffffe4000c08, __first=0xffffe4000bf0) at /usr/include/c++/11/bits/alloc_traits.h:848
  #6  std::vector<libcamera::DmaSyncer, std::allocator<libcamera::DmaSyncer> >::_M_realloc_insert<libcamera::SharedFD const&, libcamera::DmaSyncer::SyncType> (this=0xffffefe0d738, __position=..., 
      __position at entry=...) at /usr/include/c++/11/bits/vector.tcc:498
  #7  0x0000fffff7f691b4 in std::vector<libcamera::DmaSyncer, std::allocator<libcamera::DmaSyncer> >::emplace_back<libcamera::SharedFD const&, libcamera::DmaSyncer::SyncType> (this=this at entry=0xffffefe0d738)
      at /usr/include/c++/11/bits/vector.tcc:121
  #8  0x0000fffff7f66d38 in libcamera::DebayerCpu::process (this=0xfffff001e8d0, frame=0, input=0xfffff0020270, output=0xfffff0018f70, params=...) at ../src/libcamera/software_isp/debayer_cpu.cpp:752

This means DmaSyncer::sync is called twice within emplace_back, the
second time from the destructor with the invalid fd.  This is weird,
what's the destroyed instance and why does it happen only with the
output plane and not the input plane?  dmaSyncers is a local variable
not used anywhere else, I suspect the vector gets resized and some
unwanted actions on the instances happen during the operation.

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