[PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable
Milan Zamazal
mzamazal at redhat.com
Mon Jan 13 17:57:04 CET 2025
Cheng-Hao Yang <chenghaoyang at chromium.org> writes:
> Hi Barnabás,
>
> On Mon, Jan 13, 2025 at 5:26 PM Barnabás Pőcze <pobrn at protonmail.com> wrote:
>>
>> Hi
>>
>>
>> 2025. január 13., hétfő 17:16 keltezéssel, Milan Zamazal <mzamazal at redhat.com> írta:
>>
>> > 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.
>>
>> When the vector is resized, it must destroy the objects in the old storage.
>> See the call to `_M_realloc_insert()` in the stack trace. Hence the call to the
>> destructor during `emplace_back()`.
>>
>> And the reason why this did not happen previously is that previously the vector
>> selected the copy constructor for moving the objects between allocation because
>> the move constructor is not `noexcept` (because `SharedFD`'s corresponding methods
>> are not `noexcept` - this should probably be fixed in any case).
>>
>> But since this change it must choose the move constructor because the copy
>> constructor is deleted. And the move constructor leaves `SharedFd::fd_ == nullptr`,
>> so `SharedFD::get()` returns -1.
>>
>> This also means that previously the syncs were probably off due to the destructor calls.
>
> Thanks! That perfectly explains the error log.
> In `DmaSyncer::sync`, it should confirm `fd_.isValid() == true` before
> executing the sync.
>
> Milan, could you try
> https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/7a376b4b2f1fa0dd06a9cea268df5dbb12d357e9?
> I'll upload another patch after the pipeline tests are passed.
Yes, this works fine. Thank you both for help.
> Thanks,
> Harvey
>
>>
>>
>> Regards,
>> Barnabás Pőcze
>>
>>
>> >
>> > > 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);
>> > >>
>> >
More information about the libcamera-devel
mailing list