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

10864 diff mbox series

Document the feature an image sensor driver has to provide to be
fully libcamera-compliant.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo@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

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@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo@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@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
>

Hi Kieran,

On Mon, Jan 18, 2021 at 09:59:55AM +0000, Kieran Bingham wrote:
> Hi Jacopo,

The series has just been pushed.

I can work on-top

>
> 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@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Signed-off-by: Jacopo Mondi <jacopo@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@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

Hi Jacopo,

On 18/01/2021 10:08, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Mon, Jan 18, 2021 at 09:59:55AM +0000, Kieran Bingham wrote:
>> Hi Jacopo,
> 
> The series has just been pushed.
> 

aha, I'd checked the series wasn't in before I hit send. Looks like I
should have taken a mutex ;-)

> I can work on-top

I think it's only the camera location that sounds like it might be
inaccurate. The other two don't matter at all.

Kieran


>>
>> 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@ideasonboard.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>>> Signed-off-by: Jacopo Mondi <jacopo@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@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