Re: [PATCH v8 23/38] media: uapi: ccs: Add media bus code for MIPI CCS embedded data

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: linux-media@vger.kernel.org, tomi.valkeinen@ideasonboard.com,
	bingbu.cao@intel.com, hongju.wang@intel.com, hverkuil@xs4all.nl,
	Andrey Konovalov <andrey.konovalov@linaro.org>,
	Jacopo Mondi <jacopo.mondi@ideasonboard.com>,
	Dmitry Perchanov <dmitry.perchanov@intel.com>,
	"Ng, Khai Wen" <khai.wen.ng@intel.com>,
	Alain Volmat <alain.volmat@foss.st.com>
Subject: Re: [PATCH v8 23/38] media: uapi: ccs: Add media bus code for MIPI CCS embedded data
Date: Sat, 20 Apr 2024 11:07:40 +0300	[thread overview]
Message-ID: <20240420080740.GA15761@pendragon.ideasonboard.com> (raw)
In-Reply-To: <Zhu0ke6RA9PYfYZi@kekkonen.localdomain>

Hi Sakari,

On Sun, Apr 14, 2024 at 10:48:49AM +0000, Sakari Ailus wrote:
> On Fri, Apr 12, 2024 at 10:07:46PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 11, 2024 at 09:04:47AM +0000, Sakari Ailus wrote:
> > > On Thu, Mar 21, 2024 at 06:49:14PM +0200, Laurent Pinchart wrote:
> > > > On Wed, Mar 13, 2024 at 09:25:01AM +0200, Sakari Ailus wrote:
> > > > > Add new MIPI CCS embedded data media bus code
> > > > > (MEDIA_BUS_FMT_CCS_EMBEDDED).
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  .../media/v4l/subdev-formats.rst              | 28 +++++++++++++++++++
> > > > >  include/uapi/linux/media-bus-format.h         |  3 ++
> > > > >  2 files changed, 31 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > > > index cbd475f7cae9..c8f982411e70 100644
> > > > > --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > > > +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > > > @@ -8564,3 +8564,31 @@ and finally the bit number in subscript. "X" indicates a padding bit.
> > > > >        - X
> > > > >        - X
> > > > >        - X
> > > > > +
> > > > > +.. _MEDIA-BUS-FMT-CCS-EMBEDDED:
> > > > > +
> > > > > +MIPI CCS Embedded Data Formats
> > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > +
> > > > > +`MIPI CCS <https://www.mipi.org/specifications/camera-command-set>`_ defines a
> > > > > +metadata format for sensor embedded data, which is used to store the register
> > > > > +configuration used for capturing a given frame. The format is defined in the CCS
> > > > > +specification. The media bus code for this format is
> > > > > +``MEDIA_BUS_FMT_CCS_EMBEDDED``.
> > > > > +
> > > > > +The CCS embedded data format definition includes three levels:
> > > > > +
> > > > > +1. Padding within CSI-2 bus :ref:`Data unit <media-glossary-data-unit>` as
> > > > 
> > > > s/Data unit/Data Unit/
> > > 
> > > Yes.
> > > 
> > > > > +   documented in the MIPI CCS specification.
> > > > > +
> > > > > +2. The tagged data format as documented in the MIPI CCS specification.
> > > > > +
> > > > > +3. Register addresses and register documentation as documented in the MIPI CCS
> > > > > +   specification.
> > > > > +
> > > > > +The format definition shall be used only by devices that fulfill all three
> > > > > +levels above.
> > > > 
> > > > Hmmmm... Do we need to mandate level 3 ? There are lots of sensors that
> > > > comply with the first two levels but have their own register set. Would
> > > > you like a sensor-specific embedded data format for each of them ? If
> > > > so, how would we document it without essentially copying the datasheet ?
> > > 
> > > You could refer to the datasheet if it's publicly available.
> > 
> > What if it's not ? How would you expect documentation to be organized ?
> > Will we then end up adding one media bus code per sensor ?
> 
> Probably. The user space needs a way to find out what to expect. If a
> sensor produces its own custom metadata format, it needs to be documented.
> 
> There's also documentation for OV2740 embedded data in the set.

The OV2740 is a different case than what I had in mind. It uses level 1
of the CCS embedded data only, without using the CCS tagged format. The
documentation you've included in this patch series looks good to me.

For sensors that are compatible with CCS embedded data level 2, I'm not
sure how to proceed. Those sensors send (register, value) pairs in their
embedded data, but the register set is not CCS-compatible. Even when
documentation is available, it's hard to verify that the sensor will
never output the value of undocumented registers, as the registers
included in the embedded data may vary depending on the sensor
configuration.

How would you proceed in such cases ? I can try to write a sample
documentation patch, but I don't know what you expect. I'm thinking
about the IMX219 sensor for instance, but any sensor compatible with CCS
embedded data level 2 without a public datasheet is a good example.

-- 
Regards,

Laurent Pinchart