Re: [PATCH v4] media: uapi: v4l: Intel metadata format update

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dmitry,

On Fri, Jun 02, 2023 at 06:00:04PM +0300, Dmitry Perchanov wrote:
> Hi Laurent,
> 
> Thanks for review!
> I will gladly accept your proposed changes.
> 
> Comments inline.
> 
> On Fri, 2023-06-02 at 17:46 +0300, Laurent Pinchart wrote:
> > On Thu, Jun 01, 2023 at 07:08:46PM +0300, Dmitry Perchanov wrote:
> > > Update metadata structure for Intel RealSense UVC/MIPI cameras.
> > > Compliant to Intel Configuration version 3.
> > > 
> > > Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
> > > ---
> > >  .../media/v4l/pixfmt-meta-d4xx.rst            | 52 ++++++++++++++++---
> > >  1 file changed, 46 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/Documentation/userspace-api/media/v4l/pixfmt-meta-d4xx.rst b/Documentation/userspace-api/media/v4l/pixfmt-meta-d4xx.rst
> > > index 4e437ba97a0e..7482f298d0cc 100644
> > > --- a/Documentation/userspace-api/media/v4l/pixfmt-meta-d4xx.rst
> > > +++ b/Documentation/userspace-api/media/v4l/pixfmt-meta-d4xx.rst
> > > @@ -12,7 +12,7 @@ Intel D4xx UVC Cameras Metadata
> > >  Description
> > >  ===========
> > >  
> > > -Intel D4xx (D435 and other) cameras include per-frame metadata in their UVC
> > > +Intel D4xx (D435, D455 and others) 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,
> > > @@ -26,6 +26,8 @@ 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.
> > >  
> > > +This document implements Intel Configuration version 3 [9_].
> > > +
> > >  Below are proprietary Microsoft style metadata types, used by D4xx cameras,
> > >  where all fields are in little endian order:
> > >  
> > > @@ -43,7 +45,7 @@ where all fields are in little endian order:
> > >      * - __u32 ID
> > >        - 0x80000000
> > >      * - __u32 Size
> > > -      - Size in bytes (currently 56)
> > > +      - Size in bytes, include ID (all protocol versions: 60)
> > >      * - __u32 Version
> > >        - Version of this structure. The documentation herein corresponds to
> > >          version xxx. The version number will be incremented when new fields are
> > 
> > Should this read "version 3" instead of "version xxx" ?
> 
> This can read 1, 2 or 3, depends on firmware version.
> All cameras have same firmware. The latest will report 3.

The sentence reads

"The documentation herein corresponds to version xxx."

As you're updating the documentation to correspond to version 3, I
thought it would be best to make that explicit. Another option would be

"The documentation herein covers versions 1, 2 and 3."

I think I like that better. What do you think ?

> > > @@ -72,13 +74,17 @@ where all fields are in little endian order:
> > >        - 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
> > > +    * - __u8 Emitter mode (v3 only) (__u32 Laser mode for v1) [8_]
> > > +      - 0: off, 1: on, same as __u32 Laser mode for v1
> > > +    * - __u8 RFU byte (v3 only)
> > > +      - Spare byte for future use
> > > +    * - __u16 LED Power (v3 only)
> > > +      - Led power value 0-360 (F416 SKU)
> > >      * - :cspan:`1` *Capture Timing*
> > >      * - __u32 ID
> > >        - 0x80000001
> > >      * - __u32 Size
> > > -      - Size in bytes (currently 40)
> > > +      - Size in bytes, include ID (all protocol versions: 40)
> > >      * - __u32 Version
> > >        - Version of this structure. The documentation herein corresponds to
> > >          version xxx. The version number will be incremented when new fields are
> > > @@ -101,7 +107,7 @@ where all fields are in little endian order:
> > >      * - __u32 ID
> > >        - 0x80000002
> > >      * - __u32 Size
> > > -      - Size in bytes (currently 40)
> > > +      - Size in bytes, include ID (v1:36, v3:40)
> > >      * - __u32 Version
> > >        - Version of this structure. The documentation herein corresponds to
> > >          version xxx. The version number will be incremented when new fields are
> > > @@ -124,6 +130,14 @@ where all fields are in little endian order:
> > >        - Requested frame rate per second
> > >      * - __u16 Trigger
> > >        - Byte 0: bit 0: depth and RGB are synchronised, bit 1: external trigger
> > > +    * - __u16 Calibration count (v3 only)
> > > +      - Calibration counter, see [4_] below
> > > +    * - __u8 GPIO input data (v3 only)
> > > +      - GPIO readout, see [4_] below (Supported from FW 5.12.7.0)
> > > +    * - __u32 Sub-preset info (v3 only)
> > > +      - Sub-preset choice information, see [4_] below
> > > +    * - __u8 reserved (v3 only)
> > > +      - RFU byte.
> > >  
> > >  .. _1:
> > >  
> > > @@ -140,6 +154,8 @@ where all fields are in little endian order:
> > >    0x00000010 Exposure priority
> > >    0x00000020 AE ROI
> > >    0x00000040 Preset
> > > +  0x00000080 Emitter mode
> > > +  0x00000100 LED Power
> > >  
> > >  .. _3:
> > >  
> > > @@ -165,6 +181,8 @@ where all fields are in little endian order:
> > >    0x00000040 Framerate
> > >    0x00000080 Trigger
> > >    0x00000100 Cal count
> > > +  0x00000200 GPIO Input Data
> > > +  0x00000400 Sub-preset Info
> > >  
> > >  .. _5:
> > >  
> > > @@ -211,3 +229,25 @@ Left sensor: ::
> > >  Fish Eye sensor: ::
> > >  
> > >    1 RAW8
> > > +
> > > +.. _8:
> > > +
> > > +[8] The "Laser mode" is replaced by three different fields.
> > 
> > "... has been replaced in version 3 by ..."
> > 
> > > +"Laser" renamed to "Emitter" as there multiple technologies
> > 
> > s/there/there are/
> 
> Accepted. Thanks!
>
> > > +for camera projectors. As we have another field for "Laser Power"
> > > +we introduced "LED Power" for extra emitter.
> > > +
> > > +The __u32 "Laser mode" integer is divided by two bytes and short: ::
> > 
> > The "Laser mode" __u32 field has been split into: ::
>
> Accepted. Thanks!
> 
> > > +   1 __u8 Emitter mode
> > > +   2 __u8 RFU byte
> > > +   3 __u16 LED Power
> > > +
> > > +This is a change between versions 1 and 3. All versions 1,2 and 3
> > 
> > s/1,2/1, 2/
>
> Accepted. Thanks!
> 
> > > +are backward compatible with same data format and they are supported.
> > > +See [2_] for which attributes are valid.
> > > +
> > > +.. _9:
> > > +
> > > +[9]
> > > +LibRealSense SDK metadata source:
> > 
> > I'll remove the blank line after '[9]', that is
> > 
> > > +[9] LibRealSense SDK metadata source:
>
> Accepted. Thanks!
> 
> > I can fix all this when applying if you're fine with the changes.
>
> Yes, I agree. Thank You for your effort!
>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > > +https://github.com/IntelRealSense/librealsense/blob/master/src/metadata.h

-- 
Regards,

Laurent Pinchart