[libcamera-devel] [PATCH v4 4/4] ipu3: ipa: Allow IPA to apply controls to the lens device

[Daniel Scally]

Morning

On 24/11/2021 05:56, Laurent Pinchart wrote:
> Hi Han-lin,
>
> (CC'ing Dan)
>
> Thank you for the patch.
>
> On Tue, Nov 23, 2021 at 08:37:51PM +0800, Han-Lin Chen wrote:
>> Allow IPA to apply controls to the lens device.
>>
>> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
>> ---
>>  src/libcamera/pipeline/ipu3/cio2.cpp | 29 ++++++++++++++++++++++++++++
>>  src/libcamera/pipeline/ipu3/cio2.h   |  3 +++
>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++++++++--
>>  3 files changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
>> index 59dda56b..59b2f586 100644
>> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
>> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
>> @@ -16,6 +16,7 @@
>>  #include <libcamera/geometry.h>
>>  #include <libcamera/stream.h>
>>  
>> +#include "libcamera/internal/camera_lens.h"
>>  #include "libcamera/internal/camera_sensor.h"
>>  #include "libcamera/internal/framebuffer.h"
>>  #include "libcamera/internal/media_device.h"
>> @@ -159,6 +160,34 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>>  		return -EINVAL;
>>  	}
>>  
>> +	/*
>> +	 * \todo Read the lens model from the sensor itself or from a device
>> +	 * database. For now use default values taken from ChromeOS database.
>> +	 */
>> +	static std::unordered_map<std::string, std::string> sensorLens = {
>> +		{ "ov13858", "dw9714" },
>> +		{ "imx258", "dw9807" },
>> +		{ "imx355", "ak7375" }
>> +	};
> Dan, could you share your patches to add an ancillary link between the
> imaging sensor and the lens controller with Han-lin once they're ready ?
> This is what we should use here, and Han-lin could help testing them on
> Chrome OS (which uses different ACPI bindings for the CIO2).


Yes, certainly. I was going to share them as a patch to be applied on
top of this series, and replacing this section. This part (following the
new link and creating the CameraLens) is working now, just need to iron
out a few things and I can share something.

>> +
>> +	auto it = sensorLens.find(sensor_->model());
>> +	if (it != sensorLens.end()) {
>> +		const std::vector<MediaEntity *> &entities = media->entities();
>> +		for (auto ent : entities) {
>> +			if (ent->function() == MEDIA_ENT_F_LENS) {
>> +				lens_ = std::make_unique<CameraLens>(ent);
>> +				ret = lens_->init();
>> +				if (!ret && lens_->model() == it->second) {
>> +					break;
>> +				}
>> +				lens_.reset();
>> +			}
>> +			if (!lens_)
>> +				LOG(IPU3, Warning) << "Lens device "
>> +						   << it->second << " not found";
>> +		}
>> +	}
>> +
>>  	/*
>>  	 * \todo Define when to open and close video device nodes, as they
>>  	 * might impact on power consumption.
>> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
>> index ba8f0052..635566c8 100644
>> --- a/src/libcamera/pipeline/ipu3/cio2.h
>> +++ b/src/libcamera/pipeline/ipu3/cio2.h
>> @@ -18,6 +18,7 @@
>>  
>>  namespace libcamera {
>>  
>> +class CameraLens;
>>  class CameraSensor;
>>  class FrameBuffer;
>>  class MediaDevice;
>> @@ -52,6 +53,7 @@ public:
>>  	int stop();
>>  
>>  	CameraSensor *sensor() { return sensor_.get(); }
>> +	CameraLens *lens() { return lens_.get(); }
>>  	const CameraSensor *sensor() const { return sensor_.get(); }
>>  
>>  	FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer);
>> @@ -67,6 +69,7 @@ private:
>>  	void cio2BufferReady(FrameBuffer *buffer);
>>  
>>  	std::unique_ptr<CameraSensor> sensor_;
>> +	std::unique_ptr<CameraLens> lens_;
>>  	std::unique_ptr<V4L2Subdevice> csi2_;
>>  	std::unique_ptr<V4L2VideoDevice> output_;
>>  
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index c65afdb2..6e04ec8f 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -24,6 +24,7 @@
>>  #include <libcamera/stream.h>
>>  
>>  #include "libcamera/internal/camera.h"
>> +#include "libcamera/internal/camera_lens.h"
>>  #include "libcamera/internal/camera_sensor.h"
>>  #include "libcamera/internal/delayed_controls.h"
>>  #include "libcamera/internal/device_enumerator.h"
>> @@ -1238,8 +1239,15 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
>>  {
>>  	switch (action.op) {
>>  	case ipa::ipu3::ActionSetSensorControls: {
>> -		const ControlList &controls = action.sensorControls;
>> -		delayedCtrls_->push(controls);
>> +		const ControlList &sensorControls = action.sensorControls;
>> +		delayedCtrls_->push(sensorControls);
>> +
>> +		const ControlList lensControls = action.lensControls;
>> +		const ControlValue &focusValue =
>> +			lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);
>> +		if (!focusValue.isNone() && cio2_.lens())
>> +			cio2_.lens()->setFocusPostion(focusValue.get<int32_t>());
>> +
> The other way around, should we pass the limits of the focus control
> from the pipeline handler to the IPA ?
>
>>  		break;
>>  	}
>>  	case ipa::ipu3::ActionParamFilled: {