Re: [PATCH 1/1] media: Documentation: Interaction between routes, formats and selections

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hoi,

On 11/10/2022 13:47, Sakari Ailus wrote:
> Document how setting up routes interacts with formats and selections.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Moi,
> 
> This could be added on top of the set.
> 
> Comments are welcome.
> 
>   .../userspace-api/media/v4l/dev-subdev.rst    | 48 ++++++++++++++-----
>   1 file changed, 37 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> index 5075b1828b32d..830235eb01598 100644
> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> @@ -406,6 +406,8 @@ pixel array is not rectangular but cross-shaped or round. The maximum
>   size may also be smaller than the BOUNDS rectangle.
>   
>   
> +.. _format-propagation:
> +
>   Order of configuration and format propagation
>   ---------------------------------------------
>   
> @@ -507,12 +509,12 @@ source pads.
>   Streams, multiplexed media pads and internal routing
>   ----------------------------------------------------
>   
> -Commonly V4L2 subdevices support only separate video streams, that is, only a
> -single stream can pass through a media link and a media pad. Thus each pad
> -contains a format configuration for that single stream. In some cases a subdev
> -can do stream processing and split a stream into two or compose two streams
> -into one, but the inputs and outputs for the subdev are still a single stream
> -per pad.
> +Commonly V4L2 subdevices do not support multiple, unrelated video streams.

I wonder if we should use some other word than "commonly", as it seems 
pretty common nowadays that there is at least a embedded data. Should we 
instead say, e.g., "Simple V4L2 subdevices..."

> +Only a single stream can pass through a media link and a media

This could be a continuation of the previous sentence: "... video 
streams, and only a single stream..."

> +pad. Thus each pad contains a format and selection configuration for that single stream.
> +A subdev can do stream processing and split a stream into
> +two or compose two streams into one, but the inputs and outputs for the
> +subdev are still a single stream per pad.
>   
>   Some hardware, e.g. MIPI CSI-2, support multiplexed streams, that is, multiple
>   data streams are transmitted on the same bus, which is represented by a media
> @@ -539,15 +541,37 @@ streams from one end of the link to the other, and subdevices have routing
>   tables which describe how the incoming streams from sink pads are routed to the
>   source pads.
>   
> -A stream ID (often just "stream") is a media link-local identifier for a stream.
> +A stream ID is a media link-local identifier for a stream.
>   In other words, a particular stream ID must exist on both sides of a media
>   link, but another stream ID can be used for the same stream at the other side
> -of the subdevice.
> +of the subdevice. The same stream ID is used to refer to the stream on
> +both pads of the link on all ioctls operating on pads.
>   
>   A stream at a specific point in the media pipeline is identified with the
> -subdev and a (pad, stream) pair. For subdevices that do not support
> +subdev and a pad--stream pair. For subdevices that do not support

Is there a double-dash on purpose?

Btw, above you removed the "(often just "stream")", but here you use 
"stream" instead of "stream ID".

>   multiplexed streams the 'stream' is always 0.
>   
> +Interaction between routes, formats and selections
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The addition of routes to the V4L2 sub-device interface moves the
> +sub-device formats and selections from pads to pad--stream pairs. Instead

I guess it is, as you use it here also...

> +of the sub-device wide merging of streams from all source pads towards all
> +sink pads, this takes place separately for each route. The stream ID on
> +the sink pad for each configured route is used to configure format and
> +selections on the sink pad. Similarly, the stream ID on the source pad of
> +each configured route is used to configure format and selections on the
> +source pad.

Hmm, "stream ID is used to configure format" sounds odd to me. The ioctl 
is used to configure the format, but rather than using only pad ID to 
identify the configuration target, you now use pad ID - stream ID pair.

And is that just extra duplication above to talk separately about sink 
and source sides? They don't matter here, the point is just the 
pad-stream vs only pad.

> +
> +Any number of routes from streams on sink pads towards streams on source

This also sounds a bit odd to me: "a route from a stream on a sink pad". 
I think we're missing a word here which means the specific point of a 
stream in the pipeline.

I have suggested "stream endpoint", but it's not really an "end" point. 
"Stream waypoint"? So you would configure a format to a subdev's stream 
waypoint. And "Any number of routes from stream waypoints on sink pads 
towards stream waypoints on source pads".

So a "stream waypoint" would be a triplet of subdev-padID-streamID, or 
just padID-streamID if the subdev is obvious.

Even if we don't find a perfect or even a very good term for this, I 
think we should just use something. Using just "stream" makes things 
quite confusing, in my opinion.

> +pads is allowed, to the extent supported by drivers. For every stream on a
> +sink pad, however, only a single route is allowed.

Hmm, why is that?

> +
> +Pad--stream pairs are not static but are replaced by calls to the
> +:ref:`VIDIOC_SUBDEV_S_ROUTING <VIDIOC_SUBDEV_G_ROUTING>` ioctl. This also
> +applies to stream format and selection configurations which that are

Should that "applies to" be "affects the"?

> +reverted to driver defaults.
> +
>   Configuring streams
>   ^^^^^^^^^^^^^^^^^^^
>   
> @@ -565,8 +589,10 @@ Controller API <media_controller>`
>   setting the routing table will reset all the stream configurations in a media
>   entity.
>   
> -3) Configure streams. Each route endpoint must be configured

Oh, here I seem to use "route endpoint".

> -with :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>`.
> +3) Configure formats and selections on routes. Each route is configured

I'm not sure if we "configure formats on routes". Earlier I think we've 
talked about configuring the streams (stream waypoints).

> +separately as documented plain subdevices in :ref:`<format-propagation>`.

Is there something missing from above? Or drop "plain subdevices"?

> +The stream ID is set to the same stream ID used on sink and source pads of
> +the :ref:`VIDIOC_SUBDEV_S_ROUTING <VIDIOC_SUBDEV_G_ROUTING>` ioctl.

Yes. But I think this could be said more clearly if we have a good word 
for the padID-streamID pair.

  Tomi