[libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor: Enable to set a test pattern mode

[Hirokazu Honda]

Hi Jacopo, thank you for reviewing.

On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
>
> On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:
> > Provides a function to set the camera sensor a test pattern mode.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  1 +
> >  src/libcamera/camera_sensor.cpp            | 39 ++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index e133ebf4..8b9f84c9 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -43,6 +43,7 @@ public:
> >       {
> >               return testPatternModes_;
> >       }
> > +     int setTestPatternMode(uint8_t testPatternMode);
> >
> >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> >                                     const Size &size) const;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 70bbd97a..ce8ff274 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const
> >   * \return The list of test pattern modes
> >   */
> >
> > +/**
> > + * \brief Set the camera sensor a specified controls::TestPatternMode
>
> Doxygen is puzzling me recently... the testPatternMode paramter is not
> documented but I don't get a warning :/
>
> Also, I'm a bit debated about where to actually place this function
> (or better, it's initTestPatternModes which is misplaced)
> if we have to respect the declaration order or the way you sorted them
> here (which is more logical) is better.
>
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)
> > +{
> > +     if (std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > +                   testPatternMode) == testPatternModes_.end()) {
> > +             LOG(CameraSensor, Error) << "Unsupported test pattern mode: "
> > +                                      << testPatternMode;
> > +             return -EINVAL;
> > +     }
>
> Do we need this ? The testPatternModes_ map is constructed using the
> camera sensor properties, the below find() on the props->testPatternModes
> map should gurantee that is supported. Also, if a test pattern mode is
> not reported as part of testPatternModes_ applications shouldn't set
> it...
>
> > +
> > +     const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> > +     if (!props)
> > +             return -EINVAL;
> > +
> > +     auto it = props->testPatternModes.find(testPatternMode);
> > +     ASSERT(it != props->testPatternModes.end());
>
> Yeah, I think the assertion here is more than enough..
>
> > +     const uint8_t index = it->second;
> > +
> > +     ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });
> > +     if (ctrls.empty()) {
> > +             LOG(CameraSensor, Error)
> > +                     << "Failed to retrieve test pattern control";
> > +             return -EINVAL;
> > +     }
>
> We won't register the TestPatternMode control in first place the V4L2
> control is not supported..

Replace this with ASSERT.

>
> > +
> > +     ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);
> > +     if (value.get<int32_t>() == index)
> > +             return 0;
>
> The V4L2 control framework should take care of this by itself, I think
> we can set the control regardless
>

Yes, I put this in order to save a redundant Ioctl call.
Do you want to remove that?

-Hiro
> > +
> > +     value.set<int32_t>(index);
> > +     ctrls.set(V4L2_CID_TEST_PATTERN, value);
> > +
> > +     return setControls(&ctrls);
> > +}
> > +
> >  /**
> >   * \brief Retrieve the best sensor format for a desired output
> >   * \param[in] mbusCodes The list of acceptable media bus codes
> > --
> > 2.32.0.288.g62a8d224e6-goog
> >