Re: [PATCH v15 11/19] media: subdev: use streams in v4l2_subdev_link_validate()

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Moi,

On Mon, Oct 03, 2022 at 03:18:44PM +0300, 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@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 182 +++++++++++++++++++++++---
>  1 file changed, 162 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index be778e619694..1cea6ec750c0 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1014,7 +1014,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)) {
> @@ -1023,7 +1023,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,
> @@ -1033,31 +1037,169 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad,
>  	return -EINVAL;
>  }
>  
> +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> +
> +static void __v4l2_link_validate_get_streams(struct media_pad *pad,
> +					     u64 *streams_mask)
> +{
> +	struct v4l2_subdev_route *route;
> +	struct v4l2_subdev_state *state;
> +	struct v4l2_subdev *subdev;
> +
> +	subdev = media_entity_to_v4l2_subdev(pad->entity);
> +
> +	*streams_mask = 0;
> +
> +	state = v4l2_subdev_get_locked_active_state(subdev);
> +	if (WARN_ON(!state))
> +		return;
> +
> +	for_each_active_route(&state->routing, route) {
> +		u32 route_pad;
> +		u32 route_stream;
> +
> +		if (pad->flags & MEDIA_PAD_FL_SOURCE) {
> +			route_pad = route->source_pad;
> +			route_stream = route->source_stream;
> +		} else {
> +			route_pad = route->sink_pad;
> +			route_stream = route->sink_stream;
> +		}
> +
> +		if (route_pad != pad->index)
> +			continue;
> +
> +		*streams_mask |= BIT_ULL(route_stream);
> +	}
> +}
> +
> +#endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
> +
> +static void v4l2_link_validate_get_streams(struct media_pad *pad,
> +					   u64 *streams_mask)
> +{
> +	struct v4l2_subdev *subdev = media_entity_to_v4l2_subdev(pad->entity);
> +
> +	if (!(subdev->flags & V4L2_SUBDEV_FL_STREAMS)) {
> +		/* Non-streams subdevs have an implicit stream 0 */
> +		*streams_mask = BIT_ULL(0);
> +		return;
> +	}
> +
> +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> +	__v4l2_link_validate_get_streams(pad, streams_mask);
> +#else
> +	/* This shouldn't happen */
> +	*streams_mask = 0;
> +#endif
> +}
> +
> +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;
> +	u64 source_streams_mask;
> +	u64 sink_streams_mask;
> +	u64 dangling_sink_streams;
> +	u32 stream;
> +	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);
> +
> +	v4l2_link_validate_get_streams(link->source, &source_streams_mask);
> +	v4l2_link_validate_get_streams(link->sink, &sink_streams_mask);
> +
> +	/*
> +	 * 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.
> +	 */
> +	dangling_sink_streams = (source_streams_mask ^ sink_streams_mask) &
> +				sink_streams_mask;
> +	if (dangling_sink_streams) {
> +		dev_err(dev, "Dangling sink streams: mask %#llx\n",
> +			dangling_sink_streams);
> +		return -EINVAL;
> +	}
> +
> +	/* Validate source and sink stream formats */
> +
> +	for_each_set_bit(stream, (void *)&sink_streams_mask, 64) {

Does this work as expected? The second argument is expected to be unsigned
long (or an array of two of them) whereas you have a u64.

> +		struct v4l2_subdev_format sink_fmt, source_fmt;
> +
> +		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);
> +			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);
> +			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)
> +			return ret;
> +
> +		ret = v4l2_subdev_link_validate_default(sink_subdev, link,
> +							&source_fmt, &sink_fmt);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  int v4l2_subdev_link_validate(struct media_link *link)
>  {
> -	struct v4l2_subdev *sink;
> -	struct v4l2_subdev_format sink_fmt, source_fmt;
> -	int rval;
> +	struct v4l2_subdev *source_sd, *sink_sd;
> +	struct v4l2_subdev_state *source_state, *sink_state;
> +	int ret;
>  
> -	rval = v4l2_subdev_link_validate_get_format(
> -		link->source, &source_fmt);
> -	if (rval < 0)
> -		return 0;
> +	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
> +	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
>  
> -	rval = v4l2_subdev_link_validate_get_format(
> -		link->sink, &sink_fmt);
> -	if (rval < 0)
> -		return 0;
> +	sink_state = v4l2_subdev_get_unlocked_active_state(sink_sd);
> +	source_state = v4l2_subdev_get_unlocked_active_state(source_sd);
>  
> -	sink = media_entity_to_v4l2_subdev(link->sink->entity);
> +	if (sink_state)
> +		v4l2_subdev_lock_state(sink_state);
>  
> -	rval = v4l2_subdev_call(sink, pad, link_validate, link,
> -				&source_fmt, &sink_fmt);
> -	if (rval != -ENOIOCTLCMD)
> -		return rval;
> +	if (source_state)
> +		v4l2_subdev_lock_state(source_state);
>  
> -	return v4l2_subdev_link_validate_default(
> -		sink, link, &source_fmt, &sink_fmt);
> +	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);
>  

-- 
Terveisin,

Sakari Ailus