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

[Kieran Bingham]

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 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

-- 
Regards
--
Kieran