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

12564 diff mbox series

Provides a function to set the camera sensor a test pattern mode.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/internal/camera_sensor.h |  7 ++--
 src/libcamera/camera_sensor.cpp            | 40 ++++++++++++++++++++--
 2 files changed, 41 insertions(+), 6 deletions(-)

Hi Hiro,

On Fri, Jun 11, 2021 at 05:42:25PM +0900, Hirokazu Honda wrote:
> Provides a function to set the camera sensor a test pattern mode.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  7 ++--
>  src/libcamera/camera_sensor.cpp            | 40 ++++++++++++++++++++--
>  2 files changed, 41 insertions(+), 6 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index e133ebf4..834d3246 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -39,10 +39,8 @@ public:
>  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
>  	const std::vector<Size> &sizes() const { return sizes_; }
>  	Size resolution() const;
> -	const std::vector<int32_t> &testPatternModes() const
> -	{
> -		return testPatternModes_;
> -	}
> +	std::vector<int32_t> testPatternModes() const;
> +	int setTestPatternMode(uint8_t testPatternMode);
>
>  	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
>  				      const Size &size) const;
> @@ -84,6 +82,7 @@ private:
>  	std::vector<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
>  	std::vector<int32_t> testPatternModes_;

Are you replacing testPatternModes_ or are you adding a new map
alongside with it ? Looking at the code I would say your intention is
to replace it.

Overall, what is the usage for the index-testPattern association ?
1) Enumeration:
  - list indexes through the V4L2 API
  - Use the index to retrieve the control::TestPatternMode from the
    sensor properties
  - Fill the map
  - Expose the collected control::TestPatternMode to pipeline handlers

2) Set a test patter:
  - Receive a control::TestPatternMode from the pipeline handler
  - Set the corresponding index on the V4L2 device

I won't add a map here, we already have one in the camera sensor
properties and I would not duplicate it, it's all static information
after all.

I would rather reverse it and move it from
        map<v4l2-index, control::TestPatternMode>
to
        map<control::TestPatternMode, v4l2-index>

at enumeration time we'll have to pay a little price to search which
key (== control::TestPatternMode) corresponds to the index we got from
the V4L2, but that only happens once at initialization in a very small
map.

The at runtime when we have to set the test pattern mode we simply
access the map and get the index to set on the v4l2 subdev.

So I would:
- Reverse the map in camera sensor properties
  map<control::TestPatternMode, v4l2-index>
- Initialize the CameraSensor::testPatternModes_ vector by reverse
  inspecting the map
- Set the test pattern mode by retrieving from the camera sensor
  properties map which v4l2-index a control::TestPatternMode
  corresponds to

To be honest, if we're diligent and we trust the camera sensor
properties, we could skip the enumeration completely, and return the
list of supported modes by just
utils::map_keys(props->testPatternModes) if you agree to reverse the
map.

Thanks
   j

> +	std::map<int32_t, int32_t> testPatternModeToIndex_;
>
>  	Size pixelArraySize_;
>  	Rectangle activeArea_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 3e135353..d2f033a5 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -329,7 +329,7 @@ void CameraSensor::initTestPatternModes(
>  			continue;
>  		}
>
> -		testPatternModes_.push_back(it->second);
> +		testPatternModeToIndex_[it->second] = index;
>  	}
>  }
>
> @@ -496,12 +496,48 @@ Size CameraSensor::resolution() const
>  }
>
>  /**
> - * \fn CameraSensor::testPatternModes()
>   * \brief Retrieve all the supported test pattern modes of the camera sensor
>   * The test pattern mode values correspond to the controls::TestPattern control.
>   *
>   * \return The list of test pattern modes
>   */
> +std::vector<int32_t> CameraSensor::testPatternModes() const
> +{
> +	return utils::map_keys(testPatternModeToIndex_);
> +}
> +
> +/**
> + * \brief Set the camera sensor a specified controls::TestPatternMode
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)
> +{
> +	auto it = testPatternModeToIndex_.find(testPatternMode);
> +	if (it == testPatternModeToIndex_.end()) {
> +		LOG(CameraSensor, Error) << "Unsupported test pattern mode: "
> +					 << testPatternMode;
> +		return -EINVAL;
> +	}
> +
> +	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;
> +	}
> +
> +	ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);
> +	if (value.get<int32_t>() == index)
> +		return 0;
> +
> +	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
> --
> 2.32.0.272.g935e593368-goog
>