[libcamera-devel,v2] libcamera: pipeline: rkisp1: configure IPA from configure method instead of start method

11293 diff mbox series

From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Currently the call to the configure method of rkisp1 IPA
is called upon the 'start' of the pipeline. This should be done
in the 'configure' method instead.

[Original patch]
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[v2]
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Hi Dafna,

The IPA IPC series is ready to be merged but depends on this patch, so
I've taken the liberty to send a v2 to speed things up.

Changes in v2:
- remove path isEnabled() checks from configure()
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 62 ++++++++++++------------
 1 file changed, 30 insertions(+), 32 deletions(-)

On 15.02.21 10:38, Paul Elder wrote:
> From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> 
> Currently the call to the configure method of rkisp1 IPA
> is called upon the 'start' of the pipeline. This should be done
> in the 'configure' method instead.
> 
> [Original patch]
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> [v2]
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Hi Dafna,
> 
> The IPA IPC series is ready to be merged but depends on this patch, so
> I've taken the liberty to send a v2 to speed things up.

sure, thanks,

Dafna

> 
> Changes in v2:
> - remove path isEnabled() checks from configure()
> ---
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 62 ++++++++++++------------
>   1 file changed, 30 insertions(+), 32 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index e85979a7..7cb89eb0 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -607,11 +607,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>   		<< "ISP output pad configured with " << format.toString()
>   		<< " crop " << rect.toString();
>   
> +	std::map<unsigned int, IPAStream> streamConfig;
> +
>   	for (const StreamConfiguration &cfg : *config) {
> -		if (cfg.stream() == &data->mainPathStream_)
> +		if (cfg.stream() == &data->mainPathStream_) {
>   			ret = mainPath_.configure(cfg, format);
> -		else
> +			streamConfig[0] = {
> +				.pixelFormat = cfg.pixelFormat,
> +				.size = cfg.size,
> +			};
> +		} else {
>   			ret = selfPath_.configure(cfg, format);
> +			streamConfig[1] = {
> +				.pixelFormat = cfg.pixelFormat,
> +				.size = cfg.size,
> +			};
> +		}
>   
>   		if (ret)
>   			return ret;
> @@ -629,6 +640,23 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>   	if (ret)
>   		return ret;
>   
> +	/* Inform IPA of stream configuration and sensor controls. */
> +	CameraSensorInfo sensorInfo = {};
> +	ret = data->sensor_->sensorInfo(&sensorInfo);
> +	if (ret) {
> +		/* \todo Turn this into a hard failure. */
> +		LOG(RkISP1, Warning) << "Camera sensor information not available";
> +		sensorInfo = {};
> +		ret = 0;
> +	}
> +
> +	std::map<unsigned int, const ControlInfoMap &> entityControls;
> +	entityControls.emplace(0, data->sensor_->controls());
> +
> +	IPAOperationData ipaConfig;
> +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> +			      ipaConfig, nullptr);
> +
>   	return 0;
>   }
>   
> @@ -759,8 +787,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
>   		return ret;
>   	}
>   
> -	std::map<unsigned int, IPAStream> streamConfig;
> -
>   	if (data->mainPath_->isEnabled()) {
>   		ret = mainPath_.start();
>   		if (ret) {
> @@ -770,11 +796,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
>   			freeBuffers(camera);
>   			return ret;
>   		}
> -
> -		streamConfig[0] = {
> -			.pixelFormat = data->mainPathStream_.configuration().pixelFormat,
> -			.size = data->mainPathStream_.configuration().size,
> -		};
>   	}
>   
>   	if (data->selfPath_->isEnabled()) {
> @@ -787,34 +808,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
>   			freeBuffers(camera);
>   			return ret;
>   		}
> -
> -		streamConfig[1] = {
> -			.pixelFormat = data->selfPathStream_.configuration().pixelFormat,
> -			.size = data->selfPathStream_.configuration().size,
> -		};
>   	}
>   
>   	isp_->setFrameStartEnabled(true);
>   
>   	activeCamera_ = camera;
> -
> -	/* Inform IPA of stream configuration and sensor controls. */
> -	CameraSensorInfo sensorInfo = {};
> -	ret = data->sensor_->sensorInfo(&sensorInfo);
> -	if (ret) {
> -		/* \todo Turn this in an hard failure. */
> -		LOG(RkISP1, Warning) << "Camera sensor information not available";
> -		sensorInfo = {};
> -		ret = 0;
> -	}
> -
> -	std::map<unsigned int, const ControlInfoMap &> entityControls;
> -	entityControls.emplace(0, data->sensor_->controls());
> -
> -	IPAOperationData ipaConfig;
> -	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> -			      ipaConfig, nullptr);
> -
>   	return ret;
>   }
>   
>