Message-ID: <72e73b8e1e90c77d3f1968aca5eef8aa9a167e5e.camel@intel.com>
Date:   Fri, 02 Jun 2023 18:37:33 +0300
From:   Dmitry Perchanov <dmitry.perchanov@...el.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     linux-media@...r.kernel.org, mchehab@...nel.org,
        linux-kernel@...r.kernel.org, sakari.ailus@...el.com,
        Evgeni Raikhel <evgeni.raikhel@...el.com>, demisrael@...il.com,
        Nir Azkiel <nir.azkiel@...el.com>
Subject: Re: [PATCH v4] media: uapi: v4l: Intel metadata format update

Hi Laurent,

On Fri, 2023-06-02 at 18:25 +0300, Laurent Pinchart wrote:
> 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@...el.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 ?
That is correct, my bad.
Let's change:
"This document implements Intel Configuration version 3"
To:
"The documentation herein covers versions 1, 2 and 3."


> 
> > > > @@ -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)
As i see from the source, the GPIO input data supported from v2, so this can be modified from "v3 only: to "v2, v3"
Here is data from source:
=== CUT ===
Intel Configuration:
version 2: gpioInputData added to md_configuration (with its flag)
version 3: sub_preset_info added to md_configuration (with its flag)
=== CUT ===
Unfortunately there is no information when "Calibration counter" introduced but i can extrapolate it was from version 2.


> > > > +      - 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)
"Sub-preset info" from version 3.
> > > > +      - 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
Here we mark which fields are relevant, there cannot be confusion from version 1, 2 or 3.
> > > >  
> > > >  .. _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@...asonboard.com>
> > > 
> > > > +https://github.com/IntelRealSense/librealsense/blob/master/src/metadata.h

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Powered by blists - more mailing lists