Moi,
On Fri, Oct 14, 2022 at 02:10:35PM +0300, Tomi Valkeinen wrote:
> 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@xxxxxxxxxxxxxxxx>
> > > ---
> > > 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.
sink_streams_mask is a u64 while for_each_set_bit() expects an array of
unsigned longs. Endianness matters.
--
Terveisin,
Sakari Ailus
