[libcamera-devel] [PATCH] libcamera: ipa_proxy: Allow stop() on a stopped IPA

[Jacopo Mondi]

Hi Laurent,
   just a small nit

On Mon, Jul 06, 2020 at 02:08:47PM +0300, Laurent Pinchart wrote:
> To make error handling easier in callers, allow the stop() function to
> be called when the proxy is already stopped, or not started yet.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/internal/ipa_proxy.h   |  2 ++
>  src/libcamera/ipa_proxy.cpp              | 10 ++++++++++
>  src/libcamera/proxy/ipa_proxy_thread.cpp |  3 +++
>  3 files changed, 15 insertions(+)
>
> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
> index aec8f04ffc15..b429ce5a68a3 100644
> --- a/include/libcamera/internal/ipa_proxy.h
> +++ b/include/libcamera/internal/ipa_proxy.h
> @@ -27,6 +27,8 @@ public:
>
>  	std::string configurationFile(const std::string &file) const;
>
> +	void stop() override = 0;
> +
>  protected:
>  	std::string resolvePath(const std::string &file) const;
>
> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> index 23be24ad9bf1..01740a6e39ec 100644
> --- a/src/libcamera/ipa_proxy.cpp
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -145,6 +145,16 @@ std::string IPAProxy::configurationFile(const std::string &name) const
>  	return std::string();
>  }
>
> +/**
> + * \fn IPAProxy::stop()
> + * \brief Stop the IPA proxy
> + *
> + * This method informs the IPA module that the camera is stopped. The IPA module
> + * shall release resources prepared in start(). Calling stop() when the IPA

s/prepared/acquired ?

> + * hasn't been started or has already been stopped is valid, and the IPA shall
> + * treat this as a no-op.
> + */
> +
>  /**
>   * \brief Find a valid full path for a proxy worker for a given executable name
>   * \param[in] file File name of proxy worker executable
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index aa403e22f297..eead2883708d 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -121,6 +121,9 @@ int IPAProxyThread::start()
>
>  void IPAProxyThread::stop()
>  {
> +	if (!running_)
> +		return;
> +

In case the IPA is then stopped already, IPA won't receive the call,
making the above "hasn't been started or has already been stopped is
valid" possibly confusing.

With out without these addressed
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>  	running_ = false;
>
>  	proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel