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

14553 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>
---
 Documentation/index.rst                    |   1 +
 Documentation/lens_driver_requirements.rst |  28 +++
 Documentation/meson.build                  |   1 +
 include/libcamera/internal/camera_lens.h   |  60 +++++
 include/libcamera/internal/meson.build     |   1 +
 src/libcamera/camera_lens.cpp              | 253 +++++++++++++++++++++
 src/libcamera/meson.build                  |   1 +
 7 files changed, 345 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-11 10:49:57)
> The CameraLens class abstracts camera lens and provides helper
> functions to ease interactions with them.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> ---
>  Documentation/index.rst                    |   1 +
>  Documentation/lens_driver_requirements.rst |  28 +++
>  Documentation/meson.build                  |   1 +
>  include/libcamera/internal/camera_lens.h   |  60 +++++
>  include/libcamera/internal/meson.build     |   1 +
>  src/libcamera/camera_lens.cpp              | 253 +++++++++++++++++++++
>  src/libcamera/meson.build                  |   1 +
>  7 files changed, 345 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..fba5d1fa
> --- /dev/null
> +++ b/include/libcamera/internal/camera_lens.h
> @@ -0,0 +1,60 @@
> +/* 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>

Our checkstyle doesn't cope well with highlighting diffs on new files.
If fixed, it would report a new line to add here

This could be fixed while applying.


> +#include "libcamera/internal/v4l2_subdevice.h"
> +
> +namespace libcamera {
> +
> +class MediaEntity;
> +
> +class CameraLens : protected Loggable
> +{
> +public:
> +       explicit CameraLens(const MediaEntity *entity);
> +       ~CameraLens() = default;
> +
> +       int init();
> +
> +       const std::string &model() const { return model_; }
> +       const MediaEntity *entity() const { return entity_; }
> +
> +       const ControlInfoMap &controls() const;
> +       ControlList getControls(const std::vector<uint32_t> &ids);
> +       int setControls(ControlList *ctrls);
> +
> +       V4L2Subdevice *device() { return subdev_.get(); }
> +
> +       const ControlList &properties() const { return properties_; }
> +
> +protected:
> +       std::string logPrefix() const override;
> +
> +private:
> +       LIBCAMERA_DISABLE_COPY(CameraLens)
> +
> +       int generateId();
> +       int validateLensDriver();
> +       int initProperties();
> +
> +       const MediaEntity *entity_;
> +       std::unique_ptr<V4L2Subdevice> subdev_;
> +
> +       std::string model_;
> +       std::string id_;
> +
> +       ControlList properties_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_CAMERA_LENS_H__ */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 665fd6de..a96bbb95 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',
>      'control_serializer.h',
> diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp
> new file mode 100644
> index 00000000..944f807e
> --- /dev/null
> +++ b/src/libcamera/camera_lens.cpp
> @@ -0,0 +1,253 @@
> +/* 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>
> +#include <libcamera/property_ids.h>
> +
> +#include "libcamera/internal/media_device.h"
> +#include "libcamera/internal/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),
> +         properties_(properties::properties)
> +{
> +}
> +
> +/**
> + * \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;
> +       }
> +
> +       /* Create and open the subdev. */
> +       subdev_ = std::make_unique<V4L2Subdevice>(entity_);
> +       int ret = subdev_->open();
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = validateLensDriver();
> +       if (ret)
> +               return ret;
> +
> +       ret = initProperties();
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +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;
> +}
> +
> +int CameraLens::initProperties()
> +{
> +       /*
> +        * Extract the camera lens model name from the media entity name.
> +        *
> +        * There is no standardized naming scheme for lens entities in the
> +        * Linux kernel at the moment.
> +        *
> +        * - The most common rule, used by I2C lens, associates the model
> +        *   name with the I2C bus number and address (e.g. 'dw9714 8-000c').
> +        *
> +        * Other schemes probably exist. As a best effort heuristic, use the
> +        * part of the entity name before the first space if the name contains
> +        * an I2C address, and use the full entity name otherwise.
> +        */
> +       std::string entityName = entity_->name();
> +       std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> +       std::smatch match;
> +
> +       if (std::regex_search(entityName, match, i2cRegex))
> +               model_ = entityName.substr(0, entityName.find(' '));
> +       else
> +               model_ = entityName;
> +
> +       properties_.set(properties::Model, utils::toAscii(model_));
> +
> +       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
> + */
> +
> +/**
> + * \fn CameraLens::id()
> + * \brief Retrieve the lens ID
> + *
> + * The lens ID is a free-form string that uniquely identifies the lens in
> + * the system.
> + *
> + * \return The lens ID
> + */
> +
> +/**
> + * \fn CameraLens::entity()
> + * \brief Retrieve the lens media entity
> + * \return The lens media entity
> + */
> +
> +/**
> + * \brief Retrieve the supported V4L2 controls and their information
> + *
> + * Control information is updated automatically to reflect the current lens
> + * configuration when the setFormat() function is called, without invalidating
> + * any iterator on the ControlInfoMap.
> + *
> + * \return A map of the V4L2 controls supported by the lens
> + */
> +const ControlInfoMap &CameraLens::controls() const
> +{
> +       return subdev_->controls();
> +}
> +
> +/**
> + * \brief Read V4L2 controls from the lens
> + * \param[in] ids The list of controls to read, specified by their ID
> + *
> + * This function reads the value of all controls contained in \a ids, and
> + * returns their values as a ControlList. The control identifiers are defined by
> + * the V4L2 specification (V4L2_CID_*).
> + *
> + * If any control in \a ids is not supported by the device, is disabled (i.e.
> + * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs
> + * during validation of the requested controls, no control is read and this
> + * function returns an empty control list.
> + *
> + * \sa V4L2Device::getControls()
> + *
> + * \return The control values in a ControlList on success, or an empty list on
> + * error
> + */
> +ControlList CameraLens::getControls(const std::vector<uint32_t> &ids)
> +{
> +       return subdev_->getControls(ids);
> +}
> +
> +/**
> + * \brief Write V4L2 controls to the lens
> + * \param[in] ctrls The list of controls to write
> + *
> + * This function writes the value of all controls contained in \a ctrls, and
> + * stores the values actually applied to the device in the corresponding \a
> + * ctrls entry. The control identifiers are defined by the V4L2 specification
> + * (V4L2_CID_*).
> + *
> + * If any control in \a ctrls is not supported by the device, is disabled (i.e.
> + * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other
> + * error occurs during validation of the requested controls, no control is
> + * written and this function returns -EINVAL.
> + *
> + * If an error occurs while writing the controls, the index of the first
> + * control that couldn't be written is returned. All controls below that index
> + * are written and their values are updated in \a ctrls, while all other
> + * controls are not written and their values are not changed.
> + *
> + * \sa V4L2Device::setControls()
> + *
> + * \return 0 on success or an error code otherwise
> + * \retval -EINVAL One of the control is not supported or not accessible
> + * \retval i The index of the control that failed
> + */
> +int CameraLens::setControls(ControlList *ctrls)
> +{
> +       return subdev_->setControls(ctrls);
> +}
> +
> +/**
> + * \fn CameraLens::device()
> + * \brief Retrieve the camera lens device
> + * \todo Remove this function by integrating DelayedControl with CameraLens

Do VCM controls need to be delayed? Or is this just to be able to
synchronise what position was applied to a given frame?

No objection to keeping this as a todo though as we still need to
investigate and figure out how DelayedControls works with multiple
devices in that case.

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


I see this and the previous patch as definite building blocks that are
going to be useful moving forwards.


> + * \return The camera lens device
> + */
> +
> +/**
> + * \fn CameraLens::properties()
> + * \brief Retrieve the camera lens properties
> + * \return The list of camera lens properties
> + */
> +
> +std::string CameraLens::logPrefix() const
> +{
> +       return "'" + entity_->name() + "'";
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 6727a777..3bee8fee 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.rc1.387.gb447b232ab-goog
>

Hello,

On Fri, Nov 19, 2021 at 04:09:11PM +0000, Kieran Bingham wrote:
> Quoting Han-Lin Chen (2021-11-11 10:49:57)
> > The CameraLens class abstracts camera lens and provides helper
> > functions to ease interactions with them.
> > 
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > ---
> >  Documentation/index.rst                    |   1 +
> >  Documentation/lens_driver_requirements.rst |  28 +++
> >  Documentation/meson.build                  |   1 +
> >  include/libcamera/internal/camera_lens.h   |  60 +++++
> >  include/libcamera/internal/meson.build     |   1 +
> >  src/libcamera/camera_lens.cpp              | 253 +++++++++++++++++++++
> >  src/libcamera/meson.build                  |   1 +
> >  7 files changed, 345 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..fba5d1fa
> > --- /dev/null
> > +++ b/include/libcamera/internal/camera_lens.h
> > @@ -0,0 +1,60 @@
> > +/* 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>
> 
> Our checkstyle doesn't cope well with highlighting diffs on new files.
> If fixed, it would report a new line to add here
> 
> This could be fixed while applying.
> 
> > +#include "libcamera/internal/v4l2_subdevice.h"
> > +
> > +namespace libcamera {
> > +
> > +class MediaEntity;
> > +
> > +class CameraLens : protected Loggable
> > +{
> > +public:
> > +       explicit CameraLens(const MediaEntity *entity);
> > +       ~CameraLens() = default;

As destroying a CameraLens isn't performance-sensitive, let's move the
implementation to the .cpp file with

Camera::~CameraLens() = default;

> > +
> > +       int init();
> > +
> > +       const std::string &model() const { return model_; }
> > +       const MediaEntity *entity() const { return entity_; }

This isn't used, so I'd drop it.

> > +
> > +       const ControlInfoMap &controls() const;
> > +       ControlList getControls(const std::vector<uint32_t> &ids);
> > +       int setControls(ControlList *ctrls);
> > +
> > +       V4L2Subdevice *device() { return subdev_.get(); }
> > +
> > +       const ControlList &properties() const { return properties_; }

This doesn't seem to be used either.

> > +
> > +protected:
> > +       std::string logPrefix() const override;
> > +
> > +private:
> > +       LIBCAMERA_DISABLE_COPY(CameraLens)

Does CameraLens need to be movable, or can we disable move too ?

> > +
> > +       int generateId();
> > +       int validateLensDriver();
> > +       int initProperties();
> > +
> > +       const MediaEntity *entity_;
> > +       std::unique_ptr<V4L2Subdevice> subdev_;
> > +
> > +       std::string model_;
> > +       std::string id_;
> > +
> > +       ControlList properties_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_INTERNAL_CAMERA_LENS_H__ */
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 665fd6de..a96bbb95 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',
> >      'control_serializer.h',
> > diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp
> > new file mode 100644
> > index 00000000..944f807e
> > --- /dev/null
> > +++ b/src/libcamera/camera_lens.cpp
> > @@ -0,0 +1,253 @@
> > +/* 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>
> > +#include <libcamera/property_ids.h>
> > +
> > +#include "libcamera/internal/media_device.h"
> > +#include "libcamera/internal/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),
> > +         properties_(properties::properties)
> > +{
> > +}
> > +
> > +/**
> > + * \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;
> > +       }
> > +
> > +       /* Create and open the subdev. */
> > +       subdev_ = std::make_unique<V4L2Subdevice>(entity_);
> > +       int ret = subdev_->open();
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret = validateLensDriver();
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = initProperties();
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +
> > +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;
> > +}
> > +
> > +int CameraLens::initProperties()
> > +{
> > +       /*
> > +        * Extract the camera lens model name from the media entity name.
> > +        *
> > +        * There is no standardized naming scheme for lens entities in the
> > +        * Linux kernel at the moment.
> > +        *
> > +        * - The most common rule, used by I2C lens, associates the model
> > +        *   name with the I2C bus number and address (e.g. 'dw9714 8-000c').
> > +        *
> > +        * Other schemes probably exist. As a best effort heuristic, use the
> > +        * part of the entity name before the first space if the name contains
> > +        * an I2C address, and use the full entity name otherwise.
> > +        */
> > +       std::string entityName = entity_->name();
> > +       std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> > +       std::smatch match;
> > +
> > +       if (std::regex_search(entityName, match, i2cRegex))
> > +               model_ = entityName.substr(0, entityName.find(' '));
> > +       else
> > +               model_ = entityName;

Seems like a candidate for code refactoring, as CameraSensor implements
the same.

> > +
> > +       properties_.set(properties::Model, utils::toAscii(model_));
> > +
> > +       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
> > + */
> > +
> > +/**
> > + * \fn CameraLens::id()
> > + * \brief Retrieve the lens ID
> > + *
> > + * The lens ID is a free-form string that uniquely identifies the lens in
> > + * the system.
> > + *
> > + * \return The lens ID
> > + */
> > +
> > +/**
> > + * \fn CameraLens::entity()
> > + * \brief Retrieve the lens media entity
> > + * \return The lens media entity
> > + */
> > +
> > +/**
> > + * \brief Retrieve the supported V4L2 controls and their information
> > + *
> > + * Control information is updated automatically to reflect the current lens
> > + * configuration when the setFormat() function is called, without invalidating
> > + * any iterator on the ControlInfoMap.
> > + *
> > + * \return A map of the V4L2 controls supported by the lens
> > + */
> > +const ControlInfoMap &CameraLens::controls() const
> > +{
> > +       return subdev_->controls();
> > +}

I'd like to avoid introducing a dependency on V4L2 in the API exposed by
this class. It's fine to hardcode V4L2 usage internally, but the API
should expose either libcamera controls, or an ad-hoc API.

> > +
> > +/**
> > + * \brief Read V4L2 controls from the lens
> > + * \param[in] ids The list of controls to read, specified by their ID
> > + *
> > + * This function reads the value of all controls contained in \a ids, and
> > + * returns their values as a ControlList. The control identifiers are defined by
> > + * the V4L2 specification (V4L2_CID_*).
> > + *
> > + * If any control in \a ids is not supported by the device, is disabled (i.e.
> > + * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs
> > + * during validation of the requested controls, no control is read and this
> > + * function returns an empty control list.
> > + *
> > + * \sa V4L2Device::getControls()
> > + *
> > + * \return The control values in a ControlList on success, or an empty list on
> > + * error
> > + */
> > +ControlList CameraLens::getControls(const std::vector<uint32_t> &ids)
> > +{
> > +       return subdev_->getControls(ids);
> > +}
> > +
> > +/**
> > + * \brief Write V4L2 controls to the lens
> > + * \param[in] ctrls The list of controls to write
> > + *
> > + * This function writes the value of all controls contained in \a ctrls, and
> > + * stores the values actually applied to the device in the corresponding \a
> > + * ctrls entry. The control identifiers are defined by the V4L2 specification
> > + * (V4L2_CID_*).
> > + *
> > + * If any control in \a ctrls is not supported by the device, is disabled (i.e.
> > + * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other
> > + * error occurs during validation of the requested controls, no control is
> > + * written and this function returns -EINVAL.
> > + *
> > + * If an error occurs while writing the controls, the index of the first
> > + * control that couldn't be written is returned. All controls below that index
> > + * are written and their values are updated in \a ctrls, while all other
> > + * controls are not written and their values are not changed.
> > + *
> > + * \sa V4L2Device::setControls()
> > + *
> > + * \return 0 on success or an error code otherwise
> > + * \retval -EINVAL One of the control is not supported or not accessible
> > + * \retval i The index of the control that failed
> > + */
> > +int CameraLens::setControls(ControlList *ctrls)
> > +{
> > +       return subdev_->setControls(ctrls);
> > +}
> > +
> > +/**
> > + * \fn CameraLens::device()
> > + * \brief Retrieve the camera lens device
> > + * \todo Remove this function by integrating DelayedControl with CameraLens

The function doesn't seem to be used in this series, can't we drop it
already ?

> Do VCM controls need to be delayed? Or is this just to be able to
> synchronise what position was applied to a given frame?
> 
> No objection to keeping this as a todo though as we still need to
> investigate and figure out how DelayedControls works with multiple
> devices in that case.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> I see this and the previous patch as definite building blocks that are
> going to be useful moving forwards.
> 
> > + * \return The camera lens device
> > + */
> > +
> > +/**
> > + * \fn CameraLens::properties()
> > + * \brief Retrieve the camera lens properties
> > + * \return The list of camera lens properties
> > + */
> > +
> > +std::string CameraLens::logPrefix() const
> > +{
> > +       return "'" + entity_->name() + "'";
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 6727a777..3bee8fee 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',

On Sat, Nov 20, 2021 at 12:09 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Han-Lin Chen (2021-11-11 10:49:57)
> > The CameraLens class abstracts camera lens and provides helper
> > functions to ease interactions with them.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > ---
> >  Documentation/index.rst                    |   1 +
> >  Documentation/lens_driver_requirements.rst |  28 +++
> >  Documentation/meson.build                  |   1 +
> >  include/libcamera/internal/camera_lens.h   |  60 +++++
> >  include/libcamera/internal/meson.build     |   1 +
> >  src/libcamera/camera_lens.cpp              | 253 +++++++++++++++++++++
> >  src/libcamera/meson.build                  |   1 +
> >  7 files changed, 345 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..fba5d1fa
> > --- /dev/null
> > +++ b/include/libcamera/internal/camera_lens.h
> > @@ -0,0 +1,60 @@
> > +/* 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>
>
> Our checkstyle doesn't cope well with highlighting diffs on new files.
> If fixed, it would report a new line to add here
>
> This could be fixed while applying.
>
>
> > +#include "libcamera/internal/v4l2_subdevice.h"
> > +
> > +namespace libcamera {
> > +
> > +class MediaEntity;
> > +
> > +class CameraLens : protected Loggable
> > +{
> > +public:
> > +       explicit CameraLens(const MediaEntity *entity);
> > +       ~CameraLens() = default;
> > +
> > +       int init();
> > +
> > +       const std::string &model() const { return model_; }
> > +       const MediaEntity *entity() const { return entity_; }
> > +
> > +       const ControlInfoMap &controls() const;
> > +       ControlList getControls(const std::vector<uint32_t> &ids);
> > +       int setControls(ControlList *ctrls);
> > +
> > +       V4L2Subdevice *device() { return subdev_.get(); }
> > +
> > +       const ControlList &properties() const { return properties_; }
> > +
> > +protected:
> > +       std::string logPrefix() const override;
> > +
> > +private:
> > +       LIBCAMERA_DISABLE_COPY(CameraLens)
> > +
> > +       int generateId();
> > +       int validateLensDriver();
> > +       int initProperties();
> > +
> > +       const MediaEntity *entity_;
> > +       std::unique_ptr<V4L2Subdevice> subdev_;
> > +
> > +       std::string model_;
> > +       std::string id_;
> > +
> > +       ControlList properties_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_INTERNAL_CAMERA_LENS_H__ */
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 665fd6de..a96bbb95 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',
> >      'control_serializer.h',
> > diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp
> > new file mode 100644
> > index 00000000..944f807e
> > --- /dev/null
> > +++ b/src/libcamera/camera_lens.cpp
> > @@ -0,0 +1,253 @@
> > +/* 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>
> > +#include <libcamera/property_ids.h>
> > +
> > +#include "libcamera/internal/media_device.h"
> > +#include "libcamera/internal/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),
> > +         properties_(properties::properties)
> > +{
> > +}
> > +
> > +/**
> > + * \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;
> > +       }
> > +
> > +       /* Create and open the subdev. */
> > +       subdev_ = std::make_unique<V4L2Subdevice>(entity_);
> > +       int ret = subdev_->open();
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret = validateLensDriver();
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = initProperties();
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +
> > +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;
> > +}
> > +
> > +int CameraLens::initProperties()
> > +{
> > +       /*
> > +        * Extract the camera lens model name from the media entity name.
> > +        *
> > +        * There is no standardized naming scheme for lens entities in the
> > +        * Linux kernel at the moment.
> > +        *
> > +        * - The most common rule, used by I2C lens, associates the model
> > +        *   name with the I2C bus number and address (e.g. 'dw9714 8-000c').
> > +        *
> > +        * Other schemes probably exist. As a best effort heuristic, use the
> > +        * part of the entity name before the first space if the name contains
> > +        * an I2C address, and use the full entity name otherwise.
> > +        */
> > +       std::string entityName = entity_->name();
> > +       std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> > +       std::smatch match;
> > +
> > +       if (std::regex_search(entityName, match, i2cRegex))
> > +               model_ = entityName.substr(0, entityName.find(' '));
> > +       else
> > +               model_ = entityName;
> > +
> > +       properties_.set(properties::Model, utils::toAscii(model_));
> > +
> > +       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
> > + */
> > +
> > +/**
> > + * \fn CameraLens::id()
> > + * \brief Retrieve the lens ID
> > + *
> > + * The lens ID is a free-form string that uniquely identifies the lens in
> > + * the system.
> > + *
> > + * \return The lens ID
> > + */
> > +
> > +/**
> > + * \fn CameraLens::entity()
> > + * \brief Retrieve the lens media entity
> > + * \return The lens media entity
> > + */
> > +
> > +/**
> > + * \brief Retrieve the supported V4L2 controls and their information
> > + *
> > + * Control information is updated automatically to reflect the current lens
> > + * configuration when the setFormat() function is called, without invalidating
> > + * any iterator on the ControlInfoMap.
> > + *
> > + * \return A map of the V4L2 controls supported by the lens
> > + */
> > +const ControlInfoMap &CameraLens::controls() const
> > +{
> > +       return subdev_->controls();
> > +}
> > +
> > +/**
> > + * \brief Read V4L2 controls from the lens
> > + * \param[in] ids The list of controls to read, specified by their ID
> > + *
> > + * This function reads the value of all controls contained in \a ids, and
> > + * returns their values as a ControlList. The control identifiers are defined by
> > + * the V4L2 specification (V4L2_CID_*).
> > + *
> > + * If any control in \a ids is not supported by the device, is disabled (i.e.
> > + * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs
> > + * during validation of the requested controls, no control is read and this
> > + * function returns an empty control list.
> > + *
> > + * \sa V4L2Device::getControls()
> > + *
> > + * \return The control values in a ControlList on success, or an empty list on
> > + * error
> > + */
> > +ControlList CameraLens::getControls(const std::vector<uint32_t> &ids)
> > +{
> > +       return subdev_->getControls(ids);
> > +}
> > +
> > +/**
> > + * \brief Write V4L2 controls to the lens
> > + * \param[in] ctrls The list of controls to write
> > + *
> > + * This function writes the value of all controls contained in \a ctrls, and
> > + * stores the values actually applied to the device in the corresponding \a
> > + * ctrls entry. The control identifiers are defined by the V4L2 specification
> > + * (V4L2_CID_*).
> > + *
> > + * If any control in \a ctrls is not supported by the device, is disabled (i.e.
> > + * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other
> > + * error occurs during validation of the requested controls, no control is
> > + * written and this function returns -EINVAL.
> > + *
> > + * If an error occurs while writing the controls, the index of the first
> > + * control that couldn't be written is returned. All controls below that index
> > + * are written and their values are updated in \a ctrls, while all other
> > + * controls are not written and their values are not changed.
> > + *
> > + * \sa V4L2Device::setControls()
> > + *
> > + * \return 0 on success or an error code otherwise
> > + * \retval -EINVAL One of the control is not supported or not accessible
> > + * \retval i The index of the control that failed
> > + */
> > +int CameraLens::setControls(ControlList *ctrls)
> > +{
> > +       return subdev_->setControls(ctrls);
> > +}
> > +
> > +/**
> > + * \fn CameraLens::device()
> > + * \brief Retrieve the camera lens device
> > + * \todo Remove this function by integrating DelayedControl with CameraLens
>
> Do VCM controls need to be delayed? Or is this just to be able to
> synchronise what position was applied to a given frame?
I don't see any example on this, both from implementation known to me
or the V4L2 spec.
I suppose it would be delayed, but for some reasons it's hard to know
the exact synchronized frame, since
1. The frame SOF is controlled by the sensor device, and the VCM
somehow works independently.
2. The time needed to move the VCM might depend on the current
position of the lens, and it varies at the runtime.
>
> No objection to keeping this as a todo though as we still need to
> investigate and figure out how DelayedControls works with multiple
> devices in that case.
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>
> I see this and the previous patch as definite building blocks that are
> going to be useful moving forwards.
>
>
> > + * \return The camera lens device
> > + */
> > +
> > +/**
> > + * \fn CameraLens::properties()
> > + * \brief Retrieve the camera lens properties
> > + * \return The list of camera lens properties
> > + */
> > +
> > +std::string CameraLens::logPrefix() const
> > +{
> > +       return "'" + entity_->name() + "'";
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 6727a777..3bee8fee 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.rc1.387.gb447b232ab-goog
> >

Hello,

On Tue, Nov 23, 2021 at 08:44:59PM +0800, Hanlin Chen wrote:
> On Sat, Nov 20, 2021 at 12:09 AM Kieran Bingham wrote:
> > Quoting Han-Lin Chen (2021-11-11 10:49:57)
> > > The CameraLens class abstracts camera lens and provides helper
> > > functions to ease interactions with them.
> > >
> > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > > ---
> > >  Documentation/index.rst                    |   1 +
> > >  Documentation/lens_driver_requirements.rst |  28 +++
> > >  Documentation/meson.build                  |   1 +
> > >  include/libcamera/internal/camera_lens.h   |  60 +++++
> > >  include/libcamera/internal/meson.build     |   1 +
> > >  src/libcamera/camera_lens.cpp              | 253 +++++++++++++++++++++
> > >  src/libcamera/meson.build                  |   1 +
> > >  7 files changed, 345 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..fba5d1fa
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/camera_lens.h
> > > @@ -0,0 +1,60 @@
> > > +/* 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>
> >
> > Our checkstyle doesn't cope well with highlighting diffs on new files.
> > If fixed, it would report a new line to add here
> >
> > This could be fixed while applying.
> >
> >
> > > +#include "libcamera/internal/v4l2_subdevice.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +class MediaEntity;
> > > +
> > > +class CameraLens : protected Loggable
> > > +{
> > > +public:
> > > +       explicit CameraLens(const MediaEntity *entity);
> > > +       ~CameraLens() = default;
> > > +
> > > +       int init();
> > > +
> > > +       const std::string &model() const { return model_; }
> > > +       const MediaEntity *entity() const { return entity_; }
> > > +
> > > +       const ControlInfoMap &controls() const;
> > > +       ControlList getControls(const std::vector<uint32_t> &ids);
> > > +       int setControls(ControlList *ctrls);
> > > +
> > > +       V4L2Subdevice *device() { return subdev_.get(); }
> > > +
> > > +       const ControlList &properties() const { return properties_; }
> > > +
> > > +protected:
> > > +       std::string logPrefix() const override;
> > > +
> > > +private:
> > > +       LIBCAMERA_DISABLE_COPY(CameraLens)
> > > +
> > > +       int generateId();
> > > +       int validateLensDriver();
> > > +       int initProperties();
> > > +
> > > +       const MediaEntity *entity_;
> > > +       std::unique_ptr<V4L2Subdevice> subdev_;
> > > +
> > > +       std::string model_;
> > > +       std::string id_;
> > > +
> > > +       ControlList properties_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_INTERNAL_CAMERA_LENS_H__ */
> > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > index 665fd6de..a96bbb95 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',
> > >      'control_serializer.h',
> > > diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp
> > > new file mode 100644
> > > index 00000000..944f807e
> > > --- /dev/null
> > > +++ b/src/libcamera/camera_lens.cpp
> > > @@ -0,0 +1,253 @@
> > > +/* 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>
> > > +#include <libcamera/property_ids.h>
> > > +
> > > +#include "libcamera/internal/media_device.h"
> > > +#include "libcamera/internal/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),
> > > +         properties_(properties::properties)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \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;
> > > +       }
> > > +
> > > +       /* Create and open the subdev. */
> > > +       subdev_ = std::make_unique<V4L2Subdevice>(entity_);
> > > +       int ret = subdev_->open();
> > > +       if (ret < 0)
> > > +               return ret;
> > > +
> > > +       ret = validateLensDriver();
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       ret = initProperties();
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +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;
> > > +}
> > > +
> > > +int CameraLens::initProperties()
> > > +{
> > > +       /*
> > > +        * Extract the camera lens model name from the media entity name.
> > > +        *
> > > +        * There is no standardized naming scheme for lens entities in the
> > > +        * Linux kernel at the moment.
> > > +        *
> > > +        * - The most common rule, used by I2C lens, associates the model
> > > +        *   name with the I2C bus number and address (e.g. 'dw9714 8-000c').
> > > +        *
> > > +        * Other schemes probably exist. As a best effort heuristic, use the
> > > +        * part of the entity name before the first space if the name contains
> > > +        * an I2C address, and use the full entity name otherwise.
> > > +        */
> > > +       std::string entityName = entity_->name();
> > > +       std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> > > +       std::smatch match;
> > > +
> > > +       if (std::regex_search(entityName, match, i2cRegex))
> > > +               model_ = entityName.substr(0, entityName.find(' '));
> > > +       else
> > > +               model_ = entityName;
> > > +
> > > +       properties_.set(properties::Model, utils::toAscii(model_));
> > > +
> > > +       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
> > > + */
> > > +
> > > +/**
> > > + * \fn CameraLens::id()
> > > + * \brief Retrieve the lens ID
> > > + *
> > > + * The lens ID is a free-form string that uniquely identifies the lens in
> > > + * the system.
> > > + *
> > > + * \return The lens ID
> > > + */
> > > +
> > > +/**
> > > + * \fn CameraLens::entity()
> > > + * \brief Retrieve the lens media entity
> > > + * \return The lens media entity
> > > + */
> > > +
> > > +/**
> > > + * \brief Retrieve the supported V4L2 controls and their information
> > > + *
> > > + * Control information is updated automatically to reflect the current lens
> > > + * configuration when the setFormat() function is called, without invalidating
> > > + * any iterator on the ControlInfoMap.
> > > + *
> > > + * \return A map of the V4L2 controls supported by the lens
> > > + */
> > > +const ControlInfoMap &CameraLens::controls() const
> > > +{
> > > +       return subdev_->controls();
> > > +}
> > > +
> > > +/**
> > > + * \brief Read V4L2 controls from the lens
> > > + * \param[in] ids The list of controls to read, specified by their ID
> > > + *
> > > + * This function reads the value of all controls contained in \a ids, and
> > > + * returns their values as a ControlList. The control identifiers are defined by
> > > + * the V4L2 specification (V4L2_CID_*).
> > > + *
> > > + * If any control in \a ids is not supported by the device, is disabled (i.e.
> > > + * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs
> > > + * during validation of the requested controls, no control is read and this
> > > + * function returns an empty control list.
> > > + *
> > > + * \sa V4L2Device::getControls()
> > > + *
> > > + * \return The control values in a ControlList on success, or an empty list on
> > > + * error
> > > + */
> > > +ControlList CameraLens::getControls(const std::vector<uint32_t> &ids)
> > > +{
> > > +       return subdev_->getControls(ids);
> > > +}
> > > +
> > > +/**
> > > + * \brief Write V4L2 controls to the lens
> > > + * \param[in] ctrls The list of controls to write
> > > + *
> > > + * This function writes the value of all controls contained in \a ctrls, and
> > > + * stores the values actually applied to the device in the corresponding \a
> > > + * ctrls entry. The control identifiers are defined by the V4L2 specification
> > > + * (V4L2_CID_*).
> > > + *
> > > + * If any control in \a ctrls is not supported by the device, is disabled (i.e.
> > > + * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other
> > > + * error occurs during validation of the requested controls, no control is
> > > + * written and this function returns -EINVAL.
> > > + *
> > > + * If an error occurs while writing the controls, the index of the first
> > > + * control that couldn't be written is returned. All controls below that index
> > > + * are written and their values are updated in \a ctrls, while all other
> > > + * controls are not written and their values are not changed.
> > > + *
> > > + * \sa V4L2Device::setControls()
> > > + *
> > > + * \return 0 on success or an error code otherwise
> > > + * \retval -EINVAL One of the control is not supported or not accessible
> > > + * \retval i The index of the control that failed
> > > + */
> > > +int CameraLens::setControls(ControlList *ctrls)
> > > +{
> > > +       return subdev_->setControls(ctrls);
> > > +}
> > > +
> > > +/**
> > > + * \fn CameraLens::device()
> > > + * \brief Retrieve the camera lens device
> > > + * \todo Remove this function by integrating DelayedControl with CameraLens
> >
> > Do VCM controls need to be delayed? Or is this just to be able to
> > synchronise what position was applied to a given frame?
>
> I don't see any example on this, both from implementation known to me
> or the V4L2 spec.
> I suppose it would be delayed, but for some reasons it's hard to know
> the exact synchronized frame, since
> 1. The frame SOF is controlled by the sensor device, and the VCM
> somehow works independently.
> 2. The time needed to move the VCM might depend on the current
> position of the lens, and it varies at the runtime.

V4L2 won't help us here, it only exposes control of the VCM, it doesn't
tell how it should be used.

The lack of synchronization between the VCM and the sensor is one of the
issues that make AF implementations difficult. Not only do we lack
hardware synchronization, but the lens could even move while the image
is being captured, giving different focus values through the image with
rolling shutter sensors (I'm not sure to what extent though, it depends
on the movement speed). Algorithms will need to take that into account.

> > No objection to keeping this as a todo though as we still need to
> > investigate and figure out how DelayedControls works with multiple
> > devices in that case.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > I see this and the previous patch as definite building blocks that are
> > going to be useful moving forwards.
> >
> > > + * \return The camera lens device
> > > + */
> > > +
> > > +/**
> > > + * \fn CameraLens::properties()
> > > + * \brief Retrieve the camera lens properties
> > > + * \return The list of camera lens properties
> > > + */
> > > +
> > > +std::string CameraLens::logPrefix() const
> > > +{
> > > +       return "'" + entity_->name() + "'";
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 6727a777..3bee8fee 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',