[libcamera-devel,v7,2/3] libcamera: camera_sensor: Enable to set a test pattern mode

14729 diff mbox series

This adds a function to set a camera sensor driver a test pattern
mode.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/camera_sensor.h |  7 +++
 src/libcamera/camera_sensor.cpp            | 60 +++++++++++++++++++---
 src/libcamera/control_ids.yaml             |  5 ++
 3 files changed, 65 insertions(+), 7 deletions(-)

Hi Hiro,

Thank you for the patch.

On Wed, Nov 24, 2021 at 04:08:11AM +0900, Hirokazu Honda wrote:
> This adds a function to set a camera sensor driver a test pattern
> mode.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

The patch I've reviewed differs significantly from this one. Please drop
my tag in v8.

> ---
>  include/libcamera/internal/camera_sensor.h |  7 +++
>  src/libcamera/camera_sensor.cpp            | 60 +++++++++++++++++++---
>  src/libcamera/control_ids.yaml             |  5 ++
>  3 files changed, 65 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index edef2220..f355f323 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;

Not needed anymore.

> +
> +struct CameraSensorProperties;
>  
>  class CameraSensor : protected Loggable
>  {
> @@ -47,6 +50,7 @@ public:
>  	{
>  		return testPatternModes_;
>  	}
> +	int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
>  
>  	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
>  				      const Size &size) const;
> @@ -82,6 +86,8 @@ private:
>  	std::unique_ptr<V4L2Subdevice> subdev_;
>  	unsigned int pad_;
>  
> +	const CameraSensorProperties *staticProps_;
> +
>  	std::string model_;
>  	std::string id_;
>  
> @@ -89,6 +95,7 @@ private:
>  	std::vector<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
>  	std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> +	controls::draft::TestPatternModeEnum testPatternMode_;
>  
>  	Size pixelArraySize_;
>  	Rectangle activeArea_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index f0aa9f24..e1293980 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>

Not needed anymore either.

>  
>  #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,14 +302,14 @@ 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(
> @@ -315,7 +317,15 @@ void CameraSensor::initTestPatternModes(
>  {
>  	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
>  	if (v4l2TestPattern == controls().end()) {
> -		LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> +		LOG(CameraSensor, Debug)
> +			<< "V4L2_CID_TEST_PATTERN is not supported";
> +		return;
> +	}
> +
> +	if (testPatternModes.empty()) {
> +		// The camera sensor supports test patterns but we don't know
> +		// how to map them so this should be fixed.

We use C-style comments.

> +		LOG(CameraSensor, Error) << "No static test pattern map for \'"

No need to escape the single quote.

>  					 << model() << "\'";
>  		return;
>  	}
> @@ -531,6 +541,42 @@ 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)
> +{
> +	if (testPatternMode_ == testPatternMode)
> +		return 0;
> +
> +	if (!staticProps_ || testPatternModes_.empty())
> +		return 0;

Shouldn't this return an error ?

> +
> +	auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> +			    testPatternMode);
> +	if (it == testPatternModes_.end()) {
> +		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;

This brings the question of where control values should be cached, to
avoid applying controls that haven't changed. I don't think this need is
limited to the CameraSensor class, so a more generic solution at the
pipeline handler level may be better. This doesn't have to be addressed
now though.

> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Retrieve the best sensor format for a desired output
>   * \param[in] mbusCodes The list of acceptable media bus codes
> 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.

Why do we need this ? It's not documented in the commit message. If we
need this new value, its addition should be split to a separate commit,
with a proper explanation.

>          - name: TestPatternModeOff
>            value: 0
>            description: |

Hi Laurent,

On Wed, Nov 24, 2021 at 1:53 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Wed, Nov 24, 2021 at 04:08:11AM +0900, Hirokazu Honda wrote:
> > This adds a function to set a camera sensor driver a test pattern
> > mode.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> The patch I've reviewed differs significantly from this one. Please drop
> my tag in v8.
>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  7 +++
> >  src/libcamera/camera_sensor.cpp            | 60 +++++++++++++++++++---
> >  src/libcamera/control_ids.yaml             |  5 ++
> >  3 files changed, 65 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index edef2220..f355f323 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;
>
> Not needed anymore.
>
> > +
> > +struct CameraSensorProperties;
> >
> >  class CameraSensor : protected Loggable
> >  {
> > @@ -47,6 +50,7 @@ public:
> >       {
> >               return testPatternModes_;
> >       }
> > +     int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
> >
> >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> >                                     const Size &size) const;
> > @@ -82,6 +86,8 @@ private:
> >       std::unique_ptr<V4L2Subdevice> subdev_;
> >       unsigned int pad_;
> >
> > +     const CameraSensorProperties *staticProps_;
> > +
> >       std::string model_;
> >       std::string id_;
> >
> > @@ -89,6 +95,7 @@ private:
> >       std::vector<unsigned int> mbusCodes_;
> >       std::vector<Size> sizes_;
> >       std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> > +     controls::draft::TestPatternModeEnum testPatternMode_;
> >
> >       Size pixelArraySize_;
> >       Rectangle activeArea_;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index f0aa9f24..e1293980 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>
>
> Not needed anymore either.
>
> >
> >  #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,14 +302,14 @@ 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(
> > @@ -315,7 +317,15 @@ void CameraSensor::initTestPatternModes(
> >  {
> >       const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> >       if (v4l2TestPattern == controls().end()) {
> > -             LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> > +             LOG(CameraSensor, Debug)
> > +                     << "V4L2_CID_TEST_PATTERN is not supported";
> > +             return;
> > +     }
> > +
> > +     if (testPatternModes.empty()) {
> > +             // The camera sensor supports test patterns but we don't know
> > +             // how to map them so this should be fixed.
>
> We use C-style comments.
>
> > +             LOG(CameraSensor, Error) << "No static test pattern map for \'"
>
> No need to escape the single quote.
>
> >                                        << model() << "\'";
> >               return;
> >       }
> > @@ -531,6 +541,42 @@ 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)
> > +{
> > +     if (testPatternMode_ == testPatternMode)
> > +             return 0;
> > +
> > +     if (!staticProps_ || testPatternModes_.empty())
> > +             return 0;
>
> Shouldn't this return an error ?
>
> > +
> > +     auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > +                         testPatternMode);
> > +     if (it == testPatternModes_.end()) {
> > +             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;
>
> This brings the question of where control values should be cached, to
> avoid applying controls that haven't changed. I don't think this need is
> limited to the CameraSensor class, so a more generic solution at the
> pipeline handler level may be better. This doesn't have to be addressed
> now though.
>
> > +
> > +     return 0;
> > +}
> > +
> >  /**
> >   * \brief Retrieve the best sensor format for a desired output
> >   * \param[in] mbusCodes The list of acceptable media bus codes
> > 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.
>
> Why do we need this ? It's not documented in the commit message. If we
> need this new value, its addition should be split to a separate commit,
> with a proper explanation.
>

It was suggested by Kieran.
I originally used -1 for the initialized value of a test pattern mode
in CameraSensor.
-1 lets the first setTestPatternMode(Off) be executed.
It seems hack as -1 is invalid test pattern mode. So Kieran would like
to introduce this value.

If we move the cache of testpatternmode to ipu3 pipeline handler, this
may is not necessary.

Regards,
-Hiro
> >          - name: TestPatternModeOff
> >            value: 0
> >            description: |
>
> --
> Regards,
>
> Laurent Pinchart

Quoting Hirokazu Honda (2021-11-24 05:10:11)
> Hi Laurent,
> 
> On Wed, Nov 24, 2021 at 1:53 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Hiro,
> >
> > Thank you for the patch.
> >
> > On Wed, Nov 24, 2021 at 04:08:11AM +0900, Hirokazu Honda wrote:
> > > This adds a function to set a camera sensor driver a test pattern
> > > mode.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > The patch I've reviewed differs significantly from this one. Please drop
> > my tag in v8.
> >
> > > ---
> > >  include/libcamera/internal/camera_sensor.h |  7 +++
> > >  src/libcamera/camera_sensor.cpp            | 60 +++++++++++++++++++---
> > >  src/libcamera/control_ids.yaml             |  5 ++
> > >  3 files changed, 65 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > index edef2220..f355f323 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;
> >
> > Not needed anymore.
> >
> > > +
> > > +struct CameraSensorProperties;
> > >
> > >  class CameraSensor : protected Loggable
> > >  {
> > > @@ -47,6 +50,7 @@ public:
> > >       {
> > >               return testPatternModes_;
> > >       }
> > > +     int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
> > >
> > >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > >                                     const Size &size) const;
> > > @@ -82,6 +86,8 @@ private:
> > >       std::unique_ptr<V4L2Subdevice> subdev_;
> > >       unsigned int pad_;
> > >
> > > +     const CameraSensorProperties *staticProps_;
> > > +
> > >       std::string model_;
> > >       std::string id_;
> > >
> > > @@ -89,6 +95,7 @@ private:
> > >       std::vector<unsigned int> mbusCodes_;
> > >       std::vector<Size> sizes_;
> > >       std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> > > +     controls::draft::TestPatternModeEnum testPatternMode_;
> > >
> > >       Size pixelArraySize_;
> > >       Rectangle activeArea_;
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index f0aa9f24..e1293980 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>
> >
> > Not needed anymore either.
> >
> > >
> > >  #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,14 +302,14 @@ 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(
> > > @@ -315,7 +317,15 @@ void CameraSensor::initTestPatternModes(
> > >  {
> > >       const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > >       if (v4l2TestPattern == controls().end()) {
> > > -             LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> > > +             LOG(CameraSensor, Debug)
> > > +                     << "V4L2_CID_TEST_PATTERN is not supported";
> > > +             return;
> > > +     }
> > > +
> > > +     if (testPatternModes.empty()) {
> > > +             // The camera sensor supports test patterns but we don't know
> > > +             // how to map them so this should be fixed.
> >
> > We use C-style comments.
> >
> > > +             LOG(CameraSensor, Error) << "No static test pattern map for \'"
> >
> > No need to escape the single quote.
> >
> > >                                        << model() << "\'";
> > >               return;
> > >       }
> > > @@ -531,6 +541,42 @@ 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)
> > > +{
> > > +     if (testPatternMode_ == testPatternMode)
> > > +             return 0;
> > > +
> > > +     if (!staticProps_ || testPatternModes_.empty())
> > > +             return 0;
> >
> > Shouldn't this return an error ?
> >
> > > +
> > > +     auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > > +                         testPatternMode);
> > > +     if (it == testPatternModes_.end()) {
> > > +             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;
> >
> > This brings the question of where control values should be cached, to
> > avoid applying controls that haven't changed. I don't think this need is
> > limited to the CameraSensor class, so a more generic solution at the
> > pipeline handler level may be better. This doesn't have to be addressed
> > now though.
> >
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  /**
> > >   * \brief Retrieve the best sensor format for a desired output
> > >   * \param[in] mbusCodes The list of acceptable media bus codes
> > > 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.
> >
> > Why do we need this ? It's not documented in the commit message. If we
> > need this new value, its addition should be split to a separate commit,
> > with a proper explanation.
> >
> 
> It was suggested by Kieran.
> I originally used -1 for the initialized value of a test pattern mode
> in CameraSensor.
> -1 lets the first setTestPatternMode(Off) be executed.
> It seems hack as -1 is invalid test pattern mode. So Kieran would like
> to introduce this value.

In message-id:<163641141636.1410963.14556847121328252873@Monstersaurus>
I stated:

│       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).

The key point was "If you're sure this state is required..." which I
still don't believe it is?

I believe my original objection was that you were initialising to '-1'
as an otherwise arbitrary (but now defined) value.

I don't think you need to do that. You can initialise the
TestPatternMode as Off by 'setting' it (on the device) off when initialising.


> If we move the cache of testpatternmode to ipu3 pipeline handler, this
> may is not necessary.
> Regards,
> -Hiro
> > >          - name: TestPatternModeOff
> > >            value: 0
> > >            description: |
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Hello,

On Wed, Nov 24, 2021 at 11:23:05AM +0000, Kieran Bingham wrote:
> Quoting Hirokazu Honda (2021-11-24 05:10:11)
> > On Wed, Nov 24, 2021 at 1:53 PM Laurent Pinchart wrote:
> > > On Wed, Nov 24, 2021 at 04:08:11AM +0900, Hirokazu Honda wrote:
> > > > This adds a function to set a camera sensor driver a test pattern
> > > > mode.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > The patch I've reviewed differs significantly from this one. Please drop
> > > my tag in v8.
> > >
> > > > ---
> > > >  include/libcamera/internal/camera_sensor.h |  7 +++
> > > >  src/libcamera/camera_sensor.cpp            | 60 +++++++++++++++++++---
> > > >  src/libcamera/control_ids.yaml             |  5 ++
> > > >  3 files changed, 65 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > index edef2220..f355f323 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;
> > >
> > > Not needed anymore.
> > >
> > > > +
> > > > +struct CameraSensorProperties;
> > > >
> > > >  class CameraSensor : protected Loggable
> > > >  {
> > > > @@ -47,6 +50,7 @@ public:
> > > >       {
> > > >               return testPatternModes_;
> > > >       }
> > > > +     int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
> > > >
> > > >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > > >                                     const Size &size) const;
> > > > @@ -82,6 +86,8 @@ private:
> > > >       std::unique_ptr<V4L2Subdevice> subdev_;
> > > >       unsigned int pad_;
> > > >
> > > > +     const CameraSensorProperties *staticProps_;
> > > > +
> > > >       std::string model_;
> > > >       std::string id_;
> > > >
> > > > @@ -89,6 +95,7 @@ private:
> > > >       std::vector<unsigned int> mbusCodes_;
> > > >       std::vector<Size> sizes_;
> > > >       std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> > > > +     controls::draft::TestPatternModeEnum testPatternMode_;
> > > >
> > > >       Size pixelArraySize_;
> > > >       Rectangle activeArea_;
> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > index f0aa9f24..e1293980 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>
> > >
> > > Not needed anymore either.
> > >
> > > >
> > > >  #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,14 +302,14 @@ 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(
> > > > @@ -315,7 +317,15 @@ void CameraSensor::initTestPatternModes(
> > > >  {
> > > >       const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > >       if (v4l2TestPattern == controls().end()) {
> > > > -             LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> > > > +             LOG(CameraSensor, Debug)
> > > > +                     << "V4L2_CID_TEST_PATTERN is not supported";
> > > > +             return;
> > > > +     }
> > > > +
> > > > +     if (testPatternModes.empty()) {
> > > > +             // The camera sensor supports test patterns but we don't know
> > > > +             // how to map them so this should be fixed.
> > >
> > > We use C-style comments.
> > >
> > > > +             LOG(CameraSensor, Error) << "No static test pattern map for \'"
> > >
> > > No need to escape the single quote.
> > >
> > > >                                        << model() << "\'";
> > > >               return;
> > > >       }
> > > > @@ -531,6 +541,42 @@ 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)
> > > > +{
> > > > +     if (testPatternMode_ == testPatternMode)
> > > > +             return 0;
> > > > +
> > > > +     if (!staticProps_ || testPatternModes_.empty())
> > > > +             return 0;
> > >
> > > Shouldn't this return an error ?
> > >
> > > > +
> > > > +     auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > > > +                         testPatternMode);
> > > > +     if (it == testPatternModes_.end()) {
> > > > +             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;
> > >
> > > This brings the question of where control values should be cached, to
> > > avoid applying controls that haven't changed. I don't think this need is
> > > limited to the CameraSensor class, so a more generic solution at the
> > > pipeline handler level may be better. This doesn't have to be addressed
> > > now though.
> > >
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  /**
> > > >   * \brief Retrieve the best sensor format for a desired output
> > > >   * \param[in] mbusCodes The list of acceptable media bus codes
> > > > 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.
> > >
> > > Why do we need this ? It's not documented in the commit message. If we
> > > need this new value, its addition should be split to a separate commit,
> > > with a proper explanation.
> > 
> > It was suggested by Kieran.
> > I originally used -1 for the initialized value of a test pattern mode
> > in CameraSensor.
> > -1 lets the first setTestPatternMode(Off) be executed.
> > It seems hack as -1 is invalid test pattern mode. So Kieran would like
> > to introduce this value.
> 
> In message-id:<163641141636.1410963.14556847121328252873@Monstersaurus>
> I stated:
> 
> │       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).
> 
> The key point was "If you're sure this state is required..." which I
> still don't believe it is?
> 
> I believe my original objection was that you were initialising to '-1'
> as an otherwise arbitrary (but now defined) value.
> 
> I don't think you need to do that. You can initialise the
> TestPatternMode as Off by 'setting' it (on the device) off when initialising.

That sounds good, we should start with a known state.

> > If we move the cache of testpatternmode to ipu3 pipeline handler, this
> > may is not necessary.
> >
> > > >          - name: TestPatternModeOff
> > > >            value: 0
> > > >            description: |