[libcamera-devel] [PATCH v4 2/3] libcamera: camera_sensor: Enable to set a test pattern mode
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Nov 8 23:43:36 CET 2021
Quoting Hirokazu Honda (2021-11-08 04:34:01)
> Kieran, could you review this patch?
>
> Thanks,
> -Hiro
>
> On Thu, Nov 4, 2021 at 6:07 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > 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
It probably won't make much difference here.
But I suspect setTestPatternMode could be internal and private to
CameraSensor class if the initialisation of the TestPatternMode is moved
to init() from 3/3.
> > >
> > > 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_;
It's a shame the control id generator makes this a bit more verbose than
desired, but that's ok for now. At least the type is explicit.
> > >
> > > 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)
I think I'd make this an Error not a Debug. If we get here, then
presumably our CameraSensor Database has declared a mapping of Test
Pattern modes to V4L2 patterns. - so if the V4L2 control is not
available, then it sounds like an error worth noting on the logs.
Or of course we could do the checks the other way, and be louder if
there is a V4L2_CID_TEST_PATTERN control, but no mapping, which might be
a more realistic thing to occur - and suggest that someone should add
the mapping to the CameraSensorDatabase to enable the feature...
So I think we should do:
const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
if v4l2TestPattern == controls().end() {
LOG(Debug) << "No test pattern support on the sensor";
return;
}
if (testPatternModes.empty()) {
LOG(Error) << "No test pattern modes defined"
return;
// This is the one where we want to be loud, as the
// camera sensor supports test patterns but we don't
// know how to map them so soomeone shoudl fix that.
}
> > > + << "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
> >
I haven't verified if all those three statements are valid, but it
certainly sounds like a better check if they are all covered by
testPatternModes_.empty().
> > > +
> > > + 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);
> > > + }
Later, I'd expect this function to be more efficient, and process all
controls (or as many as possible) in one ControlList to be able to set
them as a group. But as we only have one for now, I think this is fine.
That would proabbly mean later we'll split setTestPatternMode to a
function which just does the mapping of a TestPatternMode and puts it in
a control list ready to be set ... or such ... but that can all be done
on top when we have other controls to set from a request.
> > > +
> > > + 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.
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).
> > > - name: TestPatternModeOff
> > > value: 0
> > > description: |
> > > --
> > > 2.33.1.1089.g2158813163f-goog
> > >
More information about the libcamera-devel
mailing list