Re: [PATCH v11 39/66] media: Documentation: Add subdev configuration models, raw sensor model

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Jacopo,

On Fri, Oct 03, 2025 at 09:10:06AM +0200, Jacopo Mondi wrote:
> Hi Sakari,
> 
> On Thu, Oct 02, 2025 at 09:22:32AM +0200, Jacopo Mondi wrote:
> > Hi Sakari
> >
> > On Thu, Oct 02, 2025 at 10:09:52AM +0300, Sakari Ailus wrote:
> > > Hi Jacopo,
> > >
> > > On Thu, Sep 25, 2025 at 12:31:09PM +0200, Jacopo Mondi wrote:
> > > > Hi Sakari
> > > >
> > > > On Fri, Sep 19, 2025 at 03:17:56PM +0300, Sakari Ailus wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > On Mon, Sep 01, 2025 at 07:09:29PM +0200, Jacopo Mondi wrote:
> > > > > > Hi Sakari
> > > > > >
> > > > > > On Mon, Aug 25, 2025 at 12:50:40PM +0300, Sakari Ailus wrote:
> > > > > > > Sub-device configuration models define what V4L2 API elements are
> > > > > > > available on a compliant sub-device and how do they behave.
> > > > > > >
> > > > > > > The patch also adds a model for common raw sensors.
> > > > > > >
> > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > > > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > > Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > > > > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > > ---
> > > > > > >  .../media/drivers/camera-sensor.rst           |   4 +
> > > > > > >  .../media/v4l/common-raw-sensor.dia           | 442 ++++++++++++++++++
> > > > > > >  .../media/v4l/common-raw-sensor.svg           | 134 ++++++
> > > > > > >  .../userspace-api/media/v4l/dev-subdev.rst    |   2 +
> > > > > > >  .../media/v4l/subdev-config-model.rst         | 230 +++++++++
> > > > > > >  5 files changed, 812 insertions(+)
> > > > > > >  create mode 100644 Documentation/userspace-api/media/v4l/common-raw-sensor.dia
> > > > > > >  create mode 100644 Documentation/userspace-api/media/v4l/common-raw-sensor.svg
> > > > > > >  create mode 100644 Documentation/userspace-api/media/v4l/subdev-config-model.rst
> > > > > > >
> > > > > > > diff --git a/Documentation/userspace-api/media/drivers/camera-sensor.rst b/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > > > > > > index cbbfbb0d8273..39f3f91c6733 100644
> > > > > > > --- a/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > > > > > > +++ b/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > > > > > > @@ -18,6 +18,8 @@ binning functionality. The sensor drivers belong to two distinct classes, freely
> > > > > > >  configurable and register list-based drivers, depending on how the driver
> > > > > > >  configures this functionality.
> > > > > > >
> > > > > > > +Also see :ref:`media_subdev_config_model_common_raw_sensor`.
> > > > > > > +
> > > > > > >  Freely configurable camera sensor drivers
> > > > > > >  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > >
> > > > > > > @@ -118,6 +120,8 @@ 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.
> > > > > > >
> > > > > > > +.. _media_using_camera_sensor_drivers_embedded_data:
> > > > > > > +
> > > > > > >  Embedded data
> > > > > > >  -------------
> > > > > > >
> > > > > > > diff --git a/Documentation/userspace-api/media/v4l/common-raw-sensor.dia b/Documentation/userspace-api/media/v4l/common-raw-sensor.dia
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..24b3f2b2a626
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/userspace-api/media/v4l/common-raw-sensor.dia
> > > > > >
> > > > > > [snip]
> > > > > >
> > > > > > > diff --git a/Documentation/userspace-api/media/v4l/common-raw-sensor.svg b/Documentation/userspace-api/media/v4l/common-raw-sensor.svg
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..1d6055da2519
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/userspace-api/media/v4l/common-raw-sensor.svg
> > > > > >
> > > > > > [snip]
> > > > > >
> > > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > index bb86cadfad1c..b0774b9a9b71 100644
> > > > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > @@ -846,3 +846,5 @@ stream while it may be possible to enable and disable the embedded data stream.
> > > > > > >
> > > > > > >  The embedded data format does not need to be configured on the sensor's pads as
> > > > > > >  the format is dictated by the pixel data format in this case.
> > > > > > > +
> > > > > > > +.. include:: subdev-config-model.rst
> > > > > > > diff --git a/Documentation/userspace-api/media/v4l/subdev-config-model.rst b/Documentation/userspace-api/media/v4l/subdev-config-model.rst
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..1e6c58931ea0
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/userspace-api/media/v4l/subdev-config-model.rst
> > > > > > > @@ -0,0 +1,230 @@
> > > > > > > +.. SPDX-License-Identifier: GPL-2.0 OR GFDL-1.1-no-invariants-or-later
> > > > > > > +
> > > > > > > +.. _media_subdev_config_model:
> > > > > > > +
> > > > > > > +Sub-device configuration models
> > > > > > > +===============================
> > > > > > > +
> > > > > > > +The V4L2 specification defines a subdev API that exposes three type of
> > > > > > > +configuration elements: formats, selection rectangles and controls. The
> > > > > > > +specification contains generic information about how those configuration
> > > > > > > +elements behave, but not precisely how they apply to particular hardware
> > > > > > > +features. We leave some leeway to drivers to decide how to map selection
> > > > > > > +rectangles to device features, as long as they comply with the V4L2
> > > > > > > +specification. This is needed as hardware features differ between devices, so
> > > > > > > +it's the driver's responsibility to handle this mapping.
> > > > > > > +
> > > > > > > +Unfortunately, this lack of clearly defined mapping in the specification has led
> > > > > > > +to different drivers mapping the same hardware features to different API
> > > > > > > +elements, or implementing the API elements with slightly different
> > > > > > > +behaviours. Furthermore, many drivers have implemented selection rectangles in
> > > > > > > +ways that do not comply with the V4L2 specification. All of this makes userspace
> > > > > > > +development difficult.
> > > > > > > +
> > > > > > > +Sub-device configuration models specify in detail what the user space can expect
> > > > > > > +from a sub-device in terms of V4L2 sub-device interface support, semantics
> > > > > > > +included.
> > > > > > > +
> > > > > > > +A sub-device may implement more than one configuration model at the same
> > > > > > > +time. The implemented configuration models can be obtained from the sub-device's
> > > > > > > +``V4L2_CID_CONFIG_MODEL`` control.
> > > > > > > +
> > > > > > > +.. _media_subdev_config_model_common_raw_sensor:
> > > > > > > +
> > > > > > > +Common raw camera sensor model
> > > > > > > +------------------------------
> > > > > > > +
> > > > > > > +The common raw camera sensor model defines a set of enumeration and
> > > > > > > +configuration interfaces (formats, selections etc.) that cover the vast majority
> > > > > > > +of functionality of raw camera sensors. Not all of the interfaces are
> > > > > > > +necessarily offered by all drivers.
> > > > > > > +
> > > > > > > +A sub-device complies with the common raw sensor model if the
> > > > > > > +``V4L2_CONFIG_MODEL_COMMON_RAW_SENSOR`` bit is set in the
> > > > > > > +``V4L2_CID_CONFIG_MODEL`` control of the sub-device.
> > > > > > > +
> > > > > > > +The common raw camera sensor model is aligned with
> > > > > > > +:ref:`media_using_camera_sensor_drivers`. Please refer to that regarding aspects
> > > > > > > +not specified here.
> > > > > > > +
> > > > > > > +Each camera sensor implementing the common raw sensor model exposes a single
> > > > > > > +V4L2 sub-device. The sub-device contains a single source pad (0) and two or more
> > > > > > > +internal pads: one or more image data internal pads (starting from 1) and
> > > > > > > +optionally an embedded data pad.
> > > > > > > +
> > > > > > > +Additionally, further internal pads may be supported for other features. Using
> > > > > > > +more than one image data internal pad or more than one non-image data pad
> > > > > > > +requires these pads documented separately for the given device. The indices of
> > > > > > > +the image data internal pads shall be lower than those of the non-image data
> > > > > > > +pads.
> > > > > > > +
> > > > > > > +This is shown in :ref:`media_subdev_config_model_common_raw_sensor_subdev`.
> > > > > >
> > > > > > possibly doesn't need a link as the image is just here below
> > > > > >
> > > > > > > +
> > > > > > > +.. _media_subdev_config_model_common_raw_sensor_subdev:
> > > > > > > +
> > > > > > > +.. kernel-figure:: common-raw-sensor.svg
> > > > > > > +    :alt:    common-raw-sensor.svg
> > > > > > > +    :align:  center
> > > > > > > +
> > > > > > > +    **Common raw sensor sub-device with n pads (n == 2)**
> > > > > > > +
> > > > > > > +Routes
> > > > > > > +^^^^^^
> > > > > > > +
> > > > > > > +A sub-device conforming to common raw camera sensor model implements the
> > > > > > > +following routes.
> > > > > > > +
> > > > > > > +.. flat-table:: Routes
> > > > > > > +    :header-rows: 1
> > > > > > > +
> > > > > > > +    * - Sink pad/stream
> > > > > > > +      - Source pad/stream
> > > > > > > +      - Static (X/M(aybe)/-)
> > > > > > > +      - Mandatory (X/-)
> > > > > > > +      - Synopsis
> > > > > > > +    * - 1/0
> > > > > > > +      - 0/0
> > > > > > > +      - X
> > > > > > > +      - X
> > > > > > > +      - Image data
> > > > > > > +    * - 2/0
> > > > > > > +      - 0/1
> > > > > > > +      - M
> > > > > > > +      - \-
> > > > > > > +      - Embedded data
> > > > > > > +
> > > > > > > +Support for the embedded data stream is optional. Drivers supporting the
> > > > > > > +embedded data stream may allow disabling and enabling the route when the
> > > > > > > +streaming is disabled.
> > > > > >
> > > > > > I would
> > > > > >
> > > > > > s/when the streaming is disabled//
> > > > >
> > > > > Sounds good.
> > > > >
> > > > > >
> > > > > > > +
> > > > > > > +Sensor pixel array size, cropping and binning
> > > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > > > +
> > > > > > > +The sensor's pixel array is divided into one or more areas. The areas around the
> > > > > > > +edge of the pixel array, usually one or more sides, may contain optical black
> > > > > >
> > > > > > You say that "the pixel array is divided into one or more areas" and
> > > > > > then list "the areas around the edge of the pixel array" which is confusing
> > > > > >
> > > > > > I think it would be better as
> > > > > >
> > > > > > The sensor's full pixel array is divided into one or more areas, one
> > > > > > (or multiple) active area which contains visible pixels surrounded,
> > > > > > usually on one or more sides, by non-active areas which may contain
> > > > > > optical black pixels, dummy pixels and other non-image pixels. The
> > > > > > entire pixel array areas size, including the active and non-active
> > > > > > portions is conveyed by the format on (pad, stream) pair 1/0.
> > > > > >
> > > > > > This would also better define the "visible pixels" term which is used
> > > > > > in the rest of the documentation.
> > > > >
> > > > > There indeed were issues in the terms used in the original text. How about:
> > > > >
> > > > > The sensor's pixel array is divided into one or more areas. The areas around the
> > > > > the visible area in the pixel array, usually one or more sides, may contain
> > > >
> > > > I still feel that "active area that contains visible pixels" better
> > > > defines what "visibile area" is... not a problem anyway
> > >
> > > How about "visible pixel area"?
> >
> > fine with me
> >
> 
> A recent discussion on libcamera made me wonder a few things
> 
> https://patchwork.libcamera.org/patch/24547/
> 
> In the current world (pre-RAW sensor model) the situation can be
> summarized as
> 
> TGT_NATIVE_SIZE = full pixel array (readable and non readable)
> TGT_CROP_BOUNDS = readable pixel array (visible and non visibile pixels)

Crop bounds is generally the same as native size.

> TGT_CROP_DEFAULT = visible pixels

The default could exclude not-so-great pixels, too.

> TGT_CROP = analgoue crop

This could include digital crop as well.

> 
> where:
> - visibile = pixels used for image capture purpose
> - non-visible = optically black, dummies etc
> 
> With the RAW sensor model:
> 
> format(1/0) = readable pixel array (visible and non visible)
> TGT_CROP_DEFAULT(1/0) = visible pixel area
> TGT_CROP(1/0) = analogue crop
> TGT_COMPOSE(1/0) = binning/skipping
> 
> Have we lost the ability to report the full pixel array size (readable
> and not readable) ? Is this intentional ? As if pixels cannot be read
> out they basically do no exist, and the information on the actual
> number of pixels (including non readable ones) should be kept
> somewhere else (like the libcamera sensor properties database) ?

I'd keep this information in the user space if needed. There's little
software could presumably do with this information.

> 
> All the discussion about readable/non-readable, visible/non-visibile
> and active and inactive areas make me think we would benefit from
> presenting a small glossary at the beginning of the "Sensor pixel
> array size, cropping and binning" paragraph ?

The text does not discuss active or inactive areas. I'd add some terms into
the main glossary if needed -- they are used outside this file, too.

-- 
Regards,

Sakari Ailus