On 16/03/2022 12:47, Jacopo Mondi wrote:
Hi Tomi
On Tue, Mar 01, 2022 at 06:11:47PM +0200, Tomi Valkeinen wrote:
Update v4l2_subdev_link_validate() to use routing and streams for
validation.
Instead of just looking at the format on the pad on both ends of the
link, the routing tables are used to collect all the streams going from
the source to the sink over the link, and the streams' formats on both
ends of the link are verified.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
---
drivers/media/v4l2-core/v4l2-subdev.c | 245 ++++++++++++++++++++++++--
1 file changed, 227 insertions(+), 18 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 339d7b15e26c..091b854e00d0 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -16,6 +16,7 @@
#include <linux/videodev2.h>
#include <linux/export.h>
#include <linux/version.h>
+#include <linux/sort.h>
Not sorted, but it was already like this
#include <media/v4l2-ctrls.h>
#include <media/v4l2-device.h>
@@ -1035,6 +1036,7 @@ EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
static int
v4l2_subdev_link_validate_get_format(struct media_pad *pad,
+ u32 stream,
fits on the previous line
struct v4l2_subdev_format *fmt)
{
if (is_media_entity_v4l2_subdev(pad->entity)) {
@@ -1046,6 +1048,7 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad,
fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
fmt->pad = pad->index;
+ fmt->stream = stream;
return v4l2_subdev_call(sd, pad, get_fmt, state, fmt);
}
@@ -1056,31 +1059,237 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad,
return -EINVAL;
}
-int v4l2_subdev_link_validate(struct media_link *link)
+static int cmp_u32(const void *a, const void *b)
{
- struct v4l2_subdev *sink;
- struct v4l2_subdev_format sink_fmt, source_fmt;
- int rval;
+ u32 a32 = *(u32 *)a;
+ u32 b32 = *(u32 *)b;
- rval = v4l2_subdev_link_validate_get_format(
- link->source, &source_fmt);
- if (rval < 0)
- return 0;
+ return a32 > b32 ? 1 : (a32 < b32 ? -1 : 0);
+}
+
+static int v4l2_link_validate_get_streams(struct media_link *link,
+ bool is_source, u32 *out_num_streams,
+ const u32 **out_streams,
+ bool *allocated)
+{
+ static const u32 default_streams[] = { 0 };
+ struct v4l2_subdev_krouting *routing;
+ struct v4l2_subdev *subdev;
+ u32 num_streams;
+ u32 *streams;
+ unsigned int i;
+ struct v4l2_subdev_state *state;
+
+ if (is_source)
+ subdev = media_entity_to_v4l2_subdev(link->source->entity);
+ else
+ subdev = media_entity_to_v4l2_subdev(link->sink->entity);
- rval = v4l2_subdev_link_validate_get_format(
- link->sink, &sink_fmt);
- if (rval < 0)
+ if (!(subdev->flags & V4L2_SUBDEV_FL_MULTIPLEXED)) {
+ *out_num_streams = 1;
+ *out_streams = default_streams;
+ *allocated = false;
return 0;
+ }
- sink = media_entity_to_v4l2_subdev(link->sink->entity);
+ state = v4l2_subdev_get_locked_active_state(subdev);
- rval = v4l2_subdev_call(sink, pad, link_validate, link,
- &source_fmt, &sink_fmt);
- if (rval != -ENOIOCTLCMD)
- return rval;
+ routing = &state->routing;
+
+ streams = kmalloc_array(routing->num_routes, sizeof(u32), GFP_KERNEL);
+ if (!streams)
+ return -ENOMEM;
+
+ num_streams = 0;
+
+ for (i = 0; i < routing->num_routes; ++i) {
+ struct v4l2_subdev_route *route = &routing->routes[i];
+ int j;
+ u32 route_pad;
+ u32 route_stream;
+ u32 link_pad;
+
+ if (is_source) {
+ route_pad = route->source_pad;
+ route_stream = route->source_stream;
+ link_pad = link->source->index;
+ } else {
+ route_pad = route->sink_pad;
+ route_stream = route->sink_stream;
+ link_pad = link->sink->index;
+ }
+
+ if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
+ continue;
Should this check be done before the previous if/else block ?
Yes, and I think this can use for_each_active_route.
+
+ if (route_pad != link_pad)
+ continue;
+
+ /* look for duplicates... */
+ for (j = 0; j < num_streams; ++j) {
+ if (streams[j] == route_stream)
+ break;
+ }
+
+ /* ...and drop the stream if we already have it */
+ if (j != num_streams)
+ continue;
Is this an error ? can the same stream be routed to multiple ones on a
media link ?
There must have been a reason for me to add this code, but... Indeed, we
shouldn't have duplicates. I'll change this to an error.
+
+ streams[num_streams++] = route_stream;
+ }
+
+ sort(streams, num_streams, sizeof(u32), &cmp_u32, NULL);
- return v4l2_subdev_link_validate_default(
- sink, link, &source_fmt, &sink_fmt);
+ *out_num_streams = num_streams;
+ *out_streams = streams;
+ *allocated = true;
+
+ return 0;
+}
+
+static int v4l2_subdev_link_validate_locked(struct media_link *link)
+{
+ struct v4l2_subdev *sink_subdev =
+ media_entity_to_v4l2_subdev(link->sink->entity);
+ struct device *dev = sink_subdev->entity.graph_obj.mdev->dev;
+ u32 num_source_streams;
+ const u32 *source_streams;
+ bool source_allocated;
+ u32 num_sink_streams;
+ const u32 *sink_streams;
+ bool sink_allocated;
+ unsigned int sink_idx;
+ unsigned int source_idx;
+ int ret;
+
+ dev_dbg(dev, "validating link \"%s\":%u -> \"%s\":%u\n",
+ link->source->entity->name, link->source->index,
+ link->sink->entity->name, link->sink->index);
+
+ ret = v4l2_link_validate_get_streams(link, true, &num_source_streams,
+ &source_streams,
+ &source_allocated);
+ if (ret)
+ return ret;
+
+ ret = v4l2_link_validate_get_streams(link, false, &num_sink_streams,
+ &sink_streams, &sink_allocated);
+ if (ret)
+ goto free_source;
+
+ /*
+ * It is ok to have more source streams than sink streams as extra
+ * source streams can just be ignored (i.e. they go nowhere), but extra
Nit on the "(i.e. they go nowhere)" bit:
If it's ok to have more source streams than sinks, the extra sources
will not be connected. So I would drop or change the above phrase as
to me it sounds like sink streams go somewhere, while source stream
are rather not connected (or not active).
Ok, I'll clarify.
+ * sink streams is an error.
+ */
+ if (num_source_streams < num_sink_streams) {
+ dev_err(dev,
+ "Not enough source streams: %d < %d\n",
+ num_source_streams, num_sink_streams);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ /* Validate source and sink stream formats */
+
+ source_idx = 0;
+
+ for (sink_idx = 0; sink_idx < num_sink_streams; ++sink_idx) {
+ struct v4l2_subdev_format sink_fmt, source_fmt;
+ u32 stream;
+
+ stream = sink_streams[sink_idx];
+
+ for (; source_idx < num_source_streams; ++source_idx) {
+ if (source_streams[source_idx] == stream)
+ break;
+ }
+
+ if (source_idx == num_source_streams) {
+ dev_err(dev, "No source stream for sink stream %u\n",
+ stream);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ dev_dbg(dev, "validating stream \"%s\":%u:%u -> \"%s\":%u:%u\n",
+ link->source->entity->name, link->source->index, stream,
+ link->sink->entity->name, link->sink->index, stream);
+
+ ret = v4l2_subdev_link_validate_get_format(link->source, stream,
+ &source_fmt);
+ if (ret < 0) {
+ dev_dbg(dev, "Failed to get format for \"%s\":%u:%u (but that's ok)\n",
For my education: why is it ok not to have a format for the stream ?
I don't know, but afaics that's how it has always been. In upstream
v4l2_subdev_link_validate() returns 0 if
v4l2_subdev_link_validate_get_format() fails.
Don't we risk to pass a non-initialized source/sink_fmt to
link_validate() in that case ?
We don't call link_validate, we accept the stream right away.
+ link->source->entity->name, link->source->index,
+ stream);
+ ret = 0;
+ continue;
+ }
+
+ ret = v4l2_subdev_link_validate_get_format(link->sink, stream,
+ &sink_fmt);
+ if (ret < 0) {
+ dev_dbg(dev, "Failed to get format for \"%s\":%u:%u (but that's ok)\n",
+ link->sink->entity->name, link->sink->index,
+ stream);
+ ret = 0;
+ continue;
+ }
+
+ /* TODO: add stream number to link_validate() */
+ ret = v4l2_subdev_call(sink_subdev, pad, link_validate, link,
+ &source_fmt, &sink_fmt);
+ if (!ret)
+ continue;
+
+ if (ret != -ENOIOCTLCMD)
+ goto out;
+
+ ret = v4l2_subdev_link_validate_default(sink_subdev, link,
+ &source_fmt, &sink_fmt);
+
+ if (ret)
+ goto out;
+ }
+
+out:
I would call it free_sinks:
Ok.
Tomi
