[libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens position from IPA

[Matthias Fend]

Hi Daniel, hi Jacopo,

Am 06.02.2023 um 10:45 schrieb Jacopo Mondi:
> Hi Daniel
> 
> On Thu, Jan 19, 2023 at 09:41:06AM +0100, Daniel Semkowicz via libcamera-devel wrote:
>> Allow control of lens position from the IPA, by setting corresponding
>> af fields in the IPAFrameContext structure. Controls are then passed to
>> the pipeline handler, which sets the lens position in CameraLens.
>>
> 
> As a minor nit: I would move this to the end of the series, after
> having plumbed in the algorithm implementation... Just a nit though
> 
>> Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
>> ---
>>   include/libcamera/ipa/rkisp1.mojom       |  1 +
>>   src/ipa/rkisp1/ipa_context.h             |  5 +++++
>>   src/ipa/rkisp1/rkisp1.cpp                | 12 ++++++++++++
>>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++
>>   4 files changed, 34 insertions(+)
>>
>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
>> index bf6e9141..c3ed87aa 100644
>> --- a/include/libcamera/ipa/rkisp1.mojom
>> +++ b/include/libcamera/ipa/rkisp1.mojom
>> @@ -39,5 +39,6 @@ interface IPARkISP1Interface {
>>   interface IPARkISP1EventInterface {
>>   	paramsBufferReady(uint32 frame);
>>   	setSensorControls(uint32 frame, libcamera.ControlList sensorControls);
>> +	setLensControls(libcamera.ControlList lensControls);
>>   	metadataReady(uint32 frame, libcamera.ControlList metadata);
>>   };
>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
>> index b9b20653..1fac6af9 100644
>> --- a/src/ipa/rkisp1/ipa_context.h
>> +++ b/src/ipa/rkisp1/ipa_context.h
>> @@ -53,6 +53,11 @@ struct IPASessionConfiguration {
>>   };
>>
>>   struct IPAActiveState {
>> +	struct {
>> +		uint32_t lensPosition;
>> +		bool applyLensCtrls;
> 
> Oh gosh lens are -complicated- /o\
> 
> For regular sensor controls we defer the decision to apply or not a
> control to DelayedControls on the pipeline handler side, which takes
> care of evaluating if a control has to be updated or not and how many
> frame it takes to take effect.
> 
> Lenses (well, VCM to be fair) are a bit more complex than that, in the
> sense that moving the lens might take a number of frames that depends
> on the movement range. Some VCM datasheet I've seen provide a model to
> estimate the delay depending on the movement range and the VCM
> characteristics. The risk is that updating the lens position while the
> lens hasn't reached its final position might trigger some resonation
> effect, especially if the algorithm runs in "continuous auto-focus"
> mode and tries to update the lens position too often.
> 
> I've cc-ed Matthias Fend which has recently sent a series for "more
> complex optics"
> https://patchwork.libcamera.org/project/libcamera/list/?series=3735
> (which partially overlaps with the work you've done here) as he
> certainly knows more than me about VCM and optics to know what his
> opinion is

First, thank you Daniel, for your work.

The only overlap is the patch in which I basically add the lens 
controller to the pipeline as is done in other pipelines.
This patch is there to better show what changes with the conversion.

Assuming that the calculated or desired position of the lens is always 
somehow valid, I would say for simplicity that it is OK to just always 
update the control. If the position of the lens does not change, the 
control is filtered and not passed to the driver. From there, you might 
consider optimizing applyLensCtrls away.

In my idea, the interface between IPA and pipeline consists of actual 
v4l2 controls for optics. The optic helper classes would go into the IPA 
or the IPA library. This would mean that an autofocus algorithm can 
directly use the optics to read the current position e.g. 
optics.getCurrentFocusPosition() or also set the new position e.g. 
optics.setFocusPosition(). I know that this is a bigger change, but I 
didn't know how else to fulfill all my requirements.

So how and where you want to integrate and handle the optics in the 
future has a significant impact on changes like this.
Of course, this can be changed later, but maybe it would make sense to 
think about how this should look in the end right now.

Regardless, I'm very happy to see that something is moving forward here 
regarding optic support.

Thanks
  ~Matthias

> 
> 
>> +	} af;
>> +
>>   	struct {
>>   		struct {
>>   			uint32_t exposure;
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 9e861fc0..297161b2 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -270,6 +270,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
>>   			return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
>>   		});
>>
>> +	/* Lens position is unknown at the startup, so initilize the variable
>                                                         ^ initialize
>> +	 * that holds the current position to something out of the range. */
> 
> Multiline comments as
>          /*
>           * first line
>           * next line
>           */
> 
>> +	context_.activeState.af.lensPosition = std::numeric_limits<int32_t>::max();
>> +
>>   	for (auto const &a : algorithms()) {
>>   		Algorithm *algo = static_cast<Algorithm *>(a.get());
>>
>> @@ -452,6 +456,14 @@ void IPARkISP1::setControls(unsigned int frame)
>>   	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
>>
>>   	setSensorControls.emit(frame, ctrls);
>> +
>> +	if (lensControls_ && context_.activeState.af.applyLensCtrls) {
>> +		context_.activeState.af.applyLensCtrls = false;
>> +		ControlList lensCtrls(*lensControls_);
>> +		lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
>> +			      static_cast<int32_t>(context_.activeState.af.lensPosition));
>> +		setLensControls.emit(lensCtrls);
>> +	}
>>   }
>>
>>   } /* namespace ipa::rkisp1 */
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index 0559d261..b2fedc5f 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -113,6 +113,7 @@ private:
>>   	void paramFilled(unsigned int frame);
>>   	void setSensorControls(unsigned int frame,
>>   			       const ControlList &sensorControls);
>> +	void setLensControls(const ControlList &lensControls);
>>
>>   	void metadataReady(unsigned int frame, const ControlList &metadata);
>>   };
>> @@ -337,6 +338,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>>   		return -ENOENT;
>>
>>   	ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);
>> +	ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls);
>>   	ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);
>>   	ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
>>
>> @@ -400,6 +402,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
>>   	delayedCtrls_->push(sensorControls);
>>   }
>>
>> +void RkISP1CameraData::setLensControls(const ControlList &lensControls)
>> +{
>> +	CameraLens *focusLens = sensor_->focusLens();
>> +	if (!focusLens)
>> +		return;
>> +
>> +	if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))
>> +		return;
>> +
>> +	const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);
>> +
>> +	focusLens->setFocusPosition(focusValue.get<int32_t>());
>> +}
>> +
>>   void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
>>   {
>>   	RkISP1FrameInfo *info = frameInfo_.find(frame);
>> --
>> 2.39.0
>>