Hi Tomi,
On 30/11/21 04:18PM, Tomi Valkeinen wrote:
> Add support to get and set subdev routes and to get and set
> configurations per stream.
>
> Based on work from Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
I tried this on TI's downstream fork with your routing patches. It works
fine for setting and viewing formats and routes.
Tested-by: Pratyush Yadav <p.yadav@xxxxxx>
I have not gone through the code too thoroughly. A few small things I
noticed below.
> ---
> utils/media-ctl/libmediactl.c | 41 +++++
> utils/media-ctl/libv4l2subdev.c | 256 ++++++++++++++++++++++++++++----
> utils/media-ctl/media-ctl.c | 113 +++++++++++---
> utils/media-ctl/mediactl.h | 16 ++
> utils/media-ctl/options.c | 15 +-
> utils/media-ctl/options.h | 1 +
> utils/media-ctl/v4l2subdev.h | 58 +++++++-
> 7 files changed, 444 insertions(+), 56 deletions(-)
>
[...]
> +int v4l2_subdev_get_routing(struct media_entity *entity,
> + struct v4l2_subdev_route **routes,
> + unsigned int *num_routes)
> +{
> + struct v4l2_subdev_routing routing = { 0 };
> + struct v4l2_subdev_route *r;
> + int ret;
> +
> + ret = v4l2_subdev_open(entity);
> + if (ret < 0)
> + return ret;
> +
> + routing.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +
> + ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_ROUTING, &routing);
> + if (ret == -1 && errno != ENOSPC)
> + return -errno;
> +
> + r = calloc(routing.num_routes, sizeof(*r));
> + if (!r)
calloc man page says:
If nmemb or size is 0, then calloc() returns either NULL, or a unique
pointer value that can later be successfully passed to free().
So if a subdev reports 0 routes then you could end up with a non-NULL
pointer that you should not use, other than to pass to free(). I don't
know if any implementation out there does actually return anything other
than NULL though. I suggest explicitly checking for num_routes == 0 to
avoid all this.
> + return -ENOMEM;
> +
> + routing.routes = (uintptr_t)r;
> + ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_ROUTING, &routing);
> + if (ret) {
> + free(r);
> + return ret;
> + }
> +
> + *num_routes = routing.num_routes;
> + *routes = r;
> +
> + return 0;
> +}
> +
> int v4l2_subdev_get_dv_timings_caps(struct media_entity *entity,
> struct v4l2_dv_timings_cap *caps)
> {
[...]
> @@ -306,6 +370,135 @@ int v4l2_subdev_set_frame_interval(struct media_entity *entity,
> return 0;
> }
>
> +int v4l2_subdev_parse_setup_routes(struct media_device *media, const char *p)
> +{
> + struct media_entity *entity;
> + struct v4l2_subdev_route *routes;
> + unsigned int num_routes;
> + char *end;
> + int ret;
> + int i;
> +
> + entity = media_parse_entity(media, p, &end);
> + if (!entity)
> + return -EINVAL;
> +
> + p = end;
> +
> + if (*p != '[') {
> + media_dbg(media, "Expected '['\n");
> + return -EINVAL;
> + }
> +
> + p++;
> +
> + routes = calloc(256, sizeof(routes[0]));
You are not checking for NULL here.
> + num_routes = 0;
> +
> + while (*p != 0) {
> + struct v4l2_subdev_route *r = &routes[num_routes];
> +
> + /* sink pad/stream */
> +
> + r->sink_pad = strtoul(p, &end, 10);
> +
> + if (*end != '/') {
> + media_dbg(media, "Expected '/'\n");
> + return -EINVAL;
> + }
> +
> + p = end + 1;
> +
> + r->sink_stream = strtoul(p, &end, 10);
> +
> + for (; isspace(*end); ++end);
> +
> + if (end[0] != '-' || end[1] != '>') {
> + media_dbg(media, "Expected '->'\n");
> + return -EINVAL;
> + }
> + p = end + 2;
> +
> + /* source pad/stream */
> +
> + r->source_pad = strtoul(p, &end, 10);
> +
> + if (*end != '/') {
> + media_dbg(media, "Expected '/'\n");
> + return -EINVAL;
> + }
> +
> + p = end + 1;
> +
> + r->source_stream = strtoul(p, &end, 10);
> +
> + /* flags */
> +
> + for (; isspace(*end); ++end);
> +
> + if (*end != '[') {
> + media_dbg(media, "Expected '['\n");
> + return -EINVAL;
> + }
> +
> + for (end++; isspace(*end); ++end);
> +
> + p = end;
> +
> + r->flags = strtoul(p, &end, 0);
> +
> + if (r->flags & ~(V4L2_SUBDEV_ROUTE_FL_ACTIVE |
> + V4L2_SUBDEV_ROUTE_FL_IMMUTABLE |
> + V4L2_SUBDEV_ROUTE_FL_SOURCE)) {
> + media_dbg(media, "Bad route flags %#x\n", r->flags);
> + return -EINVAL;
> + }
> +
> + for (; isspace(*end); ++end);
> +
> + if (*end != ']') {
> + media_dbg(media, "Expected ']'\n");
> + return -EINVAL;
> + }
> + end++;
> +
> + p = end;
> +
> + num_routes++;
> +
> + if (*p == ',') {
> + p++;
> + continue;
> + }
> +
> + break;
> + }
> +
> + if (*p != ']') {
> + media_dbg(media, "Expected ']'\n");
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < num_routes; ++i) {
> + struct v4l2_subdev_route *r = &routes[i];
> +
> + media_dbg(entity->media,
> + "Setting up route %s : %u/%u -> %u/%u, flags 0x%8.8x\n",
> + entity->info.name,
> + r->sink_pad, r->sink_stream,
> + r->source_pad, r->source_stream,
> + r->flags);
> + }
> +
> + ret = v4l2_subdev_set_routing(entity, routes, num_routes);
> + if (ret) {
> + printf("VIDIOC_SUBDEV_S_ROUTING failed: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int v4l2_subdev_parse_format(struct media_device *media,
> struct v4l2_mbus_framefmt *format,
> const char *p, char **endp)
[...]
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
