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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 14/10/2022 13:54, Sakari Ailus wrote:
> 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.

Where do you see that? I thought find_next_bit (used by 
for_each_set_bit) is given a start address and arbitrarily large 
bit-size number.

  Tomi