[libcamera-devel] [PATCH v4 2/3] libcamera: camera_sensor: Enable to set a test pattern mode
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 22 03:12:04 CET 2021
Hello,
On Tue, Nov 09, 2021 at 04:35:31PM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2021-11-09 08:55:58)
> > On Mon, Nov 08, 2021 at 10:43:36PM +0000, Kieran Bingham wrote:
> > > 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 wrote:
> > > > > 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.
>
> My intention was not that the priority is specified by the CameraSensor
> class, but that the CameraSensor class handles controls that are
> appropriate to it.
>
> Part of (my) aim was to prevent every pipeline handler having duplicated
> boilerplate filtering out the testpattern control to set on the camera sensor.
>
> Perhaps the key point is the name 'applyRequestControls' is disliked.
>
> But I also disliked that it lead towards having a
>
> sensor()->setTestPatternMode().
>
> which would imply we might have to add lots of setters on a sensor:
>
> sensor()->setVblank();
> sensor()->setExposure();
> sensor()->setGain();
> sensor()->setOtherPerFrameControls();?
>
> But perhaps I was wrong ... if there's not expected to be anything else
> added ...?
I agree with Jacopo that the CameraSensor class shouldn't deal with
requests. It should instead take a ControList of non-V4L2 controls
(possibly libcamera controls if we have all we need there, or a new set
of CameraSensor controls). It's the pipeline handler that should be
responsible for dispatching the contents of a request to the appropriate
components.
> > > > > 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()
> >
> > Actually the pending point here is your suggestion to add
> > applyRequestControls()
> >
> > > > > > + 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
> > > > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list