[libcamera-devel] [PATCH v2 2/5] libcamera: camera_sensor: Initialize controls

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Dec 28, 2020 at 05:55:57PM +0100, Jacopo Mondi wrote:
> Initialize the ControlInfoMap of sensor available controls by
> reporting the sensor exposure time range.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  2 +
>  src/libcamera/camera_sensor.cpp            | 45 +++++++++++++++++++++-
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index aee10aa6e3c7..0357b2a630f7 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -71,6 +71,7 @@ private:
>  	int generateId();
>  	int validateSensorDriver();
>  	int initProperties();
> +	int initControls();
>  
>  	const MediaEntity *entity_;
>  	std::unique_ptr<V4L2Subdevice> subdev_;
> @@ -85,6 +86,7 @@ private:
>  	std::vector<Size> sizes_;
>  
>  	ControlList properties_;
> +	ControlInfoMap controls_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 71d7c862e69a..18dc6d723e27 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -15,6 +15,7 @@
>  #include <regex>
>  #include <string.h>
>  
> +#include <libcamera/control_ids.h>
>  #include <libcamera/property_ids.h>
>  
>  #include "libcamera/internal/bayer_format.h"
> @@ -215,7 +216,7 @@ int CameraSensor::init()
>  	if (ret)
>  		return ret;
>  
> -	return 0;
> +	return initControls();
>  }
>  
>  int CameraSensor::validateSensorDriver()
> @@ -229,6 +230,7 @@ int CameraSensor::validateSensorDriver()
>  	const std::vector<uint32_t> mandatoryControls{
>  		V4L2_CID_PIXEL_RATE,
>  		V4L2_CID_HBLANK,
> +		V4L2_CID_EXPOSURE,
>  	};
>  
>  	ControlList ctrls = subdev_->getControls(mandatoryControls);
> @@ -428,6 +430,47 @@ int CameraSensor::initProperties()
>  	return 0;
>  }
>  
> +int CameraSensor::initControls()
> +{
> +	const ControlInfoMap &v4l2Controls = subdev_->controls();
> +
> +	/* Calculate exposure time limits expressed in micro-seconds. */
> +
> +	/* Calculate the line length in pixels. */
> +	ControlList ctrls = subdev_->getControls({ V4L2_CID_HBLANK });
> +	if (ctrls.empty())
> +		return -EINVAL;
> +	int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get<int32_t>();
> +
> +	V4L2SubdeviceFormat format{};
> +	int ret = subdev_->getFormat(pad_, &format);
> +	if (ret)
> +		return ret;
> +	int32_t ppl = format.size.width + hblank;

I'd write pixelPerLine or lineLength to make this more explicit.

> +
> +	const ControlInfo &v4l2ExposureInfo = v4l2Controls.find(V4L2_CID_EXPOSURE)->second;
> +	int32_t minExposurePixels = v4l2ExposureInfo.min().get<int32_t>() * ppl;
> +	int32_t maxExposurePixels = v4l2ExposureInfo.max().get<int32_t>() * ppl;
> +	int32_t defExposurePixels = v4l2ExposureInfo.def().get<int32_t>() * ppl;
> +
> +	/* Get the pixel rate (in useconds) and calculate the exposure timings. */
> +	const ControlInfo &pixelRateInfo = v4l2Controls.find(V4L2_CID_PIXEL_RATE)->second;
> +	float minPixelRate = pixelRateInfo.min().get<int64_t>() / 1e6f;
> +	float maxPixelRate = pixelRateInfo.max().get<int64_t>() / 1e6f;
> +	float defPixelRate = pixelRateInfo.def().get<int64_t>() / 1e6f;

The pixel rate is fixed for a given sensor configuration. Sensors could
expose different pixel rates for different configurations, but it
doesn't necessarily mean that the exposure control minimum and maximum
values will be compatible with any pixel rate. Furthermore, horizontal
blanking may vary, and the horizontal active size definitely varies
between different configurations. All this means that exposing an
exposure time from the camera sensor class, with meaningful min/max
values, will be more complicated than this.

We can start with this implementation, with a big \todo comment
explaining that the API is a bit of a hack and that the problem requires
more design work. I'd however use the current pixel rate value, not the
minimum, default and maximum, as there's a bigger chance in that case
that the result of the calculation would be meaningless.

> +
> +	int32_t minExposure = static_cast<int32_t>(minExposurePixels / maxPixelRate);
> +	int32_t maxExposure = static_cast<int32_t>(maxExposurePixels / minPixelRate);
> +	int32_t defExposure = static_cast<int32_t>(defExposurePixels / defPixelRate);
> +
> +	ControlInfoMap::Map map;
> +	map[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
> +						   defExposure);
> +	controls_ = std::move(map);
> +
> +	return 0;
> +}
> +
>  /**
>   * \fn CameraSensor::model()
>   * \brief Retrieve the sensor model name

-- 
Regards,

Laurent Pinchart