[v7,3/4] libcamera: rkisp1: Prepare for additional camera controls
diff mbox series

Message ID 20240906063444.32772-4-umang.jain@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: rkisp1: Plumb the ConverterDW100 converter
Related show

Commit Message

Umang Jain Sept. 6, 2024, 6:34 a.m. UTC 
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@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 3 deletions(-)

Comments

Kieran Bingham Sept. 16, 2024, 1:47 p.m. UTC | #1 
Quoting Umang Jain (2024-09-06 07:34:43)
> 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@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@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 the merge helpers are on the list - not the info map.

And I'll presume we'll see additions for the RkISP1 Pipeline handler
controls get registered in here somehow, so I'll take a look at the next
patch.

Ok - so we indeed need a hook point.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



> +       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
>
Stefan Klug Sept. 24, 2024, 8:18 p.m. UTC | #2 
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@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@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
>
Umang Jain Sept. 25, 2024, 6:23 a.m. UTC | #3 
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@ideasonboard.com>
>> Reviewed-by: Paul Elder <paul.elder@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
>>
Stefan Klug Sept. 25, 2024, 7 a.m. UTC | #4 
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@ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder@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@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
> > > 
>
Laurent Pinchart Sept. 25, 2024, 6:21 p.m. UTC | #5 
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@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@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;
> +
> +	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_,

Patch
diff mbox series 

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;
+
+	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_,