| Message ID | 20241211084433.3039442-1-chenghaoyang@chromium.org |
|---|---|
| State | Accepted |
| Commit | 545046a41e1767fa601c4e82608053f1912146af |
| Headers | show |
| Series |
|
| Related | show |
Hi, Harvey Yang <chenghaoyang@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: ERROR DmaBufAllocator dma_buf_allocator.cpp:344 Unable to sync dma fd: -1, err: Bad file descriptor, flags: 5 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@chromium.org> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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);
Hi Milan, On Mon, Jan 13, 2025 at 3:44 PM Milan Zamazal <mzamazal@redhat.com> wrote: > > Hi, > > Harvey Yang <chenghaoyang@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@chromium.org> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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); >
Hi Harvey, Cheng-Hao Yang <chenghaoyang@chromium.org> writes: > Hi Milan, > > On Mon, Jan 13, 2025 at 3:44 PM Milan Zamazal <mzamazal@redhat.com> wrote: >> >> Hi, >> >> Harvey Yang <chenghaoyang@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@entry=0xffffe4000bf0, step=step@entry=4) at ../src/libcamera/dma_buf_allocator.cpp:341 #1 0x0000fffff7f08228 in libcamera::DmaSyncer::~DmaSyncer (this=this@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@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@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@chromium.org> >> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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); >>
Hi 2025. január 13., hétfő 17:16 keltezéssel, Milan Zamazal <mzamazal@redhat.com> írta: > Hi Harvey, > > Cheng-Hao Yang <chenghaoyang@chromium.org> writes: > > > Hi Milan, > > > > On Mon, Jan 13, 2025 at 3:44 PM Milan Zamazal <mzamazal@redhat.com> wrote: > >> > >> Hi, > >> > >> Harvey Yang <chenghaoyang@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@entry=0xffffe4000bf0, step=step@entry=4) at ../src/libcamera/dma_buf_allocator.cpp:341 > #1 0x0000fffff7f08228 in libcamera::DmaSyncer::~DmaSyncer (this=this@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@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@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. 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@chromium.org> > >> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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); > >> >
Hi Barnabás, On Mon, Jan 13, 2025 at 5:26 PM Barnabás Pőcze <pobrn@protonmail.com> wrote: > > Hi > > > 2025. január 13., hétfő 17:16 keltezéssel, Milan Zamazal <mzamazal@redhat.com> írta: > > > Hi Harvey, > > > > Cheng-Hao Yang <chenghaoyang@chromium.org> writes: > > > > > Hi Milan, > > > > > > On Mon, Jan 13, 2025 at 3:44 PM Milan Zamazal <mzamazal@redhat.com> wrote: > > >> > > >> Hi, > > >> > > >> Harvey Yang <chenghaoyang@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@entry=0xffffe4000bf0, step=step@entry=4) at ../src/libcamera/dma_buf_allocator.cpp:341 > > #1 0x0000fffff7f08228 in libcamera::DmaSyncer::~DmaSyncer (this=this@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@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@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. 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@chromium.org> > > >> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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); > > >> > >
Cheng-Hao Yang <chenghaoyang@chromium.org> writes: > Hi Barnabás, > > On Mon, Jan 13, 2025 at 5:26 PM Barnabás Pőcze <pobrn@protonmail.com> wrote: >> >> Hi >> >> >> 2025. január 13., hétfő 17:16 keltezéssel, Milan Zamazal <mzamazal@redhat.com> írta: >> >> > Hi Harvey, >> > >> > Cheng-Hao Yang <chenghaoyang@chromium.org> writes: >> > >> > > Hi Milan, >> > > >> > > On Mon, Jan 13, 2025 at 3:44 PM Milan Zamazal <mzamazal@redhat.com> wrote: >> > >> >> > >> Hi, >> > >> >> > >> Harvey Yang <chenghaoyang@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@entry=0xffffe4000bf0, step=step@entry=4) at ../src/libcamera/dma_buf_allocator.cpp:341 >> > #1 0x0000fffff7f08228 in libcamera::DmaSyncer::~DmaSyncer (this=this@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@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@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@chromium.org> >> > >> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> > >> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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); >> > >> >> >
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);
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. Fixes: 39482d59fe71 ("DmaBufAllocator: Add Dma Buffer synchronization function & helper class") Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/dma_buf_allocator.h | 5 +++++ src/libcamera/dma_buf_allocator.cpp | 12 ++++++++++++ 2 files changed, 17 insertions(+)