[libcamera-devel] [PATCH v9 1/3] libcamera: camera_sensor: Reference test pattern modes by enum type

[Hirokazu Honda]

Hi Jacopo,

On Thu, Dec 2, 2021 at 12:04 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
>
>   while running CTS with this series applied I have noticed that once
> a test pattern is set in one of the test, the rest of the session is
> run with a test mode applied. I noticed as the displayed images are
> actually the sensor's test patterns. Running the Camera app after CTS
> has completed displays test pattern modes.
>
> It's pretty obvious that the test pattern mode is reset to Off during
> CameraSensor::init() which is called only at pipeline handler
> initialization time, so if the library is not tore down the test
> pattern mode is never reset.
>
> Could you check if the same happens on your side by running CTS please ?
>

Thanks. It is reproducible for me.
I confirmed the issue is resolved if I call setTestPatternMode(off) in
IPU3 pipeline handler configure().
Do you think it is a correct fix?

Thanks,
-Hiro



> On Mon, Nov 29, 2021 at 05:34:22PM +0900, Hirokazu Honda wrote:
> > The CameraSensor stores TestPatternModes as an int32_t. This prevents
> > the compiler from verifying the usage against the defined enum types.
> >
> > Fix references to the TestPatternMode to store the value as the
> > TestPatternModeEnum type which is defined by the control generator.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.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            | 10 +++++++---
> >  include/libcamera/internal/camera_sensor_properties.h |  3 ++-
> >  src/libcamera/camera_sensor.cpp                       |  4 ++--
> >  src/libcamera/pipeline/ipu3/ipu3.cpp                  |  7 ++++---
> >  4 files changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 2facbf3c..310571ca 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -14,8 +14,10 @@
> >  #include <libcamera/base/class.h>
> >  #include <libcamera/base/log.h>
> >
> > +#include <libcamera/control_ids.h>
> >  #include <libcamera/controls.h>
> >  #include <libcamera/geometry.h>
> > +
> >  #include <libcamera/ipa/core_ipa_interface.h>
> >
> >  #include "libcamera/internal/formats.h"
> > @@ -40,7 +42,8 @@ public:
> >       const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> >       const std::vector<Size> sizes(unsigned int mbusCode) const;
> >       Size resolution() const;
> > -     const std::vector<int32_t> &testPatternModes() const
> > +     const std::vector<controls::draft::TestPatternModeEnum>
> > +             &testPatternModes() const
> >       {
> >               return testPatternModes_;
> >       }
> > @@ -71,7 +74,8 @@ private:
> >       void initVimcDefaultProperties();
> >       void initStaticProperties();
> >       void initTestPatternModes(
> > -             const std::map<int32_t, int32_t> &testPatternModeMap);
> > +             const std::map<controls::draft::TestPatternModeEnum, int32_t>
> > +                     &testPatternModeMap);
> >       int initProperties();
> >
> >       const MediaEntity *entity_;
> > @@ -84,7 +88,7 @@ private:
> >       V4L2Subdevice::Formats formats_;
> >       std::vector<unsigned int> mbusCodes_;
> >       std::vector<Size> sizes_;
> > -     std::vector<int32_t> testPatternModes_;
> > +     std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> >
> >       Size pixelArraySize_;
> >       Rectangle activeArea_;
> > diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h
> > index af381a12..1ee3cb99 100644
> > --- a/include/libcamera/internal/camera_sensor_properties.h
> > +++ b/include/libcamera/internal/camera_sensor_properties.h
> > @@ -10,6 +10,7 @@
> >  #include <map>
> >  #include <string>
> >
> > +#include <libcamera/control_ids.h>
> >  #include <libcamera/geometry.h>
> >
> >  namespace libcamera {
> > @@ -18,7 +19,7 @@ struct CameraSensorProperties {
> >       static const CameraSensorProperties *get(const std::string &sensor);
> >
> >       Size unitCellSize;
> > -     std::map<int32_t, int32_t> testPatternModes;
> > +     std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 9fdb8c09..f0aa9f24 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -311,7 +311,7 @@ void CameraSensor::initStaticProperties()
> >  }
> >
> >  void CameraSensor::initTestPatternModes(
> > -     const std::map<int32_t, int32_t> &testPatternModes)
> > +     const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
> >  {
> >       const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> >       if (v4l2TestPattern == controls().end()) {
> > @@ -327,7 +327,7 @@ void CameraSensor::initTestPatternModes(
> >        * control index is supported in the below for loop that creates the
> >        * list of supported test patterns.
> >        */
> > -     std::map<int32_t, int32_t> indexToTestPatternMode;
> > +     std::map<int32_t, controls::draft::TestPatternModeEnum> indexToTestPatternMode;
> >       for (const auto &it : testPatternModes)
> >               indexToTestPatternMode[it.second] = it.first;
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index c65afdb2..25490dcf 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -982,13 +982,14 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)
> >               return ret;
> >
> >       ControlInfoMap::Map controls = IPU3Controls;
> > -     const std::vector<int32_t> &testPatternModes = sensor->testPatternModes();
> > +     const std::vector<controls::draft::TestPatternModeEnum>
> > +             &testPatternModes = sensor->testPatternModes();
> >       if (!testPatternModes.empty()) {
> >               std::vector<ControlValue> values;
> >               values.reserve(testPatternModes.size());
> >
> > -             for (int32_t pattern : testPatternModes)
> > -                     values.emplace_back(pattern);
> > +             for (auto pattern : testPatternModes)
> > +                     values.emplace_back(static_cast<int32_t>(pattern));
> >
> >               controls[&controls::draft::TestPatternMode] = ControlInfo(values);
> >       }
> > --
> > 2.34.0.rc2.393.gf8c9666880-goog
> >