[v1] libcamera: pipeline: simple: Rework software-isp/converter selection

28156 diff mbox series

Commit a19f72569558 ("libcamera: simple: Set the number of software ISP streams to 2")
introduced an assertion to ensure that no converter is present when using the
software-isp. However, it was not noticed that the earlier commit 4a5ebcf675a4
("libcamera: simple: Enable SoftISP for imx7-csi") enabled software-isp for
"imx7-csi", and since that driver also has an associated converter, and if it
is found, the assertion will abort.

To resolve the above situation, apply the following rules when deciding
whether to use the software-isp or one of the available converters:

  1. if the configuration file selects `software_isp: true`, then use that;
  2. if at least one converter is available, use the first available one;
  3. if the configuration file does not disable the software-isp
     and the static configuration allows it, use that;
  4. otherwise, continue without either

Link: https://patchwork.libcamera.org/patch/27955/
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 41 ++++++++++++++++--------
 1 file changed, 28 insertions(+), 13 deletions(-)

Hi Barnabás,

thank you for the patch (for whatever reason I can't see it in
patchwork).

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> Commit a19f72569558 ("libcamera: simple: Set the number of software ISP streams to 2")
> introduced an assertion to ensure that no converter is present when using the
> software-isp. However, it was not noticed that the earlier commit 4a5ebcf675a4
> ("libcamera: simple: Enable SoftISP for imx7-csi") enabled software-isp for
> "imx7-csi", and since that driver also has an associated converter, and if it
> is found, the assertion will abort.
>
> To resolve the above situation, apply the following rules when deciding
> whether to use the software-isp or one of the available converters:
>
>   1. if the configuration file selects `software_isp: true`, then use that;
>   2. if at least one converter is available, use the first available one;
>   3. if the configuration file does not disable the software-isp
>      and the static configuration allows it, use that;
>   4. otherwise, continue without either
>
> Link: https://patchwork.libcamera.org/patch/27955/
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 41 ++++++++++++++++--------
>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index ca98e74790..991f542c5d 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -250,7 +250,7 @@ struct SimplePipelineInfo {
>  	 *
>  	 * The Software ISP can't be used together with the converters.
>  	 */
> -	bool swIspEnabled;
> +	bool swIspAllowed;
>  };
>  
>  namespace {
> @@ -459,7 +459,7 @@ private:
>  	std::map<const MediaEntity *, EntityData> entities_;
>  
>  	std::shared_ptr<MediaDevice> converter_;
> -	bool swIspEnabled_;
> +	bool swIspEnabled_ = false;
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -1869,40 +1869,55 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
>  					const SimplePipelineInfo &info,
>  					DeviceEnumerator *enumerator)
>  {
> -	unsigned int numStreams = 1;
> -
> +	struct {
> +		std::shared_ptr<MediaDevice> dev;
> +		unsigned int streams;
> +	} converter = {};
>  	for (const auto &[name, streams] : info.converters) {
>  		DeviceMatch converterMatch(name);
> -		converter_ = acquireMediaDevice(enumerator, converterMatch);
> -		if (converter_) {
> -			numStreams = streams;
> +		converter.dev = acquireMediaDevice(enumerator, converterMatch);
> +		if (converter.dev) {
> +			converter.streams = streams;
>  			break;
>  		}
>  	}
>  
> -	swIspEnabled_ = info.swIspEnabled;
> +	std::optional<bool> swIspEnabled;
>  	const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();
>  	for (const ValueNode &entry :
>  	     configuration.configuration()["pipelines"]["simple"]["supported_devices"]
>  		     .asList()) {
>  		auto name = entry["driver"].get<std::string>();
> -		if (name == info.driver) {
> -			swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_);
> +		if (name != info.driver)
> +			continue;
> +
> +		swIspEnabled = entry["software_isp"].get<bool>();
> +		if (swIspEnabled) {
>  			LOG(SimplePipeline, Debug)
>  				<< "Configuration file overrides software ISP for "
> -				<< info.driver << " to " << swIspEnabled_;
> +				<< info.driver << " to " << *swIspEnabled;
>  			break;
>  		}
>  	}
>  
> -	if (swIspEnabled_) {
> +	const bool useSwIsp =
> +		(swIspEnabled && *swIspEnabled) || /* forced by configuration */
> +		(info.swIspAllowed && /* allowed by static configuration */
> +		 !converter.dev && /* and there is no converter */
> +		 !(swIspEnabled && !*swIspEnabled)); /* and not disabled by configuration */

I wonder whether the last condition could be written in a less scary
way, e.g.

  swIspEnabled.value_or(true)

or even (not recommended but possible due to the first condition)

  !swIspEnabled

Up to you, either way:

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> +
> +	unsigned int numStreams = 1; /* Only 1 "raw" stream by default. */
> +	if (useSwIsp) {
>  		/*
>  		 * When the software ISP is enabled, the simple pipeline handler
>  		 * exposes the raw stream, giving a total of two streams. This
>  		 * is mutually exclusive with the presence of a converter.
>  		 */
> -		ASSERT(!converter_);
>  		numStreams = 2;
> +		swIspEnabled_ = true;
> +	} else if (converter.dev) {
> +		converter_ = std::move(converter.dev);
> +		numStreams = converter.streams;
>  	}
>  
>  	/* Locate the sensors. */