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@xxxxxxxxxxxxxxx>
> > ---
> > .../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
