Hi Sakari,
I'll attempt a reply, Dave and Naush which first authored the patch
might provide more valuable feedback than me.
Also, please note v4 is out
On Wed, Nov 11, 2020 at 07:19:29PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, Nov 02, 2020 at 05:52:54PM +0100, 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
> > +
> > +.. _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>`_
> > +
> > +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;
>
> How about "Embedded" instead? This is called embedded data virtually
> everywhere.
I like it better too, and that's the term also used by the csi-2
and ccs specs if I'm not mistaken
>
> Is it meant that all sensors would use this mbus code, or just some? I was
> thinking we'd have sensor specific embedded data formats, but this approach
> admittedly makes implementation easier in quite a few places. What will be
> the documentation requirements for embedded data formats? Anything goes,
> or...? I'm not sure I like that idea. Thoughts, anyone?
>
> If we use an opaque format here, it'll be impossible for the receiver
> driver to know how to pack the data in memory. Although... I guess this is
> generally the responsibility of the software.
>
Dave gave a great summary of how defining less opaque formats might
not be practical in his reply to Hans.
In the current design sensors that produce embedded data expose and
additional pad where it is possible to call the usual subdev_g_fmt to
retrieve the size information. That's all the receiver needs to know
to allocate buffers and properly transfer data there.
I see the CSS specs prescribing embedded data to have the same bit
depth as the image data, but I assume not all vendors might comply,
so this should probably be advertised. The simple solution is to
return a format.width in bytes. You can see that in the unicam driver:
node->v_fmt.fmt.meta.buffersize = mbus_fmt.width
* mbus_fmt.height;
Where the mbus_fmt is fetched from pad #1 of the sensor subdev.
There's no unpacking or parsing as far as I'm aware, and this makes
sense to me as knowledge of the sensor-specific format should not be
in the receiver driver but delegated to a specialized user-space
component. Having it in kernel simply doesn't scale imho (and I fail
to see a need to do so, in the general case).
> This approach will also have the consequence that we'll have an opaque
> sensor embedded data format.
>
> How does the receiver figure out the bits per pixel for this? That'll be
> needed at least for calculating the buffer size when the data is written to
> the memory.
>
As said the current design of unicam and "augmented" sensor drivers
relies on the presence of an additional pad where the receivers simply
calls g_fmt to get the size information to calculate the meteadata
buffer size.
I know this is not v4l2 spec compliant, as the returned frame width
should be in pixels and not bytes and because of the additional pad on
the sensor subdevice. These are the reasons why this driver is in
staging at the moment :)
I think the real question is how this should be designed on mailine
and the actual questions to answer are:
1) How to handle the additional data stream. This can be generalized
to the question of how to handle CSI-2 VC/DT multiplexing
2) Based on the answer to 1) decide how to advertise the packaging
information: encode them in the format (ie
V4L2_META_FMT_EMBEDDED_DATA8_1x8) or access them through one of the
existing uapi/kapi.
In any case, I don't think any knoledge of anything else than
packaging structure should be required in the reciver: ie parsing of
the embedded data content should happen elsewhere.
As 1) seems to be the million dollar question, for the time being I
would rather define an opaque format. In v4 I went for
V4L2_META_FMT_CUSTOM_SENSOR_DATA as suggested by Hans, but this could
very well be V4L2_META_FMT_OPAQUE_EMBEDDED_DATA or similar, with the
documentation more clearly stating that the format does not convey any
information on the actual bit depth and it's up to the receiver to
figure it out.
Do we have anything like staging formats ?
Thanks
j
> >
> > 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
>
> --
> Kind regards,
>
> Sakari Ailus
