[v3,5/6] libcamera: software_isp: Dispatch messages on stop
diff mbox series

Message ID 20250225150614.20195-6-mzamazal@redhat.com
State Accepted
Headers show
Series
  • Fix occasional software ISP assertion error on stop
Related show

Commit Message

Milan Zamazal Feb. 25, 2025, 3:06 p.m. UTC 
There may be pending messages in SoftwareIsp message queue when
SoftwareIsp stops.  The call to IPAProxySoft::stop() will dispatch them
before SoftwareIsp::stop() finishes.  But this is dependent on
IPAProxySoft::stop() implementation, let's break this dependency and
dispatch messages to SoftwareIsp explicitly in SoftwareIsp::stop().

This also allows dropping `running_' flag.  Since the SoftwareIsp
messages get processed and invoke IPA calls before the IPA proxy is set
to ProxyStopping state and the SoftwareIsp worker thread is no longer
running, it's guaranteed that no new messages come to SoftwareIsp and
attempt to call the stopped IPA proxy.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../internal/software_isp/software_isp.h      |  1 -
 src/libcamera/software_isp/software_isp.cpp   | 24 ++++++++-----------
 2 files changed, 10 insertions(+), 15 deletions(-)

Comments

Laurent Pinchart Feb. 26, 2025, 12:37 a.m. UTC | #1 
Hi Milan,

Thank you for the patch.

On Tue, Feb 25, 2025 at 04:06:11PM +0100, Milan Zamazal wrote:
> There may be pending messages in SoftwareIsp message queue when
> SoftwareIsp stops.  The call to IPAProxySoft::stop() will dispatch them
> before SoftwareIsp::stop() finishes.  But this is dependent on
> IPAProxySoft::stop() implementation, let's break this dependency and
> dispatch messages to SoftwareIsp explicitly in SoftwareIsp::stop().
> 
> This also allows dropping `running_' flag.  Since the SoftwareIsp
> messages get processed and invoke IPA calls before the IPA proxy is set
> to ProxyStopping state and the SoftwareIsp worker thread is no longer
> running, it's guaranteed that no new messages come to SoftwareIsp and
> attempt to call the stopped IPA proxy.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../internal/software_isp/software_isp.h      |  1 -
>  src/libcamera/software_isp/software_isp.cpp   | 24 ++++++++-----------
>  2 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index 400a4dc5..133b545c 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -101,7 +101,6 @@ private:
>  	DmaBufAllocator dmaHeap_;
>  
>  	std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
> -	bool running_;
>  	std::deque<FrameBuffer *> queuedInputBuffers_;
>  	std::deque<FrameBuffer *> queuedOutputBuffers_;
>  };
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index beac66fc..193713b9 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -14,6 +14,7 @@
>  #include <unistd.h>
>  
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/thread.h>
>  
>  #include <libcamera/controls.h>
>  #include <libcamera/formats.h>
> @@ -323,7 +324,6 @@ int SoftwareIsp::start()
>  	int ret = ipa_->start();
>  	if (ret)
>  		return ret;
> -	running_ = true;
>  
>  	ispWorkerThread_.start();
>  	return 0;
> @@ -340,7 +340,8 @@ void SoftwareIsp::stop()
>  	ispWorkerThread_.exit();
>  	ispWorkerThread_.wait();
>  
> -	running_ = false;
> +	Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);
> +
>  	ipa_->stop();
>  
>  	for (auto buffer : queuedOutputBuffers_) {
> @@ -383,26 +384,21 @@ void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)
>  
>  void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)
>  {
> -	if (running_)
> -		ispStatsReady.emit(frame, bufferId);
> +	ispStatsReady.emit(frame, bufferId);
>  }
>  
>  void SoftwareIsp::inputReady(FrameBuffer *input)
>  {
> -	if (running_) {
> -		ASSERT(queuedInputBuffers_.front() == input);
> -		queuedInputBuffers_.pop_front();
> -		inputBufferReady.emit(input);
> -	}
> +	ASSERT(queuedInputBuffers_.front() == input);
> +	queuedInputBuffers_.pop_front();
> +	inputBufferReady.emit(input);
>  }
>  
>  void SoftwareIsp::outputReady(FrameBuffer *output)
>  {
> -	if (running_) {
> -		ASSERT(queuedOutputBuffers_.front() == output);
> -		queuedOutputBuffers_.pop_front();
> -		outputBufferReady.emit(output);
> -	}
> +	ASSERT(queuedOutputBuffers_.front() == output);
> +	queuedOutputBuffers_.pop_front();
> +	outputBufferReady.emit(output);
>  }
>  
>  } /* namespace libcamera */
Kieran Bingham March 1, 2025, 11:05 p.m. UTC | #2 
Quoting Milan Zamazal (2025-02-25 15:06:11)
> There may be pending messages in SoftwareIsp message queue when
> SoftwareIsp stops.  The call to IPAProxySoft::stop() will dispatch them
> before SoftwareIsp::stop() finishes.  But this is dependent on
> IPAProxySoft::stop() implementation, let's break this dependency and
> dispatch messages to SoftwareIsp explicitly in SoftwareIsp::stop().
> 
> This also allows dropping `running_' flag.  Since the SoftwareIsp
> messages get processed and invoke IPA calls before the IPA proxy is set
> to ProxyStopping state and the SoftwareIsp worker thread is no longer
> running, it's guaranteed that no new messages come to SoftwareIsp and
> attempt to call the stopped IPA proxy.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  .../internal/software_isp/software_isp.h      |  1 -
>  src/libcamera/software_isp/software_isp.cpp   | 24 ++++++++-----------
>  2 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index 400a4dc5..133b545c 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -101,7 +101,6 @@ private:
>         DmaBufAllocator dmaHeap_;
>  
>         std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
> -       bool running_;
>         std::deque<FrameBuffer *> queuedInputBuffers_;
>         std::deque<FrameBuffer *> queuedOutputBuffers_;
>  };
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index beac66fc..193713b9 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -14,6 +14,7 @@
>  #include <unistd.h>
>  
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/thread.h>
>  
>  #include <libcamera/controls.h>
>  #include <libcamera/formats.h>
> @@ -323,7 +324,6 @@ int SoftwareIsp::start()
>         int ret = ipa_->start();
>         if (ret)
>                 return ret;
> -       running_ = true;
>  
>         ispWorkerThread_.start();
>         return 0;
> @@ -340,7 +340,8 @@ void SoftwareIsp::stop()
>         ispWorkerThread_.exit();
>         ispWorkerThread_.wait();
>  
> -       running_ = false;
> +       Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);
> +
>         ipa_->stop();
>  
>         for (auto buffer : queuedOutputBuffers_) {
> @@ -383,26 +384,21 @@ void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)
>  
>  void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)
>  {
> -       if (running_)
> -               ispStatsReady.emit(frame, bufferId);
> +       ispStatsReady.emit(frame, bufferId);
>  }
>  
>  void SoftwareIsp::inputReady(FrameBuffer *input)
>  {
> -       if (running_) {
> -               ASSERT(queuedInputBuffers_.front() == input);
> -               queuedInputBuffers_.pop_front();
> -               inputBufferReady.emit(input);
> -       }
> +       ASSERT(queuedInputBuffers_.front() == input);
> +       queuedInputBuffers_.pop_front();
> +       inputBufferReady.emit(input);
>  }
>  
>  void SoftwareIsp::outputReady(FrameBuffer *output)
>  {
> -       if (running_) {
> -               ASSERT(queuedOutputBuffers_.front() == output);
> -               queuedOutputBuffers_.pop_front();
> -               outputBufferReady.emit(output);
> -       }
> +       ASSERT(queuedOutputBuffers_.front() == output);
> +       queuedOutputBuffers_.pop_front();
> +       outputBufferReady.emit(output);
>  }
>  
>  } /* namespace libcamera */
> -- 
> 2.48.1
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
index 400a4dc5..133b545c 100644
--- a/include/libcamera/internal/software_isp/software_isp.h
+++ b/include/libcamera/internal/software_isp/software_isp.h
@@ -101,7 +101,6 @@  private:
 	DmaBufAllocator dmaHeap_;
 
 	std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
-	bool running_;
 	std::deque<FrameBuffer *> queuedInputBuffers_;
 	std::deque<FrameBuffer *> queuedOutputBuffers_;
 };
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
index beac66fc..193713b9 100644
--- a/src/libcamera/software_isp/software_isp.cpp
+++ b/src/libcamera/software_isp/software_isp.cpp
@@ -14,6 +14,7 @@ 
 #include <unistd.h>
 
 #include <libcamera/base/log.h>
+#include <libcamera/base/thread.h>
 
 #include <libcamera/controls.h>
 #include <libcamera/formats.h>
@@ -323,7 +324,6 @@  int SoftwareIsp::start()
 	int ret = ipa_->start();
 	if (ret)
 		return ret;
-	running_ = true;
 
 	ispWorkerThread_.start();
 	return 0;
@@ -340,7 +340,8 @@  void SoftwareIsp::stop()
 	ispWorkerThread_.exit();
 	ispWorkerThread_.wait();
 
-	running_ = false;
+	Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);
+
 	ipa_->stop();
 
 	for (auto buffer : queuedOutputBuffers_) {
@@ -383,26 +384,21 @@  void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)
 
 void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)
 {
-	if (running_)
-		ispStatsReady.emit(frame, bufferId);
+	ispStatsReady.emit(frame, bufferId);
 }
 
 void SoftwareIsp::inputReady(FrameBuffer *input)
 {
-	if (running_) {
-		ASSERT(queuedInputBuffers_.front() == input);
-		queuedInputBuffers_.pop_front();
-		inputBufferReady.emit(input);
-	}
+	ASSERT(queuedInputBuffers_.front() == input);
+	queuedInputBuffers_.pop_front();
+	inputBufferReady.emit(input);
 }
 
 void SoftwareIsp::outputReady(FrameBuffer *output)
 {
-	if (running_) {
-		ASSERT(queuedOutputBuffers_.front() == output);
-		queuedOutputBuffers_.pop_front();
-		outputBufferReady.emit(output);
-	}
+	ASSERT(queuedOutputBuffers_.front() == output);
+	queuedOutputBuffers_.pop_front();
+	outputBufferReady.emit(output);
 }
 
 } /* namespace libcamera */