[PATCH v7 3/4] libcamera: rkisp1: Prepare for additional camera controls

[Stefan Klug]

Hi Umang,

Thank you for the patch.

On Fri, Sep 06, 2024 at 12:04:43PM +0530, Umang Jain wrote:
> Currently the rkisp1 pipeline handler only registers controls that are
> related to the IPA. This patch prepares the rkisp1 pipeline-handler to
> register camera controls which are not related to the IPA.
> 
> Hence, introduce an additional ControlInfoMap for IPA controls. These
> controls will be merged together with the controls in the pipeline
> handler (introduced subsequently) as part of updateControls() and
> together will be registered during the registration of the camera.
> This is similar to what IPU3 pipeline handler handles its controls.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index c02c7cf3..651258e3 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -108,6 +108,8 @@ public:
>  
>  	std::unique_ptr<ipa::rkisp1::IPAProxyRkISP1> ipa_;
>  
> +	ControlInfoMap ipaControls_;
> +
>  private:
>  	void paramFilled(unsigned int frame, unsigned int bytesused);
>  	void setSensorControls(unsigned int frame,
> @@ -183,6 +185,8 @@ private:
>  	int allocateBuffers(Camera *camera);
>  	int freeBuffers(Camera *camera);
>  
> +	int updateControls(RkISP1CameraData *data);
> +
>  	MediaDevice *media_;
>  	std::unique_ptr<V4L2Subdevice> isp_;
>  	std::unique_ptr<V4L2VideoDevice> param_;
> @@ -364,7 +368,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>  	}
>  
>  	ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> -			 sensorInfo, sensor_->controls(), &controlInfo_);
> +			 sensorInfo, sensor_->controls(), &ipaControls_);
>  	if (ret < 0) {
>  		LOG(RkISP1, Error) << "IPA initialization failure";
>  		return ret;
> @@ -814,12 +818,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	ipaConfig.sensorControls = data->sensor_->controls();
>  	ipaConfig.paramFormat = paramFormat.fourcc;
>  
> -	ret = data->ipa_->configure(ipaConfig, streamConfig, &data->controlInfo_);
> +	ret = data->ipa_->configure(ipaConfig, streamConfig, &data->ipaControls_);
>  	if (ret) {
>  		LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";
>  		return ret;
>  	}
> -	return 0;
> +
> +	return updateControls(data);
>  }
>  
>  int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> @@ -1086,6 +1091,35 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,
>  	return 0;
>  }
>  
> +/**
> + * \brief Update the camera controls
> + * \param[in] data The camera data
> + *
> + * Compute the camera controls by calculating controls which the pipeline
> + * is reponsible for and merge them with the controls computed by the IPA.
> + *
> + * This function needs data->ipaControls_ to be refreshed when a new
> + * configuration is applied to the camera by the IPA configure() function.
> + *
> + * Always call this function after IPA configure() to make sure to have a
> + * properly refreshed IPA controls list.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
> +{
> +	ControlInfoMap::Map controls;
> +
> +	/* Add the IPA registered controls to list of camera controls. */
> +	for (const auto &ipaControl : data->ipaControls_)
> +		controls[ipaControl.first] = ipaControl.second;

I guess I'm missing something here. Why is that extra ipaControls_
needed? At this point in time the controlInfo_ should be populated/reset
by the ipa and we could just add the extra controls to it here.

Best regards,
Stefan

> +
> +	data->controlInfo_ = ControlInfoMap(std::move(controls),
> +					    controls::controls);
> +
> +	return 0;
> +}
> +
>  int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  {
>  	int ret;
> @@ -1122,6 +1156,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	if (ret)
>  		return ret;
>  
> +	updateControls(data.get());
> +
>  	std::set<Stream *> streams{
>  		&data->mainPathStream_,
>  		&data->selfPathStream_,
> -- 
> 2.45.0
>