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

14890 diff mbox series

Allow IPA to apply controls to the lens device.

Signed-off-by: Han-Lin Chen <hanlinchen@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(-)

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@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@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
>

Hi Kieran,
Thanks for comments.

On Tue, Nov 30, 2021 at 8:44 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> 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@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.
I agree. Since there are similar discussion in Daniel's series too,
let's move the lens to CameraSensor.
>
> A tentative:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@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
> >