[libcamera-devel] [PATCH v7 2/3] libcamera: camera_sensor: Enable to set a test pattern mode
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Nov 25 00:02:36 CET 2021
Hello,
On Wed, Nov 24, 2021 at 11:23:05AM +0000, Kieran Bingham wrote:
> Quoting Hirokazu Honda (2021-11-24 05:10:11)
> > On Wed, Nov 24, 2021 at 1:53 PM Laurent Pinchart wrote:
> > > On Wed, Nov 24, 2021 at 04:08:11AM +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>
> > >
> > > The patch I've reviewed differs significantly from this one. Please drop
> > > my tag in v8.
> > >
> > > > ---
> > > > include/libcamera/internal/camera_sensor.h | 7 +++
> > > > src/libcamera/camera_sensor.cpp | 60 +++++++++++++++++++---
> > > > src/libcamera/control_ids.yaml | 5 ++
> > > > 3 files changed, 65 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > index edef2220..f355f323 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;
> > >
> > > Not needed anymore.
> > >
> > > > +
> > > > +struct CameraSensorProperties;
> > > >
> > > > class CameraSensor : protected Loggable
> > > > {
> > > > @@ -47,6 +50,7 @@ public:
> > > > {
> > > > return testPatternModes_;
> > > > }
> > > > + int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
> > > >
> > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > > > const Size &size) const;
> > > > @@ -82,6 +86,8 @@ private:
> > > > std::unique_ptr<V4L2Subdevice> subdev_;
> > > > unsigned int pad_;
> > > >
> > > > + const CameraSensorProperties *staticProps_;
> > > > +
> > > > std::string model_;
> > > > std::string id_;
> > > >
> > > > @@ -89,6 +95,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..e1293980 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>
> > >
> > > Not needed anymore either.
> > >
> > > >
> > > > #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,14 +302,14 @@ 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(
> > > > @@ -315,7 +317,15 @@ void CameraSensor::initTestPatternModes(
> > > > {
> > > > const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > > if (v4l2TestPattern == controls().end()) {
> > > > - LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> > > > + LOG(CameraSensor, Debug)
> > > > + << "V4L2_CID_TEST_PATTERN is not supported";
> > > > + return;
> > > > + }
> > > > +
> > > > + if (testPatternModes.empty()) {
> > > > + // The camera sensor supports test patterns but we don't know
> > > > + // how to map them so this should be fixed.
> > >
> > > We use C-style comments.
> > >
> > > > + LOG(CameraSensor, Error) << "No static test pattern map for \'"
> > >
> > > No need to escape the single quote.
> > >
> > > > << model() << "\'";
> > > > return;
> > > > }
> > > > @@ -531,6 +541,42 @@ 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)
> > > > +{
> > > > + if (testPatternMode_ == testPatternMode)
> > > > + return 0;
> > > > +
> > > > + if (!staticProps_ || testPatternModes_.empty())
> > > > + return 0;
> > >
> > > Shouldn't this return an error ?
> > >
> > > > +
> > > > + auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > > > + testPatternMode);
> > > > + if (it == testPatternModes_.end()) {
> > > > + 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;
> > >
> > > This brings the question of where control values should be cached, to
> > > avoid applying controls that haven't changed. I don't think this need is
> > > limited to the CameraSensor class, so a more generic solution at the
> > > pipeline handler level may be better. This doesn't have to be addressed
> > > now though.
> > >
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > /**
> > > > * \brief Retrieve the best sensor format for a desired output
> > > > * \param[in] mbusCodes The list of acceptable media bus codes
> > > > 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.
> > >
> > > Why do we need this ? It's not documented in the commit message. If we
> > > need this new value, its addition should be split to a separate commit,
> > > with a proper explanation.
> >
> > It was suggested by Kieran.
> > I originally used -1 for the initialized value of a test pattern mode
> > in CameraSensor.
> > -1 lets the first setTestPatternMode(Off) be executed.
> > It seems hack as -1 is invalid test pattern mode. So Kieran would like
> > to introduce this value.
>
> In message-id:<163641141636.1410963.14556847121328252873 at Monstersaurus>
> I stated:
>
> │ I'm still not sure this is needed, but I don't object to it being added
> │ if you're sure this state is required.
> │
> │ I think the CameraSensor class should set TestPatternModeOff at init()
> │ time if there is a test pattern control available on the sensor.
> │
> │ Then any test patterns will only be applicable if explictly requested by
> │ the application (/request).
>
> The key point was "If you're sure this state is required..." which I
> still don't believe it is?
>
> I believe my original objection was that you were initialising to '-1'
> as an otherwise arbitrary (but now defined) value.
>
> I don't think you need to do that. You can initialise the
> TestPatternMode as Off by 'setting' it (on the device) off when initialising.
That sounds good, we should start with a known state.
> > If we move the cache of testpatternmode to ipu3 pipeline handler, this
> > may is not necessary.
> >
> > > > - name: TestPatternModeOff
> > > > value: 0
> > > > description: |
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list