[libcamera-devel] [PATCH v4 3/4] libcamera: camera_lens: Add a new class to model a camera lens
Umang Jain
umang.jain at ideasonboard.com
Wed Nov 24 07:03:22 CET 2021
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 at chromium.org>
>> Reviewed-by: Kieran Bingham <kieran.bingham at 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
>>
More information about the libcamera-devel
mailing list