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

[Stefan Klug]

Hi Umang,

On Wed, Sep 25, 2024 at 11:53:09AM +0530, Umang Jain wrote:
> 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?

Oh yes, that makes a lot of sense. I somehow missed the private. Thanks
for clarification. Makes me think if we should question this
immutability. But that's for another day.

Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com> 

Cheers,
Stefan

> > 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
> > > 
>