[libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor: Enable to set a test pattern mode

[Jacopo Mondi]

Hi Hiro,

On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:
> Provides a function to set the camera sensor a test pattern mode.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  1 +
>  src/libcamera/camera_sensor.cpp            | 39 ++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index e133ebf4..8b9f84c9 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -43,6 +43,7 @@ public:
>  	{
>  		return testPatternModes_;
>  	}
> +	int setTestPatternMode(uint8_t testPatternMode);
>
>  	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
>  				      const Size &size) const;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 70bbd97a..ce8ff274 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const
>   * \return The list of test pattern modes
>   */
>
> +/**
> + * \brief Set the camera sensor a specified controls::TestPatternMode

Doxygen is puzzling me recently... the testPatternMode paramter is not
documented but I don't get a warning :/

Also, I'm a bit debated about where to actually place this function
(or better, it's initTestPatternModes which is misplaced)
if we have to respect the declaration order or the way you sorted them
here (which is more logical) is better.

> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)
> +{
> +	if (std::find(testPatternModes_.begin(), testPatternModes_.end(),
> +		      testPatternMode) == testPatternModes_.end()) {
> +		LOG(CameraSensor, Error) << "Unsupported test pattern mode: "
> +					 << testPatternMode;
> +		return -EINVAL;
> +	}

Do we need this ? The testPatternModes_ map is constructed using the
camera sensor properties, the below find() on the props->testPatternModes
map should gurantee that is supported. Also, if a test pattern mode is
not reported as part of testPatternModes_ applications shouldn't set
it...

> +
> +	const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> +	if (!props)
> +		return -EINVAL;
> +
> +	auto it = props->testPatternModes.find(testPatternMode);
> +	ASSERT(it != props->testPatternModes.end());

Yeah, I think the assertion here is more than enough..

> +	const uint8_t index = it->second;
> +
> +	ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });
> +	if (ctrls.empty()) {
> +		LOG(CameraSensor, Error)
> +			<< "Failed to retrieve test pattern control";
> +		return -EINVAL;
> +	}

We won't register the TestPatternMode control in first place the V4L2
control is not supported..

> +
> +	ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);
> +	if (value.get<int32_t>() == index)
> +		return 0;

The V4L2 control framework should take care of this by itself, I think
we can set the control regardless

> +
> +	value.set<int32_t>(index);
> +	ctrls.set(V4L2_CID_TEST_PATTERN, value);
> +
> +	return setControls(&ctrls);
> +}
> +
>  /**
>   * \brief Retrieve the best sensor format for a desired output
>   * \param[in] mbusCodes The list of acceptable media bus codes
> --
> 2.32.0.288.g62a8d224e6-goog
>