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

[Kieran Bingham]

Quoting Han-Lin Chen (2021-11-30 10:51:57)
> 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 |  1 +
>  src/libcamera/pipeline/ipu3/cio2.h   |  3 +++
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++++++++--
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 59dda56b..ff795310 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"
> 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_;

I had assumed we'd model a lens as part of a sensor (i.e. create it in
the CameraSensor class).

Is there ever a case where we would need to keep the lens controller
separate from the Sensor?



>         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>());
> +

And if this was handled by the CameraSensor class, it gets handled for
all pipelines (that use the CameraSensor class...)

This will have to be repeated ... somewhat verbatim in the RkISP
pipeline handler right?

Anyway, this progresses AF at least, so it's something we can build
upon but I really think this should get reworked sometime.

A tentative:


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

Of course, this doesn't actually have a lens connected yet, but given we
have three developers trying to work on the same thing, I think we need
to get a base ground to continue so I wouldn't object to merging this to
support ongoing development.


>                 break;
>         }
>         case ipa::ipu3::ActionParamFilled: {
> -- 
> 2.34.0.rc2.393.gf8c9666880-goog
>