[libcamera-devel,v4,3/4] libcamera: camera_lens: Add a new class to model a camera lens

14707 diff mbox series

The CameraLens class abstracts camera lens and provides helper
functions to ease interactions with them.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 Documentation/index.rst                    |   1 +
 Documentation/lens_driver_requirements.rst |  28 ++++
 Documentation/meson.build                  |   1 +
 include/libcamera/internal/camera_lens.h   |  48 +++++++
 include/libcamera/internal/meson.build     |   1 +
 src/libcamera/camera_lens.cpp              | 150 +++++++++++++++++++++
 src/libcamera/meson.build                  |   1 +
 7 files changed, 230 insertions(+)
 create mode 100644 Documentation/lens_driver_requirements.rst
 create mode 100644 include/libcamera/internal/camera_lens.h
 create mode 100644 src/libcamera/camera_lens.cpp

Quoting Han-Lin Chen (2021-11-23 12:37:50)
> The CameraLens class abstracts camera lens and provides helper
> functions to ease interactions with them.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  Documentation/index.rst                    |   1 +
>  Documentation/lens_driver_requirements.rst |  28 ++++
>  Documentation/meson.build                  |   1 +
>  include/libcamera/internal/camera_lens.h   |  48 +++++++
>  include/libcamera/internal/meson.build     |   1 +
>  src/libcamera/camera_lens.cpp              | 150 +++++++++++++++++++++
>  src/libcamera/meson.build                  |   1 +
>  7 files changed, 230 insertions(+)
>  create mode 100644 Documentation/lens_driver_requirements.rst
>  create mode 100644 include/libcamera/internal/camera_lens.h
>  create mode 100644 src/libcamera/camera_lens.cpp
> 
> diff --git a/Documentation/index.rst b/Documentation/index.rst
> index 1f4fc485..0ee10044 100644
> --- a/Documentation/index.rst
> +++ b/Documentation/index.rst
> @@ -21,3 +21,4 @@
>     Tracing guide <guides/tracing>
>     Environment variables <environment_variables>
>     Sensor driver requirements <sensor_driver_requirements>
> +   Lens driver requirements <lens_driver_requirements>
> diff --git a/Documentation/lens_driver_requirements.rst b/Documentation/lens_driver_requirements.rst
> new file mode 100644
> index 00000000..afc300cf
> --- /dev/null
> +++ b/Documentation/lens_driver_requirements.rst
> @@ -0,0 +1,28 @@
> +.. SPDX-License-Identifier: CC-BY-SA-4.0
> +
> +.. _lens-driver-requirements:
> +
> +Lens Driver Requirements
> +========================
> +
> +libcamera handles lens devices in the CameraLens class and defines
> +a consistent interface through its API towards other library components.
> +
> +The CameraLens class uses the V4L2 subdev kernel API to interface with the
> +camera lens through one or multiple sub-devices exposed in userspace by
> +the lens driver.
> +
> +In order for libcamera to be fully operational and provide all the required
> +information to interface with the camera lens to applications and pipeline
> +handlers, a set of mandatory features the driver has to support has been defined.
> +
> +Mandatory Requirements
> +----------------------
> +
> +The lens driver is assumed to be fully compliant with the V4L2 specification.
> +
> +The lens driver shall support the following V4L2 controls:
> +
> +* `V4L2_CID_FOCUS_ABSOLUTE`_
> +
> +.. _V4L2_CID_FOCUS_ABSOLUTE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-camera.html
> diff --git a/Documentation/meson.build b/Documentation/meson.build
> index 4c972675..33af82aa 100644
> --- a/Documentation/meson.build
> +++ b/Documentation/meson.build
> @@ -66,6 +66,7 @@ if sphinx.found()
>          'guides/pipeline-handler.rst',
>          'guides/tracing.rst',
>          'index.rst',
> +        'lens_driver_requirements.rst',
>          'sensor_driver_requirements.rst',
>         '../README.rst',
>      ]
> diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h
> new file mode 100644
> index 00000000..fe83d4ad
> --- /dev/null
> +++ b/include/libcamera/internal/camera_lens.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * camera_lens.h - A camera lens
> + */
> +#ifndef __LIBCAMERA_INTERNAL_CAMERA_LENS_H__
> +#define __LIBCAMERA_INTERNAL_CAMERA_LENS_H__
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/v4l2_subdevice.h"
> +
> +namespace libcamera {
> +
> +class MediaEntity;
> +
> +class CameraLens : protected Loggable
> +{
> +public:
> +       explicit CameraLens(const MediaEntity *entity);
> +       ~CameraLens();
> +
> +       int init();
> +       int setFocusPostion(int32_t position);
> +
> +       const std::string &model() const { return model_; }
> +
> +protected:
> +       std::string logPrefix() const override;
> +
> +private:
> +       LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraLens)
> +
> +       int validateLensDriver();
> +
> +       const MediaEntity *entity_;
> +       std::unique_ptr<V4L2Subdevice> subdev_;
> +
> +       std::string model_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_CAMERA_LENS_H__ */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index de8630fe..7e9b9998 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -14,6 +14,7 @@ libcamera_internal_headers = files([
>      'byte_stream_buffer.h',
>      'camera.h',
>      'camera_controls.h',
> +    'camera_lens.h',
>      'camera_sensor.h',
>      'camera_sensor_properties.h',
>      'camera_utils.h',
> diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp
> new file mode 100644
> index 00000000..cb7159fe
> --- /dev/null
> +++ b/src/libcamera/camera_lens.cpp
> @@ -0,0 +1,150 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * camera_lens.cpp - A camera lens
> + */
> +
> +#include "libcamera/internal/camera_lens.h"
> +
> +#include <regex>

I don't think regex is used anymore.

> +#include <libcamera/property_ids.h>
> +
> +#include "libcamera/internal/camera_utils.h"
> +#include "libcamera/internal/media_device.h"
> +#include "libcamera/internal/sysfs.h"

nor sysfs.h...

> +
> +/**
> + * \file camera_lens.h
> + * \brief A camera lens
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(CameraLens)
> +
> +/**
> + * \class CameraLens
> + * \brief A camera lens based on V4L2 subdevices
> + *
> + * The CameraLens class eases handling of lens for pipeline handlers by
> + * hiding the details of the V4L2 subdevice kernel API and caching lens
> + * information.
> + *
> + * The implementation is currently limited to lens that expose a single V4L2
> + * subdevice. It will be extended to support more complex devices as the needs
> + * arise.
> + */
> +
> +/**
> + * \brief Construct a CameraLens
> + * \param[in] entity The media entity backing the camera lens
> + *
> + * Once constructed the instance must be initialized with init().
> + */
> +CameraLens::CameraLens(const MediaEntity *entity)
> +       : entity_(entity)
> +{
> +}
> +
> +/**
> + * \brief Destroy a CameraLens
> + */
> +CameraLens::~CameraLens() = default;
> +
> +/**
> + * \brief Initialize the camera lens instance
> + *
> + * This function performs the initialisation steps of the CameraLens that may
> + * fail. It shall be called once and only once after constructing the instance.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CameraLens::init()
> +{
> +       if (entity_->function() != MEDIA_ENT_F_LENS) {
> +               LOG(CameraLens, Error)
> +                       << "Invalid lens function "
> +                       << utils::hex(entity_->function());
> +               return -EINVAL;
> +       }
> +
> +       model_ = extractModelFromEntityName(entity_->name());
> +
> +       /* Create and open the subdev. */
> +       subdev_ = std::make_unique<V4L2Subdevice>(entity_);
> +       int ret = subdev_->open();
> +       if (ret < 0)
> +               return ret;

This is probably why I think I'd make the model handled by the
V4L2Subdevice.

We /know/ we're creating a V4L2Subdevice here, so we could assign
something like

	model_ = subdev_->model();

Or simply have model() call directly onto the subdev which would have
the implementation...


> +
> +       ret = validateLensDriver();
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +/**
> + * \brief This function sets the focal point of the lens to a specific position.
> + * \param[in] position The focal point of the lens
> + *
> + * This function sets the value of focal point of the lens as in \a position.
> + *
> + * \return 0 on success or -EINVAL otherwise
> + */
> +int CameraLens::setFocusPostion(int32_t position)
> +{
> +       ControlList lensCtrls(subdev_->controls());
> +       lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast<int32_t>(position));
> +
> +       if (subdev_->setControls(&lensCtrls))
> +               return -EINVAL;
> +
> +       return 0;
> +}

I fear this is going the same way that the TestPatternMode is going on
the CameraSensor.

I don't think we should expose a function for each control and setting
on the devices.

The device should accept the controls, parse it and apply it.

The setFocusPosition helper function is probably good to keep as it is -
but I think a CameraLens should be taking a ControlList (of libcamera
controls) and doing something like:


+/**
+ * \brief Write libcamera controls to the lens
+ * \param[in] list The list of controls to write
+ *
+ * Supported libcamera controls are interpreted and converted to V4L2
+ * controls to write to the device.
+ *
+ * \return 0 on success or an error code otherwise
+ * \retval -EINVAL One of the control is not supported or not accessible
+ */
+int CameraLens::setControls(ControlList *list)
+{
+	if (!list)
+		return -ENODATA;
+
+	const ControlIdMap *idmap = list->idMap();
+	if (idmap != &controls::controls) {
+		LOG(CameraLens, Error) << "Invalid Control List";
+		return -EINVAL;
+	}
+
+	ControlList subdevControls;
+
+	for (const auto &ctrl : *list) {
+		unsigned int id = ctrl.first;
+		const ControlValue &value = ctrl.second;
+
+		// switch case would be nicer here.... but isn't
+		// currently possible
+
+		// Also not a libcamera control yet
+		if (id == controls::FocusAbsolute) {
+			int position = value.get<int>();
+			subdevControls.set(V4L2_CID_FOCUS_ABSOLUTE, position);
+		}
+
+		// Not a real libcamera control currently
+		if (id == controls::FocusRelative) {
+			// Not implemented, showing other control
+			// handling only
+		}
+	}
+
+	return subdev_->setControls(&subdevControls);
+}
+

I think this is what CameraSensor should be doing too, but we have this
awkward precedent that it can already take a V4L2ControlList directly...


Given that a lens can only currently set a position anyway, it's not
such a big deal if this is considered if/when we need more controls
instead, and we keep the direct and simple setting function.

--
Kieran


> +
> +int CameraLens::validateLensDriver()
> +{
> +       int err = 0;
> +       static constexpr uint32_t mandatoryControls[] = {
> +               V4L2_CID_FOCUS_ABSOLUTE
> +       };
> +
> +       const ControlInfoMap &controls = subdev_->controls();
> +       for (uint32_t ctrl : mandatoryControls) {
> +               if (!controls.count(ctrl)) {
> +                       LOG(CameraLens, Error)
> +                               << "Mandatory V4L2 control " << utils::hex(ctrl)
> +                               << " not available";
> +                       err = -EINVAL;
> +               }
> +       }
> +
> +       if (err) {
> +               LOG(CameraLens, Error)
> +                       << "The lens kernel driver needs to be fixed";
> +               LOG(CameraLens, Error)
> +                       << "See Documentation/lens_driver_requirements.rst in"
> +                       << " the libcamera sources for more information";
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * \fn CameraLens::model()
> + * \brief Retrieve the lens model name
> + *
> + * The lens model name is a free-formed string that uniquely identifies the
> + * lens model.
> + *
> + * \return The lens model name
> + */
> +
> +std::string CameraLens::logPrefix() const
> +{
> +       return "'" + entity_->name() + "'";
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 563ed861..2100805d 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -5,6 +5,7 @@ libcamera_sources = files([
>      'byte_stream_buffer.cpp',
>      'camera.cpp',
>      'camera_controls.cpp',
> +    'camera_lens.cpp',
>      'camera_manager.cpp',
>      'camera_sensor.cpp',
>      'camera_sensor_properties.cpp',
> -- 
> 2.34.0.rc2.393.gf8c9666880-goog
>

Hello,

On Tue, Nov 23, 2021 at 05:10:40PM +0000, Kieran Bingham wrote:
> Quoting Han-Lin Chen (2021-11-23 12:37:50)
> > The CameraLens class abstracts camera lens and provides helper
> > functions to ease interactions with them.
> > 
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  Documentation/index.rst                    |   1 +
> >  Documentation/lens_driver_requirements.rst |  28 ++++
> >  Documentation/meson.build                  |   1 +
> >  include/libcamera/internal/camera_lens.h   |  48 +++++++
> >  include/libcamera/internal/meson.build     |   1 +
> >  src/libcamera/camera_lens.cpp              | 150 +++++++++++++++++++++
> >  src/libcamera/meson.build                  |   1 +
> >  7 files changed, 230 insertions(+)
> >  create mode 100644 Documentation/lens_driver_requirements.rst
> >  create mode 100644 include/libcamera/internal/camera_lens.h
> >  create mode 100644 src/libcamera/camera_lens.cpp
> > 
> > diff --git a/Documentation/index.rst b/Documentation/index.rst
> > index 1f4fc485..0ee10044 100644
> > --- a/Documentation/index.rst
> > +++ b/Documentation/index.rst
> > @@ -21,3 +21,4 @@
> >     Tracing guide <guides/tracing>
> >     Environment variables <environment_variables>
> >     Sensor driver requirements <sensor_driver_requirements>
> > +   Lens driver requirements <lens_driver_requirements>
> > diff --git a/Documentation/lens_driver_requirements.rst b/Documentation/lens_driver_requirements.rst
> > new file mode 100644
> > index 00000000..afc300cf
> > --- /dev/null
> > +++ b/Documentation/lens_driver_requirements.rst
> > @@ -0,0 +1,28 @@
> > +.. SPDX-License-Identifier: CC-BY-SA-4.0
> > +
> > +.. _lens-driver-requirements:
> > +
> > +Lens Driver Requirements
> > +========================
> > +
> > +libcamera handles lens devices in the CameraLens class and defines
> > +a consistent interface through its API towards other library components.
> > +
> > +The CameraLens class uses the V4L2 subdev kernel API to interface with the
> > +camera lens through one or multiple sub-devices exposed in userspace by

"... through a sub-device exposed to userspace ..."

Camera sensors may use multiple subdevs, but VCMs use a single subdev
only.

> > +the lens driver.
> > +
> > +In order for libcamera to be fully operational and provide all the required
> > +information to interface with the camera lens to applications and pipeline
> > +handlers, a set of mandatory features the driver has to support has been defined.
> > +
> > +Mandatory Requirements
> > +----------------------
> > +
> > +The lens driver is assumed to be fully compliant with the V4L2 specification.
> > +
> > +The lens driver shall support the following V4L2 controls:
> > +
> > +* `V4L2_CID_FOCUS_ABSOLUTE`_
> > +
> > +.. _V4L2_CID_FOCUS_ABSOLUTE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-camera.html
> > diff --git a/Documentation/meson.build b/Documentation/meson.build
> > index 4c972675..33af82aa 100644
> > --- a/Documentation/meson.build
> > +++ b/Documentation/meson.build
> > @@ -66,6 +66,7 @@ if sphinx.found()
> >          'guides/pipeline-handler.rst',
> >          'guides/tracing.rst',
> >          'index.rst',
> > +        'lens_driver_requirements.rst',
> >          'sensor_driver_requirements.rst',
> >         '../README.rst',
> >      ]
> > diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h
> > new file mode 100644
> > index 00000000..fe83d4ad
> > --- /dev/null
> > +++ b/include/libcamera/internal/camera_lens.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * camera_lens.h - A camera lens
> > + */
> > +#ifndef __LIBCAMERA_INTERNAL_CAMERA_LENS_H__
> > +#define __LIBCAMERA_INTERNAL_CAMERA_LENS_H__

You need to include <memory> and <string>.

> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/base/log.h>
> > +
> > +#include <libcamera/controls.h>
> > +
> > +#include "libcamera/internal/v4l2_subdevice.h"

You could forward-declare V4L2Subdevice instead.

> > +
> > +namespace libcamera {
> > +
> > +class MediaEntity;
> > +
> > +class CameraLens : protected Loggable
> > +{
> > +public:
> > +       explicit CameraLens(const MediaEntity *entity);
> > +       ~CameraLens();
> > +
> > +       int init();
> > +       int setFocusPostion(int32_t position);

Can the position be negative ?

> > +
> > +       const std::string &model() const { return model_; }
> > +
> > +protected:
> > +       std::string logPrefix() const override;
> > +
> > +private:
> > +       LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraLens)
> > +
> > +       int validateLensDriver();
> > +
> > +       const MediaEntity *entity_;
> > +       std::unique_ptr<V4L2Subdevice> subdev_;
> > +
> > +       std::string model_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_INTERNAL_CAMERA_LENS_H__ */
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index de8630fe..7e9b9998 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -14,6 +14,7 @@ libcamera_internal_headers = files([
> >      'byte_stream_buffer.h',
> >      'camera.h',
> >      'camera_controls.h',
> > +    'camera_lens.h',
> >      'camera_sensor.h',
> >      'camera_sensor_properties.h',
> >      'camera_utils.h',
> > diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp
> > new file mode 100644
> > index 00000000..cb7159fe
> > --- /dev/null
> > +++ b/src/libcamera/camera_lens.cpp
> > @@ -0,0 +1,150 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * camera_lens.cpp - A camera lens
> > + */
> > +
> > +#include "libcamera/internal/camera_lens.h"
> > +
> > +#include <regex>
> 
> I don't think regex is used anymore.
> 
> > +#include <libcamera/property_ids.h>
> > +
> > +#include "libcamera/internal/camera_utils.h"
> > +#include "libcamera/internal/media_device.h"
> > +#include "libcamera/internal/sysfs.h"
> 
> nor sysfs.h...

On the other hand, utils.h is needed.

> > +
> > +/**
> > + * \file camera_lens.h
> > + * \brief A camera lens

Maybe "A camera lens controller" ? Up to you.

We will also need to support focus lens. I'm not sure if we'll need a
separate class, we'll figure it out then.

> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(CameraLens)
> > +
> > +/**
> > + * \class CameraLens
> > + * \brief A camera lens based on V4L2 subdevices
> > + *
> > + * The CameraLens class eases handling of lens for pipeline handlers by
> > + * hiding the details of the V4L2 subdevice kernel API and caching lens
> > + * information.
> > + *
> > + * The implementation is currently limited to lens that expose a single V4L2
> > + * subdevice. It will be extended to support more complex devices as the needs
> > + * arise.

I'd drop this paragraph for the reason explained above.

> > + */
> > +
> > +/**
> > + * \brief Construct a CameraLens
> > + * \param[in] entity The media entity backing the camera lens

s/lens/lens controller/

> > + *
> > + * Once constructed the instance must be initialized with init().
> > + */
> > +CameraLens::CameraLens(const MediaEntity *entity)
> > +       : entity_(entity)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Destroy a CameraLens
> > + */
> > +CameraLens::~CameraLens() = default;
> > +
> > +/**
> > + * \brief Initialize the camera lens instance
> > + *
> > + * This function performs the initialisation steps of the CameraLens that may
> > + * fail. It shall be called once and only once after constructing the instance.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int CameraLens::init()
> > +{
> > +       if (entity_->function() != MEDIA_ENT_F_LENS) {
> > +               LOG(CameraLens, Error)
> > +                       << "Invalid lens function "
> > +                       << utils::hex(entity_->function());
> > +               return -EINVAL;
> > +       }
> > +
> > +       model_ = extractModelFromEntityName(entity_->name());
> > +
> > +       /* Create and open the subdev. */
> > +       subdev_ = std::make_unique<V4L2Subdevice>(entity_);
> > +       int ret = subdev_->open();
> > +       if (ret < 0)
> > +               return ret;
> 
> This is probably why I think I'd make the model handled by the
> V4L2Subdevice.
> 
> We /know/ we're creating a V4L2Subdevice here, so we could assign
> something like
> 
> 	model_ = subdev_->model();
> 
> Or simply have model() call directly onto the subdev which would have
> the implementation...

Agreed. I'd however like to drop use of model() in patch 4/4, in favour
of ancillary links that Daniel Scally is working on. This doesn't
prevent moving model() to V4L2Subdevice though.

> > +
> > +       ret = validateLensDriver();
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * \brief This function sets the focal point of the lens to a specific position.
> > + * \param[in] position The focal point of the lens
> > + *
> > + * This function sets the value of focal point of the lens as in \a position.
> > + *
> > + * \return 0 on success or -EINVAL otherwise
> > + */
> > +int CameraLens::setFocusPostion(int32_t position)
> > +{
> > +       ControlList lensCtrls(subdev_->controls());
> > +       lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast<int32_t>(position));
> > +
> > +       if (subdev_->setControls(&lensCtrls))
> > +               return -EINVAL;
> > +
> > +       return 0;
> > +}
> 
> I fear this is going the same way that the TestPatternMode is going on
> the CameraSensor.
> 
> I don't think we should expose a function for each control and setting
> on the devices.
> 
> The device should accept the controls, parse it and apply it.
> 
> The setFocusPosition helper function is probably good to keep as it is -
> but I think a CameraLens should be taking a ControlList (of libcamera
> controls) and doing something like:
> 
> +/**
> + * \brief Write libcamera controls to the lens
> + * \param[in] list The list of controls to write
> + *
> + * Supported libcamera controls are interpreted and converted to V4L2
> + * controls to write to the device.
> + *
> + * \return 0 on success or an error code otherwise
> + * \retval -EINVAL One of the control is not supported or not accessible
> + */
> +int CameraLens::setControls(ControlList *list)
> +{
> +	if (!list)
> +		return -ENODATA;

If it can't be null, we can pass a reference.

> +
> +	const ControlIdMap *idmap = list->idMap();
> +	if (idmap != &controls::controls) {
> +		LOG(CameraLens, Error) << "Invalid Control List";
> +		return -EINVAL;
> +	}
> +
> +	ControlList subdevControls;
> +
> +	for (const auto &ctrl : *list) {
> +		unsigned int id = ctrl.first;
> +		const ControlValue &value = ctrl.second;
> +
> +		// switch case would be nicer here.... but isn't
> +		// currently possible
> +
> +		// Also not a libcamera control yet

If we go in this direction (and I think it's a good idea), then I
believe we'll have to create internal controls for the camera sensor and
the lens controller. Some of the libcamera public controls may match,
but we'll most probably need internal controls that don't make sense in
the public API. We should have all the infrastructure we need for this,
so it shouldn't be too difficult technically (but may result in
nitpicking).

This being said, as we currently expose a single control in this class,
we could also start with an ad-hoc function.

> +		if (id == controls::FocusAbsolute) {
> +			int position = value.get<int>();
> +			subdevControls.set(V4L2_CID_FOCUS_ABSOLUTE, position);
> +		}
> +
> +		// Not a real libcamera control currently
> +		if (id == controls::FocusRelative) {
> +			// Not implemented, showing other control
> +			// handling only
> +		}
> +	}
> +
> +	return subdev_->setControls(&subdevControls);
> +}
> +
> 
> I think this is what CameraSensor should be doing too, but we have this
> awkward precedent that it can already take a V4L2ControlList directly...
> 
> Given that a lens can only currently set a position anyway, it's not
> such a big deal if this is considered if/when we need more controls
> instead, and we keep the direct and simple setting function.

Seems we agree :-)

> > +
> > +int CameraLens::validateLensDriver()
> > +{
> > +       int err = 0;
> > +       static constexpr uint32_t mandatoryControls[] = {
> > +               V4L2_CID_FOCUS_ABSOLUTE
> > +       };
> > +
> > +       const ControlInfoMap &controls = subdev_->controls();
> > +       for (uint32_t ctrl : mandatoryControls) {
> > +               if (!controls.count(ctrl)) {
> > +                       LOG(CameraLens, Error)
> > +                               << "Mandatory V4L2 control " << utils::hex(ctrl)
> > +                               << " not available";
> > +                       err = -EINVAL;
> > +               }
> > +       }
> > +
> > +       if (err) {
> > +               LOG(CameraLens, Error)
> > +                       << "The lens kernel driver needs to be fixed";
> > +               LOG(CameraLens, Error)
> > +                       << "See Documentation/lens_driver_requirements.rst in"
> > +                       << " the libcamera sources for more information";
> > +               return err;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * \fn CameraLens::model()
> > + * \brief Retrieve the lens model name
> > + *
> > + * The lens model name is a free-formed string that uniquely identifies the
> > + * lens model.
> > + *
> > + * \return The lens model name
> > + */
> > +
> > +std::string CameraLens::logPrefix() const
> > +{
> > +       return "'" + entity_->name() + "'";
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 563ed861..2100805d 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -5,6 +5,7 @@ libcamera_sources = files([
> >      'byte_stream_buffer.cpp',
> >      'camera.cpp',
> >      'camera_controls.cpp',
> > +    'camera_lens.cpp',
> >      'camera_manager.cpp',
> >      'camera_sensor.cpp',
> >      'camera_sensor_properties.cpp',

Hello,

On 11/23/21 10:40 PM, Kieran Bingham wrote:
> Quoting Han-Lin Chen (2021-11-23 12:37:50)
>> The CameraLens class abstracts camera lens and provides helper
>> functions to ease interactions with them.
>>
>> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>   Documentation/index.rst                    |   1 +
>>   Documentation/lens_driver_requirements.rst |  28 ++++
>>   Documentation/meson.build                  |   1 +
>>   include/libcamera/internal/camera_lens.h   |  48 +++++++
>>   include/libcamera/internal/meson.build     |   1 +
>>   src/libcamera/camera_lens.cpp              | 150 +++++++++++++++++++++
>>   src/libcamera/meson.build                  |   1 +
>>   7 files changed, 230 insertions(+)
>>   create mode 100644 Documentation/lens_driver_requirements.rst
>>   create mode 100644 include/libcamera/internal/camera_lens.h
>>   create mode 100644 src/libcamera/camera_lens.cpp
>>
>> diff --git a/Documentation/index.rst b/Documentation/index.rst
>> index 1f4fc485..0ee10044 100644
>> --- a/Documentation/index.rst
>> +++ b/Documentation/index.rst
>> @@ -21,3 +21,4 @@
>>      Tracing guide <guides/tracing>
>>      Environment variables <environment_variables>
>>      Sensor driver requirements <sensor_driver_requirements>
>> +   Lens driver requirements <lens_driver_requirements>
>> diff --git a/Documentation/lens_driver_requirements.rst b/Documentation/lens_driver_requirements.rst
>> new file mode 100644
>> index 00000000..afc300cf
>> --- /dev/null
>> +++ b/Documentation/lens_driver_requirements.rst
>> @@ -0,0 +1,28 @@
>> +.. SPDX-License-Identifier: CC-BY-SA-4.0
>> +
>> +.. _lens-driver-requirements:
>> +
>> +Lens Driver Requirements
>> +========================
>> +
>> +libcamera handles lens devices in the CameraLens class and defines
>> +a consistent interface through its API towards other library components.
>> +
>> +The CameraLens class uses the V4L2 subdev kernel API to interface with the
>> +camera lens through one or multiple sub-devices exposed in userspace by
>> +the lens driver.
>> +
>> +In order for libcamera to be fully operational and provide all the required
>> +information to interface with the camera lens to applications and pipeline
>> +handlers, a set of mandatory features the driver has to support has been defined.
>> +
>> +Mandatory Requirements
>> +----------------------
>> +
>> +The lens driver is assumed to be fully compliant with the V4L2 specification.
>> +
>> +The lens driver shall support the following V4L2 controls:
>> +
>> +* `V4L2_CID_FOCUS_ABSOLUTE`_
>> +
>> +.. _V4L2_CID_FOCUS_ABSOLUTE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-camera.html
>> diff --git a/Documentation/meson.build b/Documentation/meson.build
>> index 4c972675..33af82aa 100644
>> --- a/Documentation/meson.build
>> +++ b/Documentation/meson.build
>> @@ -66,6 +66,7 @@ if sphinx.found()
>>           'guides/pipeline-handler.rst',
>>           'guides/tracing.rst',
>>           'index.rst',
>> +        'lens_driver_requirements.rst',
>>           'sensor_driver_requirements.rst',
>>          '../README.rst',
>>       ]
>> diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h
>> new file mode 100644
>> index 00000000..fe83d4ad
>> --- /dev/null
>> +++ b/include/libcamera/internal/camera_lens.h
>> @@ -0,0 +1,48 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Google Inc.
>> + *
>> + * camera_lens.h - A camera lens
>> + */
>> +#ifndef __LIBCAMERA_INTERNAL_CAMERA_LENS_H__
>> +#define __LIBCAMERA_INTERNAL_CAMERA_LENS_H__
>> +
>> +#include <libcamera/base/class.h>
>> +#include <libcamera/base/log.h>
>> +
>> +#include <libcamera/controls.h>
>> +
>> +#include "libcamera/internal/v4l2_subdevice.h"
>> +
>> +namespace libcamera {
>> +
>> +class MediaEntity;
>> +
>> +class CameraLens : protected Loggable
>> +{
>> +public:
>> +       explicit CameraLens(const MediaEntity *entity);
>> +       ~CameraLens();
>> +
>> +       int init();
>> +       int setFocusPostion(int32_t position);
>> +
>> +       const std::string &model() const { return model_; }
>> +
>> +protected:
>> +       std::string logPrefix() const override;
>> +
>> +private:
>> +       LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraLens)
>> +
>> +       int validateLensDriver();
>> +
>> +       const MediaEntity *entity_;
>> +       std::unique_ptr<V4L2Subdevice> subdev_;
>> +
>> +       std::string model_;
>> +};
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_INTERNAL_CAMERA_LENS_H__ */
>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>> index de8630fe..7e9b9998 100644
>> --- a/include/libcamera/internal/meson.build
>> +++ b/include/libcamera/internal/meson.build
>> @@ -14,6 +14,7 @@ libcamera_internal_headers = files([
>>       'byte_stream_buffer.h',
>>       'camera.h',
>>       'camera_controls.h',
>> +    'camera_lens.h',
>>       'camera_sensor.h',
>>       'camera_sensor_properties.h',
>>       'camera_utils.h',
>> diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp
>> new file mode 100644
>> index 00000000..cb7159fe
>> --- /dev/null
>> +++ b/src/libcamera/camera_lens.cpp
>> @@ -0,0 +1,150 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Google Inc.
>> + *
>> + * camera_lens.cpp - A camera lens
>> + */
>> +
>> +#include "libcamera/internal/camera_lens.h"
>> +
>> +#include <regex>
> I don't think regex is used anymore.
>
>> +#include <libcamera/property_ids.h>
>> +
>> +#include "libcamera/internal/camera_utils.h"
>> +#include "libcamera/internal/media_device.h"
>> +#include "libcamera/internal/sysfs.h"
> nor sysfs.h...
>
>> +
>> +/**
>> + * \file camera_lens.h
>> + * \brief A camera lens
>> + */
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(CameraLens)
>> +
>> +/**
>> + * \class CameraLens
>> + * \brief A camera lens based on V4L2 subdevices
>> + *
>> + * The CameraLens class eases handling of lens for pipeline handlers by
>> + * hiding the details of the V4L2 subdevice kernel API and caching lens
>> + * information.
>> + *
>> + * The implementation is currently limited to lens that expose a single V4L2
>> + * subdevice. It will be extended to support more complex devices as the needs
>> + * arise.
>> + */
>> +
>> +/**
>> + * \brief Construct a CameraLens
>> + * \param[in] entity The media entity backing the camera lens
>> + *
>> + * Once constructed the instance must be initialized with init().
>> + */
>> +CameraLens::CameraLens(const MediaEntity *entity)
>> +       : entity_(entity)
>> +{
>> +}
>> +
>> +/**
>> + * \brief Destroy a CameraLens
>> + */
>> +CameraLens::~CameraLens() = default;
>> +
>> +/**
>> + * \brief Initialize the camera lens instance
>> + *
>> + * This function performs the initialisation steps of the CameraLens that may
>> + * fail. It shall be called once and only once after constructing the instance.
>> + *
>> + * \return 0 on success or a negative error code otherwise
>> + */
>> +int CameraLens::init()
>> +{
>> +       if (entity_->function() != MEDIA_ENT_F_LENS) {
>> +               LOG(CameraLens, Error)
>> +                       << "Invalid lens function "
>> +                       << utils::hex(entity_->function());
>> +               return -EINVAL;
>> +       }
>> +
>> +       model_ = extractModelFromEntityName(entity_->name());
>> +
>> +       /* Create and open the subdev. */
>> +       subdev_ = std::make_unique<V4L2Subdevice>(entity_);
>> +       int ret = subdev_->open();
>> +       if (ret < 0)
>> +               return ret;
> This is probably why I think I'd make the model handled by the
> V4L2Subdevice.
>
> We /know/ we're creating a V4L2Subdevice here, so we could assign
> something like
>
> 	model_ = subdev_->model();
>
> Or simply have model() call directly onto the subdev which would have
> the implementation...
>
>
>> +
>> +       ret = validateLensDriver();
>> +       if (ret)
>> +               return ret;
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * \brief This function sets the focal point of the lens to a specific position.
>> + * \param[in] position The focal point of the lens
>> + *
>> + * This function sets the value of focal point of the lens as in \a position.
>> + *
>> + * \return 0 on success or -EINVAL otherwise
>> + */
>> +int CameraLens::setFocusPostion(int32_t position)
>> +{
>> +       ControlList lensCtrls(subdev_->controls());
>> +       lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast<int32_t>(position));
>> +
>> +       if (subdev_->setControls(&lensCtrls))
>> +               return -EINVAL;
>> +
>> +       return 0;
>> +}
> I fear this is going the same way that the TestPatternMode is going on
> the CameraSensor.
>
> I don't think we should expose a function for each control and setting
> on the devices.
>
> The device should accept the controls, parse it and apply it.
>
> The setFocusPosition helper function is probably good to keep as it is -
> but I think a CameraLens should be taking a ControlList (of libcamera
> controls) and doing something like:
>
>
> +/**
> + * \brief Write libcamera controls to the lens
> + * \param[in] list The list of controls to write
> + *
> + * Supported libcamera controls are interpreted and converted to V4L2
> + * controls to write to the device.
> + *
> + * \return 0 on success or an error code otherwise
> + * \retval -EINVAL One of the control is not supported or not accessible
> + */
> +int CameraLens::setControls(ControlList *list)
> +{
> +	if (!list)
> +		return -ENODATA;
> +
> +	const ControlIdMap *idmap = list->idMap();
> +	if (idmap != &controls::controls) {
> +		LOG(CameraLens, Error) << "Invalid Control List";
> +		return -EINVAL;
> +	}
> +
> +	ControlList subdevControls;
> +
> +	for (const auto &ctrl : *list) {
> +		unsigned int id = ctrl.first;
> +		const ControlValue &value = ctrl.second;
> +
> +		// switch case would be nicer here.... but isn't
> +		// currently possible
> +
> +		// Also not a libcamera control yet
> +		if (id == controls::FocusAbsolute) {
> +			int position = value.get<int>();
> +			subdevControls.set(V4L2_CID_FOCUS_ABSOLUTE, position);
> +		}
> +
> +		// Not a real libcamera control currently
> +		if (id == controls::FocusRelative) {
> +			// Not implemented, showing other control
> +			// handling only
> +		}
> +	}
> +
> +	return subdev_->setControls(&subdevControls);
> +}
> +
>
> I think this is what CameraSensor should be doing too, but we have this
> awkward precedent that it can already take a V4L2ControlList directly...
>
>
> Given that a lens can only currently set a position anyway, it's not
> such a big deal if this is considered if/when we need more controls
> instead, and we keep the direct and simple setting function.


I think it's a good design to set controls via a ControlList. It makes 
sense when seen in conjunction with validateLensDriver() below, which 
(will) iterate over multiple controls deemed mandatory. Same should 
follow for setting controls, I think.

We can capture the design idea somewhere in the codebase in fact, if we 
decide to implement it later.

>
> --
> Kieran
>
>
>> +
>> +int CameraLens::validateLensDriver()
>> +{
>> +       int err = 0;


Can we make this, for simplicity

             int ret = 0;

>> +       static constexpr uint32_t mandatoryControls[] = {
>> +               V4L2_CID_FOCUS_ABSOLUTE


s/V4L2_CID_FOCUS_ABSOLUTE/V4L2_CID_FOCUS_ABSOLUTE,/  to ease off diffs later

>> +       };
>> +
>> +       const ControlInfoMap &controls = subdev_->controls();
>> +       for (uint32_t ctrl : mandatoryControls) {
>> +               if (!controls.count(ctrl)) {
>> +                       LOG(CameraLens, Error)
>> +                               << "Mandatory V4L2 control " << utils::hex(ctrl)
>> +                               << " not available";
>> +                       err = -EINVAL;
             ret = -EINVAL
>> +               }
>> +       }
>> +
>> +       if (err) {
             if (ret <0) {
>> +               LOG(CameraLens, Error)
>> +                       << "The lens kernel driver needs to be fixed";
>> +               LOG(CameraLens, Error)
>> +                       << "See Documentation/lens_driver_requirements.rst in"
>> +                       << " the libcamera sources for more information";


Should we log this as one Error message instead of two?

>> +               return err;
remove this line
>> +       }
>> +
>> +       return 0;

         return ret;

>> +}
>> +
>> +/**
>> + * \fn CameraLens::model()
>> + * \brief Retrieve the lens model name
>> + *
>> + * The lens model name is a free-formed string that uniquely identifies the
>> + * lens model.
>> + *
>> + * \return The lens model name
>> + */
>> +
>> +std::string CameraLens::logPrefix() const
>> +{
>> +       return "'" + entity_->name() + "'";
>> +}
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 563ed861..2100805d 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -5,6 +5,7 @@ libcamera_sources = files([
>>       'byte_stream_buffer.cpp',
>>       'camera.cpp',
>>       'camera_controls.cpp',
>> +    'camera_lens.cpp',
>>       'camera_manager.cpp',
>>       'camera_sensor.cpp',
>>       'camera_sensor_properties.cpp',
>> -- 
>> 2.34.0.rc2.393.gf8c9666880-goog
>>