Hi Tomi,
Thank you for the patch.
On Fri, May 28, 2021 at 02:34:55PM +0300, Tomi Valkeinen wrote:
> On 24/05/2021 13:44, Tomi Valkeinen wrote:
> > Update v4l2_subdev_link_validate() to use routing and streams for
> > validation.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/v4l2-core/v4l2-subdev.c | 184 +++++++++++++++++++++++---
> > 1 file changed, 166 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index da6ea9b14631..b30b456d8d99 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>
> >
> > #include <media/v4l2-ctrls.h>
> > #include <media/v4l2-device.h>
> > @@ -894,6 +895,7 @@ EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
> >
> > static int
> > v4l2_subdev_link_validate_get_format(struct media_pad *pad,
> > + u32 stream,
> > struct v4l2_subdev_format *fmt)
> > {
> > if (is_media_entity_v4l2_subdev(pad->entity)) {
> > @@ -902,6 +904,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, NULL, fmt);
> > }
> >
> > @@ -1012,31 +1015,176 @@ bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing,
> > }
> > EXPORT_SYMBOL_GPL(v4l2_subdev_has_route);
> >
> > +static int cmp_u32(const void *a, const void *b)
> > +{
> > + u32 a32 = *(u32 *)a;
> > + u32 b32 = *(u32 *)b;
> > +
> > + return a32 > b32 ? 1 : (a32 < b32 ? -1 : 0);
> > +}
> > +
> > int v4l2_subdev_link_validate(struct media_link *link)
> > {
> > - struct v4l2_subdev *sink;
> > - struct v4l2_subdev_format sink_fmt, source_fmt;
> > - int rval;
> > + int ret;
> > + unsigned int i;
> >
No need for a blank line, and I'd move i and ret (in that order) after
the variables below.
> > - rval = v4l2_subdev_link_validate_get_format(
> > - link->source, &source_fmt);
> > - if (rval < 0)
> > - return 0;
> > + struct v4l2_subdev *source_subdev =
> > + media_entity_to_v4l2_subdev(link->source->entity);
> > + struct v4l2_subdev *sink_subdev =
> > + media_entity_to_v4l2_subdev(link->sink->entity);
> > + struct device *dev = sink_subdev->entity.graph_obj.mdev->dev;
> >
No need for a blank line here either.
> > - rval = v4l2_subdev_link_validate_get_format(
> > - link->sink, &sink_fmt);
> > - if (rval < 0)
> > - return 0;
> > + struct v4l2_subdev_krouting routing;
> >
Same.
> > - sink = media_entity_to_v4l2_subdev(link->sink->entity);
> > + static const u32 default_streams[] = { 0 };
I'd move this one at the top.
> >
> > - rval = v4l2_subdev_call(sink, pad, link_validate, link,
> > - &source_fmt, &sink_fmt);
> > - if (rval != -ENOIOCTLCMD)
> > - return rval;
> > + u32 num_source_streams = 0;
> > + const u32 *source_streams;
> > + u32 num_sink_streams = 0;
> > + const u32 *sink_streams;
> > +
> > + dev_dbg(dev, "validating link \"%s\":%u -> \"%s\":%u\n",
> > + link->source->entity->name, link->source->index,
> > + link->sink->entity->name, link->sink->index);
> > +
> > + /* Get source streams */
> > +
> > + memset(&routing, 0, sizeof(routing));
> > +
> > + ret = v4l2_subdev_get_routing(source_subdev, NULL, &routing);
> > +
> > + if (ret && ret != -ENOIOCTLCMD)
> > + return ret;
> > +
> > + if (ret == -ENOIOCTLCMD) {
> > + num_source_streams = 1;
> > + source_streams = default_streams;
> > + } else {
> > + u32 *streams;
> > +
> > + streams = kmalloc_array(routing.num_routes, sizeof(u32),
> > + GFP_KERNEL);
> > +
> > + for (i = 0; i < routing.num_routes; ++i) {
> > + struct v4l2_subdev_route *route = &routing.routes[i];
> > +
> > + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> > + continue;
> > +
> > + if (route->source_pad == link->source->index)
> > + streams[num_source_streams++] =
> > + route->source_stream;
> > + }
> > +
> > + sort(streams, num_source_streams, sizeof(u32), &cmp_u32, NULL);
> > +
> > + source_streams = streams;
> > +
> > + v4l2_subdev_free_routing(&routing);
> > + }
Could this be moved to a separate function ? The code below is very
similar.
> > +
> > + /* Get sink streams */
> > +
> > + memset(&routing, 0, sizeof(routing));
> > +
> > + ret = v4l2_subdev_get_routing(sink_subdev, NULL, &routing);
> > +
> > + if (ret && ret != -ENOIOCTLCMD)
> > + return ret;
> > +
> > + if (ret == -ENOIOCTLCMD) {
> > + num_sink_streams = 1;
> > + sink_streams = default_streams;
> > + } else {
> > + u32 *streams;
> >
> > - return v4l2_subdev_link_validate_default(
> > - sink, link, &source_fmt, &sink_fmt);
> > + streams = kmalloc_array(routing.num_routes, sizeof(u32),
> > + GFP_KERNEL);
> > +
> > + for (i = 0; i < routing.num_routes; ++i) {
> > + struct v4l2_subdev_route *route = &routing.routes[i];
> > +
> > + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> > + continue;
> > +
> > + if (route->sink_pad == link->sink->index)
> > + streams[num_sink_streams++] =
> > + route->sink_stream;
> > + }
> > +
> > + sort(streams, num_sink_streams, sizeof(u32), &cmp_u32, NULL);
Are you aware that there can be duplicate in the streams array, as a
given stream on a sink pad can be routed to multiple source pads ? I
think that will fail the sink and source streams match test below.
> > +
> > + sink_streams = streams;
> > +
> > + v4l2_subdev_free_routing(&routing);
> > + }
> > +
> > + if (num_source_streams != num_sink_streams) {
> > + dev_err(dev,
> > + "Sink and source stream count mismatch: %d vs %d\n",
> > + num_source_streams, num_sink_streams);
> > + return -EINVAL;
> > + }
> > +
> > + /* Validate source and sink stream formats */
> > +
> > + for (i = 0; i < num_source_streams; ++i) {
> > + struct v4l2_subdev_format sink_fmt, source_fmt;
> > + u32 stream;
> > + int ret;
> > +
> > + if (source_streams[i] != sink_streams[i]) {
> > + dev_err(dev, "Sink and source streams do not match\n");
> > + return -EINVAL;
> > + }
What if the source subdev as a route enabled that produces a stream, and
the sink subdev has not corresponding enabled route ? Isn't this a valid
configuration ? For instance, when a CSI-2 sensor produces image data
and embedded data in two streams with different CSI-2 DT, the embedded
data doesn't have to be captured, it could be dropped in the CSI-2
receiver.
> > +
> > + stream = source_streams[i];
> > +
> > + 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_err(dev, "Failed to get format for \"%s\":%u:%u\n",
> > + link->source->entity->name, link->source->index,
> > + stream);
> > + return ret;
> > + }
> > +
> > + ret = v4l2_subdev_link_validate_get_format(link->sink, stream,
> > + &sink_fmt);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to get format for \"%s\":%u:%u\n",
> > + link->sink->entity->name, link->sink->index,
> > + stream);
> > + return ret;
> > + }
> > +
> > + /* 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)
> > + return ret;
> > +
> > + ret = v4l2_subdev_link_validate_default(sink_subdev, link,
> > + &source_fmt, &sink_fmt);
> > +
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (source_streams != default_streams)
> > + kfree(source_streams);
> > +
> > + if (sink_streams != default_streams)
> > + kfree(sink_streams);
> > +
> > + return 0;
> > }
> > EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
> >
>
> I noticed two issues with this patch:
>
> - It leaks memory on error cases.
>
> - The previous behavior silently ignored failures to get format from
> subdevs, and returned 0. This one fails.
>
> I'll fix those.
--
Regards,
Laurent Pinchart
