[PATCH] uvcvideo: add a D4M camera description

[PATCH] uvcvideo: add a D4M camera description

[Guennadi Liakhovetski]

From: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>

D4M is a mobile model from the D4XX family of Intel RealSense cameras.
This patch adds a descriptor for it, which enables reading per-frame
metadata from it.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
---
 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 202 ++++++++++++++++++++++
 drivers/media/usb/uvc/uvc_driver.c                |  11 ++
 include/uapi/linux/videodev2.h                    |   1 +
 3 files changed, 214 insertions(+)
 create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst

diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
new file mode 100644
index 0000000..950780d
--- /dev/null
+++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
@@ -0,0 +1,202 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _v4l2-meta-fmt-d4xx:
+
+*******************************
+V4L2_META_FMT_D4XX ('D4XX')
+*******************************
+
+D4XX Metadata
+
+
+Description
+===========
+
+D4XX (D435 and other) cameras include per-frame metadata in their UVC payload
+headers, following the Microsoft(R) UVC extension proposal [1_]. That means,
+that the private D4XX metadata, following the standard UVC header, is organised
+in blocks. D4XX cameras implement several standard block types, proposed by
+Microsoft, and several proprietary ones. Supported standard metadata types
+include MetadataId_CaptureStats (ID 3), MetadataId_CameraExtrinsics (ID 4), and
+MetadataId_CameraIntrinsics (ID 5). For their description see [1_]. This
+document describes proprietary metadata types, used by DS4XX cameras.
+
+V4L2_META_FMT_D4XX buffers follow the metadata buffer layout of
+V4L2_META_FMT_UVC with the only difference, that it also includes proprietary
+payload header data. D4XX cameras use bulk transfers and only send one payload
+per frame, therefore their headers cannot be larger than 255 bytes.
+
+Below are proprietary Microsoft style metadata types, used by D4XX cameras,
+where all fields are in little endian order:
+
+.. flat-table:: D4XX metadata
+    :widths: 1 4
+    :header-rows:  1
+    :stub-columns: 0
+
+    * - Field
+      - Description
+    * - :cspan:`1` *Depth Control*
+    * - __u32 ID
+      - 0x80000000
+    * - __u32 Size
+      - Size in bytes (currently 56)
+    * - __u32 Version
+      - Version of the struct
+    * - __u32 Flags
+      - A bitmask of flags: see [2_] below
+    * - __u32 Gain
+      - Manual gain value
+    * - __u32 Exposure
+      - Manual exposure time in microseconds
+    * - __u32 Laser power
+      - Power of the laser LED 0-360, used for depth measurement
+    * - __u32 AE mode
+      - 0: manual; 1: automatic exposure
+    * - __u32 Exposure priority
+      - Exposure priority value: 0 - constant frameerate
+    * - __u32 AE ROI left
+      - Left border of the AE Region of Interest
+    * - __u32 AE ROI right
+      - Right border of the AE Region of Interest
+    * - __u32 AE ROI top
+      - Top border of the AE Region of Interest
+    * - __u32 AE ROI bottom
+      - Bottom border of the AE Region of Interest
+    * - __u32 Preset
+      - Preset selector value
+    * - __u32 Laser mode
+      - 0: off, 1: on
+    * - :cspan:`1` *Capture Timing*
+    * - __u32 ID
+      - 0x80000001
+    * - __u32 Size
+      - Size in bytes (currently 40)
+    * - __u32 Version
+      - Version of the struct
+    * - __u32 Flags
+      - A bitmask of flags: see [3_] below
+    * - __u32 Frame counter
+      - Monotonically increasing counter
+    * - __u32 Optical time
+      - Time in microseconds from the beginning of a frame till its middle
+    * - __u32 Readout time
+      - Time, used to read out a frame in microseconds
+    * - __u32 Exposure time
+      - Frame exposure time in microseconds
+    * - __u32 Frame interval
+      - In microseconds = 1000000 / framerate
+    * - __u32 Pipe latency
+      - Time in microseconds from start of frame to data in USB buffer
+    * - :cspan:`1` *Configuration*
+    * - __u32 ID
+      - 0x80000002
+    * - __u32 Size
+      - Size in bytes (currently 40)
+    * - __u32 Version
+      - Version of the struct
+    * - __u32 Flags
+      - A bitmask of flags: see [4_] below
+    * - __u8 Hardware type
+      - Camera hardware version [5_]
+    * - __u8 SKU ID
+      - Camera hardware configuration [6_]
+    * - __u32 Cookie
+      - Internal synchronisation
+    * - __u16 Format
+      - Image format code [7_]
+    * - __u16 Width
+      - Width in pixels
+    * - __u16 Height
+      - Height in pixels
+    * - __u16 Framerate
+      - Requested framerate
+    * - __u16 Trigger
+      - Byte 0: bit 0:  depth and RGB are synchronised, bit 1: external trigger
+
+.. _1:
+
+[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extensions-1-5
+
+.. _2:
+
+[2] Depth Control flags specify, which fields are valid: ::
+
+  0x00000001 Gain
+  0x00000002 Manual exposure
+  0x00000004 Laser power
+  0x00000008 AE mode
+  0x00000010 Exposure priority
+  0x00000020 AE ROI
+  0x00000040 Preset
+
+.. _3:
+
+[3] Capture Timing flags specify, which fields are valid: ::
+
+  0x00000001 Frame counter
+  0x00000002 Optical time
+  0x00000004 Readout time
+  0x00000008 Exposure time
+  0x00000010 Frame interval
+  0x00000020 Pipe latency
+
+.. _4:
+
+[4] Configuration flags specify, which fields are valid: ::
+
+  0x00000001 Hardware type
+  0x00000002 SKU ID
+  0x00000004 Cookie
+  0x00000008 Format
+  0x00000010 Width
+  0x00000020 Height
+  0x00000040 Framerate
+  0x00000080 Trigger
+  0x00000100 Cal count
+
+.. _5:
+
+[5] Camera model: ::
+
+  0 DS5
+  1 IVCAM2
+
+.. _6:
+
+[6] 8-bit camera hardware configuration bitfield: ::
+
+  [1:0] depthCamera
+	00: no depth
+	01: standard depth
+	10: wide depth
+	11: reserved
+  [2]   depthIsActive - has a laser projector
+  [3]   RGB presence
+  [4]   IMU presence
+  [5]   projectorType
+	0: HPTG
+	1: Princeton
+  [6]   0: a projector, 1: an LED
+  [7]   reserved
+
+.. _7:
+
+[7] Image format codes per camera interface:
+
+Depth: ::
+
+  1 Z16
+  2 Z
+
+Left sensor: ::
+
+  1 Y8
+  2 UYVY
+  3 R8L8
+  4 Calibration
+  5 W10
+
+Fish Eye sensor: ::
+
+  1 RAW8
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 36061f3..30dbbbf 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2346,6 +2346,8 @@ static int uvc_clock_param_set(const char *val, struct kernel_param *kp)
 };
 
 #define UVC_QUIRK_INFO(q) (kernel_ulong_t)&(struct uvc_device_info){.quirks = q}
+#define UVC_QUIRK_META(m) (kernel_ulong_t)&(struct uvc_device_info) \
+	{.meta_format = m}
 
 /*
  * The Logitech cameras listed below have their interface class set to
@@ -2810,6 +2812,15 @@ static int uvc_clock_param_set(const char *val, struct kernel_param *kp)
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_y8 },
+	/* Intel RealSense D4M */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x8086,
+	  .idProduct		= 0x0b03,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_QUIRK_META(V4L2_META_FMT_D4XX) },
 	/* Generic USB Video Class */
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 0d07b2d..7d3fbc6 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -688,6 +688,7 @@ struct v4l2_pix_format {
 #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 */
+#define V4L2_META_FMT_D4XX        v4l2_fourcc('D', '4', 'X', 'X') /* D4XX Payload Header metadata */
 
 /* priv field value to indicates that subsequent fields are valid. */
 #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe
-- 
1.9.3

Re: [PATCH] uvcvideo: add a D4M camera description

[Sakari Ailus]

Hi Guennadi,

Thanks for the patch!

On Sat, Dec 23, 2017 at 12:11:00PM +0100, Guennadi Liakhovetski wrote:
> From: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> 
> D4M is a mobile model from the D4XX family of Intel RealSense cameras.
> This patch adds a descriptor for it, which enables reading per-frame
> metadata from it.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> ---
>  Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 202 ++++++++++++++++++++++
>  drivers/media/usb/uvc/uvc_driver.c                |  11 ++
>  include/uapi/linux/videodev2.h                    |   1 +
>  3 files changed, 214 insertions(+)
>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> 
> diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> new file mode 100644
> index 0000000..950780d
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> @@ -0,0 +1,202 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _v4l2-meta-fmt-d4xx:
> +
> +*******************************
> +V4L2_META_FMT_D4XX ('D4XX')
> +*******************************
> +
> +D4XX Metadata
> +
> +
> +Description
> +===========
> +
> +D4XX (D435 and other) cameras include per-frame metadata in their UVC payload

If this is D435 and some others, I'd simply call this D435. Say, if you get
another device in D4xx series that implements a different format, how do
you call that? Up to you.

Is there a specific list of devices that use this format? The driver patch
only appears to introduce one USB ID.

> +headers, following the Microsoft(R) UVC extension proposal [1_]. That means,
> +that the private D4XX metadata, following the standard UVC header, is organised
> +in blocks. D4XX cameras implement several standard block types, proposed by
> +Microsoft, and several proprietary ones. Supported standard metadata types
> +include MetadataId_CaptureStats (ID 3), MetadataId_CameraExtrinsics (ID 4), and
> +MetadataId_CameraIntrinsics (ID 5). For their description see [1_]. This
> +document describes proprietary metadata types, used by DS4XX cameras.
> +
> +V4L2_META_FMT_D4XX buffers follow the metadata buffer layout of
> +V4L2_META_FMT_UVC with the only difference, that it also includes proprietary
> +payload header data. D4XX cameras use bulk transfers and only send one payload
> +per frame, therefore their headers cannot be larger than 255 bytes.
> +
> +Below are proprietary Microsoft style metadata types, used by D4XX cameras,
> +where all fields are in little endian order:
> +
> +.. flat-table:: D4XX metadata
> +    :widths: 1 4
> +    :header-rows:  1
> +    :stub-columns: 0
> +
> +    * - Field
> +      - Description
> +    * - :cspan:`1` *Depth Control*
> +    * - __u32 ID
> +      - 0x80000000
> +    * - __u32 Size
> +      - Size in bytes (currently 56)
> +    * - __u32 Version
> +      - Version of the struct
> +    * - __u32 Flags
> +      - A bitmask of flags: see [2_] below
> +    * - __u32 Gain
> +      - Manual gain value
> +    * - __u32 Exposure
> +      - Manual exposure time in microseconds
> +    * - __u32 Laser power
> +      - Power of the laser LED 0-360, used for depth measurement
> +    * - __u32 AE mode
> +      - 0: manual; 1: automatic exposure
> +    * - __u32 Exposure priority
> +      - Exposure priority value: 0 - constant frameerate
> +    * - __u32 AE ROI left
> +      - Left border of the AE Region of Interest
> +    * - __u32 AE ROI right
> +      - Right border of the AE Region of Interest
> +    * - __u32 AE ROI top
> +      - Top border of the AE Region of Interest
> +    * - __u32 AE ROI bottom
> +      - Bottom border of the AE Region of Interest
> +    * - __u32 Preset
> +      - Preset selector value
> +    * - __u32 Laser mode
> +      - 0: off, 1: on
> +    * - :cspan:`1` *Capture Timing*
> +    * - __u32 ID
> +      - 0x80000001
> +    * - __u32 Size
> +      - Size in bytes (currently 40)
> +    * - __u32 Version
> +      - Version of the struct
> +    * - __u32 Flags
> +      - A bitmask of flags: see [3_] below
> +    * - __u32 Frame counter
> +      - Monotonically increasing counter
> +    * - __u32 Optical time
> +      - Time in microseconds from the beginning of a frame till its middle
> +    * - __u32 Readout time
> +      - Time, used to read out a frame in microseconds
> +    * - __u32 Exposure time
> +      - Frame exposure time in microseconds
> +    * - __u32 Frame interval
> +      - In microseconds = 1000000 / framerate
> +    * - __u32 Pipe latency
> +      - Time in microseconds from start of frame to data in USB buffer
> +    * - :cspan:`1` *Configuration*
> +    * - __u32 ID
> +      - 0x80000002
> +    * - __u32 Size
> +      - Size in bytes (currently 40)
> +    * - __u32 Version
> +      - Version of the struct
> +    * - __u32 Flags
> +      - A bitmask of flags: see [4_] below
> +    * - __u8 Hardware type
> +      - Camera hardware version [5_]
> +    * - __u8 SKU ID
> +      - Camera hardware configuration [6_]
> +    * - __u32 Cookie
> +      - Internal synchronisation
> +    * - __u16 Format
> +      - Image format code [7_]
> +    * - __u16 Width
> +      - Width in pixels
> +    * - __u16 Height
> +      - Height in pixels
> +    * - __u16 Framerate
> +      - Requested framerate
> +    * - __u16 Trigger
> +      - Byte 0: bit 0:  depth and RGB are synchronised, bit 1: external trigger
> +
> +.. _1:
> +
> +[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extensions-1-5
> +
> +.. _2:
> +
> +[2] Depth Control flags specify, which fields are valid: ::
> +
> +  0x00000001 Gain
> +  0x00000002 Manual exposure
> +  0x00000004 Laser power
> +  0x00000008 AE mode
> +  0x00000010 Exposure priority
> +  0x00000020 AE ROI
> +  0x00000040 Preset
> +
> +.. _3:
> +
> +[3] Capture Timing flags specify, which fields are valid: ::
> +
> +  0x00000001 Frame counter
> +  0x00000002 Optical time
> +  0x00000004 Readout time
> +  0x00000008 Exposure time
> +  0x00000010 Frame interval
> +  0x00000020 Pipe latency
> +
> +.. _4:
> +
> +[4] Configuration flags specify, which fields are valid: ::
> +
> +  0x00000001 Hardware type
> +  0x00000002 SKU ID
> +  0x00000004 Cookie
> +  0x00000008 Format
> +  0x00000010 Width
> +  0x00000020 Height
> +  0x00000040 Framerate
> +  0x00000080 Trigger
> +  0x00000100 Cal count
> +
> +.. _5:
> +
> +[5] Camera model: ::
> +
> +  0 DS5
> +  1 IVCAM2
> +
> +.. _6:
> +
> +[6] 8-bit camera hardware configuration bitfield: ::
> +
> +  [1:0] depthCamera
> +	00: no depth
> +	01: standard depth
> +	10: wide depth
> +	11: reserved
> +  [2]   depthIsActive - has a laser projector
> +  [3]   RGB presence
> +  [4]   IMU presence
> +  [5]   projectorType
> +	0: HPTG
> +	1: Princeton
> +  [6]   0: a projector, 1: an LED
> +  [7]   reserved
> +
> +.. _7:
> +
> +[7] Image format codes per camera interface:
> +
> +Depth: ::
> +
> +  1 Z16
> +  2 Z
> +
> +Left sensor: ::
> +
> +  1 Y8
> +  2 UYVY
> +  3 R8L8
> +  4 Calibration
> +  5 W10
> +
> +Fish Eye sensor: ::
> +
> +  1 RAW8
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 36061f3..30dbbbf 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2346,6 +2346,8 @@ static int uvc_clock_param_set(const char *val, struct kernel_param *kp)
>  };
>  
>  #define UVC_QUIRK_INFO(q) (kernel_ulong_t)&(struct uvc_device_info){.quirks = q}
> +#define UVC_QUIRK_META(m) (kernel_ulong_t)&(struct uvc_device_info) \
> +	{.meta_format = m}
>  
>  /*
>   * The Logitech cameras listed below have their interface class set to
> @@ -2810,6 +2812,15 @@ static int uvc_clock_param_set(const char *val, struct kernel_param *kp)
>  	  .bInterfaceSubClass	= 1,
>  	  .bInterfaceProtocol	= 0,
>  	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_y8 },
> +	/* Intel RealSense D4M */
> +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> +				| USB_DEVICE_ID_MATCH_INT_INFO,
> +	  .idVendor		= 0x8086,
> +	  .idProduct		= 0x0b03,
> +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> +	  .bInterfaceSubClass	= 1,
> +	  .bInterfaceProtocol	= 0,
> +	  .driver_info		= UVC_QUIRK_META(V4L2_META_FMT_D4XX) },
>  	/* Generic USB Video Class */
>  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
>  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 0d07b2d..7d3fbc6 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -688,6 +688,7 @@ struct v4l2_pix_format {
>  #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 */
> +#define V4L2_META_FMT_D4XX        v4l2_fourcc('D', '4', 'X', 'X') /* D4XX Payload Header metadata */
>  
>  /* priv field value to indicates that subsequent fields are valid. */
>  #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH] uvcvideo: add a D4M camera description

[Guennadi Liakhovetski]

Hi Sakari,

On Wed, 27 Dec 2017, Sakari Ailus wrote:

> Hi Guennadi,
> 
> Thanks for the patch!
> 
> On Sat, Dec 23, 2017 at 12:11:00PM +0100, Guennadi Liakhovetski wrote:
> > From: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > 
> > D4M is a mobile model from the D4XX family of Intel RealSense cameras.
> > This patch adds a descriptor for it, which enables reading per-frame
> > metadata from it.
> > 
> > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > ---
> >  Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 202 ++++++++++++++++++++++
> >  drivers/media/usb/uvc/uvc_driver.c                |  11 ++
> >  include/uapi/linux/videodev2.h                    |   1 +
> >  3 files changed, 214 insertions(+)
> >  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > 
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > new file mode 100644
> > index 0000000..950780d
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > @@ -0,0 +1,202 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _v4l2-meta-fmt-d4xx:
> > +
> > +*******************************
> > +V4L2_META_FMT_D4XX ('D4XX')
> > +*******************************
> > +
> > +D4XX Metadata
> > +
> > +
> > +Description
> > +===========
> > +
> > +D4XX (D435 and other) cameras include per-frame metadata in their UVC payload
> 
> If this is D435 and some others, I'd simply call this D435. Say, if you get
> another device in D4xx series that implements a different format, how do
> you call that? Up to you.

As far as I understand, all D4XX cameras are using this format, but they 
might use different sets of metadata blocks, in which case any new such 
blocks should be added to this documentation. The format remains the same 
though, hence the name.

> Is there a specific list of devices that use this format? The driver patch
> only appears to introduce one USB ID.

I don't have such a list, sorry. I think any D4XX camera should work with 
this, so it would be a matter of adding those product ID values.

Thanks
Guennadi

Re: [PATCH] uvcvideo: add a D4M camera description

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

On Saturday, 23 December 2017 13:11:00 EEST Guennadi Liakhovetski wrote:
> From: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> 
> D4M is a mobile model from the D4XX family of Intel RealSense cameras.
> This patch adds a descriptor for it, which enables reading per-frame
> metadata from it.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> ---
>  Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 202 +++++++++++++++++++
>  drivers/media/usb/uvc/uvc_driver.c                |  11 ++
>  include/uapi/linux/videodev2.h                    |   1 +
>  3 files changed, 214 insertions(+)
>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> 
> diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst new file mode 100644
> index 0000000..950780d
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> @@ -0,0 +1,202 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _v4l2-meta-fmt-d4xx:
> +
> +*******************************
> +V4L2_META_FMT_D4XX ('D4XX')
> +*******************************
> +
> +D4XX Metadata

How about "Intel D4xx UVC Cameras Metadata" ?

> +
> +
> +Description
> +===========
> +
> +D4XX (D435 and other) cameras include per-frame metadata in their UVC
> payload

Should this be "Intel D4XX" ?

> +headers, following the Microsoft(R) UVC extension proposal [1_]. That
> means,
> +that the private D4XX metadata, following the standard UVC header, is
> organised
> +in blocks. D4XX cameras implement several standard block types, proposed by
> +Microsoft, and several proprietary ones. Supported standard metadata types
> +include MetadataId_CaptureStats (ID 3), MetadataId_CameraExtrinsics (ID 4),
> and
> +MetadataId_CameraIntrinsics (ID 5). For their description see [1_].

Does "including" mean that the list isn't exhaustive and that other standard 
types could be returned too ? If so, would it be possible to get an exhaustive 
list ? And if the list is exhaustive, could you word this paragraph to make 
that clear ?

> This
> +document describes proprietary metadata types, used by DS4XX cameras.

Is it D4XX or DS4XX ?

> +V4L2_META_FMT_D4XX buffers follow the metadata buffer layout of
> +V4L2_META_FMT_UVC with the only difference, that it also includes
> proprietary
> +payload header data. D4XX cameras use bulk transfers and only send one
> payload
> +per frame, therefore their headers cannot be larger than 255 bytes.
> +
> +Below are proprietary Microsoft style metadata types, used by D4XX cameras,
> +where all fields are in little endian order:
> +
> +.. flat-table:: D4XX metadata
> +    :widths: 1 4
> +    :header-rows:  1
> +    :stub-columns: 0
> +
> +    * - Field
> +      - Description
> +    * - :cspan:`1` *Depth Control*
> +    * - __u32 ID
> +      - 0x80000000
> +    * - __u32 Size
> +      - Size in bytes (currently 56)
> +    * - __u32 Version
> +      - Version of the struct

What is this field used for ?

> +    * - __u32 Flags
> +      - A bitmask of flags: see [2_] below
> +    * - __u32 Gain
> +      - Manual gain value

What is the gain unit ?

> +    * - __u32 Exposure
> +      - Manual exposure time in microseconds

When auto-exposure is enabled, does this reflect the actual exposure time used 
to capture the image ? If so I'd name the field just "exposure time", and 
expand the document to explain this. Maybe something like

"Exposure time (in microseconds) that was used to capture the frame."

It would also be useful to explain what happens when auto-exposure is 
disabled.

This comment applies to the gain as well.

> +    * - __u32 Laser power
> +      - Power of the laser LED 0-360, used for depth measurement
> +    * - __u32 AE mode
> +      - 0: manual; 1: automatic exposure
> +    * - __u32 Exposure priority
> +      - Exposure priority value: 0 - constant frameerate

s/frameerate/frame rate/

No other value than 0 is valid ?

> +    * - __u32 AE ROI left
> +      - Left border of the AE Region of Interest
> +    * - __u32 AE ROI right
> +      - Right border of the AE Region of Interest
> +    * - __u32 AE ROI top
> +      - Top border of the AE Region of Interest
> +    * - __u32 AE ROI bottom
> +      - Bottom border of the AE Region of Interest

What are the units and range for those fields ?

> +    * - __u32 Preset
> +      - Preset selector value

Could you elaborate a bit on what the preset selector value is ?

> +    * - __u32 Laser mode
> +      - 0: off, 1: on
> +    * - :cspan:`1` *Capture Timing*
> +    * - __u32 ID
> +      - 0x80000001
> +    * - __u32 Size
> +      - Size in bytes (currently 40)
> +    * - __u32 Version
> +      - Version of the struct
> +    * - __u32 Flags
> +      - A bitmask of flags: see [3_] below
> +    * - __u32 Frame counter
> +      - Monotonically increasing counter

That's interesting. Does it increase by exactly one for every frame ? I think 
it would be useful to document that.

> +    * - __u32 Optical time
> +      - Time in microseconds from the beginning of a frame till its middle

That's interesting too. Just for my information, is that exactly half the time 
between the beginning of a frame and its end, or can exposure vary through the 
frame ?

> +    * - __u32 Readout time
> +      - Time, used to read out a frame in microseconds
> +    * - __u32 Exposure time
> +      - Frame exposure time in microseconds

Is that the same as the above manual exposure time ? Or does the first one 
apply to the depth image only ? It would be useful to document that.

> +    * - __u32 Frame interval
> +      - In microseconds = 1000000 / framerate
> +    * - __u32 Pipe latency
> +      - Time in microseconds from start of frame to data in USB buffer
> +    * - :cspan:`1` *Configuration*
> +    * - __u32 ID
> +      - 0x80000002
> +    * - __u32 Size
> +      - Size in bytes (currently 40)
> +    * - __u32 Version
> +      - Version of the struct
> +    * - __u32 Flags
> +      - A bitmask of flags: see [4_] below
> +    * - __u8 Hardware type
> +      - Camera hardware version [5_]
> +    * - __u8 SKU ID
> +      - Camera hardware configuration [6_]
> +    * - __u32 Cookie
> +      - Internal synchronisation

Internal synchronisation with what ? :-)

> +    * - __u16 Format
> +      - Image format code [7_]
> +    * - __u16 Width
> +      - Width in pixels
> +    * - __u16 Height
> +      - Height in pixels
> +    * - __u16 Framerate
> +      - Requested framerate

What's the unit of this value ?

> +    * - __u16 Trigger
> +      - Byte 0: bit 0:  depth and RGB are synchronised, bit 1: external
> trigger
> +
> +.. _1:
> +
> +[1]
> https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extens
> ions-1-5

Should we at some point replicate that documentation in the V4L2 spec ? 
Without copying it of course, as that would be a copyright violation.

> +.. _2:
> +
> +[2] Depth Control flags specify, which fields are valid: ::

s/specify,/specify/ or s/ specify,/, specify/

Same comment for the other locations below.

> +
> +  0x00000001 Gain
> +  0x00000002 Manual exposure
> +  0x00000004 Laser power
> +  0x00000008 AE mode
> +  0x00000010 Exposure priority
> +  0x00000020 AE ROI
> +  0x00000040 Preset

What happens to the corresponding field when a bit isn't set, will it be zero 
?

> +.. _3:
> +
> +[3] Capture Timing flags specify, which fields are valid: ::
> +
> +  0x00000001 Frame counter
> +  0x00000002 Optical time
> +  0x00000004 Readout time
> +  0x00000008 Exposure time
> +  0x00000010 Frame interval
> +  0x00000020 Pipe latency
> +
> +.. _4:
> +
> +[4] Configuration flags specify, which fields are valid: ::
> +
> +  0x00000001 Hardware type
> +  0x00000002 SKU ID
> +  0x00000004 Cookie
> +  0x00000008 Format
> +  0x00000010 Width
> +  0x00000020 Height
> +  0x00000040 Framerate
> +  0x00000080 Trigger
> +  0x00000100 Cal count
> +
> +.. _5:
> +
> +[5] Camera model: ::
> +
> +  0 DS5
> +  1 IVCAM2
> +
> +.. _6:
> +
> +[6] 8-bit camera hardware configuration bitfield: ::
> +
> +  [1:0] depthCamera
> +	00: no depth
> +	01: standard depth
> +	10: wide depth
> +	11: reserved
> +  [2]   depthIsActive - has a laser projector
> +  [3]   RGB presence
> +  [4]   IMU presence

What does IMU mean ?

> +  [5]   projectorType
> +	0: HPTG
> +	1: Princeton

What does this mean ?

> +  [6]   0: a projector, 1: an LED

This would also benefit from a bit more explanation.

> +  [7]   reserved
> +
> +.. _7:
> +
> +[7] Image format codes per camera interface:
> +
> +Depth: ::
> +
> +  1 Z16
> +  2 Z
> +
> +Left sensor: ::
> +
> +  1 Y8
> +  2 UYVY
> +  3 R8L8
> +  4 Calibration
> +  5 W10
> +
> +Fish Eye sensor: ::
> +
> +  1 RAW8

There's a single field in the above structures that references this. When 
should it be interpreted as depth, left sensor or fish eye sensor format ?

> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 36061f3..30dbbbf 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2346,6 +2346,8 @@ static int uvc_clock_param_set(const char *val, struct
> kernel_param *kp) };
> 
>  #define UVC_QUIRK_INFO(q) (kernel_ulong_t)&(struct uvc_device_info){.quirks
> = q}
> +#define UVC_QUIRK_META(m) (kernel_ulong_t)&(struct uvc_device_info) \
> +	{.meta_format = m}

I'd name this macro UVC_INFO_META as it doesn't define a quirk. Should we also 
rename UVC_QUIRK_INFO to UVC_INFO_QUIRK ?

>  /*
>   * The Logitech cameras listed below have their interface class set to
> @@ -2810,6 +2812,15 @@ static int uvc_clock_param_set(const char *val,
> struct kernel_param *kp)
>  	  .bInterfaceSubClass	= 1,
>  	  .bInterfaceProtocol	= 0,
>  	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_y8 },
> +	/* Intel RealSense D4M */
> +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> +				| USB_DEVICE_ID_MATCH_INT_INFO,
> +	  .idVendor		= 0x8086,
> +	  .idProduct		= 0x0b03,
> +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> +	  .bInterfaceSubClass	= 1,
> +	  .bInterfaceProtocol	= 0,
> +	  .driver_info		= UVC_QUIRK_META(V4L2_META_FMT_D4XX) },
>  	/* Generic USB Video Class */
>  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
>  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 0d07b2d..7d3fbc6 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -688,6 +688,7 @@ struct v4l2_pix_format {
>  #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 */
> +#define V4L2_META_FMT_D4XX        v4l2_fourcc('D', '4', 'X', 'X') /* D4XX
> Payload Header metadata */
> 
>  /* priv field value to indicates that subsequent fields are valid. */
>  #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: add a D4M camera description

[Guennadi Liakhovetski]

Hi Laurent,

Thanks for the review. A general note: I think you're requesting a rather 
detailed information about many parameters. That isn't a problem by 
itself, however, it is difficult to obtain some of that information. I'll 
address whatever comments I can in an updated version, just answering some 
questions here. I directed youor questions, that I couldn't answer myself 
to respective people, but I have no idea if and when I get replies. So, 
it's up to you whether to wait for that additional information or to take 
at least what we have now.

On Sun, 29 Jul 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thank you for the patch.
> 
> On Saturday, 23 December 2017 13:11:00 EEST Guennadi Liakhovetski wrote:
> > From: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > 
> > D4M is a mobile model from the D4XX family of Intel RealSense cameras.
> > This patch adds a descriptor for it, which enables reading per-frame
> > metadata from it.
> > 
> > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > ---
> >  Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 202 +++++++++++++++++++
> >  drivers/media/usb/uvc/uvc_driver.c                |  11 ++
> >  include/uapi/linux/videodev2.h                    |   1 +
> >  3 files changed, 214 insertions(+)
> >  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > 
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst new file mode 100644
> > index 0000000..950780d
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > @@ -0,0 +1,202 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _v4l2-meta-fmt-d4xx:
> > +
> > +*******************************
> > +V4L2_META_FMT_D4XX ('D4XX')
> > +*******************************
> > +
> > +D4XX Metadata
> 
> How about "Intel D4xx UVC Cameras Metadata" ?
> 
> > +
> > +
> > +Description
> > +===========
> > +
> > +D4XX (D435 and other) cameras include per-frame metadata in their UVC
> > payload
> 
> Should this be "Intel D4XX" ?
> 
> > +headers, following the Microsoft(R) UVC extension proposal [1_]. That
> > means,
> > +that the private D4XX metadata, following the standard UVC header, is
> > organised
> > +in blocks. D4XX cameras implement several standard block types, proposed by
> > +Microsoft, and several proprietary ones. Supported standard metadata types
> > +include MetadataId_CaptureStats (ID 3), MetadataId_CameraExtrinsics (ID 4),
> > and
> > +MetadataId_CameraIntrinsics (ID 5). For their description see [1_].
> 
> Does "including" mean that the list isn't exhaustive and that other standard 
> types could be returned too ? If so, would it be possible to get an exhaustive 
> list ? And if the list is exhaustive, could you word this paragraph to make 
> that clear ?
> 
> > This
> > +document describes proprietary metadata types, used by DS4XX cameras.
> 
> Is it D4XX or DS4XX ?
> 
> > +V4L2_META_FMT_D4XX buffers follow the metadata buffer layout of
> > +V4L2_META_FMT_UVC with the only difference, that it also includes
> > proprietary
> > +payload header data. D4XX cameras use bulk transfers and only send one
> > payload
> > +per frame, therefore their headers cannot be larger than 255 bytes.
> > +
> > +Below are proprietary Microsoft style metadata types, used by D4XX cameras,
> > +where all fields are in little endian order:
> > +
> > +.. flat-table:: D4XX metadata
> > +    :widths: 1 4
> > +    :header-rows:  1
> > +    :stub-columns: 0
> > +
> > +    * - Field
> > +      - Description
> > +    * - :cspan:`1` *Depth Control*
> > +    * - __u32 ID
> > +      - 0x80000000
> > +    * - __u32 Size
> > +      - Size in bytes (currently 56)
> > +    * - __u32 Version
> > +      - Version of the struct
> 
> What is this field used for ?

For future changes to this (and all other) struct(s). If in the future a 
new field is added to this struct, the version will be incremented to 
inform the user.

> > +    * - __u32 Flags
> > +      - A bitmask of flags: see [2_] below
> > +    * - __u32 Gain
> > +      - Manual gain value
> 
> What is the gain unit ?

It's in internal units. I guess librealsense has formulas to convert them 
to ISO or something else standard. It's the same units as the 
V4L2_CID_GAIN control.

> > +    * - __u32 Exposure
> > +      - Manual exposure time in microseconds
> 
> When auto-exposure is enabled, does this reflect the actual exposure time used 
> to capture the image ? If so I'd name the field just "exposure time", and 
> expand the document to explain this. Maybe something like
> 
> "Exposure time (in microseconds) that was used to capture the frame."
> 
> It would also be useful to explain what happens when auto-exposure is 
> disabled.
> 
> This comment applies to the gain as well.
> 
> > +    * - __u32 Laser power
> > +      - Power of the laser LED 0-360, used for depth measurement
> > +    * - __u32 AE mode
> > +      - 0: manual; 1: automatic exposure
> > +    * - __u32 Exposure priority
> > +      - Exposure priority value: 0 - constant frameerate
> 
> s/frameerate/frame rate/
> 
> No other value than 0 is valid ?

So far - no

> 
> > +    * - __u32 AE ROI left
> > +      - Left border of the AE Region of Interest
> > +    * - __u32 AE ROI right
> > +      - Right border of the AE Region of Interest
> > +    * - __u32 AE ROI top
> > +      - Top border of the AE Region of Interest
> > +    * - __u32 AE ROI bottom
> > +      - Bottom border of the AE Region of Interest
> 
> What are the units and range for those fields ?

Pixels, between 0 and max width or height.

> > +    * - __u32 Preset
> > +      - Preset selector value
> 
> Could you elaborate a bit on what the preset selector value is ?

Some cameras can have certain fixed configurations. In those cases it is 
possible to select one of them, using an XU control, which then will be 
reflected here.

> > +    * - __u32 Laser mode
> > +      - 0: off, 1: on
> > +    * - :cspan:`1` *Capture Timing*
> > +    * - __u32 ID
> > +      - 0x80000001
> > +    * - __u32 Size
> > +      - Size in bytes (currently 40)
> > +    * - __u32 Version
> > +      - Version of the struct
> > +    * - __u32 Flags
> > +      - A bitmask of flags: see [3_] below
> > +    * - __u32 Frame counter
> > +      - Monotonically increasing counter
> 
> That's interesting. Does it increase by exactly one for every frame ? I think 
> it would be useful to document that.
> 
> > +    * - __u32 Optical time
> > +      - Time in microseconds from the beginning of a frame till its middle
> 
> That's interesting too. Just for my information, is that exactly half the time 
> between the beginning of a frame and its end, or can exposure vary through the 
> frame ?
> 
> > +    * - __u32 Readout time
> > +      - Time, used to read out a frame in microseconds
> > +    * - __u32 Exposure time
> > +      - Frame exposure time in microseconds
> 
> Is that the same as the above manual exposure time ? Or does the first one 
> apply to the depth image only ? It would be useful to document that.
> 
> > +    * - __u32 Frame interval
> > +      - In microseconds = 1000000 / framerate
> > +    * - __u32 Pipe latency
> > +      - Time in microseconds from start of frame to data in USB buffer
> > +    * - :cspan:`1` *Configuration*
> > +    * - __u32 ID
> > +      - 0x80000002
> > +    * - __u32 Size
> > +      - Size in bytes (currently 40)
> > +    * - __u32 Version
> > +      - Version of the struct
> > +    * - __u32 Flags
> > +      - A bitmask of flags: see [4_] below
> > +    * - __u8 Hardware type
> > +      - Camera hardware version [5_]
> > +    * - __u8 SKU ID
> > +      - Camera hardware configuration [6_]
> > +    * - __u32 Cookie
> > +      - Internal synchronisation
> 
> Internal synchronisation with what ? :-)
> 
> > +    * - __u16 Format
> > +      - Image format code [7_]
> > +    * - __u16 Width
> > +      - Width in pixels
> > +    * - __u16 Height
> > +      - Height in pixels
> > +    * - __u16 Framerate
> > +      - Requested framerate
> 
> What's the unit of this value ?

Is anything other than frames per second used in V4L?

> > +    * - __u16 Trigger
> > +      - Byte 0: bit 0:  depth and RGB are synchronised, bit 1: external
> > trigger
> > +
> > +.. _1:
> > +
> > +[1]
> > https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extens
> > ions-1-5
> 
> Should we at some point replicate that documentation in the V4L2 spec ? 
> Without copying it of course, as that would be a copyright violation.

Well, we don't replicate the UVC itself or any other standards, do we? Of 
course, that document doesn't have the same status as an official 
vendor-neutral standard, but still, we don't replicate data sheets either. 
Besides, I think there are cameras that use this, and windows supports 
this, so, don't think it will disappear overnight...

> > +.. _2:
> > +
> > +[2] Depth Control flags specify, which fields are valid: ::
> 
> s/specify,/specify/ or s/ specify,/, specify/
> 
> Same comment for the other locations below.
> 
> > +
> > +  0x00000001 Gain
> > +  0x00000002 Manual exposure
> > +  0x00000004 Laser power
> > +  0x00000008 AE mode
> > +  0x00000010 Exposure priority
> > +  0x00000020 AE ROI
> > +  0x00000040 Preset
> 
> What happens to the corresponding field when a bit isn't set, will it be zero 
> ?

It will be invalid, so, I wouldn't rely on it being any specific value 
like 0 or anything else.

> > +.. _3:
> > +
> > +[3] Capture Timing flags specify, which fields are valid: ::
> > +
> > +  0x00000001 Frame counter
> > +  0x00000002 Optical time
> > +  0x00000004 Readout time
> > +  0x00000008 Exposure time
> > +  0x00000010 Frame interval
> > +  0x00000020 Pipe latency
> > +
> > +.. _4:
> > +
> > +[4] Configuration flags specify, which fields are valid: ::
> > +
> > +  0x00000001 Hardware type
> > +  0x00000002 SKU ID
> > +  0x00000004 Cookie
> > +  0x00000008 Format
> > +  0x00000010 Width
> > +  0x00000020 Height
> > +  0x00000040 Framerate
> > +  0x00000080 Trigger
> > +  0x00000100 Cal count
> > +
> > +.. _5:
> > +
> > +[5] Camera model: ::
> > +
> > +  0 DS5
> > +  1 IVCAM2
> > +
> > +.. _6:
> > +
> > +[6] 8-bit camera hardware configuration bitfield: ::
> > +
> > +  [1:0] depthCamera
> > +	00: no depth
> > +	01: standard depth
> > +	10: wide depth
> > +	11: reserved
> > +  [2]   depthIsActive - has a laser projector
> > +  [3]   RGB presence
> > +  [4]   IMU presence
> 
> What does IMU mean ?

https://en.wikipedia.org/wiki/Inertial_measurement_unit - it's a common 
abbreviation.

> 
> > +  [5]   projectorType
> > +	0: HPTG
> > +	1: Princeton
> 
> What does this mean ?
> 
> > +  [6]   0: a projector, 1: an LED
> 
> This would also benefit from a bit more explanation.
> 
> > +  [7]   reserved
> > +
> > +.. _7:
> > +
> > +[7] Image format codes per camera interface:
> > +
> > +Depth: ::
> > +
> > +  1 Z16
> > +  2 Z
> > +
> > +Left sensor: ::
> > +
> > +  1 Y8
> > +  2 UYVY
> > +  3 R8L8
> > +  4 Calibration
> > +  5 W10
> > +
> > +Fish Eye sensor: ::
> > +
> > +  1 RAW8
> 
> There's a single field in the above structures that references this. When 
> should it be interpreted as depth, left sensor or fish eye sensor format ?

you get a metadata node per video streaming interface. So, whatever you're 
streaming on that video node, the respective metadata is available on the 
respective metadatanode.

> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index 36061f3..30dbbbf 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2346,6 +2346,8 @@ static int uvc_clock_param_set(const char *val, struct
> > kernel_param *kp) };
> > 
> >  #define UVC_QUIRK_INFO(q) (kernel_ulong_t)&(struct uvc_device_info){.quirks
> > = q}
> > +#define UVC_QUIRK_META(m) (kernel_ulong_t)&(struct uvc_device_info) \
> > +	{.meta_format = m}
> 
> I'd name this macro UVC_INFO_META as it doesn't define a quirk. Should we also 
> rename UVC_QUIRK_INFO to UVC_INFO_QUIRK ?
> 
> >  /*
> >   * The Logitech cameras listed below have their interface class set to
> > @@ -2810,6 +2812,15 @@ static int uvc_clock_param_set(const char *val,
> > struct kernel_param *kp)
> >  	  .bInterfaceSubClass	= 1,
> >  	  .bInterfaceProtocol	= 0,
> >  	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_y8 },
> > +	/* Intel RealSense D4M */
> > +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> > +				| USB_DEVICE_ID_MATCH_INT_INFO,
> > +	  .idVendor		= 0x8086,
> > +	  .idProduct		= 0x0b03,
> > +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> > +	  .bInterfaceSubClass	= 1,
> > +	  .bInterfaceProtocol	= 0,
> > +	  .driver_info		= UVC_QUIRK_META(V4L2_META_FMT_D4XX) },
> >  	/* Generic USB Video Class */
> >  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
> >  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 0d07b2d..7d3fbc6 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -688,6 +688,7 @@ struct v4l2_pix_format {
> >  #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 */
> > +#define V4L2_META_FMT_D4XX        v4l2_fourcc('D', '4', 'X', 'X') /* D4XX
> > Payload Header metadata */
> > 
> >  /* priv field value to indicates that subsequent fields are valid. */
> >  #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe

Thanks
Guennadi

> -- 
> Regards,
> 
> Laurent Pinchart

[PATCH v2 2/2] uvcvideo: add a D4M camera description

[Guennadi Liakhovetski]

D4M is a mobile model from the D4XX family of Intel RealSense cameras.
This patch adds a descriptor for it, which enables reading per-frame
metadata from it.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
---
 Documentation/media/uapi/v4l/meta-formats.rst     |   1 +
 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 204 ++++++++++++++++++++++
 drivers/media/usb/uvc/uvc_driver.c                |  11 ++
 include/uapi/linux/videodev2.h                    |   1 +
 4 files changed, 217 insertions(+)
 create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst

diff --git a/Documentation/media/uapi/v4l/meta-formats.rst b/Documentation/media/uapi/v4l/meta-formats.rst
index 0c4e1ec..cf971d5 100644
--- a/Documentation/media/uapi/v4l/meta-formats.rst
+++ b/Documentation/media/uapi/v4l/meta-formats.rst
@@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata` interface only.
 .. toctree::
     :maxdepth: 1
 
+    pixfmt-meta-d4xx
     pixfmt-meta-uvc
     pixfmt-meta-vsp1-hgo
     pixfmt-meta-vsp1-hgt
diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
new file mode 100644
index 0000000..57ecfd9
--- /dev/null
+++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
@@ -0,0 +1,204 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _v4l2-meta-fmt-d4xx:
+
+*******************************
+V4L2_META_FMT_D4XX ('D4XX')
+*******************************
+
+Intel D4xx UVC Cameras Metadata
+
+
+Description
+===========
+
+Intel D4xx (D435 and other) cameras include per-frame metadata in their UVC
+payload headers, following the Microsoft(R) UVC extension proposal [1_]. That
+means, that the private D4XX metadata, following the standard UVC header, is
+organised in blocks. D4XX cameras implement several standard block types,
+proposed by Microsoft, and several proprietary ones. Supported standard metadata
+types are MetadataId_CaptureStats (ID 3), MetadataId_CameraExtrinsics (ID 4),
+and MetadataId_CameraIntrinsics (ID 5). For their description see [1_]. This
+document describes proprietary metadata types, used by D4xx cameras.
+
+V4L2_META_FMT_D4XX buffers follow the metadata buffer layout of
+V4L2_META_FMT_UVC with the only difference, that it also includes proprietary
+payload header data. D4xx cameras use bulk transfers and only send one payload
+per frame, therefore their headers cannot be larger than 255 bytes.
+
+Below are proprietary Microsoft style metadata types, used by D4xx cameras,
+where all fields are in little endian order:
+
+.. flat-table:: D4xx metadata
+    :widths: 1 4
+    :header-rows:  1
+    :stub-columns: 0
+
+    * - Field
+      - Description
+    * - :cspan:`1` *Depth Control*
+    * - __u32 ID
+      - 0x80000000
+    * - __u32 Size
+      - Size in bytes (currently 56)
+    * - __u32 Version
+      - Version of the struct
+    * - __u32 Flags
+      - A bitmask of flags: see [2_] below
+    * - __u32 Gain
+      - Gain value in internal units, same as the V4L2_CID_GAIN control, used to
+	capture the frame
+    * - __u32 Exposure
+      - Exposure time (in microseconds) used to capture the frame
+    * - __u32 Laser power
+      - Power of the laser LED 0-360, used for depth measurement
+    * - __u32 AE mode
+      - 0: manual; 1: automatic exposure
+    * - __u32 Exposure priority
+      - Exposure priority value: 0 - constant frame rate
+    * - __u32 AE ROI left
+      - Left border of the AE Region of Interest (all ROI values are in pixels
+	and lie between 0 and maximum width or height respectively)
+    * - __u32 AE ROI right
+      - Right border of the AE Region of Interest
+    * - __u32 AE ROI top
+      - Top border of the AE Region of Interest
+    * - __u32 AE ROI bottom
+      - Bottom border of the AE Region of Interest
+    * - __u32 Preset
+      - Preset selector value, default: 0, unless changed by the user
+    * - __u32 Laser mode
+      - 0: off, 1: on
+    * - :cspan:`1` *Capture Timing*
+    * - __u32 ID
+      - 0x80000001
+    * - __u32 Size
+      - Size in bytes (currently 40)
+    * - __u32 Version
+      - Version of the struct
+    * - __u32 Flags
+      - A bitmask of flags: see [3_] below
+    * - __u32 Frame counter
+      - Monotonically increasing counter
+    * - __u32 Optical time
+      - Time in microseconds from the beginning of a frame till its middle
+    * - __u32 Readout time
+      - Time, used to read out a frame in microseconds
+    * - __u32 Exposure time
+      - Frame exposure time in microseconds
+    * - __u32 Frame interval
+      - In microseconds = 1000000 / framerate
+    * - __u32 Pipe latency
+      - Time in microseconds from start of frame to data in USB buffer
+    * - :cspan:`1` *Configuration*
+    * - __u32 ID
+      - 0x80000002
+    * - __u32 Size
+      - Size in bytes (currently 40)
+    * - __u32 Version
+      - Version of the struct
+    * - __u32 Flags
+      - A bitmask of flags: see [4_] below
+    * - __u8 Hardware type
+      - Camera hardware version [5_]
+    * - __u8 SKU ID
+      - Camera hardware configuration [6_]
+    * - __u32 Cookie
+      - Internal synchronisation
+    * - __u16 Format
+      - Image format code [7_]
+    * - __u16 Width
+      - Width in pixels
+    * - __u16 Height
+      - Height in pixels
+    * - __u16 Framerate
+      - Requested frame rate per second
+    * - __u16 Trigger
+      - Byte 0: bit 0: depth and RGB are synchronised, bit 1: external trigger
+
+.. _1:
+
+[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extensions-1-5
+
+.. _2:
+
+[2] Depth Control flags specify which fields are valid: ::
+
+  0x00000001 Gain
+  0x00000002 Exposure
+  0x00000004 Laser power
+  0x00000008 AE mode
+  0x00000010 Exposure priority
+  0x00000020 AE ROI
+  0x00000040 Preset
+
+.. _3:
+
+[3] Capture Timing flags specify which fields are valid: ::
+
+  0x00000001 Frame counter
+  0x00000002 Optical time
+  0x00000004 Readout time
+  0x00000008 Exposure time
+  0x00000010 Frame interval
+  0x00000020 Pipe latency
+
+.. _4:
+
+[4] Configuration flags specify which fields are valid: ::
+
+  0x00000001 Hardware type
+  0x00000002 SKU ID
+  0x00000004 Cookie
+  0x00000008 Format
+  0x00000010 Width
+  0x00000020 Height
+  0x00000040 Framerate
+  0x00000080 Trigger
+  0x00000100 Cal count
+
+.. _5:
+
+[5] Camera model: ::
+
+  0 DS5
+  1 IVCAM2
+
+.. _6:
+
+[6] 8-bit camera hardware configuration bitfield: ::
+
+  [1:0] depthCamera
+	00: no depth
+	01: standard depth
+	10: wide depth
+	11: reserved
+  [2]   depthIsActive - has a laser projector
+  [3]   RGB presence
+  [4]   IMU presence
+  [5]   projectorType
+	0: HPTG
+	1: Princeton
+  [6]   0: a projector, 1: an LED
+  [7]   reserved
+
+.. _7:
+
+[7] Image format codes per camera interface:
+
+Depth: ::
+
+  1 Z16
+  2 Z
+
+Left sensor: ::
+
+  1 Y8
+  2 UYVY
+  3 R8L8
+  4 Calibration
+  5 W10
+
+Fish Eye sensor: ::
+
+  1 RAW8
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 699984b..1dcd250 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2345,6 +2345,8 @@ static int uvc_clock_param_set(const char *val, const struct kernel_param *kp)
 };
 
 #define UVC_INFO_QUIRK(q) (kernel_ulong_t)&(struct uvc_device_info){.quirks = q}
+#define UVC_INFO_META(m) (kernel_ulong_t)&(struct uvc_device_info) \
+	{.meta_format = m}
 
 /*
  * The Logitech cameras listed below have their interface class set to
@@ -2818,6 +2820,15 @@ static int uvc_clock_param_set(const char *val, const struct kernel_param *kp)
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_y8 },
+	/* Intel RealSense D4M */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x8086,
+	  .idProduct		= 0x0b03,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_INFO_META(V4L2_META_FMT_D4XX) },
 	/* Generic USB Video Class */
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index d8b3309..40aff26 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -697,6 +697,7 @@ struct v4l2_pix_format {
 #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 */
+#define V4L2_META_FMT_D4XX        v4l2_fourcc('D', '4', 'X', 'X') /* D4XX Payload Header metadata */
 
 /* priv field value to indicates that subsequent fields are valid. */
 #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe
-- 
1.9.3

Re: [PATCH v2 2/2] uvcvideo: add a D4M camera description

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

Overall this looks good to me, I only have small comments. Please see below, 
with a summary at the end.

On Friday, 3 August 2018 14:37:08 EEST Guennadi Liakhovetski wrote:
> D4M is a mobile model from the D4XX family of Intel RealSense cameras.
> This patch adds a descriptor for it, which enables reading per-frame
> metadata from it.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> ---
>  Documentation/media/uapi/v4l/meta-formats.rst     |   1 +
>  Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 204 +++++++++++++++++++
>  drivers/media/usb/uvc/uvc_driver.c                |  11 ++
>  include/uapi/linux/videodev2.h                    |   1 +
>  4 files changed, 217 insertions(+)
>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst

[snip]

> diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst new file mode 100644
> index 0000000..57ecfd9
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> @@ -0,0 +1,204 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _v4l2-meta-fmt-d4xx:
> +
> +*******************************
> +V4L2_META_FMT_D4XX ('D4XX')
> +*******************************
> +
> +Intel D4xx UVC Cameras Metadata
> +
> +
> +Description
> +===========
> +
> +Intel D4xx (D435 and other) cameras include per-frame metadata in their UVC
> +payload headers, following the Microsoft(R) UVC extension proposal [1_].
> That +means, that the private D4XX metadata, following the standard UVC
> header, is +organised in blocks. D4XX cameras implement several standard
> block types, +proposed by Microsoft, and several proprietary ones.
> Supported standard metadata +types are MetadataId_CaptureStats (ID 3),
> MetadataId_CameraExtrinsics (ID 4), +and MetadataId_CameraIntrinsics (ID
> 5). For their description see [1_]. This +document describes proprietary
> metadata types, used by D4xx cameras.
> +
> +V4L2_META_FMT_D4XX buffers follow the metadata buffer layout of
> +V4L2_META_FMT_UVC with the only difference, that it also includes
> proprietary +payload header data. D4xx cameras use bulk transfers and only
> send one payload +per frame, therefore their headers cannot be larger than
> 255 bytes.
> +
> +Below are proprietary Microsoft style metadata types, used by D4xx cameras,
> +where all fields are in little endian order:
> +
> +.. flat-table:: D4xx metadata
> +    :widths: 1 4
> +    :header-rows:  1
> +    :stub-columns: 0
> +
> +    * - Field
> +      - Description
> +    * - :cspan:`1` *Depth Control*

Does this mean that all fields in this structure apply to the depth image only 
? If so, do you mind if I mention that explicitly ?

> +    * - __u32 ID
> +      - 0x80000000
> +    * - __u32 Size
> +      - Size in bytes (currently 56)
> +    * - __u32 Version
> +      - Version of the struct

I would elaborate a bit here, how about the following ?

"Version of this structure. The documentation herein corresponds to version 
xxx. The version number will be incremented when new fields are added."

If you can provide me with the current version number I can update this when 
applying the patch (I would get it myself, but I'm about to board a plane and 
haven't taken the camera with me :-/).

> +    * - __u32 Flags
> +      - A bitmask of flags: see [2_] below
> +    * - __u32 Gain
> +      - Gain value in internal units, same as the V4L2_CID_GAIN control,
> used to
> +        capture the frame
> +    * - __u32 Exposure
> +      - Exposure time (in microseconds) used to capture the frame
> +    * - __u32 Laser power
> +      - Power of the laser LED 0-360, used for depth measurement
> +    * - __u32 AE mode
> +      - 0: manual; 1: automatic exposure
> +    * - __u32 Exposure priority
> +      - Exposure priority value: 0 - constant frame rate
> +    * - __u32 AE ROI left
> +      - Left border of the AE Region of Interest (all ROI values are in
> pixels
> +        and lie between 0 and maximum width or height respectively)
> +    * - __u32 AE ROI right
> +      - Right border of the AE Region of Interest
> +    * - __u32 AE ROI top
> +      - Top border of the AE Region of Interest
> +    * - __u32 AE ROI bottom
> +      - Bottom border of the AE Region of Interest
> +    * - __u32 Preset
> +      - Preset selector value, default: 0, unless changed by the user

I won't block this patch for this, but could we get the documentation of the 
corresponding XU control ? Even better, if Intel could publish documentation 
of the full XUs, that would be great :-)

> +    * - __u32 Laser mode
> +      - 0: off, 1: on
> +    * - :cspan:`1` *Capture Timing*

Similarly, what does this apply to ? The left sensor, the fish eye camera, or 
both (or something else) ?

> +    * - __u32 ID
> +      - 0x80000001
> +    * - __u32 Size
> +      - Size in bytes (currently 40)
> +    * - __u32 Version
> +      - Version of the struct
> +    * - __u32 Flags
> +      - A bitmask of flags: see [3_] below
> +    * - __u32 Frame counter
> +      - Monotonically increasing counter

I assume this increases by exactly one for every frame. I'll test it when I 
get back home, and will update the documentation accordingly.

> +    * - __u32 Optical time
> +      - Time in microseconds from the beginning of a frame till its middle

I'm still puzzled by this value, as I expect it to be exactly half the 
exposure time, which is reported separately. If that's not the case I'd like 
to know what this represents.

> +    * - __u32 Readout time
> +      - Time, used to read out a frame in microseconds
> +    * - __u32 Exposure time
> +      - Frame exposure time in microseconds
> +    * - __u32 Frame interval
> +      - In microseconds = 1000000 / framerate
> +    * - __u32 Pipe latency
> +      - Time in microseconds from start of frame to data in USB buffer
> +    * - :cspan:`1` *Configuration*
> +    * - __u32 ID
> +      - 0x80000002
> +    * - __u32 Size
> +      - Size in bytes (currently 40)
> +    * - __u32 Version
> +      - Version of the struct
> +    * - __u32 Flags
> +      - A bitmask of flags: see [4_] below
> +    * - __u8 Hardware type
> +      - Camera hardware version [5_]
> +    * - __u8 SKU ID
> +      - Camera hardware configuration [6_]
> +    * - __u32 Cookie
> +      - Internal synchronisation
> +    * - __u16 Format
> +      - Image format code [7_]
> +    * - __u16 Width
> +      - Width in pixels
> +    * - __u16 Height
> +      - Height in pixels
> +    * - __u16 Framerate
> +      - Requested frame rate per second
> +    * - __u16 Trigger
> +      - Byte 0: bit 0: depth and RGB are synchronised, bit 1: external
> trigger
> +
> +.. _1:
> +
> +[1]
> https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extens
> ions-1-5
> +
> +.. _2:
> +
> +[2] Depth Control flags specify which fields are valid: ::
> +
> +  0x00000001 Gain
> +  0x00000002 Exposure
> +  0x00000004 Laser power
> +  0x00000008 AE mode
> +  0x00000010 Exposure priority
> +  0x00000020 AE ROI
> +  0x00000040 Preset
> +
> +.. _3:
> +
> +[3] Capture Timing flags specify which fields are valid: ::
> +
> +  0x00000001 Frame counter
> +  0x00000002 Optical time
> +  0x00000004 Readout time
> +  0x00000008 Exposure time
> +  0x00000010 Frame interval
> +  0x00000020 Pipe latency
> +
> +.. _4:
> +
> +[4] Configuration flags specify which fields are valid: ::
> +
> +  0x00000001 Hardware type
> +  0x00000002 SKU ID
> +  0x00000004 Cookie
> +  0x00000008 Format
> +  0x00000010 Width
> +  0x00000020 Height
> +  0x00000040 Framerate
> +  0x00000080 Trigger
> +  0x00000100 Cal count
> +
> +.. _5:
> +
> +[5] Camera model: ::
> +
> +  0 DS5
> +  1 IVCAM2
> +
> +.. _6:
> +
> +[6] 8-bit camera hardware configuration bitfield: ::
> +
> +  [1:0] depthCamera
> +	00: no depth
> +	01: standard depth
> +	10: wide depth
> +	11: reserved
> +  [2]   depthIsActive - has a laser projector
> +  [3]   RGB presence
> +  [4]   IMU presence

Do you mind if I write this "Inertial Measurement Unit (IMU) presense" ? Or is 
it just me who's not familiar enough with depth cameras ? :-)

On a side note, does the camera expose IMU data to the host over USB ?

> +  [5]   projectorType
> +	0: HPTG
> +	1: Princeton
> +  [6]   0: a projector, 1: an LED
> +  [7]   reserved
> +
> +.. _7:
> +
> +[7] Image format codes per camera interface:

I was initially puzzled by this until I read your explanation. How about 
wording it as "Image format codes per video streaming interface" to use the 
UVC vocabulary ?

> +
> +Depth: ::
> +
> +  1 Z16
> +  2 Z
> +
> +Left sensor: ::
> +
> +  1 Y8
> +  2 UYVY
> +  3 R8L8
> +  4 Calibration
> +  5 W10
> +
> +Fish Eye sensor: ::
> +
> +  1 RAW8

If you agree with the comments above, I'll update the patch when applying it 
to my tree. I would however still like to get the following information

- What are the version of the three structures documented in this patch ? I 
can check that when I get back home, but if you have the information it would 
be faster.

- Do the fields in the Depth Control structure apply to the depth video stream 
only ? (I assume they do)

- What do the fields in the Capture Control structure apply to ? (I assume the 
left sensor and/or fish eye video streams)

- Does the optical time differ from half the exposure time ?

If you think it will take time to get this information and we risk missing the 
next kernel version, I'm OK applying the patch already, but please then submit 
a follow-up patch (or just drop a mail in reply to this one with the 
information and I can turn that into a patch).

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: add a D4M camera description

[Laurent Pinchart]

Hi Guennadi,

On Friday, 3 August 2018 14:07:12 EEST Guennadi Liakhovetski wrote:
> Hi Laurent,
> 
> Thanks for the review. A general note: I think you're requesting a rather
> detailed information about many parameters. That isn't a problem by
> itself, however, it is difficult to obtain some of that information. I'll
> address whatever comments I can in an updated version, just answering some
> questions here. I directed youor questions, that I couldn't answer myself
> to respective people, but I have no idea if and when I get replies. So,
> it's up to you whether to wait for that additional information or to take
> at least what we have now.

I've replied to v2, and apart from a few minor points, I think we can apply 
the current version. There are a few small questions I would still like to 
have answers to, but if it takes to long to obtain that, let's not miss v4.20.

> On Sun, 29 Jul 2018, Laurent Pinchart wrote:
> > On Saturday, 23 December 2017 13:11:00 EEST Guennadi Liakhovetski wrote:
> >> From: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> >> 
> >> D4M is a mobile model from the D4XX family of Intel RealSense cameras.
> >> This patch adds a descriptor for it, which enables reading per-frame
> >> metadata from it.
> >> 
> >> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> >> ---
> >> 
> >>  Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 202 ++++++++++++++++
> >>  drivers/media/usb/uvc/uvc_driver.c                |  11 ++
> >>  include/uapi/linux/videodev2.h                    |   1 +
> >>  3 files changed, 214 insertions(+)
> >>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> >> 
> >> diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> >> b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst new file mode 100644
> >> index 0000000..950780d
> >> --- /dev/null
> >> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst

[snip]

> >> +    * - :cspan:`1` *Configuration*
> >> +    * - __u32 ID
> >> +      - 0x80000002
> >> +    * - __u32 Size
> >> +      - Size in bytes (currently 40)
> >> +    * - __u32 Version
> >> +      - Version of the struct
> >> +    * - __u32 Flags
> >> +      - A bitmask of flags: see [4_] below
> >> +    * - __u8 Hardware type
> >> +      - Camera hardware version [5_]
> >> +    * - __u8 SKU ID
> >> +      - Camera hardware configuration [6_]
> >> +    * - __u32 Cookie
> >> +      - Internal synchronisation
> > 
> > Internal synchronisation with what ? :-)

This is still something I'd like to understand (and I understand it may still 
take time to receive an answer from the right person).

> >> +    * - __u16 Format
> >> +      - Image format code [7_]
> >> +    * - __u16 Width
> >> +      - Width in pixels
> >> +    * - __u16 Height
> >> +      - Height in pixels
> >> +    * - __u16 Framerate
> >> +      - Requested framerate
> > 
> > What's the unit of this value ?
> 
> Is anything other than frames per second used in V4L?

V4L2 expresses the frame rate as a fraction, hence my question, to know 
whether this field contained the number of frames per second as an integer, or 
used a different representation (such as a fixed point decimal value for 
instance).

> >> +    * - __u16 Trigger
> >> +      - Byte 0: bit 0:  depth and RGB are synchronised, bit 1: external
> >> trigger
> >> +
> >> +.. _1:
> >> +
> >> +[1]
> >> https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-ext
> >> ensions-1-5
> > 
> > Should we at some point replicate that documentation in the V4L2 spec ?
> > Without copying it of course, as that would be a copyright violation.
> 
> Well, we don't replicate the UVC itself or any other standards, do we? Of
> course, that document doesn't have the same status as an official
> vendor-neutral standard, but still, we don't replicate data sheets either.
> Besides, I think there are cameras that use this, and windows supports
> this, so, don't think it will disappear overnight...

Probably not overnight, you're right. I'm a bit worried about the link 
becoming invalid though. In any case that's not a blocker, but I might at some 
point decide to replicate the documentation.

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] uvcvideo: add a D4M camera description

[Guennadi Liakhovetski]

Hi Laurent,

Thanks for the review.

On Sat, 25 Aug 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thank you for the patch.
> 
> Overall this looks good to me, I only have small comments. Please see below, 
> with a summary at the end.
> 
> On Friday, 3 August 2018 14:37:08 EEST Guennadi Liakhovetski wrote:
> > D4M is a mobile model from the D4XX family of Intel RealSense cameras.
> > This patch adds a descriptor for it, which enables reading per-frame
> > metadata from it.
> > 
> > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > ---
> >  Documentation/media/uapi/v4l/meta-formats.rst     |   1 +
> >  Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 204 +++++++++++++++++++
> >  drivers/media/usb/uvc/uvc_driver.c                |  11 ++
> >  include/uapi/linux/videodev2.h                    |   1 +
> >  4 files changed, 217 insertions(+)
> >  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> 
> [snip]
> 
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst new file mode 100644
> > index 0000000..57ecfd9
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > @@ -0,0 +1,204 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _v4l2-meta-fmt-d4xx:
> > +
> > +*******************************
> > +V4L2_META_FMT_D4XX ('D4XX')
> > +*******************************
> > +
> > +Intel D4xx UVC Cameras Metadata
> > +
> > +
> > +Description
> > +===========
> > +
> > +Intel D4xx (D435 and other) cameras include per-frame metadata in their UVC
> > +payload headers, following the Microsoft(R) UVC extension proposal [1_].
> > That +means, that the private D4XX metadata, following the standard UVC
> > header, is +organised in blocks. D4XX cameras implement several standard
> > block types, +proposed by Microsoft, and several proprietary ones.
> > Supported standard metadata +types are MetadataId_CaptureStats (ID 3),
> > MetadataId_CameraExtrinsics (ID 4), +and MetadataId_CameraIntrinsics (ID
> > 5). For their description see [1_]. This +document describes proprietary
> > metadata types, used by D4xx cameras.
> > +
> > +V4L2_META_FMT_D4XX buffers follow the metadata buffer layout of
> > +V4L2_META_FMT_UVC with the only difference, that it also includes
> > proprietary +payload header data. D4xx cameras use bulk transfers and only
> > send one payload +per frame, therefore their headers cannot be larger than
> > 255 bytes.
> > +
> > +Below are proprietary Microsoft style metadata types, used by D4xx cameras,
> > +where all fields are in little endian order:
> > +
> > +.. flat-table:: D4xx metadata
> > +    :widths: 1 4
> > +    :header-rows:  1
> > +    :stub-columns: 0
> > +
> > +    * - Field
> > +      - Description
> > +    * - :cspan:`1` *Depth Control*
> 
> Does this mean that all fields in this structure apply to the depth image only 
> ? If so, do you mind if I mention that explicitly ?

I don't think that's the case. E.g. only this struct has laser parameters 
and the laser can be used without the depth node being active. In fact I 
just tried streaming from the other node and reading metadata from its 
metadata node - it also included ID 0x80000000 with flags set to 0xff, 
i.e. all fields valid.

> > +    * - __u32 ID
> > +      - 0x80000000
> > +    * - __u32 Size
> > +      - Size in bytes (currently 56)
> > +    * - __u32 Version
> > +      - Version of the struct
> 
> I would elaborate a bit here, how about the following ?
> 
> "Version of this structure. The documentation herein corresponds to version 
> xxx. The version number will be incremented when new fields are added."
> 
> If you can provide me with the current version number I can update this when 
> applying the patch (I would get it myself, but I'm about to board a plane and 
> haven't taken the camera with me :-/).

Sure, sounds good, the "depth control" is version 2, the rest is version 
1.

> > +    * - __u32 Flags
> > +      - A bitmask of flags: see [2_] below
> > +    * - __u32 Gain
> > +      - Gain value in internal units, same as the V4L2_CID_GAIN control,
> > used to
> > +        capture the frame
> > +    * - __u32 Exposure
> > +      - Exposure time (in microseconds) used to capture the frame
> > +    * - __u32 Laser power
> > +      - Power of the laser LED 0-360, used for depth measurement
> > +    * - __u32 AE mode
> > +      - 0: manual; 1: automatic exposure
> > +    * - __u32 Exposure priority
> > +      - Exposure priority value: 0 - constant frame rate
> > +    * - __u32 AE ROI left
> > +      - Left border of the AE Region of Interest (all ROI values are in
> > pixels
> > +        and lie between 0 and maximum width or height respectively)
> > +    * - __u32 AE ROI right
> > +      - Right border of the AE Region of Interest
> > +    * - __u32 AE ROI top
> > +      - Top border of the AE Region of Interest
> > +    * - __u32 AE ROI bottom
> > +      - Bottom border of the AE Region of Interest
> > +    * - __u32 Preset
> > +      - Preset selector value, default: 0, unless changed by the user
> 
> I won't block this patch for this, but could we get the documentation of the 
> corresponding XU control ? Even better, if Intel could publish documentation 
> of the full XUs, that would be great :-)

I think it's mostly up to you, if you insist, I'll ask for the information 
and tell them, that the patch won't go in without it. If you don't, it's 
the same answer I'm afraid - I asked for details and reminded about my 
query and still got no response back. The same holds for all other 
information.

> > +    * - __u32 Laser mode
> > +      - 0: off, 1: on
> > +    * - :cspan:`1` *Capture Timing*
> 
> Similarly, what does this apply to ? The left sensor, the fish eye camera, or 
> both (or something else) ?

You have a metadata node for each of those nodes, so, you just read from 
it when capturing from the respective video streaming node and use 
whatever is there.

> > +    * - __u32 ID
> > +      - 0x80000001
> > +    * - __u32 Size
> > +      - Size in bytes (currently 40)
> > +    * - __u32 Version
> > +      - Version of the struct
> > +    * - __u32 Flags
> > +      - A bitmask of flags: see [3_] below
> > +    * - __u32 Frame counter
> > +      - Monotonically increasing counter
> 
> I assume this increases by exactly one for every frame. I'll test it when I 
> get back home, and will update the documentation accordingly.

I think since it's formulated as it is, they didn't want the user to rely 
on any specific increment step. And I don't think testing a couple of 
use-cases would provide a relyable answer to this...

> > +    * - __u32 Optical time
> > +      - Time in microseconds from the beginning of a frame till its middle
> 
> I'm still puzzled by this value, as I expect it to be exactly half the 
> exposure time, which is reported separately. If that's not the case I'd like 
> to know what this represents.

Sorry, I really don't know more than what's there.

> > +    * - __u32 Readout time
> > +      - Time, used to read out a frame in microseconds
> > +    * - __u32 Exposure time
> > +      - Frame exposure time in microseconds
> > +    * - __u32 Frame interval
> > +      - In microseconds = 1000000 / framerate
> > +    * - __u32 Pipe latency
> > +      - Time in microseconds from start of frame to data in USB buffer
> > +    * - :cspan:`1` *Configuration*
> > +    * - __u32 ID
> > +      - 0x80000002
> > +    * - __u32 Size
> > +      - Size in bytes (currently 40)
> > +    * - __u32 Version
> > +      - Version of the struct
> > +    * - __u32 Flags
> > +      - A bitmask of flags: see [4_] below
> > +    * - __u8 Hardware type
> > +      - Camera hardware version [5_]
> > +    * - __u8 SKU ID
> > +      - Camera hardware configuration [6_]
> > +    * - __u32 Cookie
> > +      - Internal synchronisation
> > +    * - __u16 Format
> > +      - Image format code [7_]
> > +    * - __u16 Width
> > +      - Width in pixels
> > +    * - __u16 Height
> > +      - Height in pixels
> > +    * - __u16 Framerate
> > +      - Requested frame rate per second
> > +    * - __u16 Trigger
> > +      - Byte 0: bit 0: depth and RGB are synchronised, bit 1: external
> > trigger
> > +
> > +.. _1:
> > +
> > +[1]
> > https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extens
> > ions-1-5
> > +
> > +.. _2:
> > +
> > +[2] Depth Control flags specify which fields are valid: ::
> > +
> > +  0x00000001 Gain
> > +  0x00000002 Exposure
> > +  0x00000004 Laser power
> > +  0x00000008 AE mode
> > +  0x00000010 Exposure priority
> > +  0x00000020 AE ROI
> > +  0x00000040 Preset
> > +
> > +.. _3:
> > +
> > +[3] Capture Timing flags specify which fields are valid: ::
> > +
> > +  0x00000001 Frame counter
> > +  0x00000002 Optical time
> > +  0x00000004 Readout time
> > +  0x00000008 Exposure time
> > +  0x00000010 Frame interval
> > +  0x00000020 Pipe latency
> > +
> > +.. _4:
> > +
> > +[4] Configuration flags specify which fields are valid: ::
> > +
> > +  0x00000001 Hardware type
> > +  0x00000002 SKU ID
> > +  0x00000004 Cookie
> > +  0x00000008 Format
> > +  0x00000010 Width
> > +  0x00000020 Height
> > +  0x00000040 Framerate
> > +  0x00000080 Trigger
> > +  0x00000100 Cal count
> > +
> > +.. _5:
> > +
> > +[5] Camera model: ::
> > +
> > +  0 DS5
> > +  1 IVCAM2
> > +
> > +.. _6:
> > +
> > +[6] 8-bit camera hardware configuration bitfield: ::
> > +
> > +  [1:0] depthCamera
> > +	00: no depth
> > +	01: standard depth
> > +	10: wide depth
> > +	11: reserved
> > +  [2]   depthIsActive - has a laser projector
> > +  [3]   RGB presence
> > +  [4]   IMU presence
> 
> Do you mind if I write this "Inertial Measurement Unit (IMU) presense" ? Or is 
> it just me who's not familiar enough with depth cameras ? :-)

IMUs aren't only present in depth cameras, in fact, they are more usually 
present outside of depth cameras. E.g. in phones, drones, robots, etc. 
Everywhere where you need orientation, acceleration, and similar stuff. 
But up to you, if you think, this isn't common enough, feel free to 
explain.

> On a side note, does the camera expose IMU data to the host over USB ?

I haven't worked with such cameras, but if there's an IMU in a USB camera, 
USB is the only way to present it to the user, and yes, I think it would 
be presented to the user and not only used internally. But again, I 
haven't worked with them personally.

> > +  [5]   projectorType
> > +	0: HPTG
> > +	1: Princeton
> > +  [6]   0: a projector, 1: an LED
> > +  [7]   reserved
> > +
> > +.. _7:
> > +
> > +[7] Image format codes per camera interface:
> 
> I was initially puzzled by this until I read your explanation. How about 
> wording it as "Image format codes per video streaming interface" to use the 
> UVC vocabulary ?

Sure, sounds good!

> 
> > +
> > +Depth: ::
> > +
> > +  1 Z16
> > +  2 Z
> > +
> > +Left sensor: ::
> > +
> > +  1 Y8
> > +  2 UYVY
> > +  3 R8L8
> > +  4 Calibration
> > +  5 W10
> > +
> > +Fish Eye sensor: ::
> > +
> > +  1 RAW8
> 
> If you agree with the comments above, I'll update the patch when applying it 
> to my tree. I would however still like to get the following information
> 
> - What are the version of the three structures documented in this patch ? I 
> can check that when I get back home, but if you have the information it would 
> be faster.
> 
> - Do the fields in the Depth Control structure apply to the depth video stream 
> only ? (I assume they do)
> 
> - What do the fields in the Capture Control structure apply to ? (I assume the 
> left sensor and/or fish eye video streams)
> 
> - Does the optical time differ from half the exposure time ?

All responses above.

Thanks
Guennadi

> 
> If you think it will take time to get this information and we risk missing the 
> next kernel version, I'm OK applying the patch already, but please then submit 
> a follow-up patch (or just drop a mail in reply to this one with the 
> information and I can turn that into a patch).
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

Re: [PATCH] uvcvideo: add a D4M camera description

[Guennadi Liakhovetski]

Hi Laurent,

On Sat, 25 Aug 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Friday, 3 August 2018 14:07:12 EEST Guennadi Liakhovetski wrote:
> > Hi Laurent,
> > 
> > Thanks for the review. A general note: I think you're requesting a rather
> > detailed information about many parameters. That isn't a problem by
> > itself, however, it is difficult to obtain some of that information. I'll
> > address whatever comments I can in an updated version, just answering some
> > questions here. I directed youor questions, that I couldn't answer myself
> > to respective people, but I have no idea if and when I get replies. So,
> > it's up to you whether to wait for that additional information or to take
> > at least what we have now.
> 
> I've replied to v2, and apart from a few minor points, I think we can apply 
> the current version. There are a few small questions I would still like to 
> have answers to, but if it takes to long to obtain that, let's not miss v4.20.
> 
> > On Sun, 29 Jul 2018, Laurent Pinchart wrote:
> > > On Saturday, 23 December 2017 13:11:00 EEST Guennadi Liakhovetski wrote:
> > >> From: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > >> 
> > >> D4M is a mobile model from the D4XX family of Intel RealSense cameras.
> > >> This patch adds a descriptor for it, which enables reading per-frame
> > >> metadata from it.
> > >> 
> > >> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > >> ---
> > >> 
> > >>  Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 202 ++++++++++++++++
> > >>  drivers/media/usb/uvc/uvc_driver.c                |  11 ++
> > >>  include/uapi/linux/videodev2.h                    |   1 +
> > >>  3 files changed, 214 insertions(+)
> > >>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > >> 
> > >> diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > >> b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst new file mode 100644
> > >> index 0000000..950780d
> > >> --- /dev/null
> > >> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> 
> [snip]
> 
> > >> +    * - :cspan:`1` *Configuration*
> > >> +    * - __u32 ID
> > >> +      - 0x80000002
> > >> +    * - __u32 Size
> > >> +      - Size in bytes (currently 40)
> > >> +    * - __u32 Version
> > >> +      - Version of the struct
> > >> +    * - __u32 Flags
> > >> +      - A bitmask of flags: see [4_] below
> > >> +    * - __u8 Hardware type
> > >> +      - Camera hardware version [5_]
> > >> +    * - __u8 SKU ID
> > >> +      - Camera hardware configuration [6_]
> > >> +    * - __u32 Cookie
> > >> +      - Internal synchronisation
> > > 
> > > Internal synchronisation with what ? :-)
> 
> This is still something I'd like to understand (and I understand it may still 
> take time to receive an answer from the right person).

Sorry, no idea, that flag wasn't even set when I was testing it.

> > >> +    * - __u16 Format
> > >> +      - Image format code [7_]
> > >> +    * - __u16 Width
> > >> +      - Width in pixels
> > >> +    * - __u16 Height
> > >> +      - Height in pixels
> > >> +    * - __u16 Framerate
> > >> +      - Requested framerate
> > > 
> > > What's the unit of this value ?
> > 
> > Is anything other than frames per second used in V4L?
> 
> V4L2 expresses the frame rate as a fraction, hence my question, to know 
> whether this field contained the number of frames per second as an integer, or 
> used a different representation (such as a fixed point decimal value for 
> instance).

Nono, sorry, just an integer FPS.

Thanks
Guennadi

> > >> +    * - __u16 Trigger
> > >> +      - Byte 0: bit 0:  depth and RGB are synchronised, bit 1: external
> > >> trigger
> > >> +
> > >> +.. _1:
> > >> +
> > >> +[1]
> > >> https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-ext
> > >> ensions-1-5
> > > 
> > > Should we at some point replicate that documentation in the V4L2 spec ?
> > > Without copying it of course, as that would be a copyright violation.
> > 
> > Well, we don't replicate the UVC itself or any other standards, do we? Of
> > course, that document doesn't have the same status as an official
> > vendor-neutral standard, but still, we don't replicate data sheets either.
> > Besides, I think there are cameras that use this, and windows supports
> > this, so, don't think it will disappear overnight...
> 
> Probably not overnight, you're right. I'm a bit worried about the link 
> becoming invalid though. In any case that's not a blocker, but I might at some 
> point decide to replicate the documentation.
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
>