[v14,19/34] media: Documentation: Add GS_ROUTING documentation

  From: Jacopo Mondi <jacopo+renesas@jmondi.org>

Add documentation for VIDIOC_SUBDEV_G/S_ROUTING ioctl and add
description of multiplexed media pads and internal routing to the
V4L2-subdev documentation section.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../userspace-api/media/v4l/dev-subdev.rst    |   2 +
 .../userspace-api/media/v4l/user-func.rst     |   1 +
 .../media/v4l/vidioc-subdev-g-routing.rst     | 150 ++++++++++++++++++
 3 files changed, 153 insertions(+)
 create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
  

Moi,

On Wed, Aug 31, 2022 at 05:13:42PM +0300, Tomi Valkeinen wrote:
> From: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> Add documentation for VIDIOC_SUBDEV_G/S_ROUTING ioctl and add
> description of multiplexed media pads and internal routing to the
> V4L2-subdev documentation section.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  .../userspace-api/media/v4l/dev-subdev.rst    |   2 +
>  .../userspace-api/media/v4l/user-func.rst     |   1 +
>  .../media/v4l/vidioc-subdev-g-routing.rst     | 150 ++++++++++++++++++
>  3 files changed, 153 insertions(+)
>  create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> 
> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> index fd1de0a73a9f..a67c2749089a 100644
> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> @@ -29,6 +29,8 @@ will feature a character device node on which ioctls can be called to
>  
>  -  negotiate image formats on individual pads
>  
> +-  inspect and modify internal data routing between pads of the same entity
> +
>  Sub-device character device nodes, conventionally named
>  ``/dev/v4l-subdev*``, use major number 81.
>  
> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
> index 53e604bd7d60..228c1521f190 100644
> --- a/Documentation/userspace-api/media/v4l/user-func.rst
> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
> @@ -70,6 +70,7 @@ Function Reference
>      vidioc-subdev-g-crop
>      vidioc-subdev-g-fmt
>      vidioc-subdev-g-frame-interval
> +    vidioc-subdev-g-routing
>      vidioc-subdev-g-selection
>      vidioc-subdev-querycap
>      vidioc-subscribe-event
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> new file mode 100644
> index 000000000000..a0d9c79e162f
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> @@ -0,0 +1,150 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +.. c:namespace:: V4L
> +
> +.. _VIDIOC_SUBDEV_G_ROUTING:
> +
> +******************************************************
> +ioctl VIDIOC_SUBDEV_G_ROUTING, VIDIOC_SUBDEV_S_ROUTING
> +******************************************************
> +
> +Name
> +====
> +
> +VIDIOC_SUBDEV_G_ROUTING - VIDIOC_SUBDEV_S_ROUTING - Get or set routing between streams of media pads in a media entity.
> +
> +
> +Synopsis
> +========
> +
> +.. c:function:: int ioctl( int fd, VIDIOC_SUBDEV_G_ROUTING, struct v4l2_subdev_routing *argp )
> +    :name: VIDIOC_SUBDEV_G_ROUTING
> +
> +.. c:function:: int ioctl( int fd, VIDIOC_SUBDEV_S_ROUTING, struct v4l2_subdev_routing *argp )
> +    :name: VIDIOC_SUBDEV_S_ROUTING
> +
> +
> +Arguments
> +=========
> +
> +``fd``
> +    File descriptor returned by :ref:`open() <func-open>`.
> +
> +``argp``
> +    Pointer to struct :c:type:`v4l2_subdev_routing`.
> +
> +
> +Description
> +===========
> +
> +These ioctls are used to get and set the routing in a media entity.
> +The routing configuration determines the flows of data inside an entity.
> +
> +Drivers report their current routing tables using the
> +``VIDIOC_SUBDEV_G_ROUTING`` ioctl and application may enable or disable routes
> +with the ``VIDIOC_SUBDEV_S_ROUTING`` ioctl, by adding or removing routes and
> +setting or clearing flags of the  ``flags`` field of a
> +struct :c:type:`v4l2_subdev_route`.

How about adding:

Routes that have V4L2_SUBDEV_ROUTE_FL_IMMUTABLE flag cannot be removed.
Depending on the driver, their V4L2_SUBDEV_ROUTE_FL_ACTIVE flag may be set
or reset.

Also see a comment later on.

> +
> +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This
> +means that the userspace mut reconfigure all streams after calling the ioctl
> +with e.g. ``VIDIOC_SUBDEV_S_FMT``.

How about this:

Calling ``VIDIOC_SUBDEV_S_ROUTING`` will cause the selections and subdev
formats being propagated from the sink pads towards the sources.

> +
> +A special case for routing are routes marked with
> +``V4L2_SUBDEV_ROUTE_FL_SOURCE`` flag. These routes are used to describe
> +source endpoints on sensors and the sink fields are unused.
> +
> +When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application
> +provided ``num_routes`` is not big enough to contain all the available routes
> +the subdevice exposes, drivers return the ENOSPC error code and adjust the
> +value of the ``num_routes`` field. Application should then reserve enough memory
> +for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again.
> +
> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
> +
> +.. c:type:: v4l2_subdev_routing
> +
> +.. flat-table:: struct v4l2_subdev_routing
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - __u32
> +      - ``which``
> +      - Format to modified, from enum
> +        :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
> +    * - struct :c:type:`v4l2_subdev_route`
> +      - ``routes[]``
> +      - Array of struct :c:type:`v4l2_subdev_route` entries
> +    * - __u32
> +      - ``num_routes``
> +      - Number of entries of the routes array
> +    * - __u32
> +      - ``reserved``\ [5]
> +      - Reserved for future extensions. Applications and drivers must set
> +	the array to zero.
> +
> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
> +
> +.. c:type:: v4l2_subdev_route
> +
> +.. flat-table:: struct v4l2_subdev_route
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - __u32
> +      - ``sink_pad``
> +      - Sink pad number.
> +    * - __u32
> +      - ``sink_stream``
> +      - Sink pad stream number.
> +    * - __u32
> +      - ``source_pad``
> +      - Source pad number.
> +    * - __u32
> +      - ``source_stream``
> +      - Source pad stream number.
> +    * - __u32
> +      - ``flags``
> +      - Route enable/disable flags
> +	:ref:`v4l2_subdev_routing_flags <v4l2-subdev-routing-flags>`.
> +    * - __u32
> +      - ``reserved``\ [5]
> +      - Reserved for future extensions. Applications and drivers must set
> +	the array to zero.
> +
> +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
> +
> +.. _v4l2-subdev-routing-flags:
> +
> +.. flat-table:: enum v4l2_subdev_routing_flags
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       3 1 4
> +
> +    * - V4L2_SUBDEV_ROUTE_FL_ACTIVE
> +      - 0
> +      - The route is enabled. Set by applications.
> +    * - V4L2_SUBDEV_ROUTE_FL_IMMUTABLE

How about calling this STATIC instead of IMMUTABLE? IMMUTABLE is used as a
link flag to mean a link that may not be changed in any way. In this case
we rather want to say that the route is always there, albeit you can still
enable or disable it.

> +      - 1
> +      - The route is immutable. Set by the driver.
> +    * - V4L2_SUBDEV_ROUTE_FL_SOURCE
> +      - 2
> +      - The route is a source route, and the ``sink_pad`` and ``sink_stream``
> +        fields are unused. Set by the driver.
> +
> +Return Value
> +============
> +
> +On success 0 is returned, on error -1 and the ``errno`` variable is set
> +appropriately. The generic error codes are described at the
> +:ref:`Generic Error Codes <gen-errors>` chapter.
> +
> +ENOSPC
> +   The number of provided route entries is less than the available ones.

What does "available ones" mean in this context? More than is supported?
Wouldn't E2BIG be the appropriate code in that case?

> +
> +EINVAL
> +   The sink or source pad identifiers reference a non-existing pad, or reference
> +   pads of different types (ie. the sink_pad identifiers refers to a source pad)
> +   or the sink or source stream identifiers reference a non-existing stream on
> +   the sink or source pad.
  

On 27/09/2022 08:59, Sakari Ailus wrote:
> Moi,
> 
> On Wed, Aug 31, 2022 at 05:13:42PM +0300, Tomi Valkeinen wrote:
>> From: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>
>> Add documentation for VIDIOC_SUBDEV_G/S_ROUTING ioctl and add
>> description of multiplexed media pads and internal routing to the
>> V4L2-subdev documentation section.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>   .../userspace-api/media/v4l/dev-subdev.rst    |   2 +
>>   .../userspace-api/media/v4l/user-func.rst     |   1 +
>>   .../media/v4l/vidioc-subdev-g-routing.rst     | 150 ++++++++++++++++++
>>   3 files changed, 153 insertions(+)
>>   create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
>>
>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>> index fd1de0a73a9f..a67c2749089a 100644
>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>> @@ -29,6 +29,8 @@ will feature a character device node on which ioctls can be called to
>>   
>>   -  negotiate image formats on individual pads
>>   
>> +-  inspect and modify internal data routing between pads of the same entity
>> +
>>   Sub-device character device nodes, conventionally named
>>   ``/dev/v4l-subdev*``, use major number 81.
>>   
>> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
>> index 53e604bd7d60..228c1521f190 100644
>> --- a/Documentation/userspace-api/media/v4l/user-func.rst
>> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
>> @@ -70,6 +70,7 @@ Function Reference
>>       vidioc-subdev-g-crop
>>       vidioc-subdev-g-fmt
>>       vidioc-subdev-g-frame-interval
>> +    vidioc-subdev-g-routing
>>       vidioc-subdev-g-selection
>>       vidioc-subdev-querycap
>>       vidioc-subscribe-event
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
>> new file mode 100644
>> index 000000000000..a0d9c79e162f
>> --- /dev/null
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
>> @@ -0,0 +1,150 @@
>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>> +.. c:namespace:: V4L
>> +
>> +.. _VIDIOC_SUBDEV_G_ROUTING:
>> +
>> +******************************************************
>> +ioctl VIDIOC_SUBDEV_G_ROUTING, VIDIOC_SUBDEV_S_ROUTING
>> +******************************************************
>> +
>> +Name
>> +====
>> +
>> +VIDIOC_SUBDEV_G_ROUTING - VIDIOC_SUBDEV_S_ROUTING - Get or set routing between streams of media pads in a media entity.
>> +
>> +
>> +Synopsis
>> +========
>> +
>> +.. c:function:: int ioctl( int fd, VIDIOC_SUBDEV_G_ROUTING, struct v4l2_subdev_routing *argp )
>> +    :name: VIDIOC_SUBDEV_G_ROUTING
>> +
>> +.. c:function:: int ioctl( int fd, VIDIOC_SUBDEV_S_ROUTING, struct v4l2_subdev_routing *argp )
>> +    :name: VIDIOC_SUBDEV_S_ROUTING
>> +
>> +
>> +Arguments
>> +=========
>> +
>> +``fd``
>> +    File descriptor returned by :ref:`open() <func-open>`.
>> +
>> +``argp``
>> +    Pointer to struct :c:type:`v4l2_subdev_routing`.
>> +
>> +
>> +Description
>> +===========
>> +
>> +These ioctls are used to get and set the routing in a media entity.
>> +The routing configuration determines the flows of data inside an entity.
>> +
>> +Drivers report their current routing tables using the
>> +``VIDIOC_SUBDEV_G_ROUTING`` ioctl and application may enable or disable routes
>> +with the ``VIDIOC_SUBDEV_S_ROUTING`` ioctl, by adding or removing routes and
>> +setting or clearing flags of the  ``flags`` field of a
>> +struct :c:type:`v4l2_subdev_route`.
> 
> How about adding:
> 
> Routes that have V4L2_SUBDEV_ROUTE_FL_IMMUTABLE flag cannot be removed.
> Depending on the driver, their V4L2_SUBDEV_ROUTE_FL_ACTIVE flag may be set
> or reset.

I have dropped the IMMUTABLE flag in my WIP branch, as I couldn't figure 
out a use for it. The only immutable routes are source routes, which are 
already special and there's no need for an extra flag.

> Also see a comment later on.
> 
>> +
>> +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This
>> +means that the userspace mut reconfigure all streams after calling the ioctl
>> +with e.g. ``VIDIOC_SUBDEV_S_FMT``.
> 
> How about this:
> 
> Calling ``VIDIOC_SUBDEV_S_ROUTING`` will cause the selections and subdev
> formats being propagated from the sink pads towards the sources.

Hmm, but that's not true. The selections and formats will be zeroed, 
unless the driver initializes them to a value. There's no propagation done.

>> +
>> +A special case for routing are routes marked with
>> +``V4L2_SUBDEV_ROUTE_FL_SOURCE`` flag. These routes are used to describe
>> +source endpoints on sensors and the sink fields are unused.
>> +
>> +When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application
>> +provided ``num_routes`` is not big enough to contain all the available routes
>> +the subdevice exposes, drivers return the ENOSPC error code and adjust the
>> +value of the ``num_routes`` field. Application should then reserve enough memory
>> +for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again.
>> +
>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
>> +
>> +.. c:type:: v4l2_subdev_routing
>> +
>> +.. flat-table:: struct v4l2_subdev_routing
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +    :widths:       1 1 2
>> +
>> +    * - __u32
>> +      - ``which``
>> +      - Format to modified, from enum
>> +        :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
>> +    * - struct :c:type:`v4l2_subdev_route`
>> +      - ``routes[]``
>> +      - Array of struct :c:type:`v4l2_subdev_route` entries
>> +    * - __u32
>> +      - ``num_routes``
>> +      - Number of entries of the routes array
>> +    * - __u32
>> +      - ``reserved``\ [5]
>> +      - Reserved for future extensions. Applications and drivers must set
>> +	the array to zero.
>> +
>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
>> +
>> +.. c:type:: v4l2_subdev_route
>> +
>> +.. flat-table:: struct v4l2_subdev_route
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +    :widths:       1 1 2
>> +
>> +    * - __u32
>> +      - ``sink_pad``
>> +      - Sink pad number.
>> +    * - __u32
>> +      - ``sink_stream``
>> +      - Sink pad stream number.
>> +    * - __u32
>> +      - ``source_pad``
>> +      - Source pad number.
>> +    * - __u32
>> +      - ``source_stream``
>> +      - Source pad stream number.
>> +    * - __u32
>> +      - ``flags``
>> +      - Route enable/disable flags
>> +	:ref:`v4l2_subdev_routing_flags <v4l2-subdev-routing-flags>`.
>> +    * - __u32
>> +      - ``reserved``\ [5]
>> +      - Reserved for future extensions. Applications and drivers must set
>> +	the array to zero.
>> +
>> +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
>> +
>> +.. _v4l2-subdev-routing-flags:
>> +
>> +.. flat-table:: enum v4l2_subdev_routing_flags
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +    :widths:       3 1 4
>> +
>> +    * - V4L2_SUBDEV_ROUTE_FL_ACTIVE
>> +      - 0
>> +      - The route is enabled. Set by applications.
>> +    * - V4L2_SUBDEV_ROUTE_FL_IMMUTABLE
> 
> How about calling this STATIC instead of IMMUTABLE? IMMUTABLE is used as a
> link flag to mean a link that may not be changed in any way. In this case
> we rather want to say that the route is always there, albeit you can still
> enable or disable it.

If we think there's a need for this, I can add it back and name it 
static. I think what it then should mean is that the user can 
enable/disable it and also set the stream id, but the route must always 
exist.

But as I said above, I haven't figured out a use for this.

>> +      - 1
>> +      - The route is immutable. Set by the driver.
>> +    * - V4L2_SUBDEV_ROUTE_FL_SOURCE
>> +      - 2
>> +      - The route is a source route, and the ``sink_pad`` and ``sink_stream``
>> +        fields are unused. Set by the driver.
>> +
>> +Return Value
>> +============
>> +
>> +On success 0 is returned, on error -1 and the ``errno`` variable is set
>> +appropriately. The generic error codes are described at the
>> +:ref:`Generic Error Codes <gen-errors>` chapter.
>> +
>> +ENOSPC
>> +   The number of provided route entries is less than the available ones.
> 
> What does "available ones" mean in this context? More than is supported?
> Wouldn't E2BIG be the appropriate code in that case?

Good question. I don't think I wrote this part =). ENOSPC refers to the 
case where VIDIOC_SUBDEV_G_ROUTING is called without enough space for 
the routing table. So "available ones" mean the routes in the subdev's 
routing table, and "provided route entries" refers to the userspace 
target routing table.

It sounds pretty odd, and obviously needs a clarification.

  Tomi
  

Heipparallaa Tomi,

On Tue, Sep 27, 2022 at 12:32:43PM +0300, Tomi Valkeinen wrote:
> On 27/09/2022 08:59, Sakari Ailus wrote:
> > Moi,
> > 
> > On Wed, Aug 31, 2022 at 05:13:42PM +0300, Tomi Valkeinen wrote:
> > > From: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > 
> > > Add documentation for VIDIOC_SUBDEV_G/S_ROUTING ioctl and add
> > > description of multiplexed media pads and internal routing to the
> > > V4L2-subdev documentation section.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > ---
> > >   .../userspace-api/media/v4l/dev-subdev.rst    |   2 +
> > >   .../userspace-api/media/v4l/user-func.rst     |   1 +
> > >   .../media/v4l/vidioc-subdev-g-routing.rst     | 150 ++++++++++++++++++
> > >   3 files changed, 153 insertions(+)
> > >   create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> > > 
> > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > index fd1de0a73a9f..a67c2749089a 100644
> > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > @@ -29,6 +29,8 @@ will feature a character device node on which ioctls can be called to
> > >   -  negotiate image formats on individual pads
> > > +-  inspect and modify internal data routing between pads of the same entity
> > > +
> > >   Sub-device character device nodes, conventionally named
> > >   ``/dev/v4l-subdev*``, use major number 81.
> > > diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
> > > index 53e604bd7d60..228c1521f190 100644
> > > --- a/Documentation/userspace-api/media/v4l/user-func.rst
> > > +++ b/Documentation/userspace-api/media/v4l/user-func.rst
> > > @@ -70,6 +70,7 @@ Function Reference
> > >       vidioc-subdev-g-crop
> > >       vidioc-subdev-g-fmt
> > >       vidioc-subdev-g-frame-interval
> > > +    vidioc-subdev-g-routing
> > >       vidioc-subdev-g-selection
> > >       vidioc-subdev-querycap
> > >       vidioc-subscribe-event
> > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> > > new file mode 100644
> > > index 000000000000..a0d9c79e162f
> > > --- /dev/null
> > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> > > @@ -0,0 +1,150 @@
> > > +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> > > +.. c:namespace:: V4L
> > > +
> > > +.. _VIDIOC_SUBDEV_G_ROUTING:
> > > +
> > > +******************************************************
> > > +ioctl VIDIOC_SUBDEV_G_ROUTING, VIDIOC_SUBDEV_S_ROUTING
> > > +******************************************************
> > > +
> > > +Name
> > > +====
> > > +
> > > +VIDIOC_SUBDEV_G_ROUTING - VIDIOC_SUBDEV_S_ROUTING - Get or set routing between streams of media pads in a media entity.
> > > +
> > > +
> > > +Synopsis
> > > +========
> > > +
> > > +.. c:function:: int ioctl( int fd, VIDIOC_SUBDEV_G_ROUTING, struct v4l2_subdev_routing *argp )
> > > +    :name: VIDIOC_SUBDEV_G_ROUTING
> > > +
> > > +.. c:function:: int ioctl( int fd, VIDIOC_SUBDEV_S_ROUTING, struct v4l2_subdev_routing *argp )
> > > +    :name: VIDIOC_SUBDEV_S_ROUTING
> > > +
> > > +
> > > +Arguments
> > > +=========
> > > +
> > > +``fd``
> > > +    File descriptor returned by :ref:`open() <func-open>`.
> > > +
> > > +``argp``
> > > +    Pointer to struct :c:type:`v4l2_subdev_routing`.
> > > +
> > > +
> > > +Description
> > > +===========
> > > +
> > > +These ioctls are used to get and set the routing in a media entity.
> > > +The routing configuration determines the flows of data inside an entity.
> > > +
> > > +Drivers report their current routing tables using the
> > > +``VIDIOC_SUBDEV_G_ROUTING`` ioctl and application may enable or disable routes
> > > +with the ``VIDIOC_SUBDEV_S_ROUTING`` ioctl, by adding or removing routes and
> > > +setting or clearing flags of the  ``flags`` field of a
> > > +struct :c:type:`v4l2_subdev_route`.
> > 
> > How about adding:
> > 
> > Routes that have V4L2_SUBDEV_ROUTE_FL_IMMUTABLE flag cannot be removed.
> > Depending on the driver, their V4L2_SUBDEV_ROUTE_FL_ACTIVE flag may be set
> > or reset.
> 
> I have dropped the IMMUTABLE flag in my WIP branch, as I couldn't figure out
> a use for it. The only immutable routes are source routes, which are already
> special and there's no need for an extra flag.

The two flags still mean different things... but let's continue the
discussion later in the message.

> 
> > Also see a comment later on.
> > 
> > > +
> > > +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This
> > > +means that the userspace mut reconfigure all streams after calling the ioctl
> > > +with e.g. ``VIDIOC_SUBDEV_S_FMT``.
> > 
> > How about this:
> > 
> > Calling ``VIDIOC_SUBDEV_S_ROUTING`` will cause the selections and subdev
> > formats being propagated from the sink pads towards the sources.
> 
> Hmm, but that's not true. The selections and formats will be zeroed, unless
> the driver initializes them to a value. There's no propagation done.

They need to be propagated. The driver is responsible for maintaining a
valid configuration for the processing steps in a sub-device, and with
routes that must apply to routes as well.

> 
> > > +
> > > +A special case for routing are routes marked with
> > > +``V4L2_SUBDEV_ROUTE_FL_SOURCE`` flag. These routes are used to describe
> > > +source endpoints on sensors and the sink fields are unused.
> > > +
> > > +When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application
> > > +provided ``num_routes`` is not big enough to contain all the available routes
> > > +the subdevice exposes, drivers return the ENOSPC error code and adjust the
> > > +value of the ``num_routes`` field. Application should then reserve enough memory
> > > +for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again.
> > > +
> > > +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
> > > +
> > > +.. c:type:: v4l2_subdev_routing
> > > +
> > > +.. flat-table:: struct v4l2_subdev_routing
> > > +    :header-rows:  0
> > > +    :stub-columns: 0
> > > +    :widths:       1 1 2
> > > +
> > > +    * - __u32
> > > +      - ``which``
> > > +      - Format to modified, from enum
> > > +        :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
> > > +    * - struct :c:type:`v4l2_subdev_route`
> > > +      - ``routes[]``
> > > +      - Array of struct :c:type:`v4l2_subdev_route` entries
> > > +    * - __u32
> > > +      - ``num_routes``
> > > +      - Number of entries of the routes array
> > > +    * - __u32
> > > +      - ``reserved``\ [5]
> > > +      - Reserved for future extensions. Applications and drivers must set
> > > +	the array to zero.
> > > +
> > > +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
> > > +
> > > +.. c:type:: v4l2_subdev_route
> > > +
> > > +.. flat-table:: struct v4l2_subdev_route
> > > +    :header-rows:  0
> > > +    :stub-columns: 0
> > > +    :widths:       1 1 2
> > > +
> > > +    * - __u32
> > > +      - ``sink_pad``
> > > +      - Sink pad number.
> > > +    * - __u32
> > > +      - ``sink_stream``
> > > +      - Sink pad stream number.
> > > +    * - __u32
> > > +      - ``source_pad``
> > > +      - Source pad number.
> > > +    * - __u32
> > > +      - ``source_stream``
> > > +      - Source pad stream number.
> > > +    * - __u32
> > > +      - ``flags``
> > > +      - Route enable/disable flags
> > > +	:ref:`v4l2_subdev_routing_flags <v4l2-subdev-routing-flags>`.
> > > +    * - __u32
> > > +      - ``reserved``\ [5]
> > > +      - Reserved for future extensions. Applications and drivers must set
> > > +	the array to zero.
> > > +
> > > +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
> > > +
> > > +.. _v4l2-subdev-routing-flags:
> > > +
> > > +.. flat-table:: enum v4l2_subdev_routing_flags
> > > +    :header-rows:  0
> > > +    :stub-columns: 0
> > > +    :widths:       3 1 4
> > > +
> > > +    * - V4L2_SUBDEV_ROUTE_FL_ACTIVE
> > > +      - 0
> > > +      - The route is enabled. Set by applications.
> > > +    * - V4L2_SUBDEV_ROUTE_FL_IMMUTABLE
> > 
> > How about calling this STATIC instead of IMMUTABLE? IMMUTABLE is used as a
> > link flag to mean a link that may not be changed in any way. In this case
> > we rather want to say that the route is always there, albeit you can still
> > enable or disable it.
> 
> If we think there's a need for this, I can add it back and name it static. I
> think what it then should mean is that the user can enable/disable it and
> also set the stream id, but the route must always exist.

But the static routes are recognised by the stream ID only, aren't they?

I think we'll definitely need a way to determine which routes are always
there and which ones can be removed at will.

> 
> But as I said above, I haven't figured out a use for this.
> 
> > > +      - 1
> > > +      - The route is immutable. Set by the driver.
> > > +    * - V4L2_SUBDEV_ROUTE_FL_SOURCE
> > > +      - 2
> > > +      - The route is a source route, and the ``sink_pad`` and ``sink_stream``
> > > +        fields are unused. Set by the driver.
> > > +
> > > +Return Value
> > > +============
> > > +
> > > +On success 0 is returned, on error -1 and the ``errno`` variable is set
> > > +appropriately. The generic error codes are described at the
> > > +:ref:`Generic Error Codes <gen-errors>` chapter.
> > > +
> > > +ENOSPC
> > > +   The number of provided route entries is less than the available ones.
> > 
> > What does "available ones" mean in this context? More than is supported?
> > Wouldn't E2BIG be the appropriate code in that case?
> 
> Good question. I don't think I wrote this part =). ENOSPC refers to the case
> where VIDIOC_SUBDEV_G_ROUTING is called without enough space for the routing
> table. So "available ones" mean the routes in the subdev's routing table,
> and "provided route entries" refers to the userspace target routing table.
> 
> It sounds pretty odd, and obviously needs a clarification.

I think I actually can think what this did mean. It means that the
num_routes is smaller than the number of routes in a routing table when
G_ROUTING is called. For that I think ENOSPC is the right code actually.

But also I think we need to document what happens when there are too many
routes. For that E2BIG would be appropriate.

Should we have a field for telling which route was the bad one, if it was
one of them? That can be done later, too, if we'll ever need it.
  

On 27/09/2022 13:23, Sakari Ailus wrote:

<snip>

>>>> +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This
>>>> +means that the userspace mut reconfigure all streams after calling the ioctl
>>>> +with e.g. ``VIDIOC_SUBDEV_S_FMT``.
>>>
>>> How about this:
>>>
>>> Calling ``VIDIOC_SUBDEV_S_ROUTING`` will cause the selections and subdev
>>> formats being propagated from the sink pads towards the sources.
>>
>> Hmm, but that's not true. The selections and formats will be zeroed, unless
>> the driver initializes them to a value. There's no propagation done.
> 
> They need to be propagated. The driver is responsible for maintaining a
> valid configuration for the processing steps in a sub-device, and with
> routes that must apply to routes as well.

Hmm, no, they don't need to be propagated. The driver needs to 
initialize the formats and selections to valid configuration, that is 
true, but it doesn't mean the driver propagates settings from the sink 
pads to the source pads. In theory the formats on sink and source sides 
could be different.

I guess you could say that the driver initializes the sink side streams 
and then propagates those to the source side, but at least to me that 
gives the wrong impression. What the driver does is initialize the 
configuration, or reset the configuration to defaults (which it could do 
with propagation if it so wants).

The framework provides v4l2_subdev_set_routing_with_fmt() helper to 
achieve this, which sets the given format to all streams.

Anyway, I think we can talk about propagation if that helps, but I think 
the main point there is that the settings are reset or initialized.

>>>> +
>>>> +A special case for routing are routes marked with
>>>> +``V4L2_SUBDEV_ROUTE_FL_SOURCE`` flag. These routes are used to describe
>>>> +source endpoints on sensors and the sink fields are unused.
>>>> +
>>>> +When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application
>>>> +provided ``num_routes`` is not big enough to contain all the available routes
>>>> +the subdevice exposes, drivers return the ENOSPC error code and adjust the
>>>> +value of the ``num_routes`` field. Application should then reserve enough memory
>>>> +for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again.
>>>> +
>>>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
>>>> +
>>>> +.. c:type:: v4l2_subdev_routing
>>>> +
>>>> +.. flat-table:: struct v4l2_subdev_routing
>>>> +    :header-rows:  0
>>>> +    :stub-columns: 0
>>>> +    :widths:       1 1 2
>>>> +
>>>> +    * - __u32
>>>> +      - ``which``
>>>> +      - Format to modified, from enum
>>>> +        :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
>>>> +    * - struct :c:type:`v4l2_subdev_route`
>>>> +      - ``routes[]``
>>>> +      - Array of struct :c:type:`v4l2_subdev_route` entries
>>>> +    * - __u32
>>>> +      - ``num_routes``
>>>> +      - Number of entries of the routes array
>>>> +    * - __u32
>>>> +      - ``reserved``\ [5]
>>>> +      - Reserved for future extensions. Applications and drivers must set
>>>> +	the array to zero.
>>>> +
>>>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
>>>> +
>>>> +.. c:type:: v4l2_subdev_route
>>>> +
>>>> +.. flat-table:: struct v4l2_subdev_route
>>>> +    :header-rows:  0
>>>> +    :stub-columns: 0
>>>> +    :widths:       1 1 2
>>>> +
>>>> +    * - __u32
>>>> +      - ``sink_pad``
>>>> +      - Sink pad number.
>>>> +    * - __u32
>>>> +      - ``sink_stream``
>>>> +      - Sink pad stream number.
>>>> +    * - __u32
>>>> +      - ``source_pad``
>>>> +      - Source pad number.
>>>> +    * - __u32
>>>> +      - ``source_stream``
>>>> +      - Source pad stream number.
>>>> +    * - __u32
>>>> +      - ``flags``
>>>> +      - Route enable/disable flags
>>>> +	:ref:`v4l2_subdev_routing_flags <v4l2-subdev-routing-flags>`.
>>>> +    * - __u32
>>>> +      - ``reserved``\ [5]
>>>> +      - Reserved for future extensions. Applications and drivers must set
>>>> +	the array to zero.
>>>> +
>>>> +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
>>>> +
>>>> +.. _v4l2-subdev-routing-flags:
>>>> +
>>>> +.. flat-table:: enum v4l2_subdev_routing_flags
>>>> +    :header-rows:  0
>>>> +    :stub-columns: 0
>>>> +    :widths:       3 1 4
>>>> +
>>>> +    * - V4L2_SUBDEV_ROUTE_FL_ACTIVE
>>>> +      - 0
>>>> +      - The route is enabled. Set by applications.
>>>> +    * - V4L2_SUBDEV_ROUTE_FL_IMMUTABLE
>>>
>>> How about calling this STATIC instead of IMMUTABLE? IMMUTABLE is used as a
>>> link flag to mean a link that may not be changed in any way. In this case
>>> we rather want to say that the route is always there, albeit you can still
>>> enable or disable it.
>>
>> If we think there's a need for this, I can add it back and name it static. I
>> think what it then should mean is that the user can enable/disable it and
>> also set the stream id, but the route must always exist.
> 
> But the static routes are recognised by the stream ID only, aren't they?
> 
> I think we'll definitely need a way to determine which routes are always
> there and which ones can be removed at will.

That's the V4L2_SUBDEV_ROUTE_FL_SOURCE. V4L2_SUBDEV_ROUTE_FL_SOURCE 
routes are always there, and the user can only enable or disable them.

This is why I dropped IMMUTABLE, as SOURCE is already immutable (or 
rather, static), and I don't see a need for a non-source route to be static.

>>
>> But as I said above, I haven't figured out a use for this.
>>
>>>> +      - 1
>>>> +      - The route is immutable. Set by the driver.
>>>> +    * - V4L2_SUBDEV_ROUTE_FL_SOURCE
>>>> +      - 2
>>>> +      - The route is a source route, and the ``sink_pad`` and ``sink_stream``
>>>> +        fields are unused. Set by the driver.
>>>> +
>>>> +Return Value
>>>> +============
>>>> +
>>>> +On success 0 is returned, on error -1 and the ``errno`` variable is set
>>>> +appropriately. The generic error codes are described at the
>>>> +:ref:`Generic Error Codes <gen-errors>` chapter.
>>>> +
>>>> +ENOSPC
>>>> +   The number of provided route entries is less than the available ones.
>>>
>>> What does "available ones" mean in this context? More than is supported?
>>> Wouldn't E2BIG be the appropriate code in that case?
>>
>> Good question. I don't think I wrote this part =). ENOSPC refers to the case
>> where VIDIOC_SUBDEV_G_ROUTING is called without enough space for the routing
>> table. So "available ones" mean the routes in the subdev's routing table,
>> and "provided route entries" refers to the userspace target routing table.
>>
>> It sounds pretty odd, and obviously needs a clarification.
> 
> I think I actually can think what this did mean. It means that the
> num_routes is smaller than the number of routes in a routing table when
> G_ROUTING is called. For that I think ENOSPC is the right code actually.
> 
> But also I think we need to document what happens when there are too many
> routes. For that E2BIG would be appropriate.

v4l2-ioctl.c returns EINVAL if there are over 256 routes. The drivers 
should, of course, do additional check if needed. In v4l2-ioctl.c it 
seems common to return EINVAL if there's too much data, but we can of 
course define E2BIG for routing ioctls.

> Should we have a field for telling which route was the bad one, if it was
> one of them? That can be done later, too, if we'll ever need it.

Hmm maybe, although I wonder how often the drivers can say that this 
particular route is the problem, and what would the userspace do with 
that information...

Do you have any examples in mind?

  Tomi
  

Moi,

On Tue, Sep 27, 2022 at 03:33:15PM +0300, Tomi Valkeinen wrote:
> On 27/09/2022 13:23, Sakari Ailus wrote:
> 
> <snip>
> 
> > > > > +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This
> > > > > +means that the userspace mut reconfigure all streams after calling the ioctl
> > > > > +with e.g. ``VIDIOC_SUBDEV_S_FMT``.
> > > > 
> > > > How about this:
> > > > 
> > > > Calling ``VIDIOC_SUBDEV_S_ROUTING`` will cause the selections and subdev
> > > > formats being propagated from the sink pads towards the sources.
> > > 
> > > Hmm, but that's not true. The selections and formats will be zeroed, unless
> > > the driver initializes them to a value. There's no propagation done.
> > 
> > They need to be propagated. The driver is responsible for maintaining a
> > valid configuration for the processing steps in a sub-device, and with
> > routes that must apply to routes as well.
> 
> Hmm, no, they don't need to be propagated. The driver needs to initialize
> the formats and selections to valid configuration, that is true, but it
> doesn't mean the driver propagates settings from the sink pads to the source
> pads. In theory the formats on sink and source sides could be different.

After propagation, the user may set the format (or selection) later on in
the processing steps. The propagation is required by the spec and I don't
see why it would be different for drivers with support for streams. Of
course this needs to take place taking hardware limitations into account.

> 
> I guess you could say that the driver initializes the sink side streams and
> then propagates those to the source side, but at least to me that gives the
> wrong impression. What the driver does is initialize the configuration, or
> reset the configuration to defaults (which it could do with propagation if
> it so wants).
> 
> The framework provides v4l2_subdev_set_routing_with_fmt() helper to achieve
> this, which sets the given format to all streams.
> 
> Anyway, I think we can talk about propagation if that helps, but I think the
> main point there is that the settings are reset or initialized.
> 
> > > > > +
> > > > > +A special case for routing are routes marked with
> > > > > +``V4L2_SUBDEV_ROUTE_FL_SOURCE`` flag. These routes are used to describe
> > > > > +source endpoints on sensors and the sink fields are unused.
> > > > > +
> > > > > +When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application
> > > > > +provided ``num_routes`` is not big enough to contain all the available routes
> > > > > +the subdevice exposes, drivers return the ENOSPC error code and adjust the
> > > > > +value of the ``num_routes`` field. Application should then reserve enough memory
> > > > > +for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again.
> > > > > +
> > > > > +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
> > > > > +
> > > > > +.. c:type:: v4l2_subdev_routing
> > > > > +
> > > > > +.. flat-table:: struct v4l2_subdev_routing
> > > > > +    :header-rows:  0
> > > > > +    :stub-columns: 0
> > > > > +    :widths:       1 1 2
> > > > > +
> > > > > +    * - __u32
> > > > > +      - ``which``
> > > > > +      - Format to modified, from enum
> > > > > +        :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
> > > > > +    * - struct :c:type:`v4l2_subdev_route`
> > > > > +      - ``routes[]``
> > > > > +      - Array of struct :c:type:`v4l2_subdev_route` entries
> > > > > +    * - __u32
> > > > > +      - ``num_routes``
> > > > > +      - Number of entries of the routes array
> > > > > +    * - __u32
> > > > > +      - ``reserved``\ [5]
> > > > > +      - Reserved for future extensions. Applications and drivers must set
> > > > > +	the array to zero.
> > > > > +
> > > > > +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
> > > > > +
> > > > > +.. c:type:: v4l2_subdev_route
> > > > > +
> > > > > +.. flat-table:: struct v4l2_subdev_route
> > > > > +    :header-rows:  0
> > > > > +    :stub-columns: 0
> > > > > +    :widths:       1 1 2
> > > > > +
> > > > > +    * - __u32
> > > > > +      - ``sink_pad``
> > > > > +      - Sink pad number.
> > > > > +    * - __u32
> > > > > +      - ``sink_stream``
> > > > > +      - Sink pad stream number.
> > > > > +    * - __u32
> > > > > +      - ``source_pad``
> > > > > +      - Source pad number.
> > > > > +    * - __u32
> > > > > +      - ``source_stream``
> > > > > +      - Source pad stream number.
> > > > > +    * - __u32
> > > > > +      - ``flags``
> > > > > +      - Route enable/disable flags
> > > > > +	:ref:`v4l2_subdev_routing_flags <v4l2-subdev-routing-flags>`.
> > > > > +    * - __u32
> > > > > +      - ``reserved``\ [5]
> > > > > +      - Reserved for future extensions. Applications and drivers must set
> > > > > +	the array to zero.
> > > > > +
> > > > > +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
> > > > > +
> > > > > +.. _v4l2-subdev-routing-flags:
> > > > > +
> > > > > +.. flat-table:: enum v4l2_subdev_routing_flags
> > > > > +    :header-rows:  0
> > > > > +    :stub-columns: 0
> > > > > +    :widths:       3 1 4
> > > > > +
> > > > > +    * - V4L2_SUBDEV_ROUTE_FL_ACTIVE
> > > > > +      - 0
> > > > > +      - The route is enabled. Set by applications.
> > > > > +    * - V4L2_SUBDEV_ROUTE_FL_IMMUTABLE
> > > > 
> > > > How about calling this STATIC instead of IMMUTABLE? IMMUTABLE is used as a
> > > > link flag to mean a link that may not be changed in any way. In this case
> > > > we rather want to say that the route is always there, albeit you can still
> > > > enable or disable it.
> > > 
> > > If we think there's a need for this, I can add it back and name it static. I
> > > think what it then should mean is that the user can enable/disable it and
> > > also set the stream id, but the route must always exist.
> > 
> > But the static routes are recognised by the stream ID only, aren't they?
> > 
> > I think we'll definitely need a way to determine which routes are always
> > there and which ones can be removed at will.
> 
> That's the V4L2_SUBDEV_ROUTE_FL_SOURCE. V4L2_SUBDEV_ROUTE_FL_SOURCE routes
> are always there, and the user can only enable or disable them.
> 
> This is why I dropped IMMUTABLE, as SOURCE is already immutable (or rather,
> static), and I don't see a need for a non-source route to be static.

We don't know of such a device at the moment but I'm fairly certain they
exist. A number of older CSI-2 receivers do not support directing different
VC/DT pairs to different buffers in any generic sort of way.

Using one flag for two different purposes may thus prove problematic over
time. I'd thus define another for the other purpose. In the worst case it
won't be needed and we can make it obsolete later on.

> 
> > > 
> > > But as I said above, I haven't figured out a use for this.
> > > 
> > > > > +      - 1
> > > > > +      - The route is immutable. Set by the driver.
> > > > > +    * - V4L2_SUBDEV_ROUTE_FL_SOURCE
> > > > > +      - 2
> > > > > +      - The route is a source route, and the ``sink_pad`` and ``sink_stream``
> > > > > +        fields are unused. Set by the driver.
> > > > > +
> > > > > +Return Value
> > > > > +============
> > > > > +
> > > > > +On success 0 is returned, on error -1 and the ``errno`` variable is set
> > > > > +appropriately. The generic error codes are described at the
> > > > > +:ref:`Generic Error Codes <gen-errors>` chapter.
> > > > > +
> > > > > +ENOSPC
> > > > > +   The number of provided route entries is less than the available ones.
> > > > 
> > > > What does "available ones" mean in this context? More than is supported?
> > > > Wouldn't E2BIG be the appropriate code in that case?
> > > 
> > > Good question. I don't think I wrote this part =). ENOSPC refers to the case
> > > where VIDIOC_SUBDEV_G_ROUTING is called without enough space for the routing
> > > table. So "available ones" mean the routes in the subdev's routing table,
> > > and "provided route entries" refers to the userspace target routing table.
> > > 
> > > It sounds pretty odd, and obviously needs a clarification.
> > 
> > I think I actually can think what this did mean. It means that the
> > num_routes is smaller than the number of routes in a routing table when
> > G_ROUTING is called. For that I think ENOSPC is the right code actually.
> > 
> > But also I think we need to document what happens when there are too many
> > routes. For that E2BIG would be appropriate.
> 
> v4l2-ioctl.c returns EINVAL if there are over 256 routes. The drivers
> should, of course, do additional check if needed. In v4l2-ioctl.c it seems
> common to return EINVAL if there's too much data, but we can of course
> define E2BIG for routing ioctls.

The number (256) is just the current limit. I don't expect more though.

But the user space could know the number is too large if we have a proper
error code for it. Up to you. However at least documentation needs to be
amended since this case remains undocumented.

> 
> > Should we have a field for telling which route was the bad one, if it was
> > one of them? That can be done later, too, if we'll ever need it.
> 
> Hmm maybe, although I wonder how often the drivers can say that this
> particular route is the problem, and what would the userspace do with that
> information...
> 
> Do you have any examples in mind?

I think it would be mainly useful for debugging purposes, software as such
probably wouldn't need it. Say, if you have a problem somewhere in your,
say, 256 routes, it could be hard to figure out which one of them is the
faulty one.
  

Hi,

On 28/09/2022 00:13, Sakari Ailus wrote:
> Moi,
> 
> On Tue, Sep 27, 2022 at 03:33:15PM +0300, Tomi Valkeinen wrote:
>> On 27/09/2022 13:23, Sakari Ailus wrote:
>>
>> <snip>
>>
>>>>>> +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This
>>>>>> +means that the userspace mut reconfigure all streams after calling the ioctl
>>>>>> +with e.g. ``VIDIOC_SUBDEV_S_FMT``.
>>>>>
>>>>> How about this:
>>>>>
>>>>> Calling ``VIDIOC_SUBDEV_S_ROUTING`` will cause the selections and subdev
>>>>> formats being propagated from the sink pads towards the sources.
>>>>
>>>> Hmm, but that's not true. The selections and formats will be zeroed, unless
>>>> the driver initializes them to a value. There's no propagation done.
>>>
>>> They need to be propagated. The driver is responsible for maintaining a
>>> valid configuration for the processing steps in a sub-device, and with
>>> routes that must apply to routes as well.
>>
>> Hmm, no, they don't need to be propagated. The driver needs to initialize
>> the formats and selections to valid configuration, that is true, but it
>> doesn't mean the driver propagates settings from the sink pads to the source
>> pads. In theory the formats on sink and source sides could be different.
> 
> After propagation, the user may set the format (or selection) later on in
> the processing steps. The propagation is required by the spec and I don't
> see why it would be different for drivers with support for streams. Of
> course this needs to take place taking hardware limitations into account.

I don't disagree with the above, but I still don't see why it matters here.

So do you suggest replacing the current paragraph with your version? Or 
adding a new paragraph?

Why is propagation important here? Isn't the resetting of the 
configuration the important thing?

For me, the propagation concept is important in the case where the user 
configures the sink pads, as then the settings need to be propagated. 
Here it doesn't matter.

>> I guess you could say that the driver initializes the sink side streams and
>> then propagates those to the source side, but at least to me that gives the
>> wrong impression. What the driver does is initialize the configuration, or
>> reset the configuration to defaults (which it could do with propagation if
>> it so wants).
>>
>> The framework provides v4l2_subdev_set_routing_with_fmt() helper to achieve
>> this, which sets the given format to all streams.
>>
>> Anyway, I think we can talk about propagation if that helps, but I think the
>> main point there is that the settings are reset or initialized.
>>
>>>>>> +
>>>>>> +A special case for routing are routes marked with
>>>>>> +``V4L2_SUBDEV_ROUTE_FL_SOURCE`` flag. These routes are used to describe
>>>>>> +source endpoints on sensors and the sink fields are unused.
>>>>>> +
>>>>>> +When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application
>>>>>> +provided ``num_routes`` is not big enough to contain all the available routes
>>>>>> +the subdevice exposes, drivers return the ENOSPC error code and adjust the
>>>>>> +value of the ``num_routes`` field. Application should then reserve enough memory
>>>>>> +for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again.
>>>>>> +
>>>>>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
>>>>>> +
>>>>>> +.. c:type:: v4l2_subdev_routing
>>>>>> +
>>>>>> +.. flat-table:: struct v4l2_subdev_routing
>>>>>> +    :header-rows:  0
>>>>>> +    :stub-columns: 0
>>>>>> +    :widths:       1 1 2
>>>>>> +
>>>>>> +    * - __u32
>>>>>> +      - ``which``
>>>>>> +      - Format to modified, from enum
>>>>>> +        :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
>>>>>> +    * - struct :c:type:`v4l2_subdev_route`
>>>>>> +      - ``routes[]``
>>>>>> +      - Array of struct :c:type:`v4l2_subdev_route` entries
>>>>>> +    * - __u32
>>>>>> +      - ``num_routes``
>>>>>> +      - Number of entries of the routes array
>>>>>> +    * - __u32
>>>>>> +      - ``reserved``\ [5]
>>>>>> +      - Reserved for future extensions. Applications and drivers must set
>>>>>> +	the array to zero.
>>>>>> +
>>>>>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
>>>>>> +
>>>>>> +.. c:type:: v4l2_subdev_route
>>>>>> +
>>>>>> +.. flat-table:: struct v4l2_subdev_route
>>>>>> +    :header-rows:  0
>>>>>> +    :stub-columns: 0
>>>>>> +    :widths:       1 1 2
>>>>>> +
>>>>>> +    * - __u32
>>>>>> +      - ``sink_pad``
>>>>>> +      - Sink pad number.
>>>>>> +    * - __u32
>>>>>> +      - ``sink_stream``
>>>>>> +      - Sink pad stream number.
>>>>>> +    * - __u32
>>>>>> +      - ``source_pad``
>>>>>> +      - Source pad number.
>>>>>> +    * - __u32
>>>>>> +      - ``source_stream``
>>>>>> +      - Source pad stream number.
>>>>>> +    * - __u32
>>>>>> +      - ``flags``
>>>>>> +      - Route enable/disable flags
>>>>>> +	:ref:`v4l2_subdev_routing_flags <v4l2-subdev-routing-flags>`.
>>>>>> +    * - __u32
>>>>>> +      - ``reserved``\ [5]
>>>>>> +      - Reserved for future extensions. Applications and drivers must set
>>>>>> +	the array to zero.
>>>>>> +
>>>>>> +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
>>>>>> +
>>>>>> +.. _v4l2-subdev-routing-flags:
>>>>>> +
>>>>>> +.. flat-table:: enum v4l2_subdev_routing_flags
>>>>>> +    :header-rows:  0
>>>>>> +    :stub-columns: 0
>>>>>> +    :widths:       3 1 4
>>>>>> +
>>>>>> +    * - V4L2_SUBDEV_ROUTE_FL_ACTIVE
>>>>>> +      - 0
>>>>>> +      - The route is enabled. Set by applications.
>>>>>> +    * - V4L2_SUBDEV_ROUTE_FL_IMMUTABLE
>>>>>
>>>>> How about calling this STATIC instead of IMMUTABLE? IMMUTABLE is used as a
>>>>> link flag to mean a link that may not be changed in any way. In this case
>>>>> we rather want to say that the route is always there, albeit you can still
>>>>> enable or disable it.
>>>>
>>>> If we think there's a need for this, I can add it back and name it static. I
>>>> think what it then should mean is that the user can enable/disable it and
>>>> also set the stream id, but the route must always exist.
>>>
>>> But the static routes are recognised by the stream ID only, aren't they?
>>>
>>> I think we'll definitely need a way to determine which routes are always
>>> there and which ones can be removed at will.
>>
>> That's the V4L2_SUBDEV_ROUTE_FL_SOURCE. V4L2_SUBDEV_ROUTE_FL_SOURCE routes
>> are always there, and the user can only enable or disable them.
>>
>> This is why I dropped IMMUTABLE, as SOURCE is already immutable (or rather,
>> static), and I don't see a need for a non-source route to be static.
> 
> We don't know of such a device at the moment but I'm fairly certain they
> exist. A number of older CSI-2 receivers do not support directing different
> VC/DT pairs to different buffers in any generic sort of way.

That is true, but the problem there is that the driver often does not 
know the number of streams.

For example, if we have a CSI-2 bridge which, say, has a single input 
and two output pads. It routes VC0 & 2 to the first output and VC1 & 3 
to the second output.

Here it would be easy to say that there are 4 static streams, going as 
described above. But that's not true, as data-types also define streams, 
so we can actually have a lot more streams there.

This, I think, essentially means that static routes can never be defined 
for any subdevice in the middle of the pipeline. The only places where 
we can have static routes are the very beginning and very end of the 
pipeline. For the beginning, i.e. the sensors, we already have source 
streams. But can we have static routes on the end side, basically just 
before the DMA?

If we have a CSI-2 receiver that has a hardcoded handling of the VC & 
DT, how does the userspace configure the routes? The userspace doesn't 
see the VCs or DTs. We could have static routes defined in the receiver 
subdevice, but does that help?

The HW I use, TI's CAL, has the means to configure VC/DT freely. But it 
has 8 DMA engines, and, of course, each stream has to go to a single DMA 
engine. So I think we could say that it has 8 static streams, and the 
user can only enable or disable them. But I'm not sure how much adding a 
new flag for this helps.

> Using one flag for two different purposes may thus prove problematic over
> time. I'd thus define another for the other purpose. In the worst case it
> won't be needed and we can make it obsolete later on.

I'd like to have a clear example of a setup where we need this flag and 
benefit from it before adding it.

In the CAL case I don't see much benefit. I think the only thing it 
gives us is a minimal discovery mechanism for the userspace to 
understand how CAL routes can be configured. I say minimal, as it still 
won't cover it fully as the validity of the routing depends on the 
actual VCs and DTs too (which will be found out only at the stream start 
time).

And this would only give us discovery for the receiver and wouldn't help 
with the bridges.

>>>>
>>>> But as I said above, I haven't figured out a use for this.
>>>>
>>>>>> +      - 1
>>>>>> +      - The route is immutable. Set by the driver.
>>>>>> +    * - V4L2_SUBDEV_ROUTE_FL_SOURCE
>>>>>> +      - 2
>>>>>> +      - The route is a source route, and the ``sink_pad`` and ``sink_stream``
>>>>>> +        fields are unused. Set by the driver.
>>>>>> +
>>>>>> +Return Value
>>>>>> +============
>>>>>> +
>>>>>> +On success 0 is returned, on error -1 and the ``errno`` variable is set
>>>>>> +appropriately. The generic error codes are described at the
>>>>>> +:ref:`Generic Error Codes <gen-errors>` chapter.
>>>>>> +
>>>>>> +ENOSPC
>>>>>> +   The number of provided route entries is less than the available ones.
>>>>>
>>>>> What does "available ones" mean in this context? More than is supported?
>>>>> Wouldn't E2BIG be the appropriate code in that case?
>>>>
>>>> Good question. I don't think I wrote this part =). ENOSPC refers to the case
>>>> where VIDIOC_SUBDEV_G_ROUTING is called without enough space for the routing
>>>> table. So "available ones" mean the routes in the subdev's routing table,
>>>> and "provided route entries" refers to the userspace target routing table.
>>>>
>>>> It sounds pretty odd, and obviously needs a clarification.
>>>
>>> I think I actually can think what this did mean. It means that the
>>> num_routes is smaller than the number of routes in a routing table when
>>> G_ROUTING is called. For that I think ENOSPC is the right code actually.
>>>
>>> But also I think we need to document what happens when there are too many
>>> routes. For that E2BIG would be appropriate.
>>
>> v4l2-ioctl.c returns EINVAL if there are over 256 routes. The drivers
>> should, of course, do additional check if needed. In v4l2-ioctl.c it seems
>> common to return EINVAL if there's too much data, but we can of course
>> define E2BIG for routing ioctls.
> 
> The number (256) is just the current limit. I don't expect more though.
> 
> But the user space could know the number is too large if we have a proper
> error code for it. Up to you. However at least documentation needs to be
> amended since this case remains undocumented.

I can change the returned error from EINVAL to E2BIG and document it. 
But everything else in check_array_args return EINVAL, so it would be 
going into different direction.

>>> Should we have a field for telling which route was the bad one, if it was
>>> one of them? That can be done later, too, if we'll ever need it.
>>
>> Hmm maybe, although I wonder how often the drivers can say that this
>> particular route is the problem, and what would the userspace do with that
>> information...
>>
>> Do you have any examples in mind?
> 
> I think it would be mainly useful for debugging purposes, software as such
> probably wouldn't need it. Say, if you have a problem somewhere in your,
> say, 256 routes, it could be hard to figure out which one of them is the
> faulty one.

Maybe... I'm a bit cautious about this, as I think it may be often 
difficult to return a good "bad route" number, and for the user do any 
proper debugging based only on it.

Similar to the pipeline validation, you need to turn on the kernel 
debugs to see where things go wrong. Which is, of course, bad as a 
normal user cannot do that, but I think for real debugging aid we'd need 
to return more than just a single number.

I wonder if something like that would work... A videodev/subdev file 
specific buffer for error strings (with a max size), which the user 
could fetch if it gets an error.

Or perhaps not. Maybe the people who work on v4l2 level things also have 
the means to enable kernel debugs.

  Tomi
  

Moi,

On Wed, Sep 28, 2022 at 10:54:44AM +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On 28/09/2022 00:13, Sakari Ailus wrote:
> > Moi,
> > 
> > On Tue, Sep 27, 2022 at 03:33:15PM +0300, Tomi Valkeinen wrote:
> > > On 27/09/2022 13:23, Sakari Ailus wrote:
> > > 
> > > <snip>
> > > 
> > > > > > > +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This
> > > > > > > +means that the userspace mut reconfigure all streams after calling the ioctl
> > > > > > > +with e.g. ``VIDIOC_SUBDEV_S_FMT``.
> > > > > > 
> > > > > > How about this:
> > > > > > 
> > > > > > Calling ``VIDIOC_SUBDEV_S_ROUTING`` will cause the selections and subdev
> > > > > > formats being propagated from the sink pads towards the sources.
> > > > > 
> > > > > Hmm, but that's not true. The selections and formats will be zeroed, unless
> > > > > the driver initializes them to a value. There's no propagation done.
> > > > 
> > > > They need to be propagated. The driver is responsible for maintaining a
> > > > valid configuration for the processing steps in a sub-device, and with
> > > > routes that must apply to routes as well.
> > > 
> > > Hmm, no, they don't need to be propagated. The driver needs to initialize
> > > the formats and selections to valid configuration, that is true, but it
> > > doesn't mean the driver propagates settings from the sink pads to the source
> > > pads. In theory the formats on sink and source sides could be different.
> > 
> > After propagation, the user may set the format (or selection) later on in
> > the processing steps. The propagation is required by the spec and I don't
> > see why it would be different for drivers with support for streams. Of
> > course this needs to take place taking hardware limitations into account.
> 
> I don't disagree with the above, but I still don't see why it matters here.

It does. The user needs to be able to rely on the ability of the driver to
maintain valid internal configuration. That user generally has less
information on this than the driver.

> 
> So do you suggest replacing the current paragraph with your version? Or
> adding a new paragraph?

I'm preparing some more documentation related to interaction between
routing, formats and selections. I can include this as well.

> 
> Why is propagation important here? Isn't the resetting of the configuration
> the important thing?
> 
> For me, the propagation concept is important in the case where the user
> configures the sink pads, as then the settings need to be propagated. Here
> it doesn't matter.

In general, if the user has configured something, it should not be changed
by configuring independent albeit somewhat related things. I'm saying that
the routing configuration shouldn't be an exception to how the interface
works in general.

> 
> > > I guess you could say that the driver initializes the sink side streams and
> > > then propagates those to the source side, but at least to me that gives the
> > > wrong impression. What the driver does is initialize the configuration, or
> > > reset the configuration to defaults (which it could do with propagation if
> > > it so wants).
> > > 
> > > The framework provides v4l2_subdev_set_routing_with_fmt() helper to achieve
> > > this, which sets the given format to all streams.
> > > 
> > > Anyway, I think we can talk about propagation if that helps, but I think the
> > > main point there is that the settings are reset or initialized.
> > > 
> > > > > > > +
> > > > > > > +A special case for routing are routes marked with
> > > > > > > +``V4L2_SUBDEV_ROUTE_FL_SOURCE`` flag. These routes are used to describe
> > > > > > > +source endpoints on sensors and the sink fields are unused.
> > > > > > > +
> > > > > > > +When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application
> > > > > > > +provided ``num_routes`` is not big enough to contain all the available routes
> > > > > > > +the subdevice exposes, drivers return the ENOSPC error code and adjust the
> > > > > > > +value of the ``num_routes`` field. Application should then reserve enough memory
> > > > > > > +for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again.
> > > > > > > +
> > > > > > > +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
> > > > > > > +
> > > > > > > +.. c:type:: v4l2_subdev_routing
> > > > > > > +
> > > > > > > +.. flat-table:: struct v4l2_subdev_routing
> > > > > > > +    :header-rows:  0
> > > > > > > +    :stub-columns: 0
> > > > > > > +    :widths:       1 1 2
> > > > > > > +
> > > > > > > +    * - __u32
> > > > > > > +      - ``which``
> > > > > > > +      - Format to modified, from enum
> > > > > > > +        :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
> > > > > > > +    * - struct :c:type:`v4l2_subdev_route`
> > > > > > > +      - ``routes[]``
> > > > > > > +      - Array of struct :c:type:`v4l2_subdev_route` entries
> > > > > > > +    * - __u32
> > > > > > > +      - ``num_routes``
> > > > > > > +      - Number of entries of the routes array
> > > > > > > +    * - __u32
> > > > > > > +      - ``reserved``\ [5]
> > > > > > > +      - Reserved for future extensions. Applications and drivers must set
> > > > > > > +	the array to zero.
> > > > > > > +
> > > > > > > +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
> > > > > > > +
> > > > > > > +.. c:type:: v4l2_subdev_route
> > > > > > > +
> > > > > > > +.. flat-table:: struct v4l2_subdev_route
> > > > > > > +    :header-rows:  0
> > > > > > > +    :stub-columns: 0
> > > > > > > +    :widths:       1 1 2
> > > > > > > +
> > > > > > > +    * - __u32
> > > > > > > +      - ``sink_pad``
> > > > > > > +      - Sink pad number.
> > > > > > > +    * - __u32
> > > > > > > +      - ``sink_stream``
> > > > > > > +      - Sink pad stream number.
> > > > > > > +    * - __u32
> > > > > > > +      - ``source_pad``
> > > > > > > +      - Source pad number.
> > > > > > > +    * - __u32
> > > > > > > +      - ``source_stream``
> > > > > > > +      - Source pad stream number.
> > > > > > > +    * - __u32
> > > > > > > +      - ``flags``
> > > > > > > +      - Route enable/disable flags
> > > > > > > +	:ref:`v4l2_subdev_routing_flags <v4l2-subdev-routing-flags>`.
> > > > > > > +    * - __u32
> > > > > > > +      - ``reserved``\ [5]
> > > > > > > +      - Reserved for future extensions. Applications and drivers must set
> > > > > > > +	the array to zero.
> > > > > > > +
> > > > > > > +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
> > > > > > > +
> > > > > > > +.. _v4l2-subdev-routing-flags:
> > > > > > > +
> > > > > > > +.. flat-table:: enum v4l2_subdev_routing_flags
> > > > > > > +    :header-rows:  0
> > > > > > > +    :stub-columns: 0
> > > > > > > +    :widths:       3 1 4
> > > > > > > +
> > > > > > > +    * - V4L2_SUBDEV_ROUTE_FL_ACTIVE
> > > > > > > +      - 0
> > > > > > > +      - The route is enabled. Set by applications.
> > > > > > > +    * - V4L2_SUBDEV_ROUTE_FL_IMMUTABLE
> > > > > > 
> > > > > > How about calling this STATIC instead of IMMUTABLE? IMMUTABLE is used as a
> > > > > > link flag to mean a link that may not be changed in any way. In this case
> > > > > > we rather want to say that the route is always there, albeit you can still
> > > > > > enable or disable it.
> > > > > 
> > > > > If we think there's a need for this, I can add it back and name it static. I
> > > > > think what it then should mean is that the user can enable/disable it and
> > > > > also set the stream id, but the route must always exist.
> > > > 
> > > > But the static routes are recognised by the stream ID only, aren't they?
> > > > 
> > > > I think we'll definitely need a way to determine which routes are always
> > > > there and which ones can be removed at will.
> > > 
> > > That's the V4L2_SUBDEV_ROUTE_FL_SOURCE. V4L2_SUBDEV_ROUTE_FL_SOURCE routes
> > > are always there, and the user can only enable or disable them.
> > > 
> > > This is why I dropped IMMUTABLE, as SOURCE is already immutable (or rather,
> > > static), and I don't see a need for a non-source route to be static.
> > 
> > We don't know of such a device at the moment but I'm fairly certain they
> > exist. A number of older CSI-2 receivers do not support directing different
> > VC/DT pairs to different buffers in any generic sort of way.
> 
> That is true, but the problem there is that the driver often does not know
> the number of streams.
> 
> For example, if we have a CSI-2 bridge which, say, has a single input and
> two output pads. It routes VC0 & 2 to the first output and VC1 & 3 to the
> second output.
> 
> Here it would be easy to say that there are 4 static streams, going as
> described above. But that's not true, as data-types also define streams, so
> we can actually have a lot more streams there.
> 
> This, I think, essentially means that static routes can never be defined for
> any subdevice in the middle of the pipeline. The only places where we can
> have static routes are the very beginning and very end of the pipeline. For
> the beginning, i.e. the sensors, we already have source streams. But can we
> have static routes on the end side, basically just before the DMA?

If you can have a single route only, isn't that effectively static?
Although in that case the sub-device should not support routing.

> 
> If we have a CSI-2 receiver that has a hardcoded handling of the VC & DT,
> how does the userspace configure the routes? The userspace doesn't see the
> VCs or DTs. We could have static routes defined in the receiver subdevice,
> but does that help?

Good point. I think not.

I guess we would then leave the routes for the user to create and driver to
try to configure the hardware accordingly or fail in link validation?

Perhaps we won't need a static route flag then after all.

> 
> The HW I use, TI's CAL, has the means to configure VC/DT freely. But it has
> 8 DMA engines, and, of course, each stream has to go to a single DMA engine.
> So I think we could say that it has 8 static streams, and the user can only
> enable or disable them. But I'm not sure how much adding a new flag for this
> helps.

Could this be limited by only allowing to create eight routes?

> 
> > Using one flag for two different purposes may thus prove problematic over
> > time. I'd thus define another for the other purpose. In the worst case it
> > won't be needed and we can make it obsolete later on.
> 
> I'd like to have a clear example of a setup where we need this flag and
> benefit from it before adding it.
> 
> In the CAL case I don't see much benefit. I think the only thing it gives us
> is a minimal discovery mechanism for the userspace to understand how CAL
> routes can be configured. I say minimal, as it still won't cover it fully as
> the validity of the routing depends on the actual VCs and DTs too (which
> will be found out only at the stream start time).
> 
> And this would only give us discovery for the receiver and wouldn't help
> with the bridges.
> 
> > > > > 
> > > > > But as I said above, I haven't figured out a use for this.
> > > > > 
> > > > > > > +      - 1
> > > > > > > +      - The route is immutable. Set by the driver.
> > > > > > > +    * - V4L2_SUBDEV_ROUTE_FL_SOURCE
> > > > > > > +      - 2
> > > > > > > +      - The route is a source route, and the ``sink_pad`` and ``sink_stream``
> > > > > > > +        fields are unused. Set by the driver.
> > > > > > > +
> > > > > > > +Return Value
> > > > > > > +============
> > > > > > > +
> > > > > > > +On success 0 is returned, on error -1 and the ``errno`` variable is set
> > > > > > > +appropriately. The generic error codes are described at the
> > > > > > > +:ref:`Generic Error Codes <gen-errors>` chapter.
> > > > > > > +
> > > > > > > +ENOSPC
> > > > > > > +   The number of provided route entries is less than the available ones.
> > > > > > 
> > > > > > What does "available ones" mean in this context? More than is supported?
> > > > > > Wouldn't E2BIG be the appropriate code in that case?
> > > > > 
> > > > > Good question. I don't think I wrote this part =). ENOSPC refers to the case
> > > > > where VIDIOC_SUBDEV_G_ROUTING is called without enough space for the routing
> > > > > table. So "available ones" mean the routes in the subdev's routing table,
> > > > > and "provided route entries" refers to the userspace target routing table.
> > > > > 
> > > > > It sounds pretty odd, and obviously needs a clarification.
> > > > 
> > > > I think I actually can think what this did mean. It means that the
> > > > num_routes is smaller than the number of routes in a routing table when
> > > > G_ROUTING is called. For that I think ENOSPC is the right code actually.
> > > > 
> > > > But also I think we need to document what happens when there are too many
> > > > routes. For that E2BIG would be appropriate.
> > > 
> > > v4l2-ioctl.c returns EINVAL if there are over 256 routes. The drivers
> > > should, of course, do additional check if needed. In v4l2-ioctl.c it seems
> > > common to return EINVAL if there's too much data, but we can of course
> > > define E2BIG for routing ioctls.
> > 
> > The number (256) is just the current limit. I don't expect more though.
> > 
> > But the user space could know the number is too large if we have a proper
> > error code for it. Up to you. However at least documentation needs to be
> > amended since this case remains undocumented.
> 
> I can change the returned error from EINVAL to E2BIG and document it. But
> everything else in check_array_args return EINVAL, so it would be going into
> different direction.

Could this be beneficial in telling the user too many routes have been
configured (as I wrote above)?

> 
> > > > Should we have a field for telling which route was the bad one, if it was
> > > > one of them? That can be done later, too, if we'll ever need it.
> > > 
> > > Hmm maybe, although I wonder how often the drivers can say that this
> > > particular route is the problem, and what would the userspace do with that
> > > information...
> > > 
> > > Do you have any examples in mind?
> > 
> > I think it would be mainly useful for debugging purposes, software as such
> > probably wouldn't need it. Say, if you have a problem somewhere in your,
> > say, 256 routes, it could be hard to figure out which one of them is the
> > faulty one.
> 
> Maybe... I'm a bit cautious about this, as I think it may be often difficult
> to return a good "bad route" number, and for the user do any proper
> debugging based only on it.
> 
> Similar to the pipeline validation, you need to turn on the kernel debugs to
> see where things go wrong. Which is, of course, bad as a normal user cannot
> do that, but I think for real debugging aid we'd need to return more than
> just a single number.
> 
> I wonder if something like that would work... A videodev/subdev file
> specific buffer for error strings (with a max size), which the user could
> fetch if it gets an error.
> 
> Or perhaps not. Maybe the people who work on v4l2 level things also have the
> means to enable kernel debugs.

That's the other way. I haven't checked if there are appropriate debug
prints there currently but it's always easy to add them later.

This is something that could be also added later if needed. I'm not
requesting it to be added now, as I wrote earlier.
  

Hei,

On 30/09/2022 14:21, Sakari Ailus wrote:
> Moi,
> 
> On Wed, Sep 28, 2022 at 10:54:44AM +0300, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 28/09/2022 00:13, Sakari Ailus wrote:
>>> Moi,
>>>
>>> On Tue, Sep 27, 2022 at 03:33:15PM +0300, Tomi Valkeinen wrote:
>>>> On 27/09/2022 13:23, Sakari Ailus wrote:
>>>>
>>>> <snip>
>>>>
>>>>>>>> +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This
>>>>>>>> +means that the userspace mut reconfigure all streams after calling the ioctl
>>>>>>>> +with e.g. ``VIDIOC_SUBDEV_S_FMT``.
>>>>>>>
>>>>>>> How about this:
>>>>>>>
>>>>>>> Calling ``VIDIOC_SUBDEV_S_ROUTING`` will cause the selections and subdev
>>>>>>> formats being propagated from the sink pads towards the sources.
>>>>>>
>>>>>> Hmm, but that's not true. The selections and formats will be zeroed, unless
>>>>>> the driver initializes them to a value. There's no propagation done.
>>>>>
>>>>> They need to be propagated. The driver is responsible for maintaining a
>>>>> valid configuration for the processing steps in a sub-device, and with
>>>>> routes that must apply to routes as well.
>>>>
>>>> Hmm, no, they don't need to be propagated. The driver needs to initialize
>>>> the formats and selections to valid configuration, that is true, but it
>>>> doesn't mean the driver propagates settings from the sink pads to the source
>>>> pads. In theory the formats on sink and source sides could be different.
>>>
>>> After propagation, the user may set the format (or selection) later on in
>>> the processing steps. The propagation is required by the spec and I don't
>>> see why it would be different for drivers with support for streams. Of
>>> course this needs to take place taking hardware limitations into account.
>>
>> I don't disagree with the above, but I still don't see why it matters here.
> 
> It does. The user needs to be able to rely on the ability of the driver to
> maintain valid internal configuration. That user generally has less
> information on this than the driver.

I had a short chat with Sakari, and it seems there's no real issue here, 
mostly just a misunderstanding.

The action point is to clarify what "reset" means (it means resetting to 
driver default values, keeping the subdev configuration valid). And 
perhaps also highlighting somewhere that when e.g. setting the formats, 
propagation happens with subdevices using routes too.

>> So do you suggest replacing the current paragraph with your version? Or
>> adding a new paragraph?
> 
> I'm preparing some more documentation related to interaction between
> routing, formats and selections. I can include this as well.
> 
>>
>> Why is propagation important here? Isn't the resetting of the configuration
>> the important thing?
>>
>> For me, the propagation concept is important in the case where the user
>> configures the sink pads, as then the settings need to be propagated. Here
>> it doesn't matter.
> 
> In general, if the user has configured something, it should not be changed
> by configuring independent albeit somewhat related things. I'm saying that
> the routing configuration shouldn't be an exception to how the interface
> works in general.
> 
>>
>>>> I guess you could say that the driver initializes the sink side streams and
>>>> then propagates those to the source side, but at least to me that gives the
>>>> wrong impression. What the driver does is initialize the configuration, or
>>>> reset the configuration to defaults (which it could do with propagation if
>>>> it so wants).
>>>>
>>>> The framework provides v4l2_subdev_set_routing_with_fmt() helper to achieve
>>>> this, which sets the given format to all streams.
>>>>
>>>> Anyway, I think we can talk about propagation if that helps, but I think the
>>>> main point there is that the settings are reset or initialized.
>>>>
>>>>>>>> +
>>>>>>>> +A special case for routing are routes marked with
>>>>>>>> +``V4L2_SUBDEV_ROUTE_FL_SOURCE`` flag. These routes are used to describe
>>>>>>>> +source endpoints on sensors and the sink fields are unused.
>>>>>>>> +
>>>>>>>> +When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application
>>>>>>>> +provided ``num_routes`` is not big enough to contain all the available routes
>>>>>>>> +the subdevice exposes, drivers return the ENOSPC error code and adjust the
>>>>>>>> +value of the ``num_routes`` field. Application should then reserve enough memory
>>>>>>>> +for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again.
>>>>>>>> +
>>>>>>>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
>>>>>>>> +
>>>>>>>> +.. c:type:: v4l2_subdev_routing
>>>>>>>> +
>>>>>>>> +.. flat-table:: struct v4l2_subdev_routing
>>>>>>>> +    :header-rows:  0
>>>>>>>> +    :stub-columns: 0
>>>>>>>> +    :widths:       1 1 2
>>>>>>>> +
>>>>>>>> +    * - __u32
>>>>>>>> +      - ``which``
>>>>>>>> +      - Format to modified, from enum
>>>>>>>> +        :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
>>>>>>>> +    * - struct :c:type:`v4l2_subdev_route`
>>>>>>>> +      - ``routes[]``
>>>>>>>> +      - Array of struct :c:type:`v4l2_subdev_route` entries
>>>>>>>> +    * - __u32
>>>>>>>> +      - ``num_routes``
>>>>>>>> +      - Number of entries of the routes array
>>>>>>>> +    * - __u32
>>>>>>>> +      - ``reserved``\ [5]
>>>>>>>> +      - Reserved for future extensions. Applications and drivers must set
>>>>>>>> +	the array to zero.
>>>>>>>> +
>>>>>>>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
>>>>>>>> +
>>>>>>>> +.. c:type:: v4l2_subdev_route
>>>>>>>> +
>>>>>>>> +.. flat-table:: struct v4l2_subdev_route
>>>>>>>> +    :header-rows:  0
>>>>>>>> +    :stub-columns: 0
>>>>>>>> +    :widths:       1 1 2
>>>>>>>> +
>>>>>>>> +    * - __u32
>>>>>>>> +      - ``sink_pad``
>>>>>>>> +      - Sink pad number.
>>>>>>>> +    * - __u32
>>>>>>>> +      - ``sink_stream``
>>>>>>>> +      - Sink pad stream number.
>>>>>>>> +    * - __u32
>>>>>>>> +      - ``source_pad``
>>>>>>>> +      - Source pad number.
>>>>>>>> +    * - __u32
>>>>>>>> +      - ``source_stream``
>>>>>>>> +      - Source pad stream number.
>>>>>>>> +    * - __u32
>>>>>>>> +      - ``flags``
>>>>>>>> +      - Route enable/disable flags
>>>>>>>> +	:ref:`v4l2_subdev_routing_flags <v4l2-subdev-routing-flags>`.
>>>>>>>> +    * - __u32
>>>>>>>> +      - ``reserved``\ [5]
>>>>>>>> +      - Reserved for future extensions. Applications and drivers must set
>>>>>>>> +	the array to zero.
>>>>>>>> +
>>>>>>>> +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
>>>>>>>> +
>>>>>>>> +.. _v4l2-subdev-routing-flags:
>>>>>>>> +
>>>>>>>> +.. flat-table:: enum v4l2_subdev_routing_flags
>>>>>>>> +    :header-rows:  0
>>>>>>>> +    :stub-columns: 0
>>>>>>>> +    :widths:       3 1 4
>>>>>>>> +
>>>>>>>> +    * - V4L2_SUBDEV_ROUTE_FL_ACTIVE
>>>>>>>> +      - 0
>>>>>>>> +      - The route is enabled. Set by applications.
>>>>>>>> +    * - V4L2_SUBDEV_ROUTE_FL_IMMUTABLE
>>>>>>>
>>>>>>> How about calling this STATIC instead of IMMUTABLE? IMMUTABLE is used as a
>>>>>>> link flag to mean a link that may not be changed in any way. In this case
>>>>>>> we rather want to say that the route is always there, albeit you can still
>>>>>>> enable or disable it.
>>>>>>
>>>>>> If we think there's a need for this, I can add it back and name it static. I
>>>>>> think what it then should mean is that the user can enable/disable it and
>>>>>> also set the stream id, but the route must always exist.
>>>>>
>>>>> But the static routes are recognised by the stream ID only, aren't they?
>>>>>
>>>>> I think we'll definitely need a way to determine which routes are always
>>>>> there and which ones can be removed at will.
>>>>
>>>> That's the V4L2_SUBDEV_ROUTE_FL_SOURCE. V4L2_SUBDEV_ROUTE_FL_SOURCE routes
>>>> are always there, and the user can only enable or disable them.
>>>>
>>>> This is why I dropped IMMUTABLE, as SOURCE is already immutable (or rather,
>>>> static), and I don't see a need for a non-source route to be static.
>>>
>>> We don't know of such a device at the moment but I'm fairly certain they
>>> exist. A number of older CSI-2 receivers do not support directing different
>>> VC/DT pairs to different buffers in any generic sort of way.
>>
>> That is true, but the problem there is that the driver often does not know
>> the number of streams.
>>
>> For example, if we have a CSI-2 bridge which, say, has a single input and
>> two output pads. It routes VC0 & 2 to the first output and VC1 & 3 to the
>> second output.
>>
>> Here it would be easy to say that there are 4 static streams, going as
>> described above. But that's not true, as data-types also define streams, so
>> we can actually have a lot more streams there.
>>
>> This, I think, essentially means that static routes can never be defined for
>> any subdevice in the middle of the pipeline. The only places where we can
>> have static routes are the very beginning and very end of the pipeline. For
>> the beginning, i.e. the sensors, we already have source streams. But can we
>> have static routes on the end side, basically just before the DMA?
> 
> If you can have a single route only, isn't that effectively static?
> Although in that case the sub-device should not support routing.

Yes, I think that's effectively static (but only in a end side, just 
before DMA, not in a subdev along the pipeline).

>> If we have a CSI-2 receiver that has a hardcoded handling of the VC & DT,
>> how does the userspace configure the routes? The userspace doesn't see the
>> VCs or DTs. We could have static routes defined in the receiver subdevice,
>> but does that help?
> 
> Good point. I think not.
> 
> I guess we would then leave the routes for the user to create and driver to
> try to configure the hardware accordingly or fail in link validation?
> 
> Perhaps we won't need a static route flag then after all.
> 
>>
>> The HW I use, TI's CAL, has the means to configure VC/DT freely. But it has
>> 8 DMA engines, and, of course, each stream has to go to a single DMA engine.
>> So I think we could say that it has 8 static streams, and the user can only
>> enable or disable them. But I'm not sure how much adding a new flag for this
>> helps.
> 
> Could this be limited by only allowing to create eight routes?

Yes, and the driver should do that. But the driver should also verify 
the routing, so that each DMA engine only gets one route.

So here it might be possible to use a STATIC flag for the routes. 
Afaics, the only benefit of that would be to give a hint to the 
userspace about the possible routes. It's difficult to say if it's worth 
the trouble.

>>> Using one flag for two different purposes may thus prove problematic over
>>> time. I'd thus define another for the other purpose. In the worst case it
>>> won't be needed and we can make it obsolete later on.
>>
>> I'd like to have a clear example of a setup where we need this flag and
>> benefit from it before adding it.
>>
>> In the CAL case I don't see much benefit. I think the only thing it gives us
>> is a minimal discovery mechanism for the userspace to understand how CAL
>> routes can be configured. I say minimal, as it still won't cover it fully as
>> the validity of the routing depends on the actual VCs and DTs too (which
>> will be found out only at the stream start time).
>>
>> And this would only give us discovery for the receiver and wouldn't help
>> with the bridges.
>>
>>>>>>
>>>>>> But as I said above, I haven't figured out a use for this.
>>>>>>
>>>>>>>> +      - 1
>>>>>>>> +      - The route is immutable. Set by the driver.
>>>>>>>> +    * - V4L2_SUBDEV_ROUTE_FL_SOURCE
>>>>>>>> +      - 2
>>>>>>>> +      - The route is a source route, and the ``sink_pad`` and ``sink_stream``
>>>>>>>> +        fields are unused. Set by the driver.
>>>>>>>> +
>>>>>>>> +Return Value
>>>>>>>> +============
>>>>>>>> +
>>>>>>>> +On success 0 is returned, on error -1 and the ``errno`` variable is set
>>>>>>>> +appropriately. The generic error codes are described at the
>>>>>>>> +:ref:`Generic Error Codes <gen-errors>` chapter.
>>>>>>>> +
>>>>>>>> +ENOSPC
>>>>>>>> +   The number of provided route entries is less than the available ones.
>>>>>>>
>>>>>>> What does "available ones" mean in this context? More than is supported?
>>>>>>> Wouldn't E2BIG be the appropriate code in that case?
>>>>>>
>>>>>> Good question. I don't think I wrote this part =). ENOSPC refers to the case
>>>>>> where VIDIOC_SUBDEV_G_ROUTING is called without enough space for the routing
>>>>>> table. So "available ones" mean the routes in the subdev's routing table,
>>>>>> and "provided route entries" refers to the userspace target routing table.
>>>>>>
>>>>>> It sounds pretty odd, and obviously needs a clarification.
>>>>>
>>>>> I think I actually can think what this did mean. It means that the
>>>>> num_routes is smaller than the number of routes in a routing table when
>>>>> G_ROUTING is called. For that I think ENOSPC is the right code actually.
>>>>>
>>>>> But also I think we need to document what happens when there are too many
>>>>> routes. For that E2BIG would be appropriate.
>>>>
>>>> v4l2-ioctl.c returns EINVAL if there are over 256 routes. The drivers
>>>> should, of course, do additional check if needed. In v4l2-ioctl.c it seems
>>>> common to return EINVAL if there's too much data, but we can of course
>>>> define E2BIG for routing ioctls.
>>>
>>> The number (256) is just the current limit. I don't expect more though.
>>>
>>> But the user space could know the number is too large if we have a proper
>>> error code for it. Up to you. However at least documentation needs to be
>>> amended since this case remains undocumented.
>>
>> I can change the returned error from EINVAL to E2BIG and document it. But
>> everything else in check_array_args return EINVAL, so it would be going into
>> different direction.
> 
> Could this be beneficial in telling the user too many routes have been
> configured (as I wrote above)?

Yes, I think the driver should return E2BIG if there are too many routes.

But my question is, should v4l2-ioctl.c return E2BIG for > 256 routes? 
That's not how it works for all the other ioctls there, they return 
EINVAL. I don't mind changing that to E2BIG, but usually it's nice if 
the code is consistent.

  Tomi
  

Moi,

On Fri, Sep 30, 2022 at 03:10:16PM +0300, Tomi Valkeinen wrote:
> Hei,
> 
> On 30/09/2022 14:21, Sakari Ailus wrote:
> > Moi,
> > 
> > On Wed, Sep 28, 2022 at 10:54:44AM +0300, Tomi Valkeinen wrote:
> > > Hi,
> > > 
> > > On 28/09/2022 00:13, Sakari Ailus wrote:
> > > > Moi,
> > > > 
> > > > On Tue, Sep 27, 2022 at 03:33:15PM +0300, Tomi Valkeinen wrote:
> > > > > On 27/09/2022 13:23, Sakari Ailus wrote:
> > > > > 
> > > > > <snip>
> > > > > 
> > > > > > > > > +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This
> > > > > > > > > +means that the userspace mut reconfigure all streams after calling the ioctl
> > > > > > > > > +with e.g. ``VIDIOC_SUBDEV_S_FMT``.
> > > > > > > > 
> > > > > > > > How about this:
> > > > > > > > 
> > > > > > > > Calling ``VIDIOC_SUBDEV_S_ROUTING`` will cause the selections and subdev
> > > > > > > > formats being propagated from the sink pads towards the sources.
> > > > > > > 
> > > > > > > Hmm, but that's not true. The selections and formats will be zeroed, unless
> > > > > > > the driver initializes them to a value. There's no propagation done.
> > > > > > 
> > > > > > They need to be propagated. The driver is responsible for maintaining a
> > > > > > valid configuration for the processing steps in a sub-device, and with
> > > > > > routes that must apply to routes as well.
> > > > > 
> > > > > Hmm, no, they don't need to be propagated. The driver needs to initialize
> > > > > the formats and selections to valid configuration, that is true, but it
> > > > > doesn't mean the driver propagates settings from the sink pads to the source
> > > > > pads. In theory the formats on sink and source sides could be different.
> > > > 
> > > > After propagation, the user may set the format (or selection) later on in
> > > > the processing steps. The propagation is required by the spec and I don't
> > > > see why it would be different for drivers with support for streams. Of
> > > > course this needs to take place taking hardware limitations into account.
> > > 
> > > I don't disagree with the above, but I still don't see why it matters here.
> > 
> > It does. The user needs to be able to rely on the ability of the driver to
> > maintain valid internal configuration. That user generally has less
> > information on this than the driver.
> 
> I had a short chat with Sakari, and it seems there's no real issue here,
> mostly just a misunderstanding.
> 
> The action point is to clarify what "reset" means (it means resetting to
> driver default values, keeping the subdev configuration valid). And perhaps
> also highlighting somewhere that when e.g. setting the formats, propagation
> happens with subdevices using routes too.

Thanks. Sometimes e-mail just isn't enough. :-)

...

> > > If we have a CSI-2 receiver that has a hardcoded handling of the VC & DT,
> > > how does the userspace configure the routes? The userspace doesn't see the
> > > VCs or DTs. We could have static routes defined in the receiver subdevice,
> > > but does that help?
> > 
> > Good point. I think not.
> > 
> > I guess we would then leave the routes for the user to create and driver to
> > try to configure the hardware accordingly or fail in link validation?
> > 
> > Perhaps we won't need a static route flag then after all.
> > 
> > > 
> > > The HW I use, TI's CAL, has the means to configure VC/DT freely. But it has
> > > 8 DMA engines, and, of course, each stream has to go to a single DMA engine.
> > > So I think we could say that it has 8 static streams, and the user can only
> > > enable or disable them. But I'm not sure how much adding a new flag for this
> > > helps.
> > 
> > Could this be limited by only allowing to create eight routes?
> 
> Yes, and the driver should do that. But the driver should also verify the
> routing, so that each DMA engine only gets one route.
> 
> So here it might be possible to use a STATIC flag for the routes. Afaics,
> the only benefit of that would be to give a hint to the userspace about the
> possible routes. It's difficult to say if it's worth the trouble.

I'm fine with leaving out the flag. It seems it wouldn't have been as
useful in practice as I originally thought.

> 
> > > > Using one flag for two different purposes may thus prove problematic over
> > > > time. I'd thus define another for the other purpose. In the worst case it
> > > > won't be needed and we can make it obsolete later on.
> > > 
> > > I'd like to have a clear example of a setup where we need this flag and
> > > benefit from it before adding it.
> > > 
> > > In the CAL case I don't see much benefit. I think the only thing it gives us
> > > is a minimal discovery mechanism for the userspace to understand how CAL
> > > routes can be configured. I say minimal, as it still won't cover it fully as
> > > the validity of the routing depends on the actual VCs and DTs too (which
> > > will be found out only at the stream start time).
> > > 
> > > And this would only give us discovery for the receiver and wouldn't help
> > > with the bridges.
> > > 
> > > > > > > 
> > > > > > > But as I said above, I haven't figured out a use for this.
> > > > > > > 
> > > > > > > > > +      - 1
> > > > > > > > > +      - The route is immutable. Set by the driver.
> > > > > > > > > +    * - V4L2_SUBDEV_ROUTE_FL_SOURCE
> > > > > > > > > +      - 2
> > > > > > > > > +      - The route is a source route, and the ``sink_pad`` and ``sink_stream``
> > > > > > > > > +        fields are unused. Set by the driver.
> > > > > > > > > +
> > > > > > > > > +Return Value
> > > > > > > > > +============
> > > > > > > > > +
> > > > > > > > > +On success 0 is returned, on error -1 and the ``errno`` variable is set
> > > > > > > > > +appropriately. The generic error codes are described at the
> > > > > > > > > +:ref:`Generic Error Codes <gen-errors>` chapter.
> > > > > > > > > +
> > > > > > > > > +ENOSPC
> > > > > > > > > +   The number of provided route entries is less than the available ones.
> > > > > > > > 
> > > > > > > > What does "available ones" mean in this context? More than is supported?
> > > > > > > > Wouldn't E2BIG be the appropriate code in that case?
> > > > > > > 
> > > > > > > Good question. I don't think I wrote this part =). ENOSPC refers to the case
> > > > > > > where VIDIOC_SUBDEV_G_ROUTING is called without enough space for the routing
> > > > > > > table. So "available ones" mean the routes in the subdev's routing table,
> > > > > > > and "provided route entries" refers to the userspace target routing table.
> > > > > > > 
> > > > > > > It sounds pretty odd, and obviously needs a clarification.
> > > > > > 
> > > > > > I think I actually can think what this did mean. It means that the
> > > > > > num_routes is smaller than the number of routes in a routing table when
> > > > > > G_ROUTING is called. For that I think ENOSPC is the right code actually.
> > > > > > 
> > > > > > But also I think we need to document what happens when there are too many
> > > > > > routes. For that E2BIG would be appropriate.
> > > > > 
> > > > > v4l2-ioctl.c returns EINVAL if there are over 256 routes. The drivers
> > > > > should, of course, do additional check if needed. In v4l2-ioctl.c it seems
> > > > > common to return EINVAL if there's too much data, but we can of course
> > > > > define E2BIG for routing ioctls.
> > > > 
> > > > The number (256) is just the current limit. I don't expect more though.
> > > > 
> > > > But the user space could know the number is too large if we have a proper
> > > > error code for it. Up to you. However at least documentation needs to be
> > > > amended since this case remains undocumented.
> > > 
> > > I can change the returned error from EINVAL to E2BIG and document it. But
> > > everything else in check_array_args return EINVAL, so it would be going into
> > > different direction.
> > 
> > Could this be beneficial in telling the user too many routes have been
> > configured (as I wrote above)?
> 
> Yes, I think the driver should return E2BIG if there are too many routes.
> 
> But my question is, should v4l2-ioctl.c return E2BIG for > 256 routes?
> That's not how it works for all the other ioctls there, they return EINVAL.
> I don't mind changing that to E2BIG, but usually it's nice if the code is
> consistent.

From the user's point of view the "too many routes" condition is the same
independently of whether the information comes from the framework or the
driver. So I think using -E2BIG in the framework is the right thing to do.