mbox series

[v5,00/24] v4l: subdev internal routing

Message ID 20210415130450.421168-1-tomi.valkeinen@ideasonboard.com (mailing list archive)
Headers show
Series v4l: subdev internal routing | expand

Message

Tomi Valkeinen April 15, 2021, 1:04 p.m. UTC
Hi,

This is an RFC for subdev internal routing which is needed for
multiplexed streams support. I believe this is essentially a v5 of the
series, the v4 posted here:

https://lore.kernel.org/linux-media/20190328200608.9463-1-jacopo+renesas@jmondi.org/

Most of the patches are not changed (aside from fixing rebase issues
etc). The major changes in this version are:

1) Added 'which' field to the routing structs. It is currently not used,
as implementing it is not trivial. However, I think it's good to add it
to the uAPI now, and require the field to be set to
V4L2_SUBDEV_FORMAT_ACTIVE for now. See this RFC for an idea how this
could be implemented:

https://lore.kernel.org/linux-media/20210409133659.389544-1-tomi.valkeinen@ideasonboard.com/

2) No hardcoded maximum number of routes. Defining a maximum is not
possible, as there can be an arbitrary amount of routes per pad, and
there can be an arbitrary amount of pads per subdev. This series
allocates space for the routing table dynamically, which unfortunately
leads to not-just-a-few allocs and frees.

3) When searching for a format for a stream, the v4 looked for a
non-multiplexed pad only as far as the "other" side of the subdev. It
wouldn't work for a subdev which has multiplexed sink and source pads.
This series implements a "deep" get-format (v4l2_subdev_get_format_dir)
which follows a stream either towards the original source or the final
sink, while looking for a non-multiplexed pad with a format.

Some thoughts:

1) Link validation and v4l2_subdev_get_format_dir need to look at the
routing, and this leads to multiple allocs to get a copy of the routing
table. There might be a possibility here to keep a table allocated and
re-use it in consecutive get_routing calls.

Or even better, perhaps the kAPI could be changed so that allocs are not
needed. I thought about a kAPI where the subdev just returns a pointer
to its routing table, but then we hit the life-cycle problem: how to
ensure the table won't be freed or changed until the caller is done.

2) The routing uAPI is a bit vague. There is no way for the userspace to
figure out what kind of routing is allowed. Also, the existence of a
route in the routing table already indicates that the route is active,
but we also have V4L2_SUBDEV_ROUTE_FL_ACTIVE. I decided to keep
V4L2_SUBDEV_ROUTE_FL_ACTIVE for now, even if it doesn't really provide
any feature.

3) V4L2_FRAME_DESC_ENTRY_MAX is defined as 8 (I change it from 4 to 8 in
this series). This limits the number of streams per pad to 8. Preferably
the number of frame descs would be unlimited, but I didn't start
tackling this. I believe 8 is quite safe number (4 pixel streams and 4
embedded data stream).

4) Link validation ends up following the same routes multiple times, as
each stream in each subdev is validated separately.

 Tomi

Jacopo Mondi (2):
  media: entity: Add iterator helper for entity pads
  media: Documentation: Add GS_ROUTING documentation

Laurent Pinchart (4):
  media: entity: Add has_route entity operation
  media: entity: Add media_entity_has_route() function
  media: entity: Use routing information during graph traversal
  v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations

Sakari Ailus (14):
  media: entity: Use pad as a starting point for graph walk
  media: entity: Use pads instead of entities in the media graph walk
    stack
  media: entity: Walk the graph based on pads
  v4l: mc: Start walk from a specific pad in use count calculation
  media: entity: Move the pipeline from entity to pads
  media: entity: Use pad as the starting point for a pipeline
  media: entity: Skip link validation for pads to which there is no
    route to
  media: entity: Add an iterator helper for connected pads
  media: entity: Add only connected pads to the pipeline
  media: entity: Add debug information in graph walk route check
  v4l: Add bus type to frame descriptors
  v4l: Add CSI-2 bus configuration to frame descriptors
  v4l: Add stream to frame descriptor
  v4l: mc: Add an S_ROUTING helper function for power state changes

Tomi Valkeinen (4):
  v4l: subdev: routing kernel helper functions
  v4l: subdev: add v4l2_subdev_get_format_dir()
  v4l: subdev: Take routing information into account in link validation
  v4l: subdev: increase V4L2_FRAME_DESC_ENTRY_MAX to 8

 Documentation/driver-api/media/mc-core.rst    |  15 +-
 .../userspace-api/media/v4l/dev-subdev.rst    |  92 +++++
 .../userspace-api/media/v4l/user-func.rst     |   1 +
 .../media/v4l/vidioc-subdev-g-routing.rst     | 143 +++++++
 drivers/media/mc/mc-device.c                  |  13 +-
 drivers/media/mc/mc-entity.c                  | 239 +++++++-----
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |   6 +-
 .../media/platform/exynos4-is/fimc-capture.c  |   8 +-
 .../platform/exynos4-is/fimc-isp-video.c      |   8 +-
 drivers/media/platform/exynos4-is/fimc-isp.c  |   2 +-
 drivers/media/platform/exynos4-is/fimc-lite.c |  10 +-
 drivers/media/platform/exynos4-is/media-dev.c |  20 +-
 drivers/media/platform/omap3isp/isp.c         |   2 +-
 drivers/media/platform/omap3isp/ispvideo.c    |  25 +-
 drivers/media/platform/omap3isp/ispvideo.h    |   2 +-
 .../media/platform/qcom/camss/camss-video.c   |   6 +-
 drivers/media/platform/rcar-vin/rcar-core.c   |  16 +-
 drivers/media/platform/rcar-vin/rcar-dma.c    |   8 +-
 .../platform/rockchip/rkisp1/rkisp1-capture.c |   6 +-
 .../media/platform/s3c-camif/camif-capture.c  |   6 +-
 drivers/media/platform/stm32/stm32-dcmi.c     |   6 +-
 .../platform/sunxi/sun4i-csi/sun4i_dma.c      |   6 +-
 .../platform/sunxi/sun6i-csi/sun6i_video.c    |   6 +-
 drivers/media/platform/ti-vpe/cal-video.c     |   6 +-
 drivers/media/platform/vsp1/vsp1_video.c      |  18 +-
 drivers/media/platform/xilinx/xilinx-dma.c    |  20 +-
 drivers/media/platform/xilinx/xilinx-dma.h    |   2 +-
 .../media/test-drivers/vimc/vimc-capture.c    |   6 +-
 drivers/media/usb/au0828/au0828-core.c        |   8 +-
 drivers/media/v4l2-core/v4l2-ioctl.c          |  25 +-
 drivers/media/v4l2-core/v4l2-mc.c             |  77 ++--
 drivers/media/v4l2-core/v4l2-subdev.c         | 368 ++++++++++++++++++
 drivers/staging/media/imx/imx-media-utils.c   |   8 +-
 drivers/staging/media/ipu3/ipu3-v4l2.c        |   6 +-
 drivers/staging/media/omap4iss/iss.c          |   2 +-
 drivers/staging/media/omap4iss/iss_video.c    |  38 +-
 drivers/staging/media/omap4iss/iss_video.h    |   2 +-
 drivers/staging/media/tegra-video/tegra210.c  |   6 +-
 include/media/media-entity.h                  | 143 +++++--
 include/media/v4l2-mc.h                       |  22 ++
 include/media/v4l2-subdev.h                   |  93 ++++-
 include/uapi/linux/v4l2-subdev.h              |  44 +++
 42 files changed, 1241 insertions(+), 299 deletions(-)
 create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst

Comments

Niklas Söderlund April 16, 2021, 8:38 a.m. UTC | #1
Hi Tomi,

I'm very happy to see this being worked on again!

Is there code somewhere that demonstrates the v5 API in use? I still 
have old branches of this series and it would be nice to see how the API 
have evolved for drivers.

Likewise are there some user-space code around that can be used to test 
the API? For v2 and v3 I had some hack patches [1], do they still work?  
More likely they have gone stale by now :-)

1. git://git.ragnatech.se/v4l-utils routing

On 2021-04-15 16:04:26 +0300, Tomi Valkeinen wrote:
> Hi,
> 
> This is an RFC for subdev internal routing which is needed for
> multiplexed streams support. I believe this is essentially a v5 of the
> series, the v4 posted here:
> 
> https://lore.kernel.org/linux-media/20190328200608.9463-1-jacopo+renesas@jmondi.org/
> 
> Most of the patches are not changed (aside from fixing rebase issues
> etc). The major changes in this version are:
> 
> 1) Added 'which' field to the routing structs. It is currently not used,
> as implementing it is not trivial. However, I think it's good to add it
> to the uAPI now, and require the field to be set to
> V4L2_SUBDEV_FORMAT_ACTIVE for now. See this RFC for an idea how this
> could be implemented:
> 
> https://lore.kernel.org/linux-media/20210409133659.389544-1-tomi.valkeinen@ideasonboard.com/
> 
> 2) No hardcoded maximum number of routes. Defining a maximum is not
> possible, as there can be an arbitrary amount of routes per pad, and
> there can be an arbitrary amount of pads per subdev. This series
> allocates space for the routing table dynamically, which unfortunately
> leads to not-just-a-few allocs and frees.
> 
> 3) When searching for a format for a stream, the v4 looked for a
> non-multiplexed pad only as far as the "other" side of the subdev. It
> wouldn't work for a subdev which has multiplexed sink and source pads.
> This series implements a "deep" get-format (v4l2_subdev_get_format_dir)
> which follows a stream either towards the original source or the final
> sink, while looking for a non-multiplexed pad with a format.
> 
> Some thoughts:
> 
> 1) Link validation and v4l2_subdev_get_format_dir need to look at the
> routing, and this leads to multiple allocs to get a copy of the routing
> table. There might be a possibility here to keep a table allocated and
> re-use it in consecutive get_routing calls.
> 
> Or even better, perhaps the kAPI could be changed so that allocs are not
> needed. I thought about a kAPI where the subdev just returns a pointer
> to its routing table, but then we hit the life-cycle problem: how to
> ensure the table won't be freed or changed until the caller is done.
> 
> 2) The routing uAPI is a bit vague. There is no way for the userspace to
> figure out what kind of routing is allowed. Also, the existence of a
> route in the routing table already indicates that the route is active,
> but we also have V4L2_SUBDEV_ROUTE_FL_ACTIVE. I decided to keep
> V4L2_SUBDEV_ROUTE_FL_ACTIVE for now, even if it doesn't really provide
> any feature.
> 
> 3) V4L2_FRAME_DESC_ENTRY_MAX is defined as 8 (I change it from 4 to 8 in
> this series). This limits the number of streams per pad to 8. Preferably
> the number of frame descs would be unlimited, but I didn't start
> tackling this. I believe 8 is quite safe number (4 pixel streams and 4
> embedded data stream).
> 
> 4) Link validation ends up following the same routes multiple times, as
> each stream in each subdev is validated separately.
> 
>  Tomi
> 
> Jacopo Mondi (2):
>   media: entity: Add iterator helper for entity pads
>   media: Documentation: Add GS_ROUTING documentation
> 
> Laurent Pinchart (4):
>   media: entity: Add has_route entity operation
>   media: entity: Add media_entity_has_route() function
>   media: entity: Use routing information during graph traversal
>   v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations
> 
> Sakari Ailus (14):
>   media: entity: Use pad as a starting point for graph walk
>   media: entity: Use pads instead of entities in the media graph walk
>     stack
>   media: entity: Walk the graph based on pads
>   v4l: mc: Start walk from a specific pad in use count calculation
>   media: entity: Move the pipeline from entity to pads
>   media: entity: Use pad as the starting point for a pipeline
>   media: entity: Skip link validation for pads to which there is no
>     route to
>   media: entity: Add an iterator helper for connected pads
>   media: entity: Add only connected pads to the pipeline
>   media: entity: Add debug information in graph walk route check
>   v4l: Add bus type to frame descriptors
>   v4l: Add CSI-2 bus configuration to frame descriptors
>   v4l: Add stream to frame descriptor
>   v4l: mc: Add an S_ROUTING helper function for power state changes
> 
> Tomi Valkeinen (4):
>   v4l: subdev: routing kernel helper functions
>   v4l: subdev: add v4l2_subdev_get_format_dir()
>   v4l: subdev: Take routing information into account in link validation
>   v4l: subdev: increase V4L2_FRAME_DESC_ENTRY_MAX to 8
> 
>  Documentation/driver-api/media/mc-core.rst    |  15 +-
>  .../userspace-api/media/v4l/dev-subdev.rst    |  92 +++++
>  .../userspace-api/media/v4l/user-func.rst     |   1 +
>  .../media/v4l/vidioc-subdev-g-routing.rst     | 143 +++++++
>  drivers/media/mc/mc-device.c                  |  13 +-
>  drivers/media/mc/mc-entity.c                  | 239 +++++++-----
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |   6 +-
>  .../media/platform/exynos4-is/fimc-capture.c  |   8 +-
>  .../platform/exynos4-is/fimc-isp-video.c      |   8 +-
>  drivers/media/platform/exynos4-is/fimc-isp.c  |   2 +-
>  drivers/media/platform/exynos4-is/fimc-lite.c |  10 +-
>  drivers/media/platform/exynos4-is/media-dev.c |  20 +-
>  drivers/media/platform/omap3isp/isp.c         |   2 +-
>  drivers/media/platform/omap3isp/ispvideo.c    |  25 +-
>  drivers/media/platform/omap3isp/ispvideo.h    |   2 +-
>  .../media/platform/qcom/camss/camss-video.c   |   6 +-
>  drivers/media/platform/rcar-vin/rcar-core.c   |  16 +-
>  drivers/media/platform/rcar-vin/rcar-dma.c    |   8 +-
>  .../platform/rockchip/rkisp1/rkisp1-capture.c |   6 +-
>  .../media/platform/s3c-camif/camif-capture.c  |   6 +-
>  drivers/media/platform/stm32/stm32-dcmi.c     |   6 +-
>  .../platform/sunxi/sun4i-csi/sun4i_dma.c      |   6 +-
>  .../platform/sunxi/sun6i-csi/sun6i_video.c    |   6 +-
>  drivers/media/platform/ti-vpe/cal-video.c     |   6 +-
>  drivers/media/platform/vsp1/vsp1_video.c      |  18 +-
>  drivers/media/platform/xilinx/xilinx-dma.c    |  20 +-
>  drivers/media/platform/xilinx/xilinx-dma.h    |   2 +-
>  .../media/test-drivers/vimc/vimc-capture.c    |   6 +-
>  drivers/media/usb/au0828/au0828-core.c        |   8 +-
>  drivers/media/v4l2-core/v4l2-ioctl.c          |  25 +-
>  drivers/media/v4l2-core/v4l2-mc.c             |  77 ++--
>  drivers/media/v4l2-core/v4l2-subdev.c         | 368 ++++++++++++++++++
>  drivers/staging/media/imx/imx-media-utils.c   |   8 +-
>  drivers/staging/media/ipu3/ipu3-v4l2.c        |   6 +-
>  drivers/staging/media/omap4iss/iss.c          |   2 +-
>  drivers/staging/media/omap4iss/iss_video.c    |  38 +-
>  drivers/staging/media/omap4iss/iss_video.h    |   2 +-
>  drivers/staging/media/tegra-video/tegra210.c  |   6 +-
>  include/media/media-entity.h                  | 143 +++++--
>  include/media/v4l2-mc.h                       |  22 ++
>  include/media/v4l2-subdev.h                   |  93 ++++-
>  include/uapi/linux/v4l2-subdev.h              |  44 +++
>  42 files changed, 1241 insertions(+), 299 deletions(-)
>  create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> 
> -- 
> 2.25.1
>
Tomi Valkeinen April 16, 2021, 8:47 a.m. UTC | #2
Hi Niklas,

On 16/04/2021 11:38, Niklas Söderlund wrote:
> Hi Tomi,
> 
> I'm very happy to see this being worked on again!
> 
> Is there code somewhere that demonstrates the v5 API in use? I still
> have old branches of this series and it would be nice to see how the API
> have evolved for drivers.
> 
> Likewise are there some user-space code around that can be used to test
> the API? For v2 and v3 I had some hack patches [1], do they still work?
> More likely they have gone stale by now :-)
> 
> 1. git://git.ragnatech.se/v4l-utils routing

Yes for both. I didn't share those as they're not in a presentable state =).

But, with the disclaimer that your eyes may bleed when reading the code:

git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git 
multistream/work

git://github.com/tomba/kmsxx.git multistream

On the kernel side, the CAL driver is in relatively good shape. UB960 
driver is somewhat messy but not totally horrible. OV10635 is horrible, 
as it's used to fake metadata stream even if the sensor doesn't really 
have such a thing.

For testing I have used kms++ with python bindings. My test script is 
py/tests/cam.py

The uAPI has changed as there's now the 'which' field. But I think 
that's the only clear change, although the behavior could be slightly 
different wrt. setting formats.

  Tomi
Niklas Söderlund April 16, 2021, 8:56 a.m. UTC | #3
Hi Tomi,

On 2021-04-16 11:47:46 +0300, Tomi Valkeinen wrote:
> Hi Niklas,
> 
> On 16/04/2021 11:38, Niklas Söderlund wrote:
> > Hi Tomi,
> > 
> > I'm very happy to see this being worked on again!
> > 
> > Is there code somewhere that demonstrates the v5 API in use? I still
> > have old branches of this series and it would be nice to see how the API
> > have evolved for drivers.
> > 
> > Likewise are there some user-space code around that can be used to test
> > the API? For v2 and v3 I had some hack patches [1], do they still work?
> > More likely they have gone stale by now :-)
> > 
> > 1. git://git.ragnatech.se/v4l-utils routing
> 
> Yes for both. I didn't share those as they're not in a presentable state =).
> 
> But, with the disclaimer that your eyes may bleed when reading the code:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git
> multistream/work
> 
> git://github.com/tomba/kmsxx.git multistream
> 
> On the kernel side, the CAL driver is in relatively good shape. UB960 driver
> is somewhat messy but not totally horrible. OV10635 is horrible, as it's
> used to fake metadata stream even if the sensor doesn't really have such a
> thing.
> 
> For testing I have used kms++ with python bindings. My test script is
> py/tests/cam.py
> 
> The uAPI has changed as there's now the 'which' field. But I think that's
> the only clear change, although the behavior could be slightly different
> wrt. setting formats.

Thanks!

> 
>  Tomi
Laurent Pinchart April 18, 2021, 5:32 p.m. UTC | #4
Hi Tomi,

Nice to see a v5 of this plagued patch series :-)

On Thu, Apr 15, 2021 at 04:04:26PM +0300, Tomi Valkeinen wrote:
> Hi,
> 
> This is an RFC for subdev internal routing which is needed for
> multiplexed streams support. I believe this is essentially a v5 of the
> series, the v4 posted here:
> 
> https://lore.kernel.org/linux-media/20190328200608.9463-1-jacopo+renesas@jmondi.org/
> 
> Most of the patches are not changed (aside from fixing rebase issues
> etc). The major changes in this version are:
> 
> 1) Added 'which' field to the routing structs. It is currently not used,
> as implementing it is not trivial. However, I think it's good to add it
> to the uAPI now, and require the field to be set to
> V4L2_SUBDEV_FORMAT_ACTIVE for now. See this RFC for an idea how this
> could be implemented:
> 
> https://lore.kernel.org/linux-media/20210409133659.389544-1-tomi.valkeinen@ideasonboard.com/

I've reviewed that, and I like it, but it's not straightforward to
understand from that patch how you envision TRY to be implemented.

> 2) No hardcoded maximum number of routes. Defining a maximum is not
> possible, as there can be an arbitrary amount of routes per pad, and
> there can be an arbitrary amount of pads per subdev. This series
> allocates space for the routing table dynamically, which unfortunately
> leads to not-just-a-few allocs and frees.
> 
> 3) When searching for a format for a stream, the v4 looked for a
> non-multiplexed pad only as far as the "other" side of the subdev. It
> wouldn't work for a subdev which has multiplexed sink and source pads.
> This series implements a "deep" get-format (v4l2_subdev_get_format_dir)
> which follows a stream either towards the original source or the final
> sink, while looking for a non-multiplexed pad with a format.
> 
> Some thoughts:
> 
> 1) Link validation and v4l2_subdev_get_format_dir need to look at the
> routing, and this leads to multiple allocs to get a copy of the routing
> table. There might be a possibility here to keep a table allocated and
> re-use it in consecutive get_routing calls.
> 
> Or even better, perhaps the kAPI could be changed so that allocs are not
> needed. I thought about a kAPI where the subdev just returns a pointer
> to its routing table, but then we hit the life-cycle problem: how to
> ensure the table won't be freed or changed until the caller is done.

Storing the routing table in the v4l2_subdev_config (or
v4l2_subdev_state) would be one way to do so, and I'd like to explore
that direction. State lifetime is indeed an issue, and one simple option
would be to just take the graph lock to modify the routing.

> 2) The routing uAPI is a bit vague. There is no way for the userspace to
> figure out what kind of routing is allowed. Also, the existence of a
> route in the routing table already indicates that the route is active,
> but we also have V4L2_SUBDEV_ROUTE_FL_ACTIVE. I decided to keep
> V4L2_SUBDEV_ROUTE_FL_ACTIVE for now, even if it doesn't really provide
> any feature.

We can't report all possible routes if we take streams into account, but
maybe we could report all possible routes between pads ? This could go
through a separate ioctl.

> 3) V4L2_FRAME_DESC_ENTRY_MAX is defined as 8 (I change it from 4 to 8 in
> this series). This limits the number of streams per pad to 8. Preferably
> the number of frame descs would be unlimited, but I didn't start
> tackling this. I believe 8 is quite safe number (4 pixel streams and 4
> embedded data stream).

A more dynamic solution would be nice, but if this is internal to the
kernel, I suppose we can stored with a fixed limit.

> 4) Link validation ends up following the same routes multiple times, as
> each stream in each subdev is validated separately.

Caching routes would likely be too much trouble for too little gain. If
we can avoid the allocation of routing tables every time, then I think
this is an acceptable limitation.

> Jacopo Mondi (2):
>   media: entity: Add iterator helper for entity pads
>   media: Documentation: Add GS_ROUTING documentation
> 
> Laurent Pinchart (4):
>   media: entity: Add has_route entity operation
>   media: entity: Add media_entity_has_route() function
>   media: entity: Use routing information during graph traversal
>   v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations
> 
> Sakari Ailus (14):
>   media: entity: Use pad as a starting point for graph walk
>   media: entity: Use pads instead of entities in the media graph walk
>     stack
>   media: entity: Walk the graph based on pads
>   v4l: mc: Start walk from a specific pad in use count calculation
>   media: entity: Move the pipeline from entity to pads
>   media: entity: Use pad as the starting point for a pipeline
>   media: entity: Skip link validation for pads to which there is no
>     route to
>   media: entity: Add an iterator helper for connected pads
>   media: entity: Add only connected pads to the pipeline
>   media: entity: Add debug information in graph walk route check
>   v4l: Add bus type to frame descriptors
>   v4l: Add CSI-2 bus configuration to frame descriptors
>   v4l: Add stream to frame descriptor
>   v4l: mc: Add an S_ROUTING helper function for power state changes
> 
> Tomi Valkeinen (4):
>   v4l: subdev: routing kernel helper functions
>   v4l: subdev: add v4l2_subdev_get_format_dir()
>   v4l: subdev: Take routing information into account in link validation
>   v4l: subdev: increase V4L2_FRAME_DESC_ENTRY_MAX to 8
> 
>  Documentation/driver-api/media/mc-core.rst    |  15 +-
>  .../userspace-api/media/v4l/dev-subdev.rst    |  92 +++++
>  .../userspace-api/media/v4l/user-func.rst     |   1 +
>  .../media/v4l/vidioc-subdev-g-routing.rst     | 143 +++++++
>  drivers/media/mc/mc-device.c                  |  13 +-
>  drivers/media/mc/mc-entity.c                  | 239 +++++++-----
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |   6 +-
>  .../media/platform/exynos4-is/fimc-capture.c  |   8 +-
>  .../platform/exynos4-is/fimc-isp-video.c      |   8 +-
>  drivers/media/platform/exynos4-is/fimc-isp.c  |   2 +-
>  drivers/media/platform/exynos4-is/fimc-lite.c |  10 +-
>  drivers/media/platform/exynos4-is/media-dev.c |  20 +-
>  drivers/media/platform/omap3isp/isp.c         |   2 +-
>  drivers/media/platform/omap3isp/ispvideo.c    |  25 +-
>  drivers/media/platform/omap3isp/ispvideo.h    |   2 +-
>  .../media/platform/qcom/camss/camss-video.c   |   6 +-
>  drivers/media/platform/rcar-vin/rcar-core.c   |  16 +-
>  drivers/media/platform/rcar-vin/rcar-dma.c    |   8 +-
>  .../platform/rockchip/rkisp1/rkisp1-capture.c |   6 +-
>  .../media/platform/s3c-camif/camif-capture.c  |   6 +-
>  drivers/media/platform/stm32/stm32-dcmi.c     |   6 +-
>  .../platform/sunxi/sun4i-csi/sun4i_dma.c      |   6 +-
>  .../platform/sunxi/sun6i-csi/sun6i_video.c    |   6 +-
>  drivers/media/platform/ti-vpe/cal-video.c     |   6 +-
>  drivers/media/platform/vsp1/vsp1_video.c      |  18 +-
>  drivers/media/platform/xilinx/xilinx-dma.c    |  20 +-
>  drivers/media/platform/xilinx/xilinx-dma.h    |   2 +-
>  .../media/test-drivers/vimc/vimc-capture.c    |   6 +-
>  drivers/media/usb/au0828/au0828-core.c        |   8 +-
>  drivers/media/v4l2-core/v4l2-ioctl.c          |  25 +-
>  drivers/media/v4l2-core/v4l2-mc.c             |  77 ++--
>  drivers/media/v4l2-core/v4l2-subdev.c         | 368 ++++++++++++++++++
>  drivers/staging/media/imx/imx-media-utils.c   |   8 +-
>  drivers/staging/media/ipu3/ipu3-v4l2.c        |   6 +-
>  drivers/staging/media/omap4iss/iss.c          |   2 +-
>  drivers/staging/media/omap4iss/iss_video.c    |  38 +-
>  drivers/staging/media/omap4iss/iss_video.h    |   2 +-
>  drivers/staging/media/tegra-video/tegra210.c  |   6 +-
>  include/media/media-entity.h                  | 143 +++++--
>  include/media/v4l2-mc.h                       |  22 ++
>  include/media/v4l2-subdev.h                   |  93 ++++-
>  include/uapi/linux/v4l2-subdev.h              |  44 +++
>  42 files changed, 1241 insertions(+), 299 deletions(-)
>  create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
Tomi Valkeinen April 21, 2021, 12:57 p.m. UTC | #5
On 18/04/2021 20:32, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Nice to see a v5 of this plagued patch series :-)
> 
> On Thu, Apr 15, 2021 at 04:04:26PM +0300, Tomi Valkeinen wrote:
>> Hi,
>>
>> This is an RFC for subdev internal routing which is needed for
>> multiplexed streams support. I believe this is essentially a v5 of the
>> series, the v4 posted here:
>>
>> https://lore.kernel.org/linux-media/20190328200608.9463-1-jacopo+renesas@jmondi.org/
>>
>> Most of the patches are not changed (aside from fixing rebase issues
>> etc). The major changes in this version are:
>>
>> 1) Added 'which' field to the routing structs. It is currently not used,
>> as implementing it is not trivial. However, I think it's good to add it
>> to the uAPI now, and require the field to be set to
>> V4L2_SUBDEV_FORMAT_ACTIVE for now. See this RFC for an idea how this
>> could be implemented:
>>
>> https://lore.kernel.org/linux-media/20210409133659.389544-1-tomi.valkeinen@ideasonboard.com/
> 
> I've reviewed that, and I like it, but it's not straightforward to
> understand from that patch how you envision TRY to be implemented.

To be honest I don't have much at all experience with TRY. But, afaics, 
if we have means to store the TRY routes, and that is passed to the 
relevant ioctls in the subdev's, isn't that enough? It's up to the 
driver to implement the TRY functionality.

Although currently the S_ROUTING won't return anything to the userspace, 
it's supposed to either accept the routing table or return an error, 
whereas the S_FMT will do it's best to come up with a working setup and 
return it.

>> 2) No hardcoded maximum number of routes. Defining a maximum is not
>> possible, as there can be an arbitrary amount of routes per pad, and
>> there can be an arbitrary amount of pads per subdev. This series
>> allocates space for the routing table dynamically, which unfortunately
>> leads to not-just-a-few allocs and frees.
>>
>> 3) When searching for a format for a stream, the v4 looked for a
>> non-multiplexed pad only as far as the "other" side of the subdev. It
>> wouldn't work for a subdev which has multiplexed sink and source pads.
>> This series implements a "deep" get-format (v4l2_subdev_get_format_dir)
>> which follows a stream either towards the original source or the final
>> sink, while looking for a non-multiplexed pad with a format.
>>
>> Some thoughts:
>>
>> 1) Link validation and v4l2_subdev_get_format_dir need to look at the
>> routing, and this leads to multiple allocs to get a copy of the routing
>> table. There might be a possibility here to keep a table allocated and
>> re-use it in consecutive get_routing calls.
>>
>> Or even better, perhaps the kAPI could be changed so that allocs are not
>> needed. I thought about a kAPI where the subdev just returns a pointer
>> to its routing table, but then we hit the life-cycle problem: how to
>> ensure the table won't be freed or changed until the caller is done.
> 
> Storing the routing table in the v4l2_subdev_config (or
> v4l2_subdev_state) would be one way to do so, and I'd like to explore
> that direction. State lifetime is indeed an issue, and one simple option
> would be to just take the graph lock to modify the routing.
> 
>> 2) The routing uAPI is a bit vague. There is no way for the userspace to
>> figure out what kind of routing is allowed. Also, the existence of a
>> route in the routing table already indicates that the route is active,
>> but we also have V4L2_SUBDEV_ROUTE_FL_ACTIVE. I decided to keep
>> V4L2_SUBDEV_ROUTE_FL_ACTIVE for now, even if it doesn't really provide
>> any feature.
> 
> We can't report all possible routes if we take streams into account, but
> maybe we could report all possible routes between pads ? This could go
> through a separate ioctl.

That should be doable, but I wonder how much it helps. We should also 
somehow indicate if, say, routes from two source pads can go to the same 
sink pads, or can two streams from a single source pad go to separate 
sink pads, and so on. Is it better just to document what the driver 
supports for a specific hardware, than try to come up with a machine 
readable representation of the possible routings.

  Tomi
Laurent Pinchart April 29, 2021, 1:27 a.m. UTC | #6
Hi Tomi,

On Wed, Apr 21, 2021 at 03:57:07PM +0300, Tomi Valkeinen wrote:
> On 18/04/2021 20:32, Laurent Pinchart wrote:
> > On Thu, Apr 15, 2021 at 04:04:26PM +0300, Tomi Valkeinen wrote:
> >> Hi,
> >>
> >> This is an RFC for subdev internal routing which is needed for
> >> multiplexed streams support. I believe this is essentially a v5 of the
> >> series, the v4 posted here:
> >>
> >> https://lore.kernel.org/linux-media/20190328200608.9463-1-jacopo+renesas@jmondi.org/
> >>
> >> Most of the patches are not changed (aside from fixing rebase issues
> >> etc). The major changes in this version are:
> >>
> >> 1) Added 'which' field to the routing structs. It is currently not used,
> >> as implementing it is not trivial. However, I think it's good to add it
> >> to the uAPI now, and require the field to be set to
> >> V4L2_SUBDEV_FORMAT_ACTIVE for now. See this RFC for an idea how this
> >> could be implemented:
> >>
> >> https://lore.kernel.org/linux-media/20210409133659.389544-1-tomi.valkeinen@ideasonboard.com/
> > 
> > I've reviewed that, and I like it, but it's not straightforward to
> > understand from that patch how you envision TRY to be implemented.
> 
> To be honest I don't have much at all experience with TRY. But, afaics, 
> if we have means to store the TRY routes, and that is passed to the 
> relevant ioctls in the subdev's, isn't that enough? It's up to the 
> driver to implement the TRY functionality.

That should be fine yes.

> Although currently the S_ROUTING won't return anything to the userspace, 
> it's supposed to either accept the routing table or return an error, 
> whereas the S_FMT will do it's best to come up with a working setup and 
> return it.

That part is fine, as long as userspace has a way to figure out what
routes can be enabled, it's an good behaviour.

> >> 2) No hardcoded maximum number of routes. Defining a maximum is not
> >> possible, as there can be an arbitrary amount of routes per pad, and
> >> there can be an arbitrary amount of pads per subdev. This series
> >> allocates space for the routing table dynamically, which unfortunately
> >> leads to not-just-a-few allocs and frees.
> >>
> >> 3) When searching for a format for a stream, the v4 looked for a
> >> non-multiplexed pad only as far as the "other" side of the subdev. It
> >> wouldn't work for a subdev which has multiplexed sink and source pads.
> >> This series implements a "deep" get-format (v4l2_subdev_get_format_dir)
> >> which follows a stream either towards the original source or the final
> >> sink, while looking for a non-multiplexed pad with a format.
> >>
> >> Some thoughts:
> >>
> >> 1) Link validation and v4l2_subdev_get_format_dir need to look at the
> >> routing, and this leads to multiple allocs to get a copy of the routing
> >> table. There might be a possibility here to keep a table allocated and
> >> re-use it in consecutive get_routing calls.
> >>
> >> Or even better, perhaps the kAPI could be changed so that allocs are not
> >> needed. I thought about a kAPI where the subdev just returns a pointer
> >> to its routing table, but then we hit the life-cycle problem: how to
> >> ensure the table won't be freed or changed until the caller is done.
> > 
> > Storing the routing table in the v4l2_subdev_config (or
> > v4l2_subdev_state) would be one way to do so, and I'd like to explore
> > that direction. State lifetime is indeed an issue, and one simple option
> > would be to just take the graph lock to modify the routing.
> > 
> >> 2) The routing uAPI is a bit vague. There is no way for the userspace to
> >> figure out what kind of routing is allowed. Also, the existence of a
> >> route in the routing table already indicates that the route is active,
> >> but we also have V4L2_SUBDEV_ROUTE_FL_ACTIVE. I decided to keep
> >> V4L2_SUBDEV_ROUTE_FL_ACTIVE for now, even if it doesn't really provide
> >> any feature.
> > 
> > We can't report all possible routes if we take streams into account, but
> > maybe we could report all possible routes between pads ? This could go
> > through a separate ioctl.
> 
> That should be doable, but I wonder how much it helps. We should also 
> somehow indicate if, say, routes from two source pads can go to the same 
> sink pads, or can two streams from a single source pad go to separate 
> sink pads, and so on. Is it better just to document what the driver 
> supports for a specific hardware, than try to come up with a machine 
> readable representation of the possible routings.

I'd like userspace to have at least some level of genericity. I have two
devices in mind that have cross-bar switches between CSI-2 receivers and
DMA engines (or further processing pipelines), and I'd like that part to
be handled by userspace code that isn't device-specific. We can focus on
the API to configure routing first, and then see how discoverability
could be implemented.