[libcamera-devel] [PATCH v1 1/2] pipeline: ipa: rpi: Flag if the sensor is a mono variant

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, May 31, 2023 at 03:39:45PM +0100, Naushir Patuck via libcamera-devel wrote:
> Add a flag to the ipa->init() interface to indicate a mono sensor
> variant. This flag will be used in a future commit to handle controls
> that are invalid for mono sensors.

The patch itself seems to do the job, so

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

I'm however wondering if we could do a bit better. The need to identify
the sensor type isn't specific to Raspberry Pi, so a solution that
covers the other IPA modules would be nice. The IPU3 and RkISP1 IPA
interfaces both pass an IPACameraSensorInfo structure to the init()
function. Would it make sense to extend that with a mono flag, and use
it for the Raspberry Pi IPA module too ?

Another improvement would be to pass a CFA type instead of a mono flag,
that would pave the way to support other CFA patterns (I'm thinking
abour RGB+IR for instance). It may be too early to do so though.

Finally, we already pass the sensor model in the IPASettings structure,
which could be used on the IPA side to check what CFA pattern the sensor
uses by adding that information to the camera sensor helpers. Any
opinion about this ?

> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom             | 1 +
>  src/ipa/rpi/common/ipa_base.cpp                     | 1 +
>  src/ipa/rpi/common/ipa_base.h                       | 1 +
>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 +++-
>  4 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index ba786e647ca1..d35289ee6229 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -21,6 +21,7 @@ struct SensorConfig {
>  
>  struct InitParams {
>  	bool lensPresent;
> +	bool monoSensor;
>  };
>  
>  struct InitResult {
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index db7a0eb3a1ca..150fe433d0df 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -139,6 +139,7 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini
>  	}
>  
>  	lensPresent_ = params.lensPresent;
> +	monoSensor_ = params.monoSensor;
>  
>  	controller_.initialise();
>  
> diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
> index 6f9c46bb16b1..39d00760d012 100644
> --- a/src/ipa/rpi/common/ipa_base.h
> +++ b/src/ipa/rpi/common/ipa_base.h
> @@ -87,6 +87,7 @@ private:
>  	std::map<unsigned int, MappedFrameBuffer> buffers_;
>  
>  	bool lensPresent_;
> +	bool monoSensor_;
>  	ControlList libcameraMetadata_;
>  
>  	std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 3bb5ec531e4f..12698056cda1 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -1131,6 +1131,7 @@ int CameraData::loadPipelineConfiguration()
>  int CameraData::loadIPA(ipa::RPi::InitResult *result)
>  {
>  	ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);
> +	bool monoSensor = isMonoSensor(sensor_);
>  
>  	if (!ipa_)
>  		return -ENOENT;
> @@ -1143,7 +1144,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
>  	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE");
>  	if (!configFromEnv || *configFromEnv == '\0') {
>  		std::string model = sensor_->model();
> -		if (isMonoSensor(sensor_))
> +		if (monoSensor)
>  			model += "_mono";
>  		configurationFile = ipa_->configurationFile(model + ".json");
>  	} else {
> @@ -1154,6 +1155,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
>  	ipa::RPi::InitParams params;
>  
>  	params.lensPresent = !!sensor_->focusLens();
> +	params.monoSensor = monoSensor;
>  	int ret = platformInitIpa(params);
>  	if (ret)
>  		return ret;

-- 
Regards,

Laurent Pinchart