Message ID | 20220301161156.1119557-35-tomi.valkeinen@ideasonboard.com (mailing list archive) |
---|---|
State | Superseded |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1nP57i-004SEy-4m; Tue, 01 Mar 2022 16:13:14 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236074AbiCAQNv (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 1 Mar 2022 11:13:51 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33744 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236075AbiCAQNu (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 1 Mar 2022 11:13:50 -0500 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 329A035DEE for <linux-media@vger.kernel.org>; Tue, 1 Mar 2022 08:13:09 -0800 (PST) Received: from deskari.lan (91-156-85-209.elisa-laajakaista.fi [91.156.85.209]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 0160A1C6D; Tue, 1 Mar 2022 17:12:48 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1646151169; bh=l0HlvnqRm6HTy6W1ABUu6EwPuABmqOZTgbkeGIpaV+4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=vyUFmAYM2vJeVDQYMCG1L44sWSslLpPoOAhVQcNgiE2aNBahI+f53G5WekLBH1rbm DaY/o+UaV+glZoCf+UFAQYjycaTcNvjrMHEy7iw+0iWGDcVm1p9UVLGahnLu0A5K9k sS/S4BiXt+7DKoboI2sVzdUSjAybbGnyf1we3p9k= From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> To: linux-media@vger.kernel.org, sakari.ailus@linux.intel.com, Jacopo Mondi <jacopo+renesas@jmondi.org>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, niklas.soderlund+renesas@ragnatech.se, Mauro Carvalho Chehab <mchehab@kernel.org>, Hans Verkuil <hverkuil-cisco@xs4all.nl>, Pratyush Yadav <p.yadav@ti.com> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Subject: [PATCH v11 34/36] media: v4l2-subdev: Add v4l2_subdev_state_xlate_streams() helper Date: Tue, 1 Mar 2022 18:11:54 +0200 Message-Id: <20220301161156.1119557-35-tomi.valkeinen@ideasonboard.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20220301161156.1119557-1-tomi.valkeinen@ideasonboard.com> References: <20220301161156.1119557-1-tomi.valkeinen@ideasonboard.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_PASS,SPF_PASS, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_NONE=-0.0001 autolearn=ham autolearn_force=no |
Series |
v4l: routing and streams support
|
|
Commit Message
Tomi Valkeinen
March 1, 2022, 4:11 p.m. UTC
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Add a helper function to translate streams between two pads of a subdev, using the subdev's internal routing table. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++ include/media/v4l2-subdev.h | 23 +++++++++++++++++++++++ 2 files changed, 48 insertions(+)
Comments
Hi Tomi, On Tue, Mar 01, 2022 at 06:11:54PM +0200, Tomi Valkeinen wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Add a helper function to translate streams between two pads of a subdev, > using the subdev's internal routing table. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++ > include/media/v4l2-subdev.h | 23 +++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index e30338a53088..6a9fc62dacbf 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1529,6 +1529,31 @@ v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, > } > EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_opposite_stream_format); > > +u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, > + u32 pad0, u32 pad1, u64 *streams) > +{ > + const struct v4l2_subdev_krouting *routing = &state->routing; > + struct v4l2_subdev_route *route; > + u64 streams0 = 0; > + u64 streams1 = 0; > + > + for_each_active_route(routing, route) { > + if (route->sink_pad == pad0 && route->source_pad == pad1 && > + (*streams & BIT(route->sink_stream))) { > + streams0 |= BIT(route->sink_stream); > + streams1 |= BIT(route->source_stream); > + } > + if (route->source_pad == pad0 && route->sink_pad == pad1 && > + (*streams & BIT(route->source_stream))) { > + streams0 |= BIT(route->source_stream); > + streams1 |= BIT(route->sink_stream); > + } > + } > + > + *streams = streams0; > + return streams1; I'll probably learn how this is used later, but I found it hard to immagine how the returned mask should be used. To a sink stream mask that contains multiple streams a source maks with multiple entries will be associated In example, the sink stream mask might look lik sink_stream_mask = {1, 0, 1, 0} So we are interested in translating stream 0 and 2 Assume 0->4 and 2->1 in the routing tabe, the resulting source stream mask will be source_stream_mask = {1, 0, 0, 1} How should the caller know what stream goes where ? > +} > + > int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format) > { > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 1eced0f47296..992debe116ac 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -1497,6 +1497,29 @@ struct v4l2_mbus_framefmt * > v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, > u32 pad, u32 stream); > > +/** > + * v4l2_subdev_state_xlate_streams() - Translate streams from one pad to another Could we stress out that pads are on the same entity and this is not meant to translate over media links ? Is this necessary ? > + * > + * @state: Subdevice state > + * @pad0: The first pad > + * @pad1: The second pad > + * @streams: Streams bitmask on the first pad > + * > + * Streams on sink pads of a subdev are routed to source pads as expressed in > + * the subdev state routing table. Stream numbers don't necessarily match on > + * the sink and source side of a route. This function translates stream numbers > + * on @pad0, expressed as a bitmask in @streams, to the corresponding streams > + * on @pad1 using the routing table from the @state. It returns the stream mask > + * on @pad1, and updates @streams with the streams that have been found in the > + * routing table. > + * > + * @pad0 and @pad1 must be a sink and a source, in any order. > + * > + * Return: The bitmask of streams of @pad1 that are routed to @streams on @pad0. > + */ > +u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, > + u32 pad0, u32 pad1, u64 *streams); > + > /** > * v4l2_subdev_get_fmt() - Fill format based on state > * @sd: subdevice > -- > 2.25.1 >
On 16/03/2022 13:26, Jacopo Mondi wrote: > Hi Tomi, > > On Tue, Mar 01, 2022 at 06:11:54PM +0200, Tomi Valkeinen wrote: >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Add a helper function to translate streams between two pads of a subdev, >> using the subdev's internal routing table. >> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++ >> include/media/v4l2-subdev.h | 23 +++++++++++++++++++++++ >> 2 files changed, 48 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index e30338a53088..6a9fc62dacbf 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -1529,6 +1529,31 @@ v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, >> } >> EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_opposite_stream_format); >> >> +u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, >> + u32 pad0, u32 pad1, u64 *streams) >> +{ >> + const struct v4l2_subdev_krouting *routing = &state->routing; >> + struct v4l2_subdev_route *route; >> + u64 streams0 = 0; >> + u64 streams1 = 0; >> + >> + for_each_active_route(routing, route) { >> + if (route->sink_pad == pad0 && route->source_pad == pad1 && >> + (*streams & BIT(route->sink_stream))) { >> + streams0 |= BIT(route->sink_stream); >> + streams1 |= BIT(route->source_stream); >> + } >> + if (route->source_pad == pad0 && route->sink_pad == pad1 && >> + (*streams & BIT(route->source_stream))) { >> + streams0 |= BIT(route->source_stream); >> + streams1 |= BIT(route->sink_stream); >> + } >> + } >> + >> + *streams = streams0; >> + return streams1; > > I'll probably learn how this is used later, but I found it hard to > immagine how the returned mask should be used. > > To a sink stream mask that contains multiple streams a source maks > with multiple entries will be associated > > In example, the sink stream mask might look lik > > sink_stream_mask = {1, 0, 1, 0} > > So we are interested in translating stream 0 and 2 > Assume 0->4 and 2->1 in the routing tabe, the resulting source stream > mask will be > > source_stream_mask = {1, 0, 0, 1} > > How should the caller know what stream goes where ? The use case for the function is for cases where all the streams go from one pad to another. Probably the main use is in subdevs with a single source and sink pad. This makes it easy to implement enable_streams() op: you just basically call v4l2_subdev_state_xlate_streams() to get the streams on the other side, and call v4l2_subdev_enable_streams(). Here's a version from ub913: > static int ub913_enable_streams(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state, u32 pad, > u64 streams_mask) > { > struct ub913_data *priv = sd_to_ub913(sd); > struct media_pad *remote_pad; > u64 sink_streams; > int ret; > > if (streams_mask & priv->enabled_source_streams) > return -EALREADY; > > sink_streams = v4l2_subdev_state_xlate_streams( > state, UB913_PAD_SOURCE, UB913_PAD_SINK, &streams_mask); > > remote_pad = media_entity_remote_pad(&priv->pads[UB913_PAD_SINK]); > > ret = v4l2_subdev_enable_streams(priv->source_sd, remote_pad->index, > sink_streams); > if (ret) > return ret; > > priv->enabled_source_streams |= streams_mask; > > return 0; > } >> +} >> + >> int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, >> struct v4l2_subdev_format *format) >> { >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >> index 1eced0f47296..992debe116ac 100644 >> --- a/include/media/v4l2-subdev.h >> +++ b/include/media/v4l2-subdev.h >> @@ -1497,6 +1497,29 @@ struct v4l2_mbus_framefmt * >> v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, >> u32 pad, u32 stream); >> >> +/** >> + * v4l2_subdev_state_xlate_streams() - Translate streams from one pad to another > > Could we stress out that pads are on the same entity and this is not > meant to translate over media links ? Is this necessary ? The streams on both sides of a link are identical, so there can be no translation in that case. The doc below explains the use, I'd rather not try to squeeze in a longer explanation in the title. But I think we should emphasize that this only makes sense on a simple routing case. >> + * >> + * @state: Subdevice state >> + * @pad0: The first pad >> + * @pad1: The second pad >> + * @streams: Streams bitmask on the first pad >> + * >> + * Streams on sink pads of a subdev are routed to source pads as expressed in >> + * the subdev state routing table. Stream numbers don't necessarily match on >> + * the sink and source side of a route. This function translates stream numbers >> + * on @pad0, expressed as a bitmask in @streams, to the corresponding streams >> + * on @pad1 using the routing table from the @state. It returns the stream mask >> + * on @pad1, and updates @streams with the streams that have been found in the >> + * routing table. >> + * >> + * @pad0 and @pad1 must be a sink and a source, in any order. >> + * >> + * Return: The bitmask of streams of @pad1 that are routed to @streams on @pad0. >> + */ >> +u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, >> + u32 pad0, u32 pad1, u64 *streams); >> + >> /** >> * v4l2_subdev_get_fmt() - Fill format based on state >> * @sd: subdevice >> -- >> 2.25.1 >>
On 17/03/2022 11:27, Tomi Valkeinen wrote: > On 16/03/2022 13:26, Jacopo Mondi wrote: >> Hi Tomi, >> >> On Tue, Mar 01, 2022 at 06:11:54PM +0200, Tomi Valkeinen wrote: >>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>> Add a helper function to translate streams between two pads of a subdev, >>> using the subdev's internal routing table. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> --- >>> drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++ >>> include/media/v4l2-subdev.h | 23 +++++++++++++++++++++++ >>> 2 files changed, 48 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c >>> b/drivers/media/v4l2-core/v4l2-subdev.c >>> index e30338a53088..6a9fc62dacbf 100644 >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>> @@ -1529,6 +1529,31 @@ >>> v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state >>> *state, >>> } >>> EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_opposite_stream_format); >>> >>> +u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state >>> *state, >>> + u32 pad0, u32 pad1, u64 *streams) >>> +{ >>> + const struct v4l2_subdev_krouting *routing = &state->routing; >>> + struct v4l2_subdev_route *route; >>> + u64 streams0 = 0; >>> + u64 streams1 = 0; >>> + >>> + for_each_active_route(routing, route) { >>> + if (route->sink_pad == pad0 && route->source_pad == pad1 && >>> + (*streams & BIT(route->sink_stream))) { >>> + streams0 |= BIT(route->sink_stream); >>> + streams1 |= BIT(route->source_stream); >>> + } >>> + if (route->source_pad == pad0 && route->sink_pad == pad1 && >>> + (*streams & BIT(route->source_stream))) { >>> + streams0 |= BIT(route->source_stream); >>> + streams1 |= BIT(route->sink_stream); >>> + } >>> + } >>> + >>> + *streams = streams0; >>> + return streams1; >> >> I'll probably learn how this is used later, but I found it hard to >> immagine how the returned mask should be used. >> >> To a sink stream mask that contains multiple streams a source maks >> with multiple entries will be associated >> >> In example, the sink stream mask might look lik >> >> sink_stream_mask = {1, 0, 1, 0} >> >> So we are interested in translating stream 0 and 2 >> Assume 0->4 and 2->1 in the routing tabe, the resulting source stream >> mask will be >> >> source_stream_mask = {1, 0, 0, 1} >> >> How should the caller know what stream goes where ? > > The use case for the function is for cases where all the streams go from > one pad to another. Probably the main use is in subdevs with a single > source and sink pad. This makes it easy to implement enable_streams() > op: you just basically call v4l2_subdev_state_xlate_streams() to get the > streams on the other side, and call v4l2_subdev_enable_streams(). Here's > a version from ub913: > >> static int ub913_enable_streams(struct v4l2_subdev *sd, >> struct v4l2_subdev_state *state, u32 pad, >> u64 streams_mask) >> { >> struct ub913_data *priv = sd_to_ub913(sd); >> struct media_pad *remote_pad; >> u64 sink_streams; >> int ret; >> >> if (streams_mask & priv->enabled_source_streams) >> return -EALREADY; >> >> sink_streams = v4l2_subdev_state_xlate_streams( >> state, UB913_PAD_SOURCE, UB913_PAD_SINK, &streams_mask); >> >> remote_pad = media_entity_remote_pad(&priv->pads[UB913_PAD_SINK]); >> >> ret = v4l2_subdev_enable_streams(priv->source_sd, remote_pad->index, >> sink_streams); >> if (ret) >> return ret; >> >> priv->enabled_source_streams |= streams_mask; >> >> return 0; >> } > > > >>> +} >>> + >>> int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct >>> v4l2_subdev_state *state, >>> struct v4l2_subdev_format *format) >>> { >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >>> index 1eced0f47296..992debe116ac 100644 >>> --- a/include/media/v4l2-subdev.h >>> +++ b/include/media/v4l2-subdev.h >>> @@ -1497,6 +1497,29 @@ struct v4l2_mbus_framefmt * >>> v4l2_subdev_state_get_opposite_stream_format(struct >>> v4l2_subdev_state *state, >>> u32 pad, u32 stream); >>> >>> +/** >>> + * v4l2_subdev_state_xlate_streams() - Translate streams from one >>> pad to another >> >> Could we stress out that pads are on the same entity and this is not >> meant to translate over media links ? Is this necessary ? > > The streams on both sides of a link are identical, so there can be no > translation in that case. > > The doc below explains the use, I'd rather not try to squeeze in a > longer explanation in the title. > > But I think we should emphasize that this only makes sense on a simple > routing case. Ah, never mind. The function can be used in multi-pad cases too, so we don't need to emphasize the above. The function tells how a set of streams from pad A translate to a set of streams on pad B. So you only know the set of streams, not how a single stream goes. For that you need to look at the routing table. Tomi
Hi Tomi, Laurent, On Tue, Mar 01, 2022 at 06:11:54PM +0200, Tomi Valkeinen wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Add a helper function to translate streams between two pads of a subdev, > using the subdev's internal routing table. > Thanks for the patch! Please see my comments inline. > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++ > include/media/v4l2-subdev.h | 23 +++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index e30338a53088..6a9fc62dacbf 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1529,6 +1529,31 @@ v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, > } > EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_opposite_stream_format); > > +u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, > + u32 pad0, u32 pad1, u64 *streams) > +{ > + const struct v4l2_subdev_krouting *routing = &state->routing; > + struct v4l2_subdev_route *route; > + u64 streams0 = 0; > + u64 streams1 = 0; > + > + for_each_active_route(routing, route) { > + if (route->sink_pad == pad0 && route->source_pad == pad1 && > + (*streams & BIT(route->sink_stream))) { > + streams0 |= BIT(route->sink_stream); > + streams1 |= BIT(route->source_stream); > + } > + if (route->source_pad == pad0 && route->sink_pad == pad1 && > + (*streams & BIT(route->source_stream))) { > + streams0 |= BIT(route->source_stream); > + streams1 |= BIT(route->sink_stream); > + } > + } > + > + *streams = streams0; > + return streams1; > +} > + > int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format) > { > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 1eced0f47296..992debe116ac 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -1497,6 +1497,29 @@ struct v4l2_mbus_framefmt * > v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, > u32 pad, u32 stream); > > +/** > + * v4l2_subdev_state_xlate_streams() - Translate streams from one pad to another > + * > + * @state: Subdevice state > + * @pad0: The first pad > + * @pad1: The second pad > + * @streams: Streams bitmask on the first pad > + * > + * Streams on sink pads of a subdev are routed to source pads as expressed in > + * the subdev state routing table. Stream numbers don't necessarily match on > + * the sink and source side of a route. This function translates stream numbers > + * on @pad0, expressed as a bitmask in @streams, to the corresponding streams > + * on @pad1 using the routing table from the @state. It returns the stream mask > + * on @pad1, and updates @streams with the streams that have been found in the > + * routing table. > + * > + * @pad0 and @pad1 must be a sink and a source, in any order. > + * > + * Return: The bitmask of streams of @pad1 that are routed to @streams on @pad0. It might be just me, but somehow I associate "translate" with turning a name from one namespace into a corresponding name from another namespace. In this case I initially assumed that this function is supposed to accept stream IDs from pad0 and return corresponding stream IDs from pad1. However, it looks like this is move like - "tell me which streams on pad1 are connected to the given pad0 streams". Is this correct? If yes, maybe v4l2_subdev_state_get_routed_streams() be a better name? Best regards, Tomasz
Hi, On 07/07/2022 13:27, Tomasz Figa wrote: > Hi Tomi, Laurent, > > On Tue, Mar 01, 2022 at 06:11:54PM +0200, Tomi Valkeinen wrote: >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Add a helper function to translate streams between two pads of a subdev, >> using the subdev's internal routing table. >> > > Thanks for the patch! Please see my comments inline. > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++ >> include/media/v4l2-subdev.h | 23 +++++++++++++++++++++++ >> 2 files changed, 48 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index e30338a53088..6a9fc62dacbf 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -1529,6 +1529,31 @@ v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, >> } >> EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_opposite_stream_format); >> >> +u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, >> + u32 pad0, u32 pad1, u64 *streams) >> +{ >> + const struct v4l2_subdev_krouting *routing = &state->routing; >> + struct v4l2_subdev_route *route; >> + u64 streams0 = 0; >> + u64 streams1 = 0; >> + >> + for_each_active_route(routing, route) { >> + if (route->sink_pad == pad0 && route->source_pad == pad1 && >> + (*streams & BIT(route->sink_stream))) { >> + streams0 |= BIT(route->sink_stream); >> + streams1 |= BIT(route->source_stream); >> + } >> + if (route->source_pad == pad0 && route->sink_pad == pad1 && >> + (*streams & BIT(route->source_stream))) { >> + streams0 |= BIT(route->source_stream); >> + streams1 |= BIT(route->sink_stream); >> + } >> + } >> + >> + *streams = streams0; >> + return streams1; >> +} >> + >> int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, >> struct v4l2_subdev_format *format) >> { >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >> index 1eced0f47296..992debe116ac 100644 >> --- a/include/media/v4l2-subdev.h >> +++ b/include/media/v4l2-subdev.h >> @@ -1497,6 +1497,29 @@ struct v4l2_mbus_framefmt * >> v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, >> u32 pad, u32 stream); >> >> +/** >> + * v4l2_subdev_state_xlate_streams() - Translate streams from one pad to another >> + * >> + * @state: Subdevice state >> + * @pad0: The first pad >> + * @pad1: The second pad >> + * @streams: Streams bitmask on the first pad >> + * >> + * Streams on sink pads of a subdev are routed to source pads as expressed in >> + * the subdev state routing table. Stream numbers don't necessarily match on >> + * the sink and source side of a route. This function translates stream numbers >> + * on @pad0, expressed as a bitmask in @streams, to the corresponding streams >> + * on @pad1 using the routing table from the @state. It returns the stream mask >> + * on @pad1, and updates @streams with the streams that have been found in the >> + * routing table. >> + * >> + * @pad0 and @pad1 must be a sink and a source, in any order. >> + * >> + * Return: The bitmask of streams of @pad1 that are routed to @streams on @pad0. > > It might be just me, but somehow I associate "translate" with turning a > name from one namespace into a corresponding name from another > namespace. In this case I initially assumed that this function is > supposed to accept stream IDs from pad0 and return corresponding > stream IDs from pad1. However, it looks like this is move like - "tell > me which streams on pad1 are connected to the given pad0 streams". Is > this correct? So is your point that as the function returns a bitmask, and not a mapping of the stream IDs, this is not a translate function? > If yes, maybe v4l2_subdev_state_get_routed_streams() be a better name? I think this name would also fit quite well for a function that returns a mapping of the streams. So... I don't have a strong opinion on this. To me, neither the current name nor the proposed one clearly explains alone what the function does. Tomi
On Thu, Jul 14, 2022 at 4:41 PM Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > > Hi, > > On 07/07/2022 13:27, Tomasz Figa wrote: > > Hi Tomi, Laurent, > > > > On Tue, Mar 01, 2022 at 06:11:54PM +0200, Tomi Valkeinen wrote: > >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> > >> Add a helper function to translate streams between two pads of a subdev, > >> using the subdev's internal routing table. > >> > > > > Thanks for the patch! Please see my comments inline. > > > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >> --- > >> drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++ > >> include/media/v4l2-subdev.h | 23 +++++++++++++++++++++++ > >> 2 files changed, 48 insertions(+) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > >> index e30338a53088..6a9fc62dacbf 100644 > >> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >> @@ -1529,6 +1529,31 @@ v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, > >> } > >> EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_opposite_stream_format); > >> > >> +u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, > >> + u32 pad0, u32 pad1, u64 *streams) > >> +{ > >> + const struct v4l2_subdev_krouting *routing = &state->routing; > >> + struct v4l2_subdev_route *route; > >> + u64 streams0 = 0; > >> + u64 streams1 = 0; > >> + > >> + for_each_active_route(routing, route) { > >> + if (route->sink_pad == pad0 && route->source_pad == pad1 && > >> + (*streams & BIT(route->sink_stream))) { > >> + streams0 |= BIT(route->sink_stream); > >> + streams1 |= BIT(route->source_stream); > >> + } > >> + if (route->source_pad == pad0 && route->sink_pad == pad1 && > >> + (*streams & BIT(route->source_stream))) { > >> + streams0 |= BIT(route->source_stream); > >> + streams1 |= BIT(route->sink_stream); > >> + } > >> + } > >> + > >> + *streams = streams0; > >> + return streams1; > >> +} > >> + > >> int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, > >> struct v4l2_subdev_format *format) > >> { > >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > >> index 1eced0f47296..992debe116ac 100644 > >> --- a/include/media/v4l2-subdev.h > >> +++ b/include/media/v4l2-subdev.h > >> @@ -1497,6 +1497,29 @@ struct v4l2_mbus_framefmt * > >> v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, > >> u32 pad, u32 stream); > >> > >> +/** > >> + * v4l2_subdev_state_xlate_streams() - Translate streams from one pad to another > >> + * > >> + * @state: Subdevice state > >> + * @pad0: The first pad > >> + * @pad1: The second pad > >> + * @streams: Streams bitmask on the first pad > >> + * > >> + * Streams on sink pads of a subdev are routed to source pads as expressed in > >> + * the subdev state routing table. Stream numbers don't necessarily match on > >> + * the sink and source side of a route. This function translates stream numbers > >> + * on @pad0, expressed as a bitmask in @streams, to the corresponding streams > >> + * on @pad1 using the routing table from the @state. It returns the stream mask > >> + * on @pad1, and updates @streams with the streams that have been found in the > >> + * routing table. > >> + * > >> + * @pad0 and @pad1 must be a sink and a source, in any order. > >> + * > >> + * Return: The bitmask of streams of @pad1 that are routed to @streams on @pad0. > > > > It might be just me, but somehow I associate "translate" with turning a > > name from one namespace into a corresponding name from another > > namespace. In this case I initially assumed that this function is > > supposed to accept stream IDs from pad0 and return corresponding > > stream IDs from pad1. However, it looks like this is move like - "tell > > me which streams on pad1 are connected to the given pad0 streams". Is > > this correct? > > So is your point that as the function returns a bitmask, and not a > mapping of the stream IDs, this is not a translate function? Yep. > > > If yes, maybe v4l2_subdev_state_get_routed_streams() be a better name? > > I think this name would also fit quite well for a function that returns > a mapping of the streams. So... I don't have a strong opinion on this. > To me, neither the current name nor the proposed one clearly explains > alone what the function does. Well, since it's a kernel function, it probably can be renamed later. Anyway, I forgot to place "nit:" before the comment. It's not really a strong opinion from me either. Best regards, Tomasz
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index e30338a53088..6a9fc62dacbf 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1529,6 +1529,31 @@ v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, } EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_opposite_stream_format); +u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, + u32 pad0, u32 pad1, u64 *streams) +{ + const struct v4l2_subdev_krouting *routing = &state->routing; + struct v4l2_subdev_route *route; + u64 streams0 = 0; + u64 streams1 = 0; + + for_each_active_route(routing, route) { + if (route->sink_pad == pad0 && route->source_pad == pad1 && + (*streams & BIT(route->sink_stream))) { + streams0 |= BIT(route->sink_stream); + streams1 |= BIT(route->source_stream); + } + if (route->source_pad == pad0 && route->sink_pad == pad1 && + (*streams & BIT(route->source_stream))) { + streams0 |= BIT(route->source_stream); + streams1 |= BIT(route->sink_stream); + } + } + + *streams = streams0; + return streams1; +} + int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, struct v4l2_subdev_format *format) { diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 1eced0f47296..992debe116ac 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -1497,6 +1497,29 @@ struct v4l2_mbus_framefmt * v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, u32 pad, u32 stream); +/** + * v4l2_subdev_state_xlate_streams() - Translate streams from one pad to another + * + * @state: Subdevice state + * @pad0: The first pad + * @pad1: The second pad + * @streams: Streams bitmask on the first pad + * + * Streams on sink pads of a subdev are routed to source pads as expressed in + * the subdev state routing table. Stream numbers don't necessarily match on + * the sink and source side of a route. This function translates stream numbers + * on @pad0, expressed as a bitmask in @streams, to the corresponding streams + * on @pad1 using the routing table from the @state. It returns the stream mask + * on @pad1, and updates @streams with the streams that have been found in the + * routing table. + * + * @pad0 and @pad1 must be a sink and a source, in any order. + * + * Return: The bitmask of streams of @pad1 that are routed to @streams on @pad0. + */ +u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, + u32 pad0, u32 pad1, u64 *streams); + /** * v4l2_subdev_get_fmt() - Fill format based on state * @sd: subdevice