[libcamera-devel] [PATCH v7 01/10] libcamera: Document sensor driver requirements

[Kieran Bingham]

Hi Jacopo,

On 15/01/2021 17:00, Jacopo Mondi wrote:
> Document the feature an image sensor driver has to provide to be
> fully libcamera-compliant.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  Documentation/index.rst                      |  1 +
>  Documentation/meson.build                    |  1 +
>  Documentation/sensor_driver_requirements.rst | 64 ++++++++++++++++++++
>  3 files changed, 66 insertions(+)
>  create mode 100644 Documentation/sensor_driver_requirements.rst
> 
> diff --git a/Documentation/index.rst b/Documentation/index.rst
> index c49db18d52bd..285ca7c3e5ae 100644
> --- a/Documentation/index.rst
> +++ b/Documentation/index.rst
> @@ -18,3 +18,4 @@
>     Pipeline Handler Writer's Guide <guides/pipeline-handler>
>     Tracing guide <guides/tracing>
>     Environment variables <environment_variables>
> +   Sensor driver requirements <sensor_driver_requirements>
> diff --git a/Documentation/meson.build b/Documentation/meson.build
> index b1f5bb52474c..9950465ded6e 100644
> --- a/Documentation/meson.build
> +++ b/Documentation/meson.build
> @@ -57,6 +57,7 @@ if sphinx.found()
>          'guides/pipeline-handler.rst',
>          'guides/tracing.rst',
>          'index.rst',
> +        'sensor_driver_requirements.rst',
>         '../README.rst',
>      ]
>  
> diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst
> new file mode 100644
> index 000000000000..0e658aaa03b5
> --- /dev/null
> +++ b/Documentation/sensor_driver_requirements.rst
> @@ -0,0 +1,64 @@
> +.. SPDX-License-Identifier: CC-BY-SA-4.0
> +
> +.. _sensor-driver-requirements:
> +
> +Sensor Driver Requirements
> +==========================
> +
> +libcamera handles imaging devices in the CameraSensor class and defines
> +through its API a consistent interface towards other library components.

Optional change:

'and defines a consistent interface through it's API towards other'

> +
> +The CameraSensor class uses the V4L2 subdev kernel API to interface with the
> +camera sensor through one or multiple sub-devices exposed in userspace by
> +the sensor driver.
> +
> +In order for libcamera to be fully operational and provide to applications and
> +pipeline handlers all the required information to interface with the camera

Another re-order:

'and provide all the required information to interface with the camera
sensor to applications,'


Both of those are perfectly understandable as you have written them,
it's just that we wouldn't write it in that order in English ;-)

No idea why or what the grammar rules are, that's just a pure 'I'd write
that the other way' warning light fired in my head.


So - no need to change if you don't want to.

> +sensor, a set of mandatory and optional features the driver has to support
> +has been defined.
> +
> +Mandatory Requirements
> +----------------------
> +
> +The sensor driver is assumed to be fully compliant with the V4L2 specification.
> +
> +The sensor driver shall support the following V4L2 controls:
> +
> +* `V4L2_CID_HBLANK`_
> +* `V4L2_CID_PIXEL_RATE`_
> +
> +.. _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.
> +
> +Optional Requirements
> +---------------------
> +
> +The sensor driver should support the following V4L2 controls:
> +
> +* `V4L2_CID_CAMERA_ORIENTATION`_
> +* `V4L2_CID_CAMERA_SENSOR_ROTATION`_
> +
> +.. _V4L2_CID_CAMERA_ORIENTATION: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-camera.html
> +.. _V4L2_CID_CAMERA_SENSOR_ROTATION: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-process.html
> +
> +The controls are used to register the camera location and orientation.

Should that read the camera location and rotation?

Given the above and the definition of V4L2_CID_CAMERA_ORIENTATION, I
think the orientation is the 'location'...


With those updated or not as you see fit:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +
> +The sensor driver should implement support for the V4L2 Selection API,
> +specifically it should implement support for the
> +`VIDIOC_SUBDEV_G_SELECTION`_ ioctl with support for the following selection
> +targets:
> +
> +.. _VIDIOC_SUBDEV_G_SELECTION: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-selection.html?highlight=g_selection#c.V4L.VIDIOC_SUBDEV_G_SELECTION
> +
> +* `V4L2_SEL_TGT_CROP_BOUNDS`_ to report the readable pixel array area size
> +* `V4L2_SEL_TGT_CROP_DEFAULT`_ to report the active pixel array area size
> +* `V4L2_SEL_TGT_CROP`_ to report the analogue selection rectangle
> +
> +Support for the selection API is scheduled to become a mandatory feature in
> +the near future.
> +
> +.. _V4L2_SEL_TGT_CROP_BOUNDS: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/v4l2-selection-targets.html
> +.. _V4L2_SEL_TGT_CROP_DEFAULT: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/v4l2-selection-targets.html
> +.. _V4L2_SEL_TGT_CROP: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/v4l2-selection-targets.html
> 

-- 
Regards
--
Kieran