Re: [PATCH v3 1/5] media: uapi: v4l2-core: Add sensor ancillary data V4L2 fourcc type

[Jacopo Mondi <jacopo@jmondi.org>]

Hello,

On Fri, Nov 06, 2020 at 12:39:07PM +0100, Hans Verkuil wrote:
> On 06/11/2020 12:26, Dave Stevenson wrote:
> > Hi Hans
> >
> > On Fri, 6 Nov 2020 at 09:24, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> Hi Jacopo, Naushir,
> >>
> >> On 02/11/2020 17:52, Jacopo Mondi wrote:
> >>> From: Naushir Patuck <naush@raspberrypi.com>
> >>>
> >>> 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@raspberrypi.com>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  .../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.

Ack

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

You know, I would drop the mention of 'pad number 1' here.
This is not really well been defined for mainline and I would keep
this part as generic as possible.

Thanks
   j

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