| Message ID | 20221003121852.616745-12-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | v4l: routing and streams support | expand |
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); >
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
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@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. sink_streams_mask is a u64 while for_each_set_bit() expects an array of unsigned longs. Endianness matters.
On 17/10/2022 01:37, Sakari Ailus wrote: > 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@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. > > sink_streams_mask is a u64 while for_each_set_bit() expects an array of > unsigned longs. Endianness matters. Yes, you're right. I can't find any ready helper for this, so I'll just do it manually: for (stream = 0; stream < sizeof(sink_streams_mask) * 8; ++stream) { if (!(sink_streams_mask & BIT_ULL(stream))) continue; ... } Tomi
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) { + 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);
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(-)