Re: [PATCH v1 3/4] media-ctl: add support for routes and streams

[Pratyush Yadav <p.yadav@ti.com>]

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@linux.intel.com>.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

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@ti.com>

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.