From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>,
laurent.pinchart@ideasonboard.com,
niklas.soderlund+renesas@ragnatech.se,
linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v3 19/31] media: Documentation: Add GS_ROUTING documentation
Date: Fri, 8 Mar 2019 16:14:04 +0200 [thread overview]
Message-ID: <20190308141404.btraw5mdp6drarth@paasikivi.fi.intel.com> (raw)
In-Reply-To: <20190308133133.ium6nzkbw6g3z22r@uno.localdomain>
Hi Jacopo,
On Fri, Mar 08, 2019 at 02:31:33PM +0100, Jacopo Mondi wrote:
> Hi Sakari,
> thanks for the review
>
> On Thu, Mar 07, 2019 at 05:19:29PM +0200, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > Thanks for writing the documentation for this!
> >
> > The text is nice and informative; I have a few suggestions below.
> >
> > On Tue, Mar 05, 2019 at 07:51:38PM +0100, Jacopo Mondi wrote:
> > > 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>
> > > ---
> > > Documentation/media/uapi/v4l/dev-subdev.rst | 90 +++++++++++
> > > Documentation/media/uapi/v4l/user-func.rst | 1 +
> > > .../uapi/v4l/vidioc-subdev-g-routing.rst | 142 ++++++++++++++++++
> > > 3 files changed, 233 insertions(+)
> > > create mode 100644 Documentation/media/uapi/v4l/vidioc-subdev-g-routing.rst
> > >
> > > diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst
> > > index 2c2768c7343b..b9fbb5d2caec 100644
> > > --- a/Documentation/media/uapi/v4l/dev-subdev.rst
> > > +++ b/Documentation/media/uapi/v4l/dev-subdev.rst
> > > @@ -36,6 +36,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.
> > >
> > > @@ -461,3 +463,91 @@ source pads.
> > > :maxdepth: 1
> > >
> > > subdev-formats
> > > +
> > > +
> > > +Multiplexed media pads and internal routing
> > > +-------------------------------------------
> > > +
> > > +Subdevice drivers might expose the internal connections between media pads of an
> >
> > s/might/may/
> >
> > > +entity by providing a routing table that applications can inspect and manipulate
> >
> > s/providing/exposing/
> >
>
> Ack on both the above suggestions
>
> > > +to change the internal routing between sink and source pads' data connection
> > > +endpoints. A routing table is described by a struct
> > > +:c:type:`v4l2_subdev_routing`, which contains ``num_routes`` route entries, each
> > > +one represented by a struct :c:type:`v4l2_subdev_route`.
> > > +
> > > +Data routes do not just connect one pad to another in an entity, but they refer
> > > +instead to the ``streams`` a media pad provides. Streams are data connection
> > > +endpoints in a media pad. Multiplexed media pads expose multiple ``streams``
> > > +which represent, when the underlying hardware technology allows that, logical
> > > +data streams transported over a single physical media bus.
> >
> > How about s/streams/flows/ for this instance?
> >
>
> Agreed, there are too many "streams" already in that paragraph
>
> > > +
> > > +One of the most notable examples of logical stream multiplexing techniques is
> >
> > s/One of the most notable examples/A noteworthy example/
> >
> > > +represented by the data interleaving mechanism implemented by mean of Virtual
> > > +Channels identifiers as defined by the MIPI CSI-2 media bus specifications. A
> >
> > s/identifiers //
> >
>
> Ack on both the above suggestions
>
> > > +subdevice that implements support for Virtual Channel data interleaving might
> > > +expose up to 4 data ``streams``, one for each available Virtual Channel, on the
> > > +source media pad that represents a CSI-2 connection endpoint.
> > > +
> > > +Routes are defined as potential data connections between a ``(sink_pad,
> > > +sink_stream)`` pair and a ``(source_pad, source_stream)`` one, where
> > > +``sink_pad`` and ``source_pad`` are the indexes of two media pads part of the
> > > +same media entity, and ``sink_stream`` and ``source_stream`` are the identifiers
> > > +of the data streams to be connected in the media pads. Media pads that do not
> > > +support data multiplexing expose a single stream, usually with identifier 0.
> > > +
> > > +Routes are reported to applications in a routing table which can be
> > > +inspected and manipulated using the :ref:`routing <VIDIOC_SUBDEV_G_ROUTING>`
> > > +ioctls.
> > > +
> > > +Routes can be activated and deactivated by setting or clearing the
> > > +V4L2_SUBDEV_ROUTE_FL_ACTIVE flag in the ``flags`` field of struct
> > > +:c:type:`v4l2_subdev_route`.
> > > +
> > > +Subdev driver might create routes that cannot be modified by applications. Such
> >
> > s/S/A s/
>
> Or "Subdevice drivers" ?
I think singular is fine. And s/might/may/.
>
> >
> > > +routes are identified by the presence of the V4L2_SUBDEV_ROUTE_FL_IMMUTABLE
> > > +flag in the ``flags`` field of struct :c:type:`v4l2_subdev_route`.
> > > +
> > > +As an example, the routing table of a source pad which supports two logical
> > > +video streams and can be connected to two sink pads is here below described.
> > > +
> > > +.. flat-table::
> > > + :widths: 1 2 1
> > > +
> > > + * - Pad Index
> > > + - Function
> > > + - Number of streams
> > > + * - 0
> > > + - SINK
> > > + - 1
> > > + * - 1
> > > + - SINK
> > > + - 1
> > > + * - 2
> > > + - SOURCE
> > > + - 2
> > > +
> > > +In such an example, the source media pad will report a routing table with 4
> > > +entries, one entry for each possible ``(sink_pad, sink_stream) - (source_pad,
> > > +source_stream)`` combination.
> > > +
> > > +.. flat-table:: routing table
> > > + :widths: 2 1 2
> > > +
> > > + * - Sink Pad/Sink Stream
> > > + - ->
> > > + - Source Pad/Source Stream
> > > + * - 0/0
> > > + - ->
> > > + - 2/0
> > > + * - 0/0
> > > + - ->
> > > + - 2/1
> > > + * - 1/0
> > > + - ->
> > > + - 2/0
> > > + * - 1/0
> > > + - ->
> > > + - 2/1
> > > +
> > > +Subdev drivers are free to decide how many routes an application can enable on
> > > +a media pad at the same time, and refuse to enable or disable specific routes.
> > > diff --git a/Documentation/media/uapi/v4l/user-func.rst b/Documentation/media/uapi/v4l/user-func.rst
> > > index ca0ef21d77fe..0166446f4ab4 100644
> > > --- a/Documentation/media/uapi/v4l/user-func.rst
> > > +++ b/Documentation/media/uapi/v4l/user-func.rst
> > > @@ -77,6 +77,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-subscribe-event
> > > func-mmap
> > > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-routing.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-routing.rst
> > > new file mode 100644
> > > index 000000000000..8b592722c477
> > > --- /dev/null
> > > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-routing.rst
> > > @@ -0,0 +1,142 @@
> > > +.. Permission is granted to copy, distribute and/or modify this
> > > +.. document under the terms of the GNU Free Documentation License,
> > > +.. Version 1.1 or any later version published by the Free Software
> > > +.. Foundation, with no Invariant Sections, no Front-Cover Texts
> > > +.. and no Back-Cover Texts. A copy of the license is included at
> > > +.. Documentation/media/uapi/fdl-appendix.rst.
> > > +..
> > > +.. TODO: replace it to GFDL-1.1-or-later WITH no-invariant-sections
> > > +
> > > +.. _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 informations associated to
> > > +media pads in a media entity. Routing informations are used to enable or disable
> >
> > The routing is a property of an entity. How about
> >
> > s/the routing informations associated to media pads in/routing/
> >
> > s/R[^\.]+(?=\.)/The routing configuration determines the flows of data
> > inside an entity/
>
> Thanks, you made me discover regexr.com here
:-) Or just man perlre . :-)
> >
> > > +data connections between stream endpoints of multiplexed media pads.
> > > +
> > > +Drivers report their routing tables using VIDIOC_SUBDEV_G_ROUTING and
> > > +application use the information there reported to enable or disable data
> > > +connections between streams in a pad, by setting or clearing the
> >
> > How about:
> >
> > s/applications\K.*a pad/may enable or disable routes with the
> > VIDIOC_SUBDEV_S_ROUTING IOCTL,
> >
>
> Yes, reads better
>
> > > +V4L2_SUBDEV_ROUTE_FL_ACTIVE flag of ``flags`` field of a struct
> > > +:c:type:`v4l2_subdev_route`.
> > > +
> > > +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
> > > +``num_routes`` value. Application should then reserve enough memory for all the
> >
> > s/the\K$/value of the/
> > s/value\K\./field/
> >
>
> Will update
>
> > > +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
> > > +
> > > + * - 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_stream``
> > > + - Source pad stream number
> > > + * - __u32
> > > + - ``sink_stream``
> > > + - Sink 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
> > > + - 1
> > > + - The route is immutable. 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.
> > > +
> > > +EINVAL
> > > + The sink or source pad identifiers reference a non-existing pad, or refernce
> > > + pads of different types (ie. the sink_pad identifiers refers to a source pad)
> > > + The sink or source stream identifiers reference a non-existing stream
> >
> > s/The/or the/
> >
> > > + in the sink or source pad.
> >
> > s/i/o/
>
> Thanks, I will update all of these in v4.
Thanks!
--
Sakari Ailus
sakari.ailus@linux.intel.com
next prev parent reply other threads:[~2019-03-08 14:14 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-05 18:51 [PATCH v3 00/31] v4l: add support for multiplexed streams Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 01/31] media: entity: Use pad as a starting point for graph walk Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 02/31] media: entity: Use pads instead of entities in the media graph walk stack Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 03/31] media: entity: Walk the graph based on pads Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 04/31] v4l: mc: Start walk from a specific pad in use count calculation Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 05/31] media: entity: Add iterator helper for entity pads Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 06/31] media: entity: Move the pipeline from entity to pads Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 07/31] media: entity: Use pad as the starting point for a pipeline Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 08/31] media: entity: Add has_route entity operation Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 09/31] media: entity: Add media_has_route() function Jacopo Mondi
2019-03-14 14:45 ` Luca Ceresoli
2019-03-15 9:22 ` Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 10/31] media: entity: Use routing information during graph traversal Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 11/31] media: entity: Skip link validation for pads to which there is no route to Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 12/31] media: entity: Add an iterator helper for connected pads Jacopo Mondi
2019-03-07 10:09 ` Sakari Ailus
2019-03-07 10:27 ` Ian Arkver
2019-03-07 12:38 ` Sakari Ailus
2019-03-07 20:18 ` Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 13/31] media: entity: Add only connected pads to the pipeline Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 14/31] media: entity: Add debug information in graph walk route check Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 15/31] v4l: Add bus type to frame descriptors Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 16/31] v4l: Add CSI-2 bus configuration " Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 17/31] v4l: Add stream to frame descriptor Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 18/31] v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 19/31] media: Documentation: Add GS_ROUTING documentation Jacopo Mondi
2019-03-07 15:19 ` Sakari Ailus
2019-03-08 13:31 ` Jacopo Mondi
2019-03-08 14:14 ` Sakari Ailus [this message]
2019-03-14 14:44 ` Luca Ceresoli
2019-03-14 14:43 ` Luca Ceresoli
2020-02-13 13:36 ` Hans Verkuil
2019-03-05 18:51 ` [PATCH v3 20/31] v4l: subdev: Take routing information into account in link validation Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 21/31] v4l: subdev: Improve link format validation debug messages Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 22/31] v4l: mc: Add an S_ROUTING helper function for power state changes Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 23/31] adv748x: csi2: add translation from pixelcode to CSI-2 datatype Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 24/31] adv748x: csi2: only allow formats on sink pads Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 25/31] adv748x: csi2: describe the multiplexed stream Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 26/31] adv748x: csi2: add internal routing configuration Jacopo Mondi
2019-03-14 14:45 ` Luca Ceresoli
2019-03-15 9:45 ` Jacopo Mondi
2019-03-15 10:06 ` Sakari Ailus
2019-03-16 10:23 ` Luca Ceresoli
2019-03-20 17:14 ` Jacopo Mondi
2019-03-22 16:52 ` Luca Ceresoli
2019-03-28 15:08 ` Jacopo Mondi
2019-03-28 15:17 ` Luca Ceresoli
2019-03-05 18:51 ` [PATCH v3 27/31] adv748x: afe: add routing support Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 28/31] adv748x: afe: Implement has_route() Jacopo Mondi
2019-03-07 12:49 ` Sakari Ailus
2019-03-14 14:45 ` Luca Ceresoli
2019-03-05 18:51 ` [PATCH v3 29/31] rcar-csi2: use frame description information to configure CSI-2 bus Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 30/31] rcar-csi2: expose the subdevice internal routing Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 31/31] media: rcar-csi2: Implement has_route() Jacopo Mondi
2019-03-07 12:56 ` Sakari Ailus
2019-03-07 22:28 ` Jacopo Mondi
2019-03-07 9:47 ` [PATCH v3 00/31] v4l: add support for multiplexed streams Sakari Ailus
2019-03-08 13:19 ` Jacopo Mondi
2019-03-08 14:12 ` Sakari Ailus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190308141404.btraw5mdp6drarth@paasikivi.fi.intel.com \
--to=sakari.ailus@linux.intel.com \
--cc=jacopo+renesas@jmondi.org \
--cc=jacopo@jmondi.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).