[libcamera-devel,v9,2/3] libcamera: camera_sensor: Enable to set a test pattern mode
diff mbox series

Message ID 20211129083424.3136533-2-hiroh@chromium.org
State Superseded
Headers show
Series
  • [libcamera-devel,v9,1/3] libcamera: camera_sensor: Reference test pattern modes by enum type
Related show

Commit Message

Hirokazu Honda Nov. 29, 2021, 8:34 a.m. UTC 
This adds a function to set a camera sensor driver a test pattern
mode. CameraSensor initializes the test pattern mode by Off.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/internal/camera_sensor.h | 11 ++-
 src/libcamera/camera_sensor.cpp            | 82 +++++++++++++++++++---
 2 files changed, 80 insertions(+), 13 deletions(-)

Comments

Jacopo Mondi Dec. 1, 2021, 2:45 p.m. UTC | #1 
Hi Hiro,

On Mon, Nov 29, 2021 at 05:34:23PM +0900, Hirokazu Honda wrote:
> This adds a function to set a camera sensor driver a test pattern
> mode. CameraSensor initializes the test pattern mode by Off.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/internal/camera_sensor.h | 11 ++-
>  src/libcamera/camera_sensor.cpp            | 82 +++++++++++++++++++---
>  2 files changed, 80 insertions(+), 13 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 310571ca..3362eaff 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -28,6 +28,8 @@ namespace libcamera {
>  class BayerFormat;
>  class MediaEntity;
>
> +struct CameraSensorProperties;
> +
>  class CameraSensor : protected Loggable
>  {
>  public:
> @@ -47,6 +49,7 @@ public:
>  	{
>  		return testPatternModes_;
>  	}
> +	int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
>
>  	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
>  				      const Size &size) const;
> @@ -73,15 +76,16 @@ private:
>  	int validateSensorDriver();
>  	void initVimcDefaultProperties();
>  	void initStaticProperties();
> -	void initTestPatternModes(
> -		const std::map<controls::draft::TestPatternModeEnum, int32_t>
> -			&testPatternModeMap);
> +	void initTestPatternModes();
>  	int initProperties();
> +	int initTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
>
>  	const MediaEntity *entity_;
>  	std::unique_ptr<V4L2Subdevice> subdev_;
>  	unsigned int pad_;
>
> +	const CameraSensorProperties *staticProps_;
> +
>  	std::string model_;
>  	std::string id_;
>
> @@ -89,6 +93,7 @@ private:
>  	std::vector<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
>  	std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> +	controls::draft::TestPatternModeEnum testPatternMode_;

This doesn't seem to be initialized. I understand this will be set by
the first call to initTestPatternMode() which happens at init() time,
but I wonder if Coverity or other analysis tools might complain in
this case.

>
>  	Size pixelArraySize_;
>  	Rectangle activeArea_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index f0aa9f24..8a79b970 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), staticProps_(nullptr),
> +	  bayerFormat_(nullptr), properties_(properties::properties)
>  {
>  }
>
> @@ -161,7 +161,7 @@ int CameraSensor::init()
>  	if (ret)
>  		return ret;
>
> -	return 0;
> +	return initTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);
>  }
>
>  int CameraSensor::validateSensorDriver()
> @@ -300,22 +300,33 @@ 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();
>  }
>
> -void CameraSensor::initTestPatternModes(
> -	const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
> +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;
> +	}
> +
> +	ASSERT(staticProps_);

We can't get here it !staticProps_

> +	const auto &testPatternModes = staticProps_->testPatternModes;
> +	if (testPatternModes.empty()) {
> +		/*
> +		 * The camera sensor supports test patterns but we don't know
> +		 * how to map them so this should be fixed.
> +		 */
> +		LOG(CameraSensor, Error) << "No static test pattern map for \'"
>  					 << model() << "\'";
>  		return;
>  	}
> @@ -531,6 +542,57 @@ Size CameraSensor::resolution() const
>   * \return The list of test pattern modes
>   */
>
> +/**
> + * \brief Set the test pattern mode for the camera sensor if it is not the
> + *  currently set test pattern mode.
> + * \param[in] testPatternMode Test pattern mode control value to set the camera
> + * sensor

Drop blank lines between param and return if there is no additional
description

> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CameraSensor::setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode)
> +{
> +	if (testPatternMode_ == testPatternMode)
> +		return 0;
> +
> +	return initTestPatternMode(testPatternMode);
> +}
> +
> +/**
> + * \brief Set the test pattern mode for the camera sensor
> + * \param[in] testPatternMode Test pattern mode control value to set the camera
> + * sensor
> + *

Ditto

> + * \return 0 on success or a negative error code otherwise
    * \retval -EINVAL \a testPatternMode is not supported by the sensor
> + */
> +int CameraSensor::initTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode)
> +{
> +	if (!staticProps_ || testPatternModes_.empty())
> +		return 0;

Checking if testPatternModes_ is empty should be enough, but that's
not a big deal

> +
> +	auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> +			    testPatternMode);
> +	if (it == testPatternModes_.end()) {
> +		LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> +					 << testPatternMode;
> +		return -EINVAL;
> +	}
> +
> +	LOG(CameraSensor, Debug) << "Apply test pattern mode: " << testPatternMode;
> +
> +	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;
> +
> +	return 0;
> +}
> +

All minors that could be fixed while applying

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>  /**
>   * \brief Retrieve the best sensor format for a desired output
>   * \param[in] mbusCodes The list of acceptable media bus codes
> --
> 2.34.0.rc2.393.gf8c9666880-goog
>
Jacopo Mondi Dec. 1, 2021, 2:47 p.m. UTC | #2 
Hi Hiro,

On Mon, Nov 29, 2021 at 05:34:23PM +0900, Hirokazu Honda wrote:
> This adds a function to set a camera sensor driver a test pattern
> mode. CameraSensor initializes the test pattern mode by Off.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/internal/camera_sensor.h | 11 ++-
>  src/libcamera/camera_sensor.cpp            | 82 +++++++++++++++++++---
>  2 files changed, 80 insertions(+), 13 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 310571ca..3362eaff 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -28,6 +28,8 @@ namespace libcamera {
>  class BayerFormat;
>  class MediaEntity;
>
> +struct CameraSensorProperties;
> +
>  class CameraSensor : protected Loggable
>  {
>  public:
> @@ -47,6 +49,7 @@ public:
>  	{
>  		return testPatternModes_;
>  	}
> +	int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
>
>  	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
>  				      const Size &size) const;
> @@ -73,15 +76,16 @@ private:
>  	int validateSensorDriver();
>  	void initVimcDefaultProperties();
>  	void initStaticProperties();
> -	void initTestPatternModes(
> -		const std::map<controls::draft::TestPatternModeEnum, int32_t>
> -			&testPatternModeMap);
> +	void initTestPatternModes();

Also I think this should be called applyTestPatternMode() as it is
called by setTestPatternMode()

>  	int initProperties();
> +	int initTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
>
>  	const MediaEntity *entity_;
>  	std::unique_ptr<V4L2Subdevice> subdev_;
>  	unsigned int pad_;
>
> +	const CameraSensorProperties *staticProps_;
> +
>  	std::string model_;
>  	std::string id_;
>
> @@ -89,6 +93,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..8a79b970 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), staticProps_(nullptr),
> +	  bayerFormat_(nullptr), properties_(properties::properties)
>  {
>  }
>
> @@ -161,7 +161,7 @@ int CameraSensor::init()
>  	if (ret)
>  		return ret;
>
> -	return 0;
> +	return initTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);
>  }
>
>  int CameraSensor::validateSensorDriver()
> @@ -300,22 +300,33 @@ 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();
>  }
>
> -void CameraSensor::initTestPatternModes(
> -	const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
> +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;
> +	}
> +
> +	ASSERT(staticProps_);
> +	const auto &testPatternModes = staticProps_->testPatternModes;
> +	if (testPatternModes.empty()) {
> +		/*
> +		 * The camera sensor supports test patterns but we don't know
> +		 * how to map them so this should be fixed.
> +		 */
> +		LOG(CameraSensor, Error) << "No static test pattern map for \'"
>  					 << model() << "\'";
>  		return;
>  	}
> @@ -531,6 +542,57 @@ Size CameraSensor::resolution() const
>   * \return The list of test pattern modes
>   */
>
> +/**
> + * \brief Set the test pattern mode for the camera sensor if it is not the
> + *  currently set test pattern mode.
> + * \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;
> +
> +	return initTestPatternMode(testPatternMode);
> +}
> +
> +/**
> + * \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::initTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode)
> +{
> +	if (!staticProps_ || testPatternModes_.empty())
> +		return 0;
> +
> +	auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> +			    testPatternMode);
> +	if (it == testPatternModes_.end()) {
> +		LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> +					 << testPatternMode;
> +		return -EINVAL;
> +	}
> +
> +	LOG(CameraSensor, Debug) << "Apply test pattern mode: " << testPatternMode;
> +
> +	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;
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Retrieve the best sensor format for a desired output
>   * \param[in] mbusCodes The list of acceptable media bus codes
> --
> 2.34.0.rc2.393.gf8c9666880-goog
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index 310571ca..3362eaff 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -28,6 +28,8 @@  namespace libcamera {
 class BayerFormat;
 class MediaEntity;
 
+struct CameraSensorProperties;
+
 class CameraSensor : protected Loggable
 {
 public:
@@ -47,6 +49,7 @@  public:
 	{
 		return testPatternModes_;
 	}
+	int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
 
 	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
 				      const Size &size) const;
@@ -73,15 +76,16 @@  private:
 	int validateSensorDriver();
 	void initVimcDefaultProperties();
 	void initStaticProperties();
-	void initTestPatternModes(
-		const std::map<controls::draft::TestPatternModeEnum, int32_t>
-			&testPatternModeMap);
+	void initTestPatternModes();
 	int initProperties();
+	int initTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
 
 	const MediaEntity *entity_;
 	std::unique_ptr<V4L2Subdevice> subdev_;
 	unsigned int pad_;
 
+	const CameraSensorProperties *staticProps_;
+
 	std::string model_;
 	std::string id_;
 
@@ -89,6 +93,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..8a79b970 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), staticProps_(nullptr),
+	  bayerFormat_(nullptr), properties_(properties::properties)
 {
 }
 
@@ -161,7 +161,7 @@  int CameraSensor::init()
 	if (ret)
 		return ret;
 
-	return 0;
+	return initTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);
 }
 
 int CameraSensor::validateSensorDriver()
@@ -300,22 +300,33 @@  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();
 }
 
-void CameraSensor::initTestPatternModes(
-	const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
+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;
+	}
+
+	ASSERT(staticProps_);
+	const auto &testPatternModes = staticProps_->testPatternModes;
+	if (testPatternModes.empty()) {
+		/*
+		 * The camera sensor supports test patterns but we don't know
+		 * how to map them so this should be fixed.
+		 */
+		LOG(CameraSensor, Error) << "No static test pattern map for \'"
 					 << model() << "\'";
 		return;
 	}
@@ -531,6 +542,57 @@  Size CameraSensor::resolution() const
  * \return The list of test pattern modes
  */
 
+/**
+ * \brief Set the test pattern mode for the camera sensor if it is not the
+ *  currently set test pattern mode.
+ * \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;
+
+	return initTestPatternMode(testPatternMode);
+}
+
+/**
+ * \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::initTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode)
+{
+	if (!staticProps_ || testPatternModes_.empty())
+		return 0;
+
+	auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
+			    testPatternMode);
+	if (it == testPatternModes_.end()) {
+		LOG(CameraSensor, Error) << "Unsupported test pattern mode "
+					 << testPatternMode;
+		return -EINVAL;
+	}
+
+	LOG(CameraSensor, Debug) << "Apply test pattern mode: " << testPatternMode;
+
+	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;
+
+	return 0;
+}
+
 /**
  * \brief Retrieve the best sensor format for a desired output
  * \param[in] mbusCodes The list of acceptable media bus codes