Re: [EXT] [PATCH v12 18/30] media: subdev: use streams in v4l2_subdev_link_validate()

[Satish Nagireddy <satish.nagireddy@getcruise.com>]

Hi Tomi,

Thanks for the patch.

On Wed, Jul 27, 2022 at 3:37 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> 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@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 255 ++++++++++++++++++++++++--
>  1 file changed, 235 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index d26715cbd955..7ca2337ca8d3 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>
> @@ -1069,7 +1070,7 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
>  EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
>
>  static int
> -v4l2_subdev_link_validate_get_format(struct media_pad *pad,
> +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)) {
> @@ -1078,7 +1079,11 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad,
>
>                 fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
>                 fmt->pad = pad->index;
> -               return v4l2_subdev_call_state_active(sd, pad, get_fmt, fmt);
> +               fmt->stream = stream;
> +
> +               return v4l2_subdev_call(sd, pad, get_fmt,
> +                                       v4l2_subdev_get_locked_active_state(sd),
> +                                       fmt);
>         }
>
>         WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L,
> @@ -1088,31 +1093,241 @@ 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)

I have a suggestion to refactor this function a bit to avoid the usage
of is_source boolean, hopefully that makes this implementation look
better.
Here is my idea: Pass the media_pad (source or sink) to the function
directly and drop the parameters link and is_source. Then...

> +{
> +       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;
> +       int ret;
>
> -       rval = v4l2_subdev_link_validate_get_format(
> -               link->sink, &sink_fmt);
> -       if (rval < 0)
> +       if (is_source)
> +               subdev = media_entity_to_v4l2_subdev(link->source->entity);
> +       else
> +               subdev = media_entity_to_v4l2_subdev(link->sink->entity);

... we can avoid the if check here and directly assign subdev as below.
     subdev = media_entity_to_v4l2_subdev(pad->entity);
Then...

> +
> +       if (!(subdev->flags & V4L2_SUBDEV_FL_STREAMS)) {
> +               *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 (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> +                       continue;
> +
> +               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;
> +               }

This can be achieved by checking pad->flags against
MEDIA_PAD_FL_SOURCE or MEDIA_PAD_FL_SINK.
Please let me know your opinion.

> +
> +               if (route_pad != link_pad)
> +                       continue;
> +
> +               /* look for duplicates */
> +               for (j = 0; j < num_streams; ++j) {
> +                       if (streams[j] == route_stream) {
> +                               ret = -EINVAL;
> +                               goto free_streams;
> +                       }
> +               }

This is good logic, but seems to be a bit inefficient as it is
repeatedly checking the array from the start.
I do not have a better idea :)

> +
> +               streams[num_streams++] = route_stream;
> +       }
> +
> +       sort(streams, num_streams, sizeof(u32), &cmp_u32, NULL);
> +
> +       *out_num_streams = num_streams;
> +       *out_streams = streams;
> +       *allocated = true;
> +
> +       return 0;
> +
> +free_streams:
> +       kfree(streams);
> +
> +       return ret;
> +}
> +
> +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,

This function call can be modified as below
    v4l2_link_validate_get_streams(link->source, &num_source_streams,

> +                                            &source_streams,
> +                                            &source_allocated);
> +       if (ret)
> +               return ret;
> +
> +       ret = v4l2_link_validate_get_streams(link, false, &num_sink_streams,

This function call can be modified as below
    v4l2_link_validate_get_streams(link->sink, &num_source_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 by the receiver, but having extra
> +        * sink streams is an error as streams must have a source.
> +        */
> +       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 free_sink;
> +       }
> +
> +       /* Validate source and sink stream formats */
> +
> +       source_idx = 0;

Nit, This can be assigned at the declaration.

Regards,
Satish

> +
> +       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 free_sink;
> +               }
> +
> +               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",
> +                               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;
> +               }
>
> -       return v4l2_subdev_link_validate_default(
> -               sink, link, &source_fmt, &sink_fmt);
> +               /* 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 free_sink;
> +
> +               ret = v4l2_subdev_link_validate_default(sink_subdev, link,
> +                                                       &source_fmt, &sink_fmt);
> +
> +               if (ret)
> +                       goto free_sink;
> +       }
> +
> +free_sink:
> +       if (sink_allocated)
> +               kfree(sink_streams);
> +
> +free_source:
> +       if (source_allocated)
> +               kfree(source_streams);
> +
> +       return ret;
> +}
> +
> +int v4l2_subdev_link_validate(struct media_link *link)
> +{
> +       struct v4l2_subdev *source_sd, *sink_sd;
> +       struct v4l2_subdev_state *source_state, *sink_state;
> +       int ret;
> +
> +       sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
> +       source_sd = media_entity_to_v4l2_subdev(link->source->entity);
> +
> +       sink_state = v4l2_subdev_get_unlocked_active_state(sink_sd);
> +       source_state = v4l2_subdev_get_unlocked_active_state(source_sd);
> +
> +       if (sink_state)
> +               v4l2_subdev_lock_state(sink_state);
> +
> +       if (source_state)
> +               v4l2_subdev_lock_state(source_state);
> +
> +       ret = v4l2_subdev_link_validate_locked(link);
> +
> +       if (sink_state)
> +               v4l2_subdev_unlock_state(sink_state);
> +
> +       if (source_state)
> +               v4l2_subdev_unlock_state(source_state);
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
>
> --
> 2.34.1
>