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:   Tue, 16 May 2023 11:54:47 +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: <ZGNvBySOEbBQrflJ@kekkonen.localdomain>
References: <e16ddf4fdb83f30899e575b218e524f6346a9f50.camel@intel.com>
 <ZFoESmKficDbqwFv@kekkonen.localdomain>
 <6de8f9611e5bcf20d7d30e6d26d78f146316b164.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6de8f9611e5bcf20d7d30e6d26d78f146316b164.camel@intel.com>
Precedence: bulk

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