[0/5] Fix occasional software ISP assertion error on stop
mbox series

Message ID 20250224120854.19747-1-mzamazal@redhat.com
Headers show
Series
  • Fix occasional software ISP assertion error on stop
Related show

Message

Milan Zamazal Feb. 24, 2025, 12:08 p.m. UTC 
When software ISP is stopped, there can be pending messages in the
message queue that will attempt to call the already stopped IPA,
resulting in an assertion error like this:

  FATAL default soft_ipa_proxy.cpp:456 assertion "state_ == ProxyRunning" failed in processStatsThread()

This patch series fixes the problem and also attempts to prevent other
problems related to message handling when or after software ISP is
stopped.

Milan Zamazal (5):
  libcamera: software_isp: Emit ispStatsReady only if IPA is running
  libcamera: software_isp: Handle queued output buffers on stop
  libcamera: software_isp: Handle queued input buffers on stop
  libcamera: base: Fix formatting in thread.cpp
  libcamera: software_isp: Modify dispatching messages on stop

 include/libcamera/base/thread.h               |  3 +-
 .../internal/software_isp/software_isp.h      |  3 ++
 src/libcamera/base/thread.cpp                 | 19 ++++----
 src/libcamera/software_isp/software_isp.cpp   | 46 +++++++++++++++++--
 .../libcamera_templates/proxy_functions.tmpl  |  2 +-
 5 files changed, 58 insertions(+), 15 deletions(-)

Comments

Stanislaw Gruszka Feb. 24, 2025, 12:20 p.m. UTC | #1 
Hi all,

On Mon, Feb 24, 2025 at 01:08:49PM +0100, Milan Zamazal wrote:
> When software ISP is stopped, there can be pending messages in the
> message queue that will attempt to call the already stopped IPA,
> resulting in an assertion error like this:
> 
>   FATAL default soft_ipa_proxy.cpp:456 assertion "state_ == ProxyRunning" failed in processStatsThread()
> 
> This patch series fixes the problem and also attempts to prevent other
> problems related to message handling when or after software ISP is
> stopped.

I can happily confirm this indeed fixes the assertion on qcam stop :-)

For the series:
Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> Milan Zamazal (5):
>   libcamera: software_isp: Emit ispStatsReady only if IPA is running
>   libcamera: software_isp: Handle queued output buffers on stop
>   libcamera: software_isp: Handle queued input buffers on stop
>   libcamera: base: Fix formatting in thread.cpp
>   libcamera: software_isp: Modify dispatching messages on stop
> 
>  include/libcamera/base/thread.h               |  3 +-
>  .../internal/software_isp/software_isp.h      |  3 ++
>  src/libcamera/base/thread.cpp                 | 19 ++++----
>  src/libcamera/software_isp/software_isp.cpp   | 46 +++++++++++++++++--
>  .../libcamera_templates/proxy_functions.tmpl  |  2 +-
>  5 files changed, 58 insertions(+), 15 deletions(-)
> 
> -- 
> 2.48.1
>
Stanislaw Gruszka Feb. 24, 2025, 12:35 p.m. UTC | #2 
On Mon, Feb 24, 2025 at 01:20:46PM +0100, Stanislaw Gruszka wrote:
> Hi all,
> 
> On Mon, Feb 24, 2025 at 01:08:49PM +0100, Milan Zamazal wrote:
> > When software ISP is stopped, there can be pending messages in the
> > message queue that will attempt to call the already stopped IPA,
> > resulting in an assertion error like this:
> > 
> >   FATAL default soft_ipa_proxy.cpp:456 assertion "state_ == ProxyRunning" failed in processStatsThread()
> > 
> > This patch series fixes the problem and also attempts to prevent other
> > problems related to message handling when or after software ISP is
> > stopped.
> 
> I can happily confirm this indeed fixes the assertion on qcam stop :-)
> 
> For the series:
> Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

An update, I hit different one. Happened once and not reproduced
further since then in approx 30 closing qcam tries.

On master + this set, is possible that other patches we recently
discuss address this.

Regards
Stanislaw

[0:05:31.510204042] [3829] FATAL default thread.cpp:597 assertion "!receiver->pendingMessages_" failed in removeMessages()
Backtrace:
libcamera::Thread::removeMessages(libcamera::Object*)+0x2c7 (../src/libcamera/base/thread.cpp:598)
libcamera::Object::~Object()+0x1f8 (../src/libcamera/base/object.cpp:113)
libcamera::ipa::soft::IPAProxySoft::~IPAProxySoft()+0x160 (src/libcamera/proxy/soft_ipa_proxy.cpp:98)
libcamera::ipa::soft::IPAProxySoft::~IPAProxySoft()+0x1c (src/libcamera/proxy/soft_ipa_proxy.cpp:98)
std::default_delete<libcamera::ipa::soft::IPAProxySoft>::operator()(libcamera::ipa::soft::IPAProxySoft*) const+0x2c (/usr/include/c++/13/bits/unique_ptr.h:100)
std::unique_ptr<libcamera::ipa::soft::IPAProxySoft, std::default_delete<libcamera::ipa::soft::IPAProxySoft> >::~unique_ptr()+0x56 (/usr/include/c++/13/bits/unique_ptr.h:405)
libcamera::SoftwareIsp::~SoftwareIsp()+0x6f (../src/libcamera/software_isp/software_isp.cpp:151)
libcamera::SoftwareIsp::~SoftwareIsp()+0x1c (../src/libcamera/software_isp/software_isp.cpp:151)
std::default_delete<libcamera::SoftwareIsp>::operator()(libcamera::SoftwareIsp*) const+0x2c (/usr/include/c++/13/bits/unique_ptr.h:100)
std::unique_ptr<libcamera::SoftwareIsp, std::default_delete<libcamera::SoftwareIsp> >::~unique_ptr()+0x56 (/usr/include/c++/13/bits/unique_ptr.h:405)
libcamera::SimpleCameraData::~SimpleCameraData()+0x34 (../src/libcamera/pipeline/simple/simple.cpp:214)
libcamera::SimpleCameraData::~SimpleCameraData()+0x1c (../src/libcamera/pipeline/simple/simple.cpp:214)
std::default_delete<libcamera::Extensible::Private>::operator()(libcamera::Extensible::Private*) const+0x2c (/usr/include/c++/13/bits/unique_ptr.h:100)
std::unique_ptr<libcamera::Extensible::Private, std::default_delete<libcamera::Extensible::Private> >::~unique_ptr()+0x56 (/usr/include/c++/13/bits/unique_ptr.h:405)
libcamera::Extensible::~Extensible()+0x1c (../include/libcamera/base/class.h:61)
libcamera::Camera::~Camera()+0x64 (../src/libcamera/camera.cpp:929)
libcamera::Camera::~Camera()+0x1c (../src/libcamera/camera.cpp:929)
libcamera::Camera::create(std::unique_ptr<libcamera::Camera::Private, std::default_delete<libcamera::Camera::Private> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::set<libcamera::Stream*, std::less<libcamera::Stream*>, std::allocator<libcamera::Stream*> > const&)::Deleter::operator()(libcamera::Camera*)+0x40 (../src/libcamera/camera.cpp:862)
std::_Sp_counted_deleter<libcamera::Camera*, libcamera::Camera::create(std::unique_ptr<libcamera::Camera::Private, std::default_delete<libcamera::Camera::Private> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::set<libcamera::Stream*, std::less<libcamera::Stream*>, std::allocator<libcamera::Stream*> > const&)::Deleter, std::allocator<void>, (__gnu_cxx::_Lock_policy)2>::_M_dispose()+0x36 (/usr/include/c++/13/bits/shared_ptr_base.h:527)
std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release_last_use()+0x27 (/usr/include/c++/13/bits/shared_ptr_base.h:187)
std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release_last_use_cold()+0x1c (/usr/include/c++/13/bits/shared_ptr_base.h:199)
std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release()+0x130 (/usr/include/c++/13/bits/shared_ptr_base.h:354)
std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count()+0x2b (/usr/include/c++/13/bits/shared_ptr_base.h:1072)
std::__shared_ptr<libcamera::Camera, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr()+0x20 (/usr/include/c++/13/bits/shared_ptr_base.h:1524)
std::shared_ptr<libcamera::Camera>::~shared_ptr()+0x1c (/usr/include/c++/13/bits/shared_ptr.h:175)
void std::_Destroy<std::shared_ptr<libcamera::Camera> >(std::shared_ptr<libcamera::Camera>*)+0x1c (/usr/include/c++/13/bits/stl_construct.h:153)
void std::_Destroy_aux<false>::__destroy<std::shared_ptr<libcamera::Camera>*>(std::shared_ptr<libcamera::Camera>*, std::shared_ptr<libcamera::Camera>*)+0x2a (/usr/include/c++/13/bits/stl_construct.h:162)
void std::_Destroy<std::shared_ptr<libcamera::Camera>*>(std::shared_ptr<libcamera::Camera>*, std::shared_ptr<libcamera::Camera>*)+0x27 (/usr/include/c++/13/bits/stl_construct.h:197)
std::vector<std::shared_ptr<libcamera::Camera>, std::allocator<std::shared_ptr<libcamera::Camera> > >::_M_erase_at_end(std::shared_ptr<libcamera::Camera>*)+0x66 (/usr/include/c++/13/bits/alloc_traits.h:949)
std::vector<std::shared_ptr<libcamera::Camera>, std::allocator<std::shared_ptr<libcamera::Camera> > >::clear()+0x26 (/usr/include/c++/13/bits/stl_vector.h:1606)
libcamera::CameraManager::Private::cleanup()+0x6b (../src/libcamera/camera_manager.cpp:181)
libcamera::CameraManager::Private::run()+0x130 (../src/libcamera/camera_manager.cpp:92)
libcamera::Thread::startThread()+0xf2 (../src/libcamera/base/thread.cpp:289)
void std::__invoke_impl<void, void (libcamera::Thread::*)(), libcamera::Thread*>(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(), libcamera::Thread*&&)+0x6a (/usr/include/c++/13/bits/invoke.h:74)
std::__invoke_result<void (libcamera::Thread::*)(), libcamera::Thread*>::type std::__invoke<void (libcamera::Thread::*)(), libcamera::Thread*>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&)+0x3b (/usr/include/c++/13/bits/invoke.h:97)
void std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>)+0x47 (/usr/include/c++/13/bits/std_thread.h:292)
std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::operator()()+0x1c (/usr/include/c++/13/bits/std_thread.h:299)
std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> > >::_M_run()+0x20 (/usr/include/c++/13/bits/std_thread.h:244)
execute_native_thread_routine+0x14 (/build/gcc-14-ig5ci0/gcc-14-14.2.0/build/x86_64-linux-gnu/libstdc++-v3/include/bits/unique_ptr.h:93)
start_thread+0x384 (./nptl/pthread_create.c:447)
__clone3+0x2c (../sysdeps/unix/sysv/linux/x86_64/clone3.S:80)
Milan Zamazal Feb. 24, 2025, 3:36 p.m. UTC | #3 
Hi Stanislaw,

Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> writes:

> On Mon, Feb 24, 2025 at 01:20:46PM +0100, Stanislaw Gruszka wrote:
>> Hi all,
>> 
>> On Mon, Feb 24, 2025 at 01:08:49PM +0100, Milan Zamazal wrote:
>> > When software ISP is stopped, there can be pending messages in the
>> > message queue that will attempt to call the already stopped IPA,
>> > resulting in an assertion error like this:
>> > 
>> >   FATAL default soft_ipa_proxy.cpp:456 assertion "state_ == ProxyRunning" failed in
>> > processStatsThread()
>> > 
>> > This patch series fixes the problem and also attempts to prevent other
>> > problems related to message handling when or after software ISP is
>> > stopped.
>> 
>> I can happily confirm this indeed fixes the assertion on qcam stop :-)

Thank you for testing.

>> For the series:
>> Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
>
> An update, I hit different one. 

This is a bug in the last patch of the series.  I'll post v2 with this
fixed.

> Happened once and not reproduced further since then in approx 30
> closing qcam tries.
>
> On master + this set, is possible that other patches we recently
> discuss address this.
>
> Regards
> Stanislaw
>
> [0:05:31.510204042] [3829] FATAL default thread.cpp:597 assertion "!receiver->pendingMessages_" failed in
> removeMessages()
> Backtrace:
> libcamera::Thread::removeMessages(libcamera::Object*)+0x2c7 (../src/libcamera/base/thread.cpp:598)
> libcamera::Object::~Object()+0x1f8 (../src/libcamera/base/object.cpp:113)
> libcamera::ipa::soft::IPAProxySoft::~IPAProxySoft()+0x160 (src/libcamera/proxy/soft_ipa_proxy.cpp:98)
> libcamera::ipa::soft::IPAProxySoft::~IPAProxySoft()+0x1c (src/libcamera/proxy/soft_ipa_proxy.cpp:98)
> std::default_delete<libcamera::ipa::soft::IPAProxySoft>::operator()(libcamera::ipa::soft::IPAProxySoft*)
> const+0x2c (/usr/include/c++/13/bits/unique_ptr.h:100)
> std::unique_ptr<libcamera::ipa::soft::IPAProxySoft,
> std::default_delete<libcamera::ipa::soft::IPAProxySoft> >::~unique_ptr()+0x56
> (/usr/include/c++/13/bits/unique_ptr.h:405)
> libcamera::SoftwareIsp::~SoftwareIsp()+0x6f (../src/libcamera/software_isp/software_isp.cpp:151)
> libcamera::SoftwareIsp::~SoftwareIsp()+0x1c (../src/libcamera/software_isp/software_isp.cpp:151)
> std::default_delete<libcamera::SoftwareIsp>::operator()(libcamera::SoftwareIsp*) const+0x2c
> (/usr/include/c++/13/bits/unique_ptr.h:100)
> std::unique_ptr<libcamera::SoftwareIsp, std::default_delete<libcamera::SoftwareIsp> >::~unique_ptr()+0x56
> (/usr/include/c++/13/bits/unique_ptr.h:405)
> libcamera::SimpleCameraData::~SimpleCameraData()+0x34 (../src/libcamera/pipeline/simple/simple.cpp:214)
> libcamera::SimpleCameraData::~SimpleCameraData()+0x1c (../src/libcamera/pipeline/simple/simple.cpp:214)
> std::default_delete<libcamera::Extensible::Private>::operator()(libcamera::Extensible::Private*)
> const+0x2c (/usr/include/c++/13/bits/unique_ptr.h:100)
> std::unique_ptr<libcamera::Extensible::Private, std::default_delete<libcamera::Extensible::Private>
>>::~unique_ptr()+0x56 (/usr/include/c++/13/bits/unique_ptr.h:405)
> libcamera::Extensible::~Extensible()+0x1c (../include/libcamera/base/class.h:61)
> libcamera::Camera::~Camera()+0x64 (../src/libcamera/camera.cpp:929)
> libcamera::Camera::~Camera()+0x1c (../src/libcamera/camera.cpp:929)
> libcamera::Camera::create(std::unique_ptr<libcamera::Camera::Private,
> std::default_delete<libcamera::Camera::Private> >, std::__cxx11::basic_string<char,
> std::char_traits<char>, std::allocator<char> > const&, std::set<libcamera::Stream*,
> std::less<libcamera::Stream*>, std::allocator<libcamera::Stream*> >
> const&)::Deleter::operator()(libcamera::Camera*)+0x40 (../src/libcamera/camera.cpp:862)
> std::_Sp_counted_deleter<libcamera::Camera*,
> libcamera::Camera::create(std::unique_ptr<libcamera::Camera::Private,
> std::default_delete<libcamera::Camera::Private> >, std::__cxx11::basic_string<char,
> std::char_traits<char>, std::allocator<char> > const&, std::set<libcamera::Stream*,
> std::less<libcamera::Stream*>, std::allocator<libcamera::Stream*> > const&)::Deleter,
> std::allocator<void>, (__gnu_cxx::_Lock_policy)2>::_M_dispose()+0x36
> (/usr/include/c++/13/bits/shared_ptr_base.h:527)
> std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release_last_use()+0x27
> (/usr/include/c++/13/bits/shared_ptr_base.h:187)
> std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release_last_use_cold()+0x1c
> (/usr/include/c++/13/bits/shared_ptr_base.h:199)
> std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release()+0x130
> (/usr/include/c++/13/bits/shared_ptr_base.h:354)
> std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count()+0x2b
> (/usr/include/c++/13/bits/shared_ptr_base.h:1072)
> std::__shared_ptr<libcamera::Camera, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr()+0x20
> (/usr/include/c++/13/bits/shared_ptr_base.h:1524)
> std::shared_ptr<libcamera::Camera>::~shared_ptr()+0x1c (/usr/include/c++/13/bits/shared_ptr.h:175)
> void std::_Destroy<std::shared_ptr<libcamera::Camera> >(std::shared_ptr<libcamera::Camera>*)+0x1c
> (/usr/include/c++/13/bits/stl_construct.h:153)
> void
> std::_Destroy_aux<false>::__destroy<std::shared_ptr<libcamera::Camera>*>(std::shared_ptr<libcamera::Camera>*,
> std::shared_ptr<libcamera::Camera>*)+0x2a (/usr/include/c++/13/bits/stl_construct.h:162)
> void std::_Destroy<std::shared_ptr<libcamera::Camera>*>(std::shared_ptr<libcamera::Camera>*,
> std::shared_ptr<libcamera::Camera>*)+0x27 (/usr/include/c++/13/bits/stl_construct.h:197)
> std::vector<std::shared_ptr<libcamera::Camera>, std::allocator<std::shared_ptr<libcamera::Camera> >
>>::_M_erase_at_end(std::shared_ptr<libcamera::Camera>*)+0x66
> (/usr/include/c++/13/bits/alloc_traits.h:949)
> std::vector<std::shared_ptr<libcamera::Camera>, std::allocator<std::shared_ptr<libcamera::Camera> >
>>::clear()+0x26 (/usr/include/c++/13/bits/stl_vector.h:1606)
> libcamera::CameraManager::Private::cleanup()+0x6b (../src/libcamera/camera_manager.cpp:181)
> libcamera::CameraManager::Private::run()+0x130 (../src/libcamera/camera_manager.cpp:92)
> libcamera::Thread::startThread()+0xf2 (../src/libcamera/base/thread.cpp:289)
> void std::__invoke_impl<void, void (libcamera::Thread::*)(),
> libcamera::Thread*>(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(),
> libcamera::Thread*&&)+0x6a (/usr/include/c++/13/bits/invoke.h:74)
> std::__invoke_result<void (libcamera::Thread::*)(), libcamera::Thread*>::type std::__invoke<void
> (libcamera::Thread::*)(), libcamera::Thread*>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&)+0x3b
> (/usr/include/c++/13/bits/invoke.h:97)
> void std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*>
>>::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>)+0x47 (/usr/include/c++/13/bits/std_thread.h:292)
> std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::operator()()+0x1c
> (/usr/include/c++/13/bits/std_thread.h:299)
> std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(),
> libcamera::Thread*> > >::_M_run()+0x20 (/usr/include/c++/13/bits/std_thread.h:244)
> execute_native_thread_routine+0x14
> (/build/gcc-14-ig5ci0/gcc-14-14.2.0/build/x86_64-linux-gnu/libstdc++-v3/include/bits/unique_ptr.h:93)
> start_thread+0x384 (./nptl/pthread_create.c:447)
> __clone3+0x2c (../sysdeps/unix/sysv/linux/x86_64/clone3.S:80)