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

14331 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>
---
 include/libcamera/internal/camera_sensor.h |  6 +++
 src/libcamera/camera_sensor.cpp            | 50 +++++++++++++++++++---
 2 files changed, 50 insertions(+), 6 deletions(-)

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@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@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.

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

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

Hi Laurent, thank you for reviewing.

On Thu, Oct 28, 2021 at 9:36 AM Laurent Pinchart
<laurent.pinchart@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@chromium.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham@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@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

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@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@chromium.org>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@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@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

Hi Hiro,

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@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@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);
>
>  	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_;
>
>  	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),
> +	  bayerFormat_(nullptr), properties_(properties::properties)

          props_(nullptr)

Also, we now have props_ and properties_, we should find a better
name.

I would s/props_/staticProps_/ but there might be better alternatives

>  {
>  }
>
> @@ -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_;

The failed lookup will already print a rather verbose warning

		LOG(CameraSensorProperties, Warning)
			<< "No static properties available for '" << sensor << "'";
		LOG(CameraSensorProperties, Warning)
			<< "Please consider updating the camera sensor properties database";

I would drop it from here


>  		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
> + * \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(int32_t testPatternMode)
> +{
> +	if (testPatternMode_ == testPatternMode)
> +		return 0;
> +
> +	if (!props_) {
> +		LOG(CameraSensor, Error) << "No property is found";
> +		return -EINVAL;
> +	}

I'm tempted to say we should either if (!props_) { return 0; } or
ASSERT(props_); as returning an error can easily get not noticed, see
in 2/2 with the un-checked call:
        cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);

Also a bit unrelated but since I just noticed that:
the test pattern mode initialization seems to require a small fix. We have:

void CameraSensor::initStaticProperties()
{
	props_ = CameraSensorProperties::get(model_);

        ....

	initTestPatternModes(props_->testPatternModes);
}

void CameraSensor::initTestPatternModes(
	const std::map<int32_t, int32_t> &testPatternModes)
{
	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
	if (v4l2TestPattern == controls().end()) {
		LOG(CameraSensor, Debug) << "No static test pattern map for \'"
					 << model() << "\'";
		return;
	}
}

The error message in CameraSensor::initTestPatternModes() says
"No static test pattern map for << model()
while what is actually missing is the V4L2 control in the driver.

Should we check that props->testPatternModes is not empty and bail out
from CameraSensor::initTestPatternModes() like we do if the v4l2
control is not found (and fix the error message while at it) so that
once we get to setTestPatternMode() we'll have an empty testPatternModes_
and do not risk to set an unsupported value ?

> +
> +	auto it = props_->testPatternModes.find(testPatternMode);

We build testPatternModes_ in initTestPatternModes() for the purpose
of matching the driver reported indexes and the entries in the sensor
database. I would use the vector to verify the test pattern is
supported instead of the raw map. If in the un-likely case the driver
drops support for a test pattern, we're safe (at the expense of an
additional lookup in a reasonably small vector). Also, in the case the
camera properties do not contain the test patterns map and
you take my above suggestion in of bailing out earlier in
initTestPatternModes() because of that, we'll bail out here by simply
veryfing the vector contains the requested test pattern.

I think you can reduce error check in this function to

        auto it = std::find(testPatternModes_, testPatternMode);
        if (it == testPatternModes_.end())
                Error or Fatal ?

and the simply access the static map (which is now guaranteed to be
present and have an entry for testPatternMode)

        int32_t index = props_->testPatternModes.find(testPatternMode)->second;
        ControlList ctrls{ controls() };
	ctrls.set(V4L2_CID_TEST_PATTERN, index);

        ...


> +	if (it == props_->testPatternModes.end()) {
> +		LOG(CameraSensor, Error) << "Unsupported test pattern mode: "
> +					 << 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
> --
> 2.33.0.1079.g6e70778dc9-goog
>