Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Jun 2023 14:30:49 +0000
From:   Sakari Ailus <sakari.ailus@intel.com>
To:     Dmitry Perchanov <dmitry.perchanov@intel.com>
Cc:     linux-media@vger.kernel.org, mchehab@kernel.org,
        linux-kernel@vger.kernel.org, laurent.pinchart@ideasonboard.com,
        evgeni.raikhel@intel.com, demisrael@gmail.com
Subject: Re: [PATCH v2] media: uapi: v4l: Intel metadata format update
Message-ID: <ZHirmZPwfTunInw+@kekkonen.localdomain>
References: <e16ddf4fdb83f30899e575b218e524f6346a9f50.camel@intel.com>
 <ZFoESmKficDbqwFv@kekkonen.localdomain>
 <6de8f9611e5bcf20d7d30e6d26d78f146316b164.camel@intel.com>
 <ZGNvBySOEbBQrflJ@kekkonen.localdomain>
 <e4e3cdd4c2193198589ff6de05be165a8237a794.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e4e3cdd4c2193198589ff6de05be165a8237a794.camel@intel.com>
Precedence: bulk

Hi Dmitry,

On Thu, Jun 01, 2023 at 05:22:15PM +0300, Dmitry Perchanov wrote:
> Fixed in v3.

Please avoid top-posting.

v3 does not

- differentiate metadata versions 1 and 3,
- elaborate laser mode vs. other fields (bug, change in format or
  otherwise) nor
- lists which fields are only available in v3.

As it appears that devices that use either of these formats continue to be
around, it would be appropriate to document both versions.

On top of this, it seems the document purports the change in size of the
data structure between the versions 1 and 3 to be 4 (60 - 56) but the patch
adds fields with total size of 8 bytes.

Is the firmware versioning the same for all uvc driver supported Realsense
devices?

> 
> On Tue, 2023-05-16 at 11:54 +0000, Sakari Ailus wrote:
> > Hi Dmitry,
> > 
> > On Tue, May 16, 2023 at 11:10:02AM +0300, Dmitry Perchanov wrote:
> > > On Tue, 2023-05-09 at 11:28 +0300, Sakari Ailus wrote:
> > > > Hi Dmitry,
> > > > 
> > > > Thanks for the patch.
> > > > 
> > > > No need to cc me to my @iki.fi address, I do read both. :-)
> > > Good.
> > > > On Tue, May 09, 2023 at 11:24:53AM +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>
> > > > > ---
> > > > 
> > > > Please detail here what changed between patch versions in future versions
> > > > of the patch.
> > > Intel Configuration:
> > > version 2: gpioInputData added to md_configuration (with its flag)
> > > version 3: sub_preset_info added to md_configuration (with its flag)
> > > > >  .../media/v4l/pixfmt-meta-d4xx.rst            | 19 ++++++++++++++++---
> > > > >  1 file changed, 16 insertions(+), 3 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..b5decde640c1 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.
> > > > 
> > > > Which version was described here before this patch?
> > > Before that patch it was "Intel Configuration version 1"
> > > > Are there devices that use that presumably different version? Or does
> > > > this depend on e.g. firmware version?
> > > These changes are extensions and backward compatible with old firmware.
> > > Users are notified in case firmware too old and some features disabled.
> > 
> > The "Laser mode" below is replaced by three different fields. Was this a
> > bug in the document or a change between versions 1 and 3? In the former
> > case there should be another patch to fix it, in the latter both versions
> > should continue to be described as they are supported.
> > 
> > > > > +
> > > > >  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 (currently 60)
> > > > >      * - __u32 Version
> > > > >        - Version of this structure. The documentation herein corresponds to
> > > > >          version xxx. The version number will be incremented when new fields are
> > > > > @@ -72,8 +74,11 @@ 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
> > > > > +    * - __u8 Emitter mode
> > > > >        - 0: off, 1: on
> > > > > +    * - __u8 RFU byte
> > > > > +    * - __u16 LED Power
> > > > > +      - Led power value 0-360 (F416 SKU)
> > > > >      * - :cspan:`1` *Capture Timing*
> > > > >      * - __u32 ID
> > > > >        - 0x80000001
> > > > > @@ -124,6 +129,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
> > > > > +    * - __u8 GPIO input data
> > > > > +      - GPIO readout
> > > > > +      - Supported from FW 5.12.7.0
> > > > > +    * - __u32 Sub-preset info
> > > > > +      - Sub-preset choice information
> > > > > +    * - __u8 reserved
> > > > > +      - RFU byte.
> > 
> > Could you add to the documentation of these fields they're only valid for
> > v3?
> > 
> > > > >  
> > > > >  .. _1:
> > > > >  
> 

-- 
Kind regards,

Sakari Ailus