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

[Hirokazu Honda]

Hi Laurent, thank you for reviewing.

On Thu, Oct 28, 2021 at 9:36 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Tue, Oct 26, 2021 at 07:39:13PM +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>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  6 +++
> >  src/libcamera/camera_sensor.cpp            | 50 +++++++++++++++++++---
> >  2 files changed, 50 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index d25a1165..d08cfec5 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -26,6 +26,8 @@ namespace libcamera {
> >  class BayerFormat;
> >  class MediaEntity;
> >
> > +struct CameraSensorProperties;
> > +
> >  class CameraSensor : protected Loggable
> >  {
> >  public:
> > @@ -44,6 +46,7 @@ public:
> >       {
> >               return testPatternModes_;
> >       }
> > +     int setTestPatternMode(int32_t testPatternMode);
>
> Could the function take a TestPatternModeEnum instead of an int32_t ?
> That's the proper type of the libcamera TestPatternMode control.
>

We return int32_t values in testPatternModes().
I think if we change the type here, we should change the types?
If yes, can I do it in a follow-up patch after submitting this patch series?

> >
> >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> >                                     const Size &size) const;
> > @@ -78,6 +81,8 @@ private:
> >       std::unique_ptr<V4L2Subdevice> subdev_;
> >       unsigned int pad_;
> >
> > +     const CameraSensorProperties *props_;
> > +
> >       std::string model_;
> >       std::string id_;
> >
> > @@ -85,6 +90,7 @@ private:
> >       std::vector<unsigned int> mbusCodes_;
> >       std::vector<Size> sizes_;
> >       std::vector<int32_t> testPatternModes_;
> > +     int32_t testPatternMode_;
>
> Same here.
>
> >
> >       Size pixelArraySize_;
> >       Rectangle activeArea_;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 9fdb8c09..47bb13f9 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -54,8 +54,8 @@ 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), testPatternMode_(-1),
>
> Shouldn't this be initialized to TestPatternModeOff ?
>

It assumes the camera sensor's current test pattern mode is ModeOff.
It is not true if other test pattern mode is set to the camera sensor
and it is closed without resetting to ModeOff.

I always set camera sensor to ModeOff in initialization by setting
testPatternMode_ to invalid value (-1).

-Hiro
> > +       bayerFormat_(nullptr), properties_(properties::properties)
> >  {
> >  }
> >
> > @@ -300,14 +300,16 @@ void CameraSensor::initVimcDefaultProperties()
> >
> >  void CameraSensor::initStaticProperties()
> >  {
> > -     const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> > -     if (!props)
> > +     props_ = CameraSensorProperties::get(model_);
> > +     if (!props_) {
> > +             LOG(CameraSensor, Debug) << "No properties for " << model_;
> >               return;
> > +     }
> >
> >       /* Register the properties retrieved from the sensor database. */
> > -     properties_.set(properties::UnitCellSize, props->unitCellSize);
> > +     properties_.set(properties::UnitCellSize, props_->unitCellSize);
> >
> > -     initTestPatternModes(props->testPatternModes);
> > +     initTestPatternModes(props_->testPatternModes);
> >  }
> >
> >  void CameraSensor::initTestPatternModes(
> > @@ -531,6 +533,42 @@ Size CameraSensor::resolution() const
> >   * \return The list of test pattern modes
> >   */
> >
> > +/**
> > + * \brief Set the camera sensor a specified controls::TestPatternMode
>
>  * \brief Set the test pattern mode for the camera sensor
>
> > + * \param[in] testPatternMode test pattern mode control value to set the camera
>
> s/test pattern/Test pattern/
>
> > + * sensor
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int CameraSensor::setTestPatternMode(int32_t testPatternMode)
> > +{
> > +     if (testPatternMode_ == testPatternMode)
> > +             return 0;
> > +
> > +     if (!props_) {
> > +             LOG(CameraSensor, Error) << "No property is found";
>
> s/property is found/properties are found/
>
> > +             return -EINVAL;
> > +     }
> > +
> > +     auto it = props_->testPatternModes.find(testPatternMode);
> > +     if (it == props_->testPatternModes.end()) {
> > +             LOG(CameraSensor, Error) << "Unsupported test pattern mode: "
>
> s/mode:/mode/
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +                                      << testPatternMode;
> > +             return -EINVAL;
> > +     }
> > +
> > +     ControlList ctrls{ controls() };
> > +     ctrls.set(V4L2_CID_TEST_PATTERN, it->second);
> > +
> > +     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
>
> --
> Regards,
>
> Laurent Pinchart