[PATCH v3 5/6] libcamera: software_isp: Dispatch messages on stop

[Laurent Pinchart]

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 at redhat.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at 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 */

-- 
Regards,

Laurent Pinchart