On 06/11/2020 12:26, Dave Stevenson wrote:
> Hi Hans
>
> On Fri, 6 Nov 2020 at 09:24, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>
>> Hi Jacopo, Naushir,
>>
>> On 02/11/2020 17:52, Jacopo Mondi wrote:
>>> From: Naushir Patuck <naush@xxxxxxxxxxxxxxx>
>>>
>>> Add V4L2_META_FMT_SENSOR_DATA format 4CC.
>>>
>>> This new format will be used to return camera sensor embedded data.
>>>
>>> Signed-off-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>>> ---
>>> .../userspace-api/media/v4l/meta-formats.rst | 1 +
>>> .../media/v4l/pixfmt-meta-sensor-data.rst | 32 +++++++++++++++++++
>>> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
>>> include/uapi/linux/videodev2.h | 1 +
>>> 4 files changed, 35 insertions(+)
>>> create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-meta-sensor-data.rst
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/meta-formats.rst b/Documentation/userspace-api/media/v4l/meta-formats.rst
>>> index fff25357fe860..b2201d1524eb6 100644
>>> --- a/Documentation/userspace-api/media/v4l/meta-formats.rst
>>> +++ b/Documentation/userspace-api/media/v4l/meta-formats.rst
>>> @@ -15,6 +15,7 @@ These formats are used for the :ref:`metadata` interface only.
>>> pixfmt-meta-d4xx
>>> pixfmt-meta-intel-ipu3
>>> pixfmt-meta-rkisp1
>>> + pixfmt-meta-sensor-data
>>> pixfmt-meta-uvc
>>> pixfmt-meta-vsp1-hgo
>>> pixfmt-meta-vsp1-hgt
>>> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-meta-sensor-data.rst b/Documentation/userspace-api/media/v4l/pixfmt-meta-sensor-data.rst
>>> new file mode 100644
>>> index 0000000000000..639ede1a8fee3
>>> --- /dev/null
>>> +++ b/Documentation/userspace-api/media/v4l/pixfmt-meta-sensor-data.rst
>>> @@ -0,0 +1,32 @@
>>> +.. Permission is granted to copy, distribute and/or modify this
>>> +.. document under the terms of the GNU Free Documentation License,
>>> +.. Version 1.1 or any later version published by the Free Software
>>> +.. Foundation, with no Invariant Sections, no Front-Cover Texts
>>> +.. and no Back-Cover Texts. A copy of the license is included at
>>> +.. Documentation/media/uapi/fdl-appendix.rst.
>>> +..
>>> +.. TODO: replace it to GFDL-1.1-or-later WITH no-invariant-sections
>>
>> You can now use this:
>>
>> .. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>
>> and drop the TODO and license notice.
>>
>>> +
>>> +.. _v4l2-meta-fmt-sensor-data:
>>> +
>>> +***********************************
>>> +V4L2_META_FMT_SENSOR_DATA ('SENS')
>>> +***********************************
>>> +
>>> +Sensor Ancillary Metadata
>>> +
>>> +Description
>>> +===========
>>> +
>>> +This format describes ancillary data generated by a camera sensor and
>>> +transmitted over a stream on the camera bus. Most sensor vendors have their
>>> +own custom format for this ancillary data. Some vendors follow a generic
>>> +CSI-2/SMIA embedded data format as described in the `CSI-2 specification.
>>> +<https://mipi.org/specifications/csi-2>`_
>>
>> There are really two metadata formats here: one is a custom sensor format and one
>> a CSI2 format. That's two pixel formats.
>>
>> Of course, if the custom format used by a sensor (or sensor vendor) is known,
>> then it should get its own fourcc.
>>
>> I suggest creating two metadata formats:
>>
>> V4L2_META_FMT_CSI2_SENSOR_DATA
>> V4L2_META_FMT_CUSTOM_SENSOR_DATA
>>
>> Where the format of the latter is unknown (unless you have the information
>> from the sensor vendor under NDA).
>
> It's possibly badly worded here, but the SMIA functional spec[1] only
> gives information on a packing, not of the actual meaning of those
> packed bytes. It defines how the sensor register set that applied for
> that captured frame has been packed into the buffer. Without
> additional sensor specific information (eg which register controls
> gain, how is the gain code computed, what's the register(s) for
> exposure and then the units for that, etc) that is still meaningless.
> A fully SMIA compliant sensor did specify the register set, and it
> included enough config information to give those formulae. However
> SMIA is dead, and the MIPI CCS is being fairly slow in gaining
> traction as the next iteration of that "generic sensor" concept.
>
> Thread from the last round of review -
> https://www.spinics.net/lists/linux-media/msg168700.html
>
> If you want V4L2_META_FMT_CSI2_SENSOR_DATA to denote that it can be
> unpacked to a (sensor specific) register set, then you actually need
> separate formats for 8, 10, 12, 14, and two 16bit packings.
> That format will change based on the image format selected from the
> sensor, so now you need to support the source changed event callback
> for any sensors that support multiple bit depths. (Change the image
> bit depth and get a source changed for the metadata)
>
> Do we just drop any mention of how this data is formatted and that the
> SMIA packing spec exists?
> If smiapp or Sakari's new CCS driver wants to expand on this at a
> later date so that you can write a generic parser to get back to
> something meaningful, then those formats get added at that point.
> Denoting SMIA packing on a non-SMIA compliant sensor is a meaningless
> distinction.
>
> Libcamera already has a sensor specific lookup based on the name to
> know how to interpret the embedded data, and that is going to be the
> case for every user (except on fully compliant SMIA or CCS sensors).
What a mess.
OK, let's pick V4L2_META_FMT_CUSTOM_SENSOR_DATA or something along those
lines that indicates that the format is sensor specific.
Just calling it V4L2_META_FMT_SENSOR_DATA is too generic and doesn't
convey that in order to understand the metadata, you have to know what
the sensor does.
Regards,
Hans
>
> Dave
>
> [1] http://read.pudn.com/downloads95/doc/project/382834/SMIA/SMIA_Functional_specification_1.0.pdf
> page 74, section 4.9.1.
>
>>> +
>>> +The size of the embedded buffer is defined as a single line with a pixel width
>>> +specified in bytes. This is obtained by a call to the
>>> +:c:type:`VIDIOC_SUBDEV_G_FMT` ioctl on the sensor subdevice where the ``pad``
>>> +field in :c:type:`v4l2_subdev_format` is set to 1. Note that this size is fixed
>>> +and cannot be modified with a call to :c:type:`VIDIOC_SUBDEV_S_FMT`.
>>> +
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index eeff398fbdcc1..d01d9ca6578df 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -1402,6 +1402,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>>> case V4L2_META_FMT_UVC: descr = "UVC Payload Header Metadata"; break;
>>> case V4L2_META_FMT_D4XX: descr = "Intel D4xx UVC Metadata"; break;
>>> case V4L2_META_FMT_VIVID: descr = "Vivid Metadata"; break;
>>> + case V4L2_META_FMT_SENSOR_DATA: descr = "Sensor Ancillary Metadata"; break;
>>>
>>> default:
>>> /* Compressed formats */
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 534eaa4d39bc8..b7e3185e66631 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -769,6 +769,7 @@ struct v4l2_pix_format {
>>> #define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* UVC Payload Header metadata */
>>> #define V4L2_META_FMT_D4XX v4l2_fourcc('D', '4', 'X', 'X') /* D4XX Payload Header metadata */
>>> #define V4L2_META_FMT_VIVID v4l2_fourcc('V', 'I', 'V', 'D') /* Vivid Metadata */
>>> +#define V4L2_META_FMT_SENSOR_DATA v4l2_fourcc('S', 'E', 'N', 'S') /* Sensor Ancillary metadata */
>>>
>>> /* priv field value to indicates that subsequent fields are valid. */
>>> #define V4L2_PIX_FMT_PRIV_MAGIC 0xfeedcafe
>>>
>>
>> Regards,
>>
>> Hans
