[libcamera-devel] [PATCH v2.1 02/11] libcamera: camera_sensor: Make V4L2_CID_EXPOSURE mandatory

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Jan 20, 2021 at 11:29:37AM +0100, Jacopo Mondi wrote:
> Add the V4L2_CID_EXPOSURE control to the list of mandatory controls the
> sensor driver has to report and document this new requirement.
> 
> While at it, re-sort the mandatory V4L2 controls in alphabetical
> order in the CameraSensor class and remove the above comment as
> the usage of the controls is better reported in the documentation.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  Documentation/sensor_driver_requirements.rst | 11 ++++++++++-
>  src/libcamera/camera_sensor.cpp              |  5 ++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst
> index 1dc8c890d16d..c45d5a7ae61b 100644
> --- a/Documentation/sensor_driver_requirements.rst
> +++ b/Documentation/sensor_driver_requirements.rst
> @@ -24,13 +24,22 @@ The sensor driver is assumed to be fully compliant with the V4L2 specification.
> 
>  The sensor driver shall support the following V4L2 controls:
> 
> +* `V4L2_CID_EXPOSURE`_
>  * `V4L2_CID_HBLANK`_
>  * `V4L2_CID_PIXEL_RATE`_
> 
> +.. _V4L2_CID_EXPOSURE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/control.html
>  .. _V4L2_CID_HBLANK: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-source.html
>  .. _V4L2_CID_PIXEL_RATE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-process.html
> 
> -Both controls are used to compute the sensor output timings.
> +The `EXPOSURE` control shall report the image integration time in number of
> +lines. While V4L2 doesn't specify a unit for the `EXPOSURE` control, libcamera
> +requires it to be expressed as a number of image lines. Camera sensor drivers

Don't these two sentences say the same thing ? I'd drop the first one.

> +that do not comply with this requirement will need to be adapted or will produce
> +incorrect results.
> +
> +The `HBLANK` and `PIXEL_RATE` controls are used to compute the sensor output
> +timings.
> 
>  Optional Requirements
>  ---------------------
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 9abb9d330664..ca33c0094088 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -239,12 +239,11 @@ int CameraSensor::validateSensorDriver()
>  	/*
>  	 * Make sure the sensor driver supports the mandatory controls
>  	 * required by the CameraSensor class.
> -	 * - V4L2_CID_PIXEL_RATE is used to calculate the sensor timings
> -	 * - V4L2_CID_HBLANK is used to calculate the line length
>  	 */
>  	const std::vector<uint32_t> mandatoryControls{
> -		V4L2_CID_PIXEL_RATE,
> +		V4L2_CID_EXPOSURE,
>  		V4L2_CID_HBLANK,
> +		V4L2_CID_PIXEL_RATE,
>  	};
> 
>  	ControlList ctrls = subdev_->getControls(mandatoryControls);

-- 
Regards,

Laurent Pinchart