[libcamera-devel] [PATCH v4 2/3] libcamera: camera_sensor: Enable to set a test pattern mode
Jacopo Mondi
jacopo at jmondi.org
Thu Nov 4 10:07:55 CET 2021
Hi Hiro
On Thu, Nov 04, 2021 at 03:42:51PM +0900, Hirokazu Honda wrote:
> This adds a function to set a camera sensor driver a test pattern
> mode.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/internal/camera_sensor.h | 8 ++
> src/libcamera/camera_sensor.cpp | 95 ++++++++++++++++++++--
> src/libcamera/control_ids.yaml | 5 ++
> 3 files changed, 100 insertions(+), 8 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index edef2220..40db792a 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -27,6 +27,9 @@ namespace libcamera {
>
> class BayerFormat;
> class MediaEntity;
> +class Request;
> +
> +struct CameraSensorProperties;
>
> class CameraSensor : protected Loggable
> {
> @@ -47,6 +50,7 @@ public:
> {
> return testPatternModes_;
> }
> + int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
Should testPatternMode be a const & ? It's a int32_t so it might not make any
difference
>
> V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> const Size &size) const;
> @@ -55,6 +59,7 @@ public:
> const ControlInfoMap &controls() const;
> ControlList getControls(const std::vector<uint32_t> &ids);
> int setControls(ControlList *ctrls);
> + int applyRequestControls(Request *request);
Please introduce this when it is used or in a separate patch before
3/3.
Also, we now have setControls(ControlList) and
applyRequestControls(Request). The first takes V4L2 controls and apply
them to the sensor subdevice. The second takes libcamera::controls
from the Request and applies one of them according to a notion of
priority that should not be defined in this class imho.
We indeed have to finalize the CameraSensor class controls interface
once all the use cases are handled, but for the moment I won't add
more stuff here. I know Kieran was instead happy with this, so let's
see what his take is.
>
> V4L2Subdevice *device() { return subdev_.get(); }
>
> @@ -82,6 +87,8 @@ private:
> std::unique_ptr<V4L2Subdevice> subdev_;
> unsigned int pad_;
>
> + const CameraSensorProperties *staticProps_;
> +
> std::string model_;
> std::string id_;
>
> @@ -89,6 +96,7 @@ private:
> std::vector<unsigned int> mbusCodes_;
> std::vector<Size> sizes_;
> std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> + controls::draft::TestPatternModeEnum testPatternMode_;
>
> Size pixelArraySize_;
> Rectangle activeArea_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index f0aa9f24..bb429b3e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -17,6 +17,7 @@
> #include <string.h>
>
> #include <libcamera/property_ids.h>
> +#include <libcamera/request.h>
>
> #include <libcamera/base/utils.h>
>
> @@ -54,8 +55,9 @@ LOG_DEFINE_CATEGORY(CameraSensor)
> * Once constructed the instance must be initialized with init().
> */
> CameraSensor::CameraSensor(const MediaEntity *entity)
> - : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),
> - properties_(properties::properties)
> + : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),
> + testPatternMode_(controls::draft::TestPatternModeUnset),
> + bayerFormat_(nullptr), properties_(properties::properties)
> {
> }
>
> @@ -300,25 +302,30 @@ void CameraSensor::initVimcDefaultProperties()
>
> void CameraSensor::initStaticProperties()
> {
> - const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> - if (!props)
> + staticProps_ = CameraSensorProperties::get(model_);
> + if (!staticProps_)
> return;
>
> /* Register the properties retrieved from the sensor database. */
> - properties_.set(properties::UnitCellSize, props->unitCellSize);
> + properties_.set(properties::UnitCellSize, staticProps_->unitCellSize);
>
> - initTestPatternModes(props->testPatternModes);
> + initTestPatternModes(staticProps_->testPatternModes);
> }
>
> void CameraSensor::initTestPatternModes(
> const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
> {
> - const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> - if (v4l2TestPattern == controls().end()) {
> + if (testPatternModes.empty()) {
> LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> << model() << "\'";
> return;
> }
> + const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> + if (v4l2TestPattern == controls().end()) {
> + LOG(CameraSensor, Debug)
> + << "V4L2_CID_TEST_PATTERN is not supported";
> + return;
> + }
>
> /*
> * Create a map that associates the V4L2 control index to the test
> @@ -531,6 +538,44 @@ Size CameraSensor::resolution() const
> * \return The list of test pattern modes
> */
>
> +/**
> + * \brief Set the test pattern mode for the camera sensor
> + * \param[in] testPatternMode Test pattern mode control value to set the camera
> + * sensor
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CameraSensor::setTestPatternMode(
> + controls::draft::TestPatternModeEnum testPatternMode)
Fits on one line maybe ?
> +{
> + if (testPatternMode_ == testPatternMode)
> + return 0;
> +
> + if (!staticProps_) {
> + return 0;
> + }
No braces.
And this anyway won't catch the !V4L2_CID_TEST_PATTERN.
As suggested in the previous version you can check for
if (testPatternModes_.empty())
return 0;
as it will evaluate to true if:
- No staticProps_
- staticProps_->testPatternModes.empty()
- V4L2_CID_TEST_PATTERN not available
> +
> + auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> + testPatternMode);
> + if (it != testPatternModes_.end()) {
You can also simply rely on this, but you will have an error message
in case any of the above three conditions is true, something the class
already complains about at initialization time and that I would rather
handle slently here.
Let's wait for comments about applyRequestControls()
Thanks
j
> + LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> + << testPatternMode;
> + return -EINVAL;
> + }
> +
> + int32_t index = staticProps_->testPatternModes.at(testPatternMode);
> + ControlList ctrls{ controls() };
> + ctrls.set(V4L2_CID_TEST_PATTERN, index);
> +
> + int ret = setControls(&ctrls);
> + if (ret)
> + return ret;
> +
> + testPatternMode_ = testPatternMode;
> +
> + return 0;
> +}
> +
> /**
> * \brief Retrieve the best sensor format for a desired output
> * \param[in] mbusCodes The list of acceptable media bus codes
> @@ -705,6 +750,40 @@ int CameraSensor::setControls(ControlList *ctrls)
> return subdev_->setControls(ctrls);
> }
>
> +/**
> + * \brief Apply controls associated with Request
> + * \param[in] request Request that may contain contorls to be applied
> + *
> + * Some controls have to be applied for a capture associated with Request.
> + * This picks up such controls and set the driver them.
> + *
> + * \return 0 on success or an error code otherwise
> + */
> +int32_t CameraSensor::applyRequestControls(Request *request)
> +{
> + /* Assumes applying the test pattern mode affects immediately. */
> + if (request->controls().contains(controls::draft::TestPatternMode)) {
> + const int32_t testPatternMode = request->controls().get(
> + controls::draft::TestPatternMode);
> +
> + LOG(CameraSensor, Debug) << "Apply test pattern mode: "
> + << testPatternMode;
> +
> + int ret = setTestPatternMode(
> + static_cast<controls::draft::TestPatternModeEnum>(testPatternMode));
> + if (ret) {
> + LOG(CameraSensor, Error)
> + << "Failed to set test pattern mode: " << ret;
> + return ret;
> + }
> +
> + request->metadata().set(controls::draft::TestPatternMode,
> + testPatternMode);
> + }
> +
> + return 0;
> +}
> +
> /**
> * \fn CameraSensor::device()
> * \brief Retrieve the camera sensor device
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 9d4638ae..083bac7b 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -639,6 +639,11 @@ controls:
> Control to select the test pattern mode. Currently identical to
> ANDROID_SENSOR_TEST_PATTERN_MODE.
> enum:
> + - name: TestPatternModeUnset
> + value: -1
> + description: |
> + No test pattern is set. Returned frames by the camera device are
> + undefined.
> - name: TestPatternModeOff
> value: 0
> description: |
> --
> 2.33.1.1089.g2158813163f-goog
>
More information about the libcamera-devel
mailing list