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

[Kieran Bingham]

Quoting Hirokazu Honda (2021-11-02 02:53:34)
> 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).
 

This throws away the ability to use the type information though.

We could add TestPatternModeUnset to initialise it to 0, and keep the
type? Or is that overkill?


> -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