Re: [PATCH v9 27/36] media: subdev: Add [GS]_ROUTING subdev ioctls and operations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/10/2021 10:36, Laurent Pinchart wrote:
Hi Tomi,

Thank you for the patch.

On Tue, Oct 05, 2021 at 11:57:41AM +0300, Tomi Valkeinen wrote:
From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

Add support for subdev internal routing. A route is defined as a single
stream from a sink pad to a source pad.

The userspace can configure the routing via two new ioctls,
VIDIOC_SUBDEV_G_ROUTING and VIDIOC_SUBDEV_S_ROUTING, and subdevs can
implement the functionality with v4l2_subdev_pad_ops.set_routing().

Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>

- Add sink and source streams for multiplexed links
- Copy the argument back in case of an error. This is needed to let the
   caller know the number of routes.

Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>

- Expand and refine documentation.
- Make the 'routes' pointer a __u64 __user pointer so that a compat32
   version of the ioctl is not required.
- Add struct v4l2_subdev_krouting to be used for subdevice operations.

Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>

- Fix typecasing warnings
- Check sink & source pad types
- Add 'which' field
- Add V4L2_SUBDEV_ROUTE_FL_SOURCE
- Routing to subdev state
- Dropped get_routing subdev op

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
---
  drivers/media/v4l2-core/v4l2-ioctl.c  | 25 ++++++++-
  drivers/media/v4l2-core/v4l2-subdev.c | 75 +++++++++++++++++++++++++++
  include/media/v4l2-subdev.h           | 22 ++++++++
  include/uapi/linux/v4l2-subdev.h      | 57 ++++++++++++++++++++
  4 files changed, 178 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 05d5db3d85e5..8e9315ffcb99 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -16,6 +16,7 @@
  #include <linux/kernel.h>
  #include <linux/version.h>
+#include <linux/v4l2-subdev.h>
  #include <linux/videodev2.h>
#include <media/v4l2-common.h>
@@ -3065,6 +3066,21 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
  		ret = 1;
  		break;
  	}
+
+	case VIDIOC_SUBDEV_G_ROUTING:
+	case VIDIOC_SUBDEV_S_ROUTING: {
+		struct v4l2_subdev_routing *routing = parg;
+
+		if (routing->num_routes > 256)
+			return -EINVAL;
+
+		*user_ptr = u64_to_user_ptr(routing->routes);
+		*kernel_ptr = (void **)&routing->routes;
+		*array_size = sizeof(struct v4l2_subdev_route)
+			    * routing->num_routes;
+		ret = 1;
+		break;
+	}
  	}
return ret;
@@ -3328,8 +3344,15 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
  	/*
  	 * Some ioctls can return an error, but still have valid
  	 * results that must be returned.
+	 *
+	 * FIXME: subdev IOCTLS are partially handled here and partially in
+	 * v4l2-subdev.c and the 'always_copy' flag can only be set for IOCTLS
+	 * defined here as part of the 'v4l2_ioctls' array. As
+	 * VIDIOC_SUBDEV_G_ROUTING needs to return results to applications even
+	 * in case of failure, but it is not defined here as part of the
+	 * 'v4l2_ioctls' array, insert an ad-hoc check to address that.
  	 */
-	if (err < 0 && !always_copy)
+	if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING)
  		goto out;
out_array_args:
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 52309a299b03..b4e1f8772d96 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -394,6 +394,12 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
  		which = ((struct v4l2_subdev_selection *)arg)->which;
  		break;
  	}
+
+	case VIDIOC_SUBDEV_G_ROUTING:
+	case VIDIOC_SUBDEV_S_ROUTING: {
+		which = ((struct v4l2_subdev_routing *)arg)->which;
+		break;
+	}
  	}
return which == V4L2_SUBDEV_FORMAT_TRY ?
@@ -710,6 +716,74 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
  	case VIDIOC_SUBDEV_QUERYSTD:
  		return v4l2_subdev_call(sd, video, querystd, arg);
+ case VIDIOC_SUBDEV_G_ROUTING: {
+		struct v4l2_subdev_routing *routing = arg;
+		struct v4l2_subdev_krouting *krouting;
+
+		if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED))
+			return -ENOIOCTLCMD;

Do we really want to condition the routing API to multiplexed streams
support ? Internal subdev routing is useful for devices that don't
support multiplexed streams, and forcing a switch to the multiplexed
streams API for those concerns me. In particular, with the multiplexed
streams API, pad formats only exist once routes are created. This is a
major change compared to the existing API (and it led to the creation of
the V4L2_SUBDEV_ROUTE_FL_SOURCE flag, which I'm still not entirely sure
I like) and I don't have a good enough view of all its side effects yet.

This being said, if conditioning formats availability on pads to the
existence of routes causes issues for such devices, maybe they would be
better solved globally, as they could affect devices supporting
multiplexed streams too.

V4L2_SUBDEV_FL_MULTIPLEXED doesn't exactly mean "the subdev supports multiplexed streams". It means "the subdev supports central active state, routing and multiplexed streams".

I thought about keeping the meaning, but renaming the flag, but I have no good idea for the name.

I thought about splitting the flag into separate parts (those three above), but the features are kind of interleaved: 1) both routing and streams depend on the new state, 2) streams depends on routing, 3) routing depends on streams.

Those dependencies exist currently in the code. With some effort I believe some or all of the dependencies could be removed.

But I would have to ask, why? What's the use case and benefit?

"Multiplexed streams support in a subdev" doesn't mean it has to have multiple streams. In fact, I believe a subdev that supports just a single stream is easier to implement with the multiplexed streams API than with the older pad-based API.

If you add routing support to a subdev, you anyway have to rethink how it works and restructure the code, why not clean it up a bit more with the multiplexed streams API at the same time?

with the multiplexed streams API, pad formats only exist once routes are created

This is true, but the driver can force the routes to always exist. It can do this in the case the device does not support multiple streams. The route can be disabled, which should allow setting the formats but still keeping the route inactive. And it should be fine to have e.g. a single sink pad and 2 source pads, with routes from both source pads to the same sink pad, as long as the other one is disabled. But I haven't tested this, there might be a bug there somewhere.

I'm not a fan of V4L2_SUBDEV_ROUTE_FL_SOURCE either, but it wasn't created for the problem you describe. V4L2_SUBDEV_ROUTE_FL_SOURCE means that a subdev has an output stream that isn't routed to an input stream. You're talking about a case where you want a (pad,stream) to be configurable even if it's not routed anywhere (yet).

I think conceptually the flag is fine (i.e. a subdev with an output stream that isn't routed to an input stream). The problem is more about the routing table, as that is the place to define "stream configuration points". This is something I raised some time back, thinking out loud if the routing table should be dropped, and instead (somehow) define these configuration points per pad. So a pad would have a userspace defined list of (input-stream, other-pad, other-stream) tuples, where "other" means the pad/stream on the other side of the subdev. That "other" pad should have a matching, but reverse, entry.

It would be a bit cumbersome, though, I think. The routing table is an easier way to manage this, with V4L2_SUBDEV_ROUTE_FL_SOURCE flag. But if needed, we could look further into separating the routing and the management of the "config points".

 Tomi



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux