| Message ID | 1501245205-15802-3-git-send-email-g.liakhovetski@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/28/2017 02:33 PM, Guennadi Liakhovetski wrote: > Add a pixel format, used by the UVC driver to stream metadata. > > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com> > --- > Documentation/media/uapi/v4l/meta-formats.rst | 1 + > Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst | 39 ++++++++++++++++++++++++ > include/uapi/linux/videodev2.h | 1 + > 3 files changed, 41 insertions(+) > create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst > > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst b/Documentation/media/uapi/v4l/meta-formats.rst > index 01e24e3..1bb45a3f 100644 > --- a/Documentation/media/uapi/v4l/meta-formats.rst > +++ b/Documentation/media/uapi/v4l/meta-formats.rst > @@ -14,3 +14,4 @@ These formats are used for the :ref:`metadata` interface only. > > pixfmt-meta-vsp1-hgo > pixfmt-meta-vsp1-hgt > + pixfmt-meta-uvc > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst b/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst > new file mode 100644 > index 0000000..58f78cb > --- /dev/null > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst > @@ -0,0 +1,39 @@ > +.. -*- coding: utf-8; mode: rst -*- > + > +.. _v4l2-meta-fmt-uvc: > + > +******************************* > +V4L2_META_FMT_UVC ('UVCH') > +******************************* > + > +UVC Payload Header Data > + > + > +Description > +=========== > + > +This format describes data, supplied by the UVC driver from metadata video > +nodes. That data includes UVC Payload Header contents and auxiliary timing > +information, required for precise interpretation of timestamps, contained in > +those headers. Buffers, streamed via UVC metadata nodes, are composed of blocks > +of variable length. Those blocks contain are described by struct uvc_meta_buf > +and contain the following fields: > + > +.. flat-table:: UVC Metadata Block > + :widths: 1 4 > + :header-rows: 1 > + :stub-columns: 0 > + > + * - Field > + - Description > + * - struct timespec ts; > + - system timestamp, measured by the driver upon reception of the payload Out of date: this is now a __u64 ns field. > + * - __u16 sof; > + - USB Frame Number, also obtained by the driver > + * - :cspan:`1` *The rest is an exact copy of the payload header:* > + * - __u8 length; > + - length of the rest of the block, including this field > + * - __u8 flags; > + - Flags, indicating presence of other standard UVC fields > + * - __u8 buf[]; > + - The rest of the header, possibly including UVC PTS and SCR fields > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 45cf735..0aad91c 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -682,6 +682,7 @@ struct v4l2_pix_format { > /* Meta-data formats */ > #define V4L2_META_FMT_VSP1_HGO v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car VSP1 1-D Histogram */ > #define V4L2_META_FMT_VSP1_HGT v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */ > +#define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* UVC Payload Header metadata */ > > /* priv field value to indicates that subsequent fields are valid. */ > #define V4L2_PIX_FMT_PRIV_MAGIC 0xfeedcafe >
Hi Guennadi, (CC'ing Sakari Ailus) Thank you for the patch. On Friday, 28 July 2017 15:33:21 EEST Guennadi Liakhovetski wrote: > Add a pixel format, used by the UVC driver to stream metadata. > > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com> > --- > Documentation/media/uapi/v4l/meta-formats.rst | 1 + > Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst | 39 +++++++++++++++++++++ > include/uapi/linux/videodev2.h | 1 + > 3 files changed, 41 insertions(+) > create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst > > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst > b/Documentation/media/uapi/v4l/meta-formats.rst index 01e24e3..1bb45a3f > 100644 > --- a/Documentation/media/uapi/v4l/meta-formats.rst > +++ b/Documentation/media/uapi/v4l/meta-formats.rst > @@ -14,3 +14,4 @@ These formats are used for the :ref:`metadata` interface > only. > > pixfmt-meta-vsp1-hgo > pixfmt-meta-vsp1-hgt > + pixfmt-meta-uvc It might make sense to keep this alphabetically sorted. > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst > b/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst new file mode 100644 > index 0000000..58f78cb > --- /dev/null > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst > @@ -0,0 +1,39 @@ > +.. -*- coding: utf-8; mode: rst -*- > + > +.. _v4l2-meta-fmt-uvc: > + > +******************************* > +V4L2_META_FMT_UVC ('UVCH') > +******************************* > + > +UVC Payload Header Data > + > + > +Description > +=========== > + > +This format describes data, supplied by the UVC driver from metadata video > +nodes. "supplied from metadata video nodes" sounds strange. How about "This format describes metadata extracted fom UVC packet headers and provided by the UVC driver through metadata video nodes." > That data includes UVC Payload Header contents and auxiliary timing > +information, required for precise interpretation of timestamps, contained > in > +those headers. Buffers, streamed via UVC metadata nodes, are composed of > blocks > +of variable length. Those blocks contain are described by struct > uvc_meta_buf > +and contain the following fields: You should stated whether a buffer can contain multiple blocks or always contains a single one. In the first case, it would also make sense to explain how many blocks an application can expect. Another item that needs to be documented is how redundant blocks can be omitted (for instance blocks corresponding to header that have the same SCR and PTS as the previous one, and no additional device-specific metadata). > +.. flat-table:: UVC Metadata Block > + :widths: 1 4 > + :header-rows: 1 > + :stub-columns: 0 > + > + * - Field > + - Description > + * - struct timespec ts; > + - system timestamp, measured by the driver upon reception of the > payload As Hans mentioned, this should be a __u64. > + * - __u16 sof; > + - USB Frame Number, also obtained by the driver You should document that the system timestamp and USB frame number are sampled as close as possible to each other and can be use to correlate the system and USB clocks. > + * - :cspan:`1` *The rest is an exact copy of the payload header:* I'd say "UVC payload header". I would reference the relevant part of the UVC specification. > + * - __u8 length; > + - length of the rest of the block, including this field > + * - __u8 flags; > + - Flags, indicating presence of other standard UVC fields > + * - __u8 buf[]; > + - The rest of the header, possibly including UVC PTS and SCR fields > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 45cf735..0aad91c 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -682,6 +682,7 @@ struct v4l2_pix_format { > /* Meta-data formats */ > #define V4L2_META_FMT_VSP1_HGO v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car > VSP1 1-D Histogram */ #define V4L2_META_FMT_VSP1_HGT v4l2_fourcc('V', > 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */ +#define V4L2_META_FMT_UVC > v4l2_fourcc('U', 'V', 'C', 'H') /* UVC Payload Header metadata */ > > /* priv field value to indicates that subsequent fields are valid. */ > #define V4L2_PIX_FMT_PRIV_MAGIC 0xfeedcafe The main thing that bothers me here is that I'm worried vendors will not play fair and will use this catch-all format to pass undocumented vendor-specific metadata to application without documenting them. It's not a new concern and we've discussed it before, without unfortunately finding a good solution. Recently in a private conversation Sakari proposed using one V4L2 pixel format per vendor metadata format. As V4L2 pixel formats have to be documented, this could put some pressure on vendors to document formats. Of course it wouldn't prevent vendors from not playing fair and providing documentation that is incomplete and/or incorrect. Another option, without introducing different formats for different vendors, is to require vendors to document their format in order to have the UVC_DEV_FLAG_METADATA_NODE flag set for their device in the uvcvideo driver. I'm not sure if one option is better than the other for the matter of getting vendors to document formats. On a related note, this series doesn't enable metadata nodes for the devices you're working on. Wouldn't it be a good idea to fix that, and document the format of your metadata as an example of what vendors are expected to provide ?
Hi Guennadi, On 07/28/2017 02:46 PM, Hans Verkuil wrote: > On 07/28/2017 02:33 PM, Guennadi Liakhovetski wrote: >> Add a pixel format, used by the UVC driver to stream metadata. >> >> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com> >> --- >> Documentation/media/uapi/v4l/meta-formats.rst | 1 + >> Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst | 39 ++++++++++++++++++++++++ >> include/uapi/linux/videodev2.h | 1 + >> 3 files changed, 41 insertions(+) >> create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst >> >> diff --git a/Documentation/media/uapi/v4l/meta-formats.rst b/Documentation/media/uapi/v4l/meta-formats.rst >> index 01e24e3..1bb45a3f 100644 >> --- a/Documentation/media/uapi/v4l/meta-formats.rst >> +++ b/Documentation/media/uapi/v4l/meta-formats.rst >> @@ -14,3 +14,4 @@ These formats are used for the :ref:`metadata` interface only. >> >> pixfmt-meta-vsp1-hgo >> pixfmt-meta-vsp1-hgt >> + pixfmt-meta-uvc >> diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst b/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst >> new file mode 100644 >> index 0000000..58f78cb >> --- /dev/null >> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst >> @@ -0,0 +1,39 @@ >> +.. -*- coding: utf-8; mode: rst -*- >> + >> +.. _v4l2-meta-fmt-uvc: >> + >> +******************************* >> +V4L2_META_FMT_UVC ('UVCH') >> +******************************* >> + >> +UVC Payload Header Data >> + >> + >> +Description >> +=========== >> + >> +This format describes data, supplied by the UVC driver from metadata video >> +nodes. That data includes UVC Payload Header contents and auxiliary timing >> +information, required for precise interpretation of timestamps, contained in >> +those headers. Buffers, streamed via UVC metadata nodes, are composed of blocks >> +of variable length. Those blocks contain are described by struct uvc_meta_buf >> +and contain the following fields: >> + >> +.. flat-table:: UVC Metadata Block >> + :widths: 1 4 >> + :header-rows: 1 >> + :stub-columns: 0 >> + >> + * - Field >> + - Description >> + * - struct timespec ts; >> + - system timestamp, measured by the driver upon reception of the payload > > Out of date: this is now a __u64 ns field. > >> + * - __u16 sof; >> + - USB Frame Number, also obtained by the driver It should be documented that these two fields are in host endian order. I assume there is no padding between the fields? (i.e., are they packed?). >> + * - :cspan:`1` *The rest is an exact copy of the payload header:* >> + * - __u8 length; >> + - length of the rest of the block, including this field >> + * - __u8 flags; >> + - Flags, indicating presence of other standard UVC fields >> + * - __u8 buf[]; >> + - The rest of the header, possibly including UVC PTS and SCR fields >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >> index 45cf735..0aad91c 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -682,6 +682,7 @@ struct v4l2_pix_format { >> /* Meta-data formats */ >> #define V4L2_META_FMT_VSP1_HGO v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car VSP1 1-D Histogram */ >> #define V4L2_META_FMT_VSP1_HGT v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */ >> +#define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* UVC Payload Header metadata */ I discussed this with Laurent last week and since the metadata for UVC starts with a standard header followed by vendor-specific data it makes sense to use V4L2_META_FMT_UVC for just the standard header. Any vendor specific formats should have their own fourcc which starts with the standard header followed by the custom data. The UVC driver would enumerate both the standard and the vendor specific fourcc. This would allow generic UVC applications to use the standard header. Applications that know about the vendor specific data can select the vendor specific format. This change would make this much more convenient to use. Regards, Hans >> >> /* priv field value to indicates that subsequent fields are valid. */ >> #define V4L2_PIX_FMT_PRIV_MAGIC 0xfeedcafe >> >
Hi Hans, Thanks for the comments. On Mon, 30 Oct 2017, Hans Verkuil wrote: > Hi Guennadi, > > On 07/28/2017 02:46 PM, Hans Verkuil wrote: > > On 07/28/2017 02:33 PM, Guennadi Liakhovetski wrote: > >> Add a pixel format, used by the UVC driver to stream metadata. > >> > >> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com> > >> --- > >> Documentation/media/uapi/v4l/meta-formats.rst | 1 + > >> Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst | 39 ++++++++++++++++++++++++ > >> include/uapi/linux/videodev2.h | 1 + > >> 3 files changed, 41 insertions(+) > >> create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst [snip] > >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > >> index 45cf735..0aad91c 100644 > >> --- a/include/uapi/linux/videodev2.h > >> +++ b/include/uapi/linux/videodev2.h > >> @@ -682,6 +682,7 @@ struct v4l2_pix_format { > >> /* Meta-data formats */ > >> #define V4L2_META_FMT_VSP1_HGO v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car VSP1 1-D Histogram */ > >> #define V4L2_META_FMT_VSP1_HGT v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */ > >> +#define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* UVC Payload Header metadata */ > > I discussed this with Laurent last week and since the metadata for UVC starts > with a standard header followed by vendor-specific data it makes sense to > use V4L2_META_FMT_UVC for just the standard header. Any vendor specific formats > should have their own fourcc which starts with the standard header followed by > the custom data. The UVC driver would enumerate both the standard and the vendor > specific fourcc. This would allow generic UVC applications to use the standard > header. Applications that know about the vendor specific data can select the > vendor specific format. > > This change would make this much more convenient to use. Then the driver should be able to decide, which private fourcc code to use for each of those devices. A natural way to do that seems to be to put that in the .driver_info field of struct usb_device_id. For that I'd replace the current use of that field for quirks with a pointer to a struct in a separate patch. Laurent, would that be acceptable? Then add a field to that struct for a private metadata fourcc code. Thanks Guennadi > Regards, > > Hans > > >> /* priv field value to indicates that subsequent fields are valid. */ > >> #define V4L2_PIX_FMT_PRIV_MAGIC 0xfeedcafe
Hi Guennadi, On Monday, 6 November 2017 16:53:10 EET Guennadi Liakhovetski wrote: > On Mon, 30 Oct 2017, Hans Verkuil wrote: > > On 07/28/2017 02:46 PM, Hans Verkuil wrote: > >> On 07/28/2017 02:33 PM, Guennadi Liakhovetski wrote: > >>> Add a pixel format, used by the UVC driver to stream metadata. > >>> > >>> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com> > >>> --- > >>> > >>> Documentation/media/uapi/v4l/meta-formats.rst | 1 + > >>> Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst | 39 +++++++++++++++++ > >>> include/uapi/linux/videodev2.h | 1 + > >>> 3 files changed, 41 insertions(+) > >>> create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst > > [snip] > > >>> diff --git a/include/uapi/linux/videodev2.h > >>> b/include/uapi/linux/videodev2.h index 45cf735..0aad91c 100644 > >>> --- a/include/uapi/linux/videodev2.h > >>> +++ b/include/uapi/linux/videodev2.h > >>> @@ -682,6 +682,7 @@ struct v4l2_pix_format { > >>> > >>> /* Meta-data formats */ > >>> #define V4L2_META_FMT_VSP1_HGO v4l2_fourcc('V', 'S', 'P', 'H') /* > >>> R-Car VSP1 1-D Histogram */ #define V4L2_META_FMT_VSP1_HGT > >>> v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */> >> > >>> +#define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* > >>> UVC Payload Header metadata */ > > > > I discussed this with Laurent last week and since the metadata for UVC > > starts with a standard header followed by vendor-specific data it makes > > sense to use V4L2_META_FMT_UVC for just the standard header. Any vendor > > specific formats should have their own fourcc which starts with the > > standard header followed by the custom data. The UVC driver would > > enumerate both the standard and the vendor specific fourcc. This would > > allow generic UVC applications to use the standard header. Applications > > that know about the vendor specific data can select the vendor specific > > format. > > > > This change would make this much more convenient to use. > > Then the driver should be able to decide, which private fourcc code to use > for each of those devices. A natural way to do that seems to be to put > that in the .driver_info field of struct usb_device_id. For that I'd > replace the current use of that field for quirks with a pointer to a > struct in a separate patch. Laurent, would that be acceptable? Then add a > field to that struct for a private metadata fourcc code. I've been thinking about doing so for some time now. If you can write a patch it would be great ! What I've been wondering is how to keep the code both readable and small. If we declared those structures separately from the devices array we could use one instance for multiple devices, but naming might become awkward. On the other hand, if we defined them inline within the devices array, we'd get rid of the naming issue, but at the expense of increased memory usage. One middle-ground option would be to allow storing either a structure pointer or quirks flags in the field, relying on the fact that the low order bit of a pointer will be NULL. We could repurpose flag BIT(0) to indicate that the field contains flags instead of a pointer. Maybe I'm over-engineering this and that the extra memory consumption won't be too bad, or separately defined structures will be easy to name. I'd appreciate your opinion on this matter.
Hi Laurent, Hans, On Tue, 7 Nov 2017, Laurent Pinchart wrote: > Hi Guennadi, > > On Monday, 6 November 2017 16:53:10 EET Guennadi Liakhovetski wrote: > > On Mon, 30 Oct 2017, Hans Verkuil wrote: > > > On 07/28/2017 02:46 PM, Hans Verkuil wrote: > > >> On 07/28/2017 02:33 PM, Guennadi Liakhovetski wrote: > > >>> Add a pixel format, used by the UVC driver to stream metadata. > > >>> > > >>> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com> > > >>> --- > > >>> > > >>> Documentation/media/uapi/v4l/meta-formats.rst | 1 + > > >>> Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst | 39 +++++++++++++++++ > > >>> include/uapi/linux/videodev2.h | 1 + > > >>> 3 files changed, 41 insertions(+) > > >>> create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst > > > > [snip] > > > > >>> diff --git a/include/uapi/linux/videodev2.h > > >>> b/include/uapi/linux/videodev2.h index 45cf735..0aad91c 100644 > > >>> --- a/include/uapi/linux/videodev2.h > > >>> +++ b/include/uapi/linux/videodev2.h > > >>> @@ -682,6 +682,7 @@ struct v4l2_pix_format { > > >>> > > >>> /* Meta-data formats */ > > >>> #define V4L2_META_FMT_VSP1_HGO v4l2_fourcc('V', 'S', 'P', 'H') /* > > >>> R-Car VSP1 1-D Histogram */ #define V4L2_META_FMT_VSP1_HGT > > >>> v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */> >> > > >>> +#define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* > > >>> UVC Payload Header metadata */ > > > > > > I discussed this with Laurent last week and since the metadata for UVC > > > starts with a standard header followed by vendor-specific data it makes > > > sense to use V4L2_META_FMT_UVC for just the standard header. Any vendor > > > specific formats should have their own fourcc which starts with the > > > standard header followed by the custom data. The UVC driver would > > > enumerate both the standard and the vendor specific fourcc. This would > > > allow generic UVC applications to use the standard header. Applications > > > that know about the vendor specific data can select the vendor specific > > > format. > > > > > > This change would make this much more convenient to use. > > > > Then the driver should be able to decide, which private fourcc code to use > > for each of those devices. A natural way to do that seems to be to put > > that in the .driver_info field of struct usb_device_id. For that I'd > > replace the current use of that field for quirks with a pointer to a > > struct in a separate patch. Laurent, would that be acceptable? Then add a > > field to that struct for a private metadata fourcc code. > > I've been thinking about doing so for some time now. If you can write a patch > it would be great ! What I've been wondering is how to keep the code both > readable and small. If we declared those structures separately from the > devices array we could use one instance for multiple devices, but naming might > become awkward. On the other hand, if we defined them inline within the > devices array, we'd get rid of the naming issue, but at the expense of > increased memory usage. We have in the meantime agreed to always use structs and to create named structs for reoccurring quirk flags and in-place structs for single-occurrance flags. While implementing this I came across yet one more compromise, that we'll have to accept with this approach: To recall the metadata buffer layout should be struct uvc_meta_buf { uint64_t ns; uint16_t sof; uint8_t length; uint8_t flags; uint8_t buf[]; } __attribute__((packed)); where all the fields, beginning with "length" are a direct copy from the UVC payload header. If multiple such payload headers have arrived for a single frame, they will be appended and .bytesused will as usually have the total byte count, used up in this frame. An application would then calculate lengths of those individual metadata blocks as sizeof(.ns) + sizeof(.sof) + .length But this won't work with the "standard" UVC metadata format where any private data, following standard fields, are dropped. In that case applications would have to look at .flags and calculate the block length based on them. Another possibility would be to rewrite the .length field in the driver to only include standard fields, but I really don't think that would be a good idea. Thanks Guennadi > One middle-ground option would be to allow storing either a structure pointer > or quirks flags in the field, relying on the fact that the low order bit of a > pointer will be NULL. We could repurpose flag BIT(0) to indicate that the > field contains flags instead of a pointer. > > Maybe I'm over-engineering this and that the extra memory consumption won't be > too bad, or separately defined structures will be easy to name. I'd appreciate > your opinion on this matter. > > -- > Regards, > > Laurent Pinchart >
Hi Guennadi, On Wednesday, 8 November 2017 12:43:46 EET Guennadi Liakhovetski wrote: > On Tue, 7 Nov 2017, Laurent Pinchart wrote: > > On Monday, 6 November 2017 16:53:10 EET Guennadi Liakhovetski wrote: > > > On Mon, 30 Oct 2017, Hans Verkuil wrote: > > > > On 07/28/2017 02:46 PM, Hans Verkuil wrote: > > > >> On 07/28/2017 02:33 PM, Guennadi Liakhovetski wrote: > > > >>> Add a pixel format, used by the UVC driver to stream metadata. > > > >>> > > > >>> Signed-off-by: Guennadi Liakhovetski > > > >>> <guennadi.liakhovetski@intel.com> > > > >>> --- > > > >>> > > > >>> Documentation/media/uapi/v4l/meta-formats.rst | 1 + > > > >>> Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst | 39 > > > >>> +++++++++++++++++ > > > >>> include/uapi/linux/videodev2.h | 1 + > > > >>> 3 files changed, 41 insertions(+) > > > >>> create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst > > > > > > [snip] > > > > > > >>> diff --git a/include/uapi/linux/videodev2.h > > > >>> b/include/uapi/linux/videodev2.h index 45cf735..0aad91c 100644 > > > >>> --- a/include/uapi/linux/videodev2.h > > > >>> +++ b/include/uapi/linux/videodev2.h > > > >>> @@ -682,6 +682,7 @@ struct v4l2_pix_format { > > > >>> > > > >>> /* Meta-data formats */ > > > >>> #define V4L2_META_FMT_VSP1_HGO v4l2_fourcc('V', 'S', 'P', 'H') > > > >>> /* > > > >>> R-Car VSP1 1-D Histogram */ #define V4L2_META_FMT_VSP1_HGT > > > >>> v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */> >> > > > >>> > > > >>> +#define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') > > > >>> /* > > > >>> UVC Payload Header metadata */ > > > > > > > > I discussed this with Laurent last week and since the metadata for UVC > > > > starts with a standard header followed by vendor-specific data it > > > > makes > > > > sense to use V4L2_META_FMT_UVC for just the standard header. Any > > > > vendor > > > > specific formats should have their own fourcc which starts with the > > > > standard header followed by the custom data. The UVC driver would > > > > enumerate both the standard and the vendor specific fourcc. This would > > > > allow generic UVC applications to use the standard header. > > > > Applications > > > > that know about the vendor specific data can select the vendor > > > > specific > > > > format. > > > > > > > > This change would make this much more convenient to use. > > > > > > Then the driver should be able to decide, which private fourcc code to > > > use > > > for each of those devices. A natural way to do that seems to be to put > > > that in the .driver_info field of struct usb_device_id. For that I'd > > > replace the current use of that field for quirks with a pointer to a > > > struct in a separate patch. Laurent, would that be acceptable? Then add > > > a > > > field to that struct for a private metadata fourcc code. > > > > I've been thinking about doing so for some time now. If you can write a > > patch it would be great ! What I've been wondering is how to keep the > > code both readable and small. If we declared those structures separately > > from the devices array we could use one instance for multiple devices, > > but naming might become awkward. On the other hand, if we defined them > > inline within the devices array, we'd get rid of the naming issue, but at > > the expense of increased memory usage. > > We have in the meantime agreed to always use structs and to create named > structs for reoccurring quirk flags and in-place structs for > single-occurrance flags. > > While implementing this I came across yet one more compromise, that we'll > have to accept with this approach: > > To recall the metadata buffer layout should be > > struct uvc_meta_buf { > uint64_t ns; > uint16_t sof; > uint8_t length; > uint8_t flags; > uint8_t buf[]; > } __attribute__((packed)); > > where all the fields, beginning with "length" are a direct copy from the > UVC payload header. If multiple such payload headers have arrived for a > single frame, they will be appended and .bytesused will as usually have > the total byte count, used up in this frame. An application would then > calculate lengths of those individual metadata blocks as > > sizeof(.ns) + sizeof(.sof) + .length > > But this won't work with the "standard" UVC metadata format where any > private data, following standard fields, are dropped. In that case > applications would have to look at .flags and calculate the block length > based on them. Another possibility would be to rewrite the .length field > in the driver to only include standard fields, but I really don't think > that would be a good idea. For the standard header the length can indeed be easily computed from the flags. I wonder, however, why you think rewriting length would be a bad idea ? > > One middle-ground option would be to allow storing either a structure > > pointer or quirks flags in the field, relying on the fact that the low > > order bit of a pointer will be NULL. We could repurpose flag BIT(0) to > > indicate that the field contains flags instead of a pointer. > > > > Maybe I'm over-engineering this and that the extra memory consumption > > won't be too bad, or separately defined structures will be easy to name. > > I'd appreciate your opinion on this matter.
Hi Laurent, On Thu, 9 Nov 2017, Laurent Pinchart wrote: > Hi Guennadi, > > On Wednesday, 8 November 2017 12:43:46 EET Guennadi Liakhovetski wrote: [snip] > > To recall the metadata buffer layout should be > > > > struct uvc_meta_buf { > > uint64_t ns; > > uint16_t sof; > > uint8_t length; > > uint8_t flags; > > uint8_t buf[]; > > } __attribute__((packed)); > > > > where all the fields, beginning with "length" are a direct copy from the > > UVC payload header. If multiple such payload headers have arrived for a > > single frame, they will be appended and .bytesused will as usually have > > the total byte count, used up in this frame. An application would then > > calculate lengths of those individual metadata blocks as > > > > sizeof(.ns) + sizeof(.sof) + .length > > > > But this won't work with the "standard" UVC metadata format where any > > private data, following standard fields, are dropped. In that case > > applications would have to look at .flags and calculate the block length > > based on them. Another possibility would be to rewrite the .length field > > in the driver to only include standard fields, but I really don't think > > that would be a good idea. > > For the standard header the length can indeed be easily computed from the > flags. I wonder, however, why you think rewriting length would be a bad idea ? Because I like the guarantee of the least intrusion. We (so far) guarantee, that the buffer contents beyond the system and the USB timestamps are a direct unmodified copy from the camera. This seems to be a good principle to stick to. Thanks Guennadi
diff --git a/Documentation/media/uapi/v4l/meta-formats.rst b/Documentation/media/uapi/v4l/meta-formats.rst index 01e24e3..1bb45a3f 100644 --- a/Documentation/media/uapi/v4l/meta-formats.rst +++ b/Documentation/media/uapi/v4l/meta-formats.rst @@ -14,3 +14,4 @@ These formats are used for the :ref:`metadata` interface only. pixfmt-meta-vsp1-hgo pixfmt-meta-vsp1-hgt + pixfmt-meta-uvc diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst b/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst new file mode 100644 index 0000000..58f78cb --- /dev/null +++ b/Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst @@ -0,0 +1,39 @@ +.. -*- coding: utf-8; mode: rst -*- + +.. _v4l2-meta-fmt-uvc: + +******************************* +V4L2_META_FMT_UVC ('UVCH') +******************************* + +UVC Payload Header Data + + +Description +=========== + +This format describes data, supplied by the UVC driver from metadata video +nodes. That data includes UVC Payload Header contents and auxiliary timing +information, required for precise interpretation of timestamps, contained in +those headers. Buffers, streamed via UVC metadata nodes, are composed of blocks +of variable length. Those blocks contain are described by struct uvc_meta_buf +and contain the following fields: + +.. flat-table:: UVC Metadata Block + :widths: 1 4 + :header-rows: 1 + :stub-columns: 0 + + * - Field + - Description + * - struct timespec ts; + - system timestamp, measured by the driver upon reception of the payload + * - __u16 sof; + - USB Frame Number, also obtained by the driver + * - :cspan:`1` *The rest is an exact copy of the payload header:* + * - __u8 length; + - length of the rest of the block, including this field + * - __u8 flags; + - Flags, indicating presence of other standard UVC fields + * - __u8 buf[]; + - The rest of the header, possibly including UVC PTS and SCR fields diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 45cf735..0aad91c 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -682,6 +682,7 @@ struct v4l2_pix_format { /* Meta-data formats */ #define V4L2_META_FMT_VSP1_HGO v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car VSP1 1-D Histogram */ #define V4L2_META_FMT_VSP1_HGT v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */ +#define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* UVC Payload Header metadata */ /* priv field value to indicates that subsequent fields are valid. */ #define V4L2_PIX_FMT_PRIV_MAGIC 0xfeedcafe
Add a pixel format, used by the UVC driver to stream metadata. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com> --- Documentation/media/uapi/v4l/meta-formats.rst | 1 + Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst | 39 ++++++++++++++++++++++++ include/uapi/linux/videodev2.h | 1 + 3 files changed, 41 insertions(+) create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst