Hi Tomi,
On Mon, Oct 11, 2021 at 11:21:14AM +0300, Tomi Valkeinen wrote:
> On 10/10/2021 17:15, Laurent Pinchart wrote:
> > On Tue, Oct 05, 2021 at 11:57:47AM +0300, Tomi Valkeinen wrote:
> >> Add two helper functions to make dealing with streams easier:
> >>
> >> v4l2_state_find_opposite_end - given a routing table and a pad + stream,
> >> return the pad + stream on the opposite side of the subdev.
> >>
> >> v4l2_state_get_opposite_stream_format - return a pointer to the format
> >> on the pad + stream on the opposite side from the given pad + stream.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> >> ---
> >> drivers/media/v4l2-core/v4l2-subdev.c | 42 +++++++++++++++++++++++++++
> >> include/media/v4l2-subdev.h | 32 ++++++++++++++++++++
> >> 2 files changed, 74 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >> index 37e2e1f907fc..9eeadad997c8 100644
> >> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >> @@ -1484,3 +1484,45 @@ v4l2_state_get_stream_format(struct v4l2_subdev_state *state, unsigned int pad,
> >> return NULL;
> >> }
> >> EXPORT_SYMBOL_GPL(v4l2_state_get_stream_format);
> >> +
> >> +int v4l2_state_find_opposite_end(struct v4l2_subdev_krouting *routing, u32 pad,
> >> + u32 stream, u32 *other_pad, u32 *other_stream)
> >
> > This function should take a state pointer given its name. I would also
> > rename it to v4l2_subdev_state_find_opposite_end(). Same for
> > v4l2_state_get_opposite_stream_format() which should be
> > v4l2_subdev_state_get_opposite_stream_format().
>
> It doesn't need the state. I think it's better to rename this to
> v4l2_routing_find_opposite_end(). Or
> v4l2_subdev_routing_find_opposite_end() if you want the "subdev" to be
> there.
Is there any use case for calling this function on a
v4l2_subdev_krouting instance that isn't stored in a v4l2_subdev_state ?
If not, I'd prefer standardizing on operating on the state in all cases.
> >> +{
> >> + unsigned int i;
> >> +
> >> + for (i = 0; i < routing->num_routes; ++i) {
> >> + struct v4l2_subdev_route *route = &routing->routes[i];
> >> +
> >> + if (route->source_pad == pad &&
> >> + route->source_stream == stream) {
> >> + *other_pad = route->sink_pad;
> >> + *other_stream = route->sink_stream;
> >
> > Can we support other_stream being NULL ? When the subdev implements the
> > routing API without multiplexed streams, the other_stream number will
> > always be 0 and it would be nice if the caller didn't have to declare a
> > placeholder variable.
> >
> > There are less use cases for other_pad being NULL, but maybe we could
> > also allow that for consistency reasons ? Up to you.
>
> Sure. But are you sure the ignored stream is 0? Should the function
> verify that the stream is indeed 0 if other_stream is NULL?
I'd expect the routing table to have been validated at .set_routing()
time, with != 0 streams rejected at that point, so ignoring the stream
when calling v4l2_state_find_opposite_end() in drivers that don't
support multiplexed streams should be safe.
> For the pad, I don't know when it would be used, so it's perhaps better
> not to add that (as we can't have similar validation as for the stream).
OK, we can only handle a NULL other_stream then.
--
Regards,
Laurent Pinchart
