[libcamera-devel,v2,1/4] ipa: ipu3: Extend ipu3 ipa interface for sensor and lens controls
diff mbox series

Message ID 20211029115917.2467936-1-hanlinchen@chromium.org
State Superseded
Headers show
Series
  • [libcamera-devel,v2,1/4] ipa: ipu3: Extend ipu3 ipa interface for sensor and lens controls
Related show

Commit Message

Hanlin Chen Oct. 29, 2021, 11:59 a.m. UTC 
IPU3Event and IPU3Action use single ControlList for both libcamera and V4L2
controls, and it's content could be either one based on the context.
Extend IPU3Event and IPU3Action for sensor and lens V4L2 controls, and preserve
the original one for only libcamera Controls to make the content of an event
more specific.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
---
 include/libcamera/ipa/ipu3.mojom     | 4 ++++
 src/ipa/ipu3/ipu3.cpp                | 2 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

Comments

Jean-Michel Hautbois Nov. 4, 2021, 11:40 a.m. UTC | #1 
Hi Han-Lin,

Thanks for the patch.

On 29/10/2021 13:59, Han-Lin Chen wrote:
> IPU3Event and IPU3Action use single ControlList for both libcamera and V4L2
> controls, and it's content could be either one based on the context.
> Extend IPU3Event and IPU3Action for sensor and lens V4L2 controls, and preserve
> the original one for only libcamera Controls to make the content of an event
> more specific.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> ---
>   include/libcamera/ipa/ipu3.mojom     | 4 ++++
>   src/ipa/ipu3/ipu3.cpp                | 2 +-
>   src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
>   3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index 2f254ed4..cc0d822f 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -23,11 +23,15 @@ struct IPU3Event {
>   	int64 frameTimestamp;
>   	uint32 bufferId;
>   	libcamera.ControlList controls;
> +	libcamera.ControlList sensorControls;
> +	libcamera.ControlList lensControls;

I would split sensorControls adding and lensControls adding in two patch 
series, as the first one could be merged quickly, and could be useful 
also for the open IPA :-). One series would be "add sensorControls to 
use effective sensor values" and the second (biggest one) would then add 
(on top of it) the lens controls.

>   };
>   
>   struct IPU3Action {
>   	IPU3Operations op;
>   	libcamera.ControlList controls;
> +	libcamera.ControlList sensorControls;
> +	libcamera.ControlList lensControls;
>   };
>   
>   struct IPAConfigInfo {
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 5c51607d..6775570e 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -646,7 +646,7 @@ void IPAIPU3::setControls(unsigned int frame)
>   	ControlList ctrls(ctrls_);
>   	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
>   	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> -	op.controls = ctrls;
> +	op.sensorControls = ctrls;
>   
>   	queueFrameAction.emit(frame, op);
>   }
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index eb714aa6..8816efc5 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1248,7 +1248,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
>   {
>   	switch (action.op) {
>   	case ipa::ipu3::ActionSetSensorControls: {
> -		const ControlList &controls = action.controls;
> +		const ControlList &controls = action.sensorControls;
>   		delayedCtrls_->push(controls);
>   		break;
>   	}
>
Jean-Michel Hautbois Nov. 4, 2021, 11:54 a.m. UTC | #2 
Hi Han-Lin,

On 29/10/2021 13:59, Han-Lin Chen wrote:
> Allow IPA to apply controls to the lens device.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> ---
>   meson.build                          |  6 ++++++
>   src/libcamera/pipeline/ipu3/cio2.cpp | 30 ++++++++++++++++++++++++++++
>   src/libcamera/pipeline/ipu3/cio2.h   |  3 +++
>   src/libcamera/pipeline/ipu3/ipu3.cpp |  9 +++++++--
>   4 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 7892a9e3..2a4b68a2 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -108,6 +108,12 @@ if cc.has_argument('-Wno-c99-designator')
>       ]
>   endif
>   
> +if get_option('android_platform') == 'cros'
> +    common_arguments += [
> +        '-DOS_CHROMEOS',
> +    ]
> +endif
> +
>   c_arguments += common_arguments
>   cpp_arguments += common_arguments
>   
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 59dda56b..143e2a95 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,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>   		return -EINVAL;
>   	}
>   
> +#if defined(OS_CHROMEOS)

What is the status about Kieran's comment for the media graph in v1 ?
I still suggest to split lens controls in a separate patch series as the 
sensor controls could probably be merged before... ?

> +	/*
> +	 * \todo Read the lens model from the sensor itself or from a device database.
> +	 * For now use default values taken from ChromeOS.
> +	 */
> +	static std::unordered_map<std::string, std::string> sensorLens = {
> +		{ "ov13858", "dw9714" },
> +		{ "imx258", "dw9807" },
> +		{ "imx355", "ak7375" }
> +	};
> +
> +	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";
> +		}
> +	}
> +#endif
> +
>   	/*
>   	 * \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 6a7f5b9a..6c44957f 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"
> @@ -1250,8 +1251,12 @@ 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);
> +		if (cio2_.lens()) {
> +			ControlList lensControls = action.lensControls;
> +			cio2_.lens()->setControls(&lensControls);
> +		}
>   		break;
>   	}
>   	case ipa::ipu3::ActionParamFilled: {
>
Hanlin Chen Nov. 10, 2021, 12:26 p.m. UTC | #3 
Hi Jean-Michel,
I'm sorry for the late reply.

On Thu, Nov 4, 2021 at 7:40 PM Jean-Michel Hautbois
<jeanmichel.hautbois@ideasonboard.com> wrote:
>
> Hi Han-Lin,
>
> Thanks for the patch.
>
> On 29/10/2021 13:59, Han-Lin Chen wrote:
> > IPU3Event and IPU3Action use single ControlList for both libcamera and V4L2
> > controls, and it's content could be either one based on the context.
> > Extend IPU3Event and IPU3Action for sensor and lens V4L2 controls, and preserve
> > the original one for only libcamera Controls to make the content of an event
> > more specific.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > ---
> >   include/libcamera/ipa/ipu3.mojom     | 4 ++++
> >   src/ipa/ipu3/ipu3.cpp                | 2 +-
> >   src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> >   3 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> > index 2f254ed4..cc0d822f 100644
> > --- a/include/libcamera/ipa/ipu3.mojom
> > +++ b/include/libcamera/ipa/ipu3.mojom
> > @@ -23,11 +23,15 @@ struct IPU3Event {
> >       int64 frameTimestamp;
> >       uint32 bufferId;
> >       libcamera.ControlList controls;
> > +     libcamera.ControlList sensorControls;
> > +     libcamera.ControlList lensControls;
>
> I would split sensorControls adding and lensControls adding in two patch
> series, as the first one could be merged quickly, and could be useful
> also for the open IPA :-). One series would be "add sensorControls to
> use effective sensor values" and the second (biggest one) would then add
> (on top of it) the lens controls.
Thanks! The patches looks great to me.
>
> >   };
> >
> >   struct IPU3Action {
> >       IPU3Operations op;
> >       libcamera.ControlList controls;
> > +     libcamera.ControlList sensorControls;
> > +     libcamera.ControlList lensControls;
> >   };
> >
> >   struct IPAConfigInfo {
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 5c51607d..6775570e 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -646,7 +646,7 @@ void IPAIPU3::setControls(unsigned int frame)
> >       ControlList ctrls(ctrls_);
> >       ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> >       ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> > -     op.controls = ctrls;
> > +     op.sensorControls = ctrls;
> >
> >       queueFrameAction.emit(frame, op);
> >   }
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index eb714aa6..8816efc5 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1248,7 +1248,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
> >   {
> >       switch (action.op) {
> >       case ipa::ipu3::ActionSetSensorControls: {
> > -             const ControlList &controls = action.controls;
> > +             const ControlList &controls = action.sensorControls;
> >               delayedCtrls_->push(controls);
> >               break;
> >       }
> >

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
index 2f254ed4..cc0d822f 100644
--- a/include/libcamera/ipa/ipu3.mojom
+++ b/include/libcamera/ipa/ipu3.mojom
@@ -23,11 +23,15 @@  struct IPU3Event {
 	int64 frameTimestamp;
 	uint32 bufferId;
 	libcamera.ControlList controls;
+	libcamera.ControlList sensorControls;
+	libcamera.ControlList lensControls;
 };
 
 struct IPU3Action {
 	IPU3Operations op;
 	libcamera.ControlList controls;
+	libcamera.ControlList sensorControls;
+	libcamera.ControlList lensControls;
 };
 
 struct IPAConfigInfo {
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 5c51607d..6775570e 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -646,7 +646,7 @@  void IPAIPU3::setControls(unsigned int frame)
 	ControlList ctrls(ctrls_);
 	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
 	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
-	op.controls = ctrls;
+	op.sensorControls = ctrls;
 
 	queueFrameAction.emit(frame, op);
 }
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index eb714aa6..8816efc5 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -1248,7 +1248,7 @@  void IPU3CameraData::queueFrameAction(unsigned int id,
 {
 	switch (action.op) {
 	case ipa::ipu3::ActionSetSensorControls: {
-		const ControlList &controls = action.controls;
+		const ControlList &controls = action.sensorControls;
 		delayedCtrls_->push(controls);
 		break;
 	}