linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.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 08/38] media: Documentation: Document embedded data guidelines for camera sensors
Date: Tue, 9 Apr 2024 11:12:04 +0000	[thread overview]
Message-ID: <ZhUihNnf6vQXWTyO@kekkonen.localdomain> (raw)
In-Reply-To: <20240320000300.GM8501@pendragon.ideasonboard.com>

Hi Laurent,

On Wed, Mar 20, 2024 at 02:03:00AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wed, Mar 13, 2024 at 09:24:46AM +0200, Sakari Ailus wrote:
> > Document how embedded data support should be implemented for camera
> > sensors, and when and how CCS embedded data format should be referenced.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  .../media/drivers/camera-sensor.rst           | 29 +++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/Documentation/userspace-api/media/drivers/camera-sensor.rst b/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > index 919a50e8b9d9..92545538b855 100644
> > --- a/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > +++ b/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > @@ -102,3 +102,32 @@ register programming sequences shall initialize the :ref:`V4L2_CID_HFLIP
> >  values programmed by the register sequences. The default values of these
> >  controls shall be 0 (disabled). Especially these controls shall not be inverted,
> >  independently of the sensor's mounting rotation.
> > +
> > +Embedded data
> > +-------------
> > +
> > +Many sensors, mostly raw sensors, support embedded data which is used to convey
> > +the sensor configuration for the captured frame back to the host. While CSI-2 is
> > +the most common bus used by such sensors, embedded data can be available on
> > +other bus types as well.
> > +
> > +Embedded data support shall use an internal source pad (a pad that has
> 
> s/source pad/sink pad/
> 
> Or do we call those "internal source pad" ?

As I wrote in the earlier e-mail, I'm quite unhappy with the "internal
source pad" term, mainly because of the conflict with the term itself and the
flags that indicate it. I'd just call them "internal sink pads" and then
explain they're actually a source of data.

> 
> As this is uAPI documentation, it is mainly targetting (in my opinion)
> userspace developers. I would write "Embedded data support uses ..." to
> describe the behaviour seen from userspace, instead of using "shall" to
> describe the requirements towards drivers.

Sounds good.

> 
> > +``MEDIA_PAD_FL_SINK <MEDIA-PAD-FL-SINK>`` and ``MEDIA_PAD_FL_INTERNAL
> > +<MEDIA-PAD-FL-INTERNAL>`` flags set) and route to the external pad. If embedded
> > +data output can be disabled in hardware, it should be possible to disable the
> > +embedded data route via ``VIDIOC_SUBDEV_S_ROUTING`` IOCTL.
> 
> You should mention the image pad here too:
> 
> Such sensors expose two internal sink pads (pads that have both the
> ``MEDIA_PAD_FL_SINK <MEDIA-PAD-FL-SINK>`` and ``MEDIA_PAD_FL_INTERNAL
> <MEDIA-PAD-FL-INTERNAL>`` flags set) to model the source of the image and
> embedded data streams. Each of those pads produces a single stream, and the

s/Each of those/Both of these/

> sub-device routes those streams to the external (source) pad. If embedded data
> output can be disabled in hardware, the sub-device allows disabling the
> embedded data route via the ``VIDIOC_SUBDEV_S_ROUTING`` IOCTL.

In the last sentence, we need to refer to the driver.

But generally I agree. I'll use:

"If the sub-device driver supports disabling embedded data, this can be
done by disabling the embedded data route via the
``VIDIOC_SUBDEV_S_ROUTING`` IOCTL."

> 
> > +
> > +In general, changing the embedded data format from the driver-configured values
> > +is not supported. The height of the metadata is hardware specific and the width
> 
> s/hardware specific/device-specific/

Yes.

> 
> > +is that (or less of that) of the image width, as configured on the pixel data
> > +stream.
> > +
> > +CCS and non-CCS embedded data
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +Embedded data which is compliant with CCS definitions shall use ``CCS embedded
> > +data format <MEDIA-BUS-FMT-CCS-EMBEDDED>``. Device specific embedded data which
> 
> You should clarify here that you're talking about the internal sink pad.
> 
> s/Device specific/Only device-specific/
> 
> > +is compliant up to MIPI CCS embedded data levels 1 or 2 only shall refer to CCS
> 
> s/up to/with the/
> s/only shall/may/
> 
> > +embedded data formats and document the level of conformance. The rest of the
> > +device specific embedded data format shall be documented in the context of the
> 
> s/device specific/device-specific/
> 
> > +data format itself.
> 
> Not sure what you mean by the context in the last sentence.

This refers to CCS embedded data support levels which appears to be
documented in a later patch in the series ("media: uapi: ccs: Add media bus
code for MIPI CCS embedded data"). I'll add this paragraph after that patch.

The paragraph became finally:

Embedded data which is fully compliant with CCS definitions uses ``CCS embedded
data format <MEDIA-BUS-FMT-CCS-EMBEDDED>`` media bus code (level
3). Device-specific embedded data compliant with either MIPI CCS embedded data
levels 1 or 2 only shall not use CCS embedded data mbus code, but may refer to
CCS embedded data documentation with the level of conformance specified and omit
documenting these aspects of the format. The rest of the device-specific
embedded data format is documented in the context of the data format itself.

-- 
Regards,

Sakari Ailus

  reply	other threads:[~2024-04-09 11:12 UTC|newest]

Thread overview: 149+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-13  7:24 [PATCH v8 00/38] Generic line based metadata support, internal pads Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 01/38] media: mc: Add INTERNAL pad flag Sakari Ailus
2024-03-14  7:17   ` Tomi Valkeinen
2024-03-19 13:21     ` Sakari Ailus
2024-03-19 22:17     ` Laurent Pinchart
2024-03-20  7:49       ` Sakari Ailus
2024-03-21 17:20         ` Laurent Pinchart
2024-03-28  9:47           ` Sakari Ailus
2024-03-28 10:05             ` Sakari Ailus
2024-03-28 15:25             ` Laurent Pinchart
2024-04-11  7:25               ` Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 02/38] media: Documentation: Add "stream" into glossary Sakari Ailus
2024-03-14  7:18   ` Tomi Valkeinen
2024-03-19 22:20   ` Laurent Pinchart
2024-03-13  7:24 ` [PATCH v8 03/38] media: uapi: Add generic serial metadata mbus formats Sakari Ailus
2024-03-14  7:30   ` Tomi Valkeinen
2024-03-19 13:27     ` Sakari Ailus
2024-03-19 14:20       ` Tomi Valkeinen
2024-03-19 22:33         ` Laurent Pinchart
2024-03-19 23:00           ` Laurent Pinchart
2024-03-20  8:48             ` Sakari Ailus
2024-03-21 17:30               ` Laurent Pinchart
2024-03-22  6:50           ` Tomi Valkeinen
2024-03-25 14:02             ` Sakari Ailus
2024-03-20  8:36         ` Sakari Ailus
2024-03-19 22:59   ` Laurent Pinchart
2024-03-20 16:23     ` Sakari Ailus
2024-03-21 17:38       ` Laurent Pinchart
2024-03-13  7:24 ` [PATCH v8 04/38] media: uapi: Document which mbus format fields are valid for metadata Sakari Ailus
2024-03-14 15:23   ` Tomi Valkeinen
2024-03-19 23:14   ` Laurent Pinchart
2024-03-20 16:49     ` Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 05/38] media: uapi: Add generic 8-bit metadata format definitions Sakari Ailus
2024-03-19 23:37   ` Laurent Pinchart
2024-04-15 14:05     ` Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 06/38] media: v4l: Support line-based metadata capture Sakari Ailus
2024-03-19 23:40   ` Laurent Pinchart
2024-03-13  7:24 ` [PATCH v8 07/38] media: Documentation: Additional streams generally don't harm capture Sakari Ailus
2024-03-19 23:48   ` Laurent Pinchart
2024-04-15 14:27     ` Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 08/38] media: Documentation: Document embedded data guidelines for camera sensors Sakari Ailus
2024-03-15 14:49   ` Julien Massot
2024-03-20  0:03   ` Laurent Pinchart
2024-04-09 11:12     ` Sakari Ailus [this message]
2024-03-13  7:24 ` [PATCH v8 09/38] media: Documentation: v4l: Document internal source pads Sakari Ailus
2024-03-15 15:32   ` Julien Massot
2024-03-19 13:47     ` Sakari Ailus
2024-03-19 14:38       ` Julien Massot
2024-03-20  0:26       ` Laurent Pinchart
2024-04-09 12:14         ` Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 10/38] media: Documentation: Document S_ROUTING behaviour Sakari Ailus
2024-03-15 15:38   ` Julien Massot
2024-03-20  0:33   ` Laurent Pinchart
2024-04-11  8:02     ` Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 11/38] media: v4l: subdev: Add a function to lock two sub-device states, use it Sakari Ailus
2024-03-15 15:42   ` Julien Massot
2024-03-20  0:36   ` Laurent Pinchart
2024-03-13  7:24 ` [PATCH v8 12/38] media: v4l: subdev: Move G_ROUTING handling below S_ROUTING Sakari Ailus
2024-03-15 15:43   ` Julien Massot
2024-03-20  0:37   ` Laurent Pinchart
2024-03-13  7:24 ` [PATCH v8 13/38] media: v4l: subdev: Copy argument back to user also for S_ROUTING Sakari Ailus
2024-03-15 15:50   ` Julien Massot
2024-03-20  0:39   ` Laurent Pinchart
2024-04-11  8:06     ` Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 14/38] media: v4l: subdev: Add len_routes field to struct v4l2_subdev_routing Sakari Ailus
2024-03-20  1:36   ` Laurent Pinchart
2024-04-16  7:09     ` Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 15/38] media: v4l: subdev: Return routes set using S_ROUTING Sakari Ailus
2024-03-20  1:45   ` Laurent Pinchart
2024-04-16  7:12     ` Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 16/38] media: v4l: subdev: Allow a larger number of routes than there's room for Sakari Ailus
2024-03-20  1:53   ` Laurent Pinchart
2024-04-16  8:08     ` Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 17/38] media: v4l: subdev: Add trivial set_routing support Sakari Ailus
2024-03-15 15:51   ` Julien Massot
2024-03-20  1:55   ` Laurent Pinchart
2024-04-01 23:41     ` Laurent Pinchart
2024-04-11  8:13       ` Sakari Ailus
2024-04-12 19:14         ` Laurent Pinchart
2024-04-15  8:10           ` Sakari Ailus
2024-03-13  7:24 ` [PATCH v8 18/38] media: ccs: No need to set streaming to false in power off Sakari Ailus
2024-03-13  9:31   ` Kieran Bingham
2024-03-21 16:35   ` Laurent Pinchart
2024-03-13  7:24 ` [PATCH v8 19/38] media: ccs: Use {enable,disable}_streams operations Sakari Ailus
2024-03-21 16:21   ` Laurent Pinchart
2024-03-13  7:24 ` [PATCH v8 20/38] media: ccs: Track streaming state Sakari Ailus
2024-03-15 15:56   ` Julien Massot
2024-03-21 16:36   ` Laurent Pinchart
2024-03-13  7:24 ` [PATCH v8 21/38] media: ccs: Move ccs_validate_csi_data_format up Sakari Ailus
2024-03-15 15:57   ` Julien Massot
2024-03-21 16:37   ` Laurent Pinchart
2024-03-13  7:25 ` [PATCH v8 22/38] media: ccs: Support frame descriptors Sakari Ailus
2024-03-15 16:02   ` Julien Massot
2024-03-21 16:44   ` Laurent Pinchart
2024-04-11  8:33     ` Sakari Ailus
2024-03-13  7:25 ` [PATCH v8 23/38] media: uapi: ccs: Add media bus code for MIPI CCS embedded data Sakari Ailus
2024-03-15 16:03   ` Julien Massot
2024-03-21 16:49   ` Laurent Pinchart
2024-04-11  9:04     ` Sakari Ailus
2024-04-12 19:07       ` Laurent Pinchart
2024-04-14 10:48         ` Sakari Ailus
2024-04-20  8:07           ` Laurent Pinchart
2024-03-13  7:25 ` [PATCH v8 24/38] media: ccs: Add support for embedded data stream Sakari Ailus
2024-03-13  7:25 ` [PATCH v8 25/38] media: ccs: Remove ccs_get_crop_compose helper Sakari Ailus
2024-03-21 18:05   ` Laurent Pinchart
2024-04-16  7:30     ` Sakari Ailus
2024-03-13  7:25 ` [PATCH v8 26/38] media: ccs: Rely on sub-device state locking Sakari Ailus
2024-03-13  7:25 ` [PATCH v8 27/38] media: ccs: Compute binning configuration from sub-device state Sakari Ailus
2024-03-21 17:57   ` Laurent Pinchart
2024-04-16  8:01     ` Sakari Ailus
2024-03-13  7:25 ` [PATCH v8 28/38] media: ccs: Compute scaling " Sakari Ailus
2024-03-21 17:50   ` Laurent Pinchart
2024-04-16  7:59     ` Sakari Ailus
2024-03-13  7:25 ` [PATCH v8 29/38] media: ccs: Remove which parameter from ccs_propagate Sakari Ailus
2024-03-21 17:39   ` Laurent Pinchart
2024-03-13  7:25 ` [PATCH v8 30/38] media: Documentation: ccs: Document routing Sakari Ailus
2024-03-21 17:43   ` Laurent Pinchart
2024-04-16  7:37     ` Sakari Ailus
2024-03-13  7:25 ` [PATCH v8 31/38] media: uapi: v4l: subdev: Enable streams API Sakari Ailus
2024-03-21 16:51   ` Laurent Pinchart
2024-03-13  7:25 ` [PATCH v8 32/38] media: uapi: Add media bus code for ov2740 embedded data Sakari Ailus
2024-03-15 16:10   ` Julien Massot
2024-03-21 16:54   ` Laurent Pinchart
2024-03-13  7:25 ` [PATCH v8 33/38] media: ov2740: Switch to {enable,disable}_streams Sakari Ailus
2024-03-15 16:13   ` Julien Massot
2024-03-21 16:56   ` Laurent Pinchart
2024-03-13  7:25 ` [PATCH v8 34/38] media: ov2740: Track streaming state Sakari Ailus
2024-03-15 16:13   ` Julien Massot
2024-03-21 16:57   ` Laurent Pinchart
2024-03-13  7:25 ` [PATCH v8 35/38] media: ov2740: Add support for embedded data Sakari Ailus
2024-03-14  7:00   ` Bingbu Cao
2024-03-19 13:13     ` Sakari Ailus
2024-03-14  8:24   ` Julien Massot
2024-03-19 13:18     ` Sakari Ailus
2024-03-21 17:16   ` Laurent Pinchart
2024-04-10 13:18     ` Sakari Ailus
2024-03-13  7:25 ` [PATCH v8 36/38] media: v4l: Add V4L2_SUBDEV_ROUTE_FL_IMMUTABLE sub-device routing flag Sakari Ailus
2024-03-13  7:34   ` Tomi Valkeinen
2024-03-13  7:39     ` Sakari Ailus
2024-03-21 17:03       ` Laurent Pinchart
2024-04-09 13:21         ` Sakari Ailus
2024-04-09 15:21           ` Laurent Pinchart
2024-03-13  7:25 ` [PATCH v8 37/38] media: ccs: Add IMMUTABLE route flag Sakari Ailus
2024-03-15 16:08   ` Julien Massot
2024-03-21 16:59   ` Laurent Pinchart
2024-04-11  9:06     ` Sakari Ailus
2024-03-13  7:25 ` [PATCH v8 38/38] media: ov2740: " Sakari Ailus
2024-03-15 16:14   ` Julien Massot
2024-03-21 17:00   ` Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZhUihNnf6vQXWTyO@kekkonen.localdomain \
    --to=sakari.ailus@linux.intel.com \
    --cc=alain.volmat@foss.st.com \
    --cc=andrey.konovalov@linaro.org \
    --cc=bingbu.cao@intel.com \
    --cc=dmitry.perchanov@intel.com \
    --cc=hongju.wang@intel.com \
    --cc=hverkuil@xs4all.nl \
    --cc=jacopo.mondi@ideasonboard.com \
    --cc=khai.wen.ng@intel.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=tomi.valkeinen@ideasonboard.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).