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

15039 diff mbox series

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

Hi Hiro,

Thank you for the patch.

On Mon, Dec 06, 2021 at 02:49:17PM +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);

As the type is fairly long, we could shorten lines by renaming the
parameter from testPatternMode to mode (here and below, but not the
testPatternMode_ member). Up to you.

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

I think I recall someone proposing renaming this function to
applyTestPatternMode(), and I think that's a good idea, as that's what
it does.

>  
>  	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_);

I'd drop this, staticProps_ is set by the caller.

> +	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() << "\'";

It could also be that none of the test patterns supported by the sensor
match the test patterns standardized by libcamera. We'll eventually want
to add support for extra patterns (and possibly for custom patterns),
but until we do so, I'd turn this into a Debug message.

>  		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

Let's keep the brief brief :-)

 * \brief Set the test pattern mode for the camera sensor
 * \param[in] testPatternMode The test pattern mode
 *
 * The new \a testPatternMode mode is applied to the sensor if it differs from
 * the active test pattern mode. Otherwise, this function is a no-op. Setting
 * the same test pattern mode for every frame thus incurs no performance
 * penalty.

> + *
> + * \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);

I wonder if we should turn testPatternModes_ into a
std::map<controls::draft::TestPatternModeEnum, int32_t>. The lookup will
be more efficient, and we'll be able to drop the lookup in staticProps_
below. This patch could thus be simplified. This could be done on top
though.

> +	if (it == testPatternModes_.end()) {
> +		LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> +					 << testPatternMode;
> +		return -EINVAL;
> +	}
> +
> +	LOG(CameraSensor, Debug) << "Apply test pattern mode: " << testPatternMode;

s/mode:/mode/

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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