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

[Umang Jain]

Hi Stefan

On 25/09/24 1:48 am, Stefan Klug wrote:
> 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.

One cannot add controls to the ControlInfoMap directly. It  has be added 
to ControlInfoMap::Map first and then a ControlInfoMap has to be 
constructed out of it.

If you read the ControlInfoMap class definition, it has a private 
inheritance from std::unordered_map<>, hence all accessors/modifiers are 
private in ControlInfoMap (for .e.g insert, emplace etc.)

Does this makes sense?
> 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
>>