[PATCH v2 0/3] media: add v4l2_subdev_state_xlate_streams()

[PATCH v2 0/3] media: add v4l2_subdev_state_xlate_streams()

[Cosmin Tanislav]

Currently, the v4l2_subdev_state_xlate_streams() function is used
to translate streams from one pad to another.
This function takes the entire subdev state as argument, but only makes
use of the routing.

Introduce a v4l2_subdev_routing_xlate_streams() function which can be
used without the entire subdev state, to avoid passing the entire state
around when not needed.

Convert all usages of v4l2_subdev_state_xlate_streams() to
v4l2_subdev_routing_xlate_streams().

Remove v4l2_subdev_state_xlate_streams().

V2:
  * Fix description of parameters

Cosmin Tanislav (3):
  media: v4l: subdev: add v4l2_subdev_routing_xlate_streams()
  media: use v4l2_subdev_routing_xlate_streams()
  media: v4l: subdev: remove v4l2_subdev_state_xlate_streams()

 drivers/media/i2c/ds90ub913.c                 | 14 ++++++-----
 drivers/media/i2c/ds90ub953.c                 | 14 ++++++-----
 drivers/media/i2c/max96714.c                  | 16 ++++++-------
 drivers/media/i2c/max96717.c                  | 23 ++++++++++---------
 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 14 ++++++-----
 .../platform/nxp/imx8-isi/imx8-isi-crossbar.c |  2 +-
 drivers/media/v4l2-core/v4l2-subdev.c         |  7 +++---
 include/media/v4l2-subdev.h                   | 10 ++++----
 8 files changed, 53 insertions(+), 47 deletions(-)

-- 
2.48.1

[PATCH v2 1/3] media: v4l: subdev: add v4l2_subdev_routing_xlate_streams()

[Cosmin Tanislav]

Currently, the v4l2_subdev_state_xlate_streams() function is used
to translate streams from one pad to another.
This function takes the entire subdev state as argument, but only makes
use of the routing.

Introduce a v4l2_subdev_routing_xlate_streams() function which can be
used without the entire subdev state, to avoid passing the entire state
around when not needed.

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 12 +++++++++---
 include/media/v4l2-subdev.h           | 20 +++++++++++++++++---
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index a3074f469b150..91fa51259237e 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2044,10 +2044,9 @@ 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)
+u64 v4l2_subdev_routing_xlate_streams(const struct v4l2_subdev_krouting *routing,
+				      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;
@@ -2068,6 +2067,13 @@ u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state,
 	*streams = streams0;
 	return streams1;
 }
+EXPORT_SYMBOL_GPL(v4l2_subdev_routing_xlate_streams);
+
+u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state,
+				    u32 pad0, u32 pad1, u64 *streams)
+{
+	return v4l2_subdev_routing_xlate_streams(&state->routing, pad0, pad1, streams);
+}
 EXPORT_SYMBOL_GPL(v4l2_subdev_state_xlate_streams);
 
 int v4l2_subdev_routing_validate(struct v4l2_subdev *sd,
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 57f2bcb4eb16c..e49dba3c59bd6 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1584,9 +1584,9 @@ 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
+ * v4l2_subdev_routing_xlate_streams() - Translate streams from one pad to another
  *
- * @state: Subdevice state
+ * @routing: Routing used to translate streams from one pad to another
  * @pad0: The first pad
  * @pad1: The second pad
  * @streams: Streams bitmask on the first pad
@@ -1595,7 +1595,7 @@ v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state,
  * 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 using the routing table from @routing. It returns the stream mask
  * on @pad1, and updates @streams with the streams that have been found in the
  * routing table.
  *
@@ -1603,6 +1603,20 @@ v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state,
  *
  * Return: The bitmask of streams of @pad1 that are routed to @streams on @pad0.
  */
+u64 v4l2_subdev_routing_xlate_streams(const struct v4l2_subdev_krouting *routing,
+				      u32 pad0, u32 pad1, u64 *streams);
+
+/**
+ * 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
+ *
+ * This is the same as v4l2_subdev_routing_xlate_streams, but takes subdevice
+ * state as parameter
+ */
 u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state,
 				    u32 pad0, u32 pad1, u64 *streams);
 
-- 
2.48.1

[PATCH v2 2/3] media: use v4l2_subdev_routing_xlate_streams()

[Cosmin Tanislav]

Replace current usages of v4l2_subdev_state_xlate_streams() with
v4l2_subdev_routing_xlate_streams() in preperations for the removal of
v4l2_subdev_state_xlate_streams().

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
---
 drivers/media/i2c/ds90ub913.c                 | 14 ++++++-----
 drivers/media/i2c/ds90ub953.c                 | 14 ++++++-----
 drivers/media/i2c/max96714.c                  | 16 ++++++-------
 drivers/media/i2c/max96717.c                  | 23 ++++++++++---------
 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 14 ++++++-----
 .../platform/nxp/imx8-isi/imx8-isi-crossbar.c |  2 +-
 6 files changed, 45 insertions(+), 38 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index fd2d2d5272bfb..b8dfcf730baa2 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -252,9 +252,10 @@ static int ub913_enable_streams(struct v4l2_subdev *sd,
 	u64 sink_streams;
 	int ret;
 
-	sink_streams = v4l2_subdev_state_xlate_streams(state, UB913_PAD_SOURCE,
-						       UB913_PAD_SINK,
-						       &streams_mask);
+	sink_streams = v4l2_subdev_routing_xlate_streams(&state->routing,
+							 UB913_PAD_SOURCE,
+							 UB913_PAD_SINK,
+							 &streams_mask);
 
 	ret = v4l2_subdev_enable_streams(priv->source_sd, priv->source_sd_pad,
 					 sink_streams);
@@ -274,9 +275,10 @@ static int ub913_disable_streams(struct v4l2_subdev *sd,
 	u64 sink_streams;
 	int ret;
 
-	sink_streams = v4l2_subdev_state_xlate_streams(state, UB913_PAD_SOURCE,
-						       UB913_PAD_SINK,
-						       &streams_mask);
+	sink_streams = v4l2_subdev_routing_xlate_streams(&state->routing,
+							 UB913_PAD_SOURCE,
+							 UB913_PAD_SINK,
+							 &streams_mask);
 
 	ret = v4l2_subdev_disable_streams(priv->source_sd, priv->source_sd_pad,
 					  sink_streams);
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index 46569381b332d..c239ede968423 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -683,9 +683,10 @@ static int ub953_enable_streams(struct v4l2_subdev *sd,
 	u64 sink_streams;
 	int ret;
 
-	sink_streams = v4l2_subdev_state_xlate_streams(state, UB953_PAD_SOURCE,
-						       UB953_PAD_SINK,
-						       &streams_mask);
+	sink_streams = v4l2_subdev_routing_xlate_streams(&state->routing,
+							 UB953_PAD_SOURCE,
+							 UB953_PAD_SINK,
+							 &streams_mask);
 
 	ret = v4l2_subdev_enable_streams(priv->source_sd, priv->source_sd_pad,
 					 sink_streams);
@@ -705,9 +706,10 @@ static int ub953_disable_streams(struct v4l2_subdev *sd,
 	u64 sink_streams;
 	int ret;
 
-	sink_streams = v4l2_subdev_state_xlate_streams(state, UB953_PAD_SOURCE,
-						       UB953_PAD_SINK,
-						       &streams_mask);
+	sink_streams = v4l2_subdev_routing_xlate_streams(&state->routing,
+							 UB953_PAD_SOURCE,
+							 UB953_PAD_SINK,
+							 &streams_mask);
 
 	ret = v4l2_subdev_disable_streams(priv->source_sd, priv->source_sd_pad,
 					  sink_streams);
diff --git a/drivers/media/i2c/max96714.c b/drivers/media/i2c/max96714.c
index 159753b13777c..c982bca708ff5 100644
--- a/drivers/media/i2c/max96714.c
+++ b/drivers/media/i2c/max96714.c
@@ -272,10 +272,10 @@ static int max96714_enable_streams(struct v4l2_subdev *sd,
 		}
 
 		sink_streams =
-			v4l2_subdev_state_xlate_streams(state,
-							MAX96714_PAD_SOURCE,
-							MAX96714_PAD_SINK,
-							&streams_mask);
+			v4l2_subdev_routing_xlate_streams(&state->routing,
+							  MAX96714_PAD_SOURCE,
+							  MAX96714_PAD_SINK,
+							  &streams_mask);
 
 		ret = v4l2_subdev_enable_streams(priv->rxport.source.sd,
 						 priv->rxport.source.pad,
@@ -306,10 +306,10 @@ static int max96714_disable_streams(struct v4l2_subdev *sd,
 		int ret;
 
 		sink_streams =
-			v4l2_subdev_state_xlate_streams(state,
-							MAX96714_PAD_SOURCE,
-							MAX96714_PAD_SINK,
-							&streams_mask);
+			v4l2_subdev_routing_xlate_streams(&state->routing,
+							  MAX96714_PAD_SOURCE,
+							  MAX96714_PAD_SINK,
+							  &streams_mask);
 
 		ret = v4l2_subdev_disable_streams(priv->rxport.source.sd,
 						  priv->rxport.source.pad,
diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
index 9259d58ba734e..e18b07b3259c1 100644
--- a/drivers/media/i2c/max96717.c
+++ b/drivers/media/i2c/max96717.c
@@ -446,9 +446,10 @@ static int max96717_set_fmt(struct v4l2_subdev *sd,
 
 	stream_source_mask = BIT(format->stream);
 
-	return v4l2_subdev_state_xlate_streams(state, MAX96717_PAD_SOURCE,
-					       MAX96717_PAD_SINK,
-					       &stream_source_mask);
+	return v4l2_subdev_routing_xlate_streams(&state->routing,
+						 MAX96717_PAD_SOURCE,
+						 MAX96717_PAD_SINK,
+						 &stream_source_mask);
 }
 
 static int max96717_init_state(struct v4l2_subdev *sd,
@@ -508,10 +509,10 @@ static int max96717_enable_streams(struct v4l2_subdev *sd,
 
 	if (!priv->pattern) {
 		sink_streams =
-			v4l2_subdev_state_xlate_streams(state,
-							MAX96717_PAD_SOURCE,
-							MAX96717_PAD_SINK,
-							&streams_mask);
+			v4l2_subdev_routing_xlate_streams(&state->routing,
+							  MAX96717_PAD_SOURCE,
+							  MAX96717_PAD_SINK,
+							  &streams_mask);
 
 		ret = v4l2_subdev_enable_streams(priv->source_sd,
 						 priv->source_sd_pad,
@@ -551,10 +552,10 @@ static int max96717_disable_streams(struct v4l2_subdev *sd,
 		int ret;
 
 		sink_streams =
-			v4l2_subdev_state_xlate_streams(state,
-							MAX96717_PAD_SOURCE,
-							MAX96717_PAD_SINK,
-							&streams_mask);
+			v4l2_subdev_routing_xlate_streams(&state->routing,
+							  MAX96717_PAD_SOURCE,
+							  MAX96717_PAD_SINK,
+							  &streams_mask);
 
 		ret = v4l2_subdev_disable_streams(priv->source_sd,
 						  priv->source_sd_pad,
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
index da8581a37e220..31fcf1695cb7c 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
@@ -354,9 +354,10 @@ static int ipu6_isys_csi2_enable_streams(struct v4l2_subdev *sd,
 	remote_pad = media_pad_remote_pad_first(&sd->entity.pads[CSI2_PAD_SINK]);
 	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
 
-	sink_streams = v4l2_subdev_state_xlate_streams(state, CSI2_PAD_SRC,
-						       CSI2_PAD_SINK,
-						       &streams_mask);
+	sink_streams = v4l2_subdev_routing_xlate_streams(&state->routing,
+							 CSI2_PAD_SRC,
+							 CSI2_PAD_SINK,
+							 &streams_mask);
 
 	ret = ipu6_isys_csi2_calc_timing(csi2, &timing, CSI2_ACCINV);
 	if (ret)
@@ -384,9 +385,10 @@ static int ipu6_isys_csi2_disable_streams(struct v4l2_subdev *sd,
 	struct media_pad *remote_pad;
 	u64 sink_streams;
 
-	sink_streams = v4l2_subdev_state_xlate_streams(state, CSI2_PAD_SRC,
-						       CSI2_PAD_SINK,
-						       &streams_mask);
+	sink_streams = v4l2_subdev_routing_xlate_streams(&state->routing,
+							 CSI2_PAD_SRC,
+							 CSI2_PAD_SINK,
+							 &streams_mask);
 
 	remote_pad = media_pad_remote_pad_first(&sd->entity.pads[CSI2_PAD_SINK]);
 	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
index 93a55c97cd173..8f61145435e32 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
@@ -141,7 +141,7 @@ mxc_isi_crossbar_xlate_streams(struct mxc_isi_crossbar *xbar,
 	 * routing table are guaranteed to have the same sink pad.
 	 *
 	 * TODO: This is likely worth a helper function, it could perhaps be
-	 * supported by v4l2_subdev_state_xlate_streams() with pad1 set to -1.
+	 * supported by v4l2_subdev_routing_xlate_streams() with pad1 set to -1.
 	 */
 	for_each_active_route(&state->routing, route) {
 		if (route->source_pad != source_pad ||
-- 
2.48.1

[PATCH v2 3/3] media: v4l: subdev: remove v4l2_subdev_state_xlate_streams()

[Cosmin Tanislav]

All usages of v4l2_subdev_state_xlate_streams() have been replaced with
v4l2_subdev_routing_xlate_streams(), remove it.

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c |  7 -------
 include/media/v4l2-subdev.h           | 14 --------------
 2 files changed, 21 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 91fa51259237e..985d30c22acae 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2069,13 +2069,6 @@ u64 v4l2_subdev_routing_xlate_streams(const struct v4l2_subdev_krouting *routing
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_routing_xlate_streams);
 
-u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state,
-				    u32 pad0, u32 pad1, u64 *streams)
-{
-	return v4l2_subdev_routing_xlate_streams(&state->routing, pad0, pad1, streams);
-}
-EXPORT_SYMBOL_GPL(v4l2_subdev_state_xlate_streams);
-
 int v4l2_subdev_routing_validate(struct v4l2_subdev *sd,
 				 const struct v4l2_subdev_krouting *routing,
 				 enum v4l2_subdev_routing_restriction disallow)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index e49dba3c59bd6..0fa732e6dd55d 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1606,20 +1606,6 @@ v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state,
 u64 v4l2_subdev_routing_xlate_streams(const struct v4l2_subdev_krouting *routing,
 				      u32 pad0, u32 pad1, u64 *streams);
 
-/**
- * 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
- *
- * This is the same as v4l2_subdev_routing_xlate_streams, but takes subdevice
- * state as parameter
- */
-u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state,
-				    u32 pad0, u32 pad1, u64 *streams);
-
 /**
  * enum v4l2_subdev_routing_restriction - Subdevice internal routing restrictions
  *
-- 
2.48.1

Re: [PATCH v2 0/3] media: add v4l2_subdev_state_xlate_streams()

[Jacopo Mondi]

Hi Cosmin

On Thu, Feb 20, 2025 at 11:20:32AM +0200, Cosmin Tanislav wrote:
> Currently, the v4l2_subdev_state_xlate_streams() function is used
> to translate streams from one pad to another.
> This function takes the entire subdev state as argument, but only makes
> use of the routing.

Correct, but is this a problem ?

Is this the first step for a larger rework or is this a drive-by
beautification ?

I'm asking because (and I know it's hard to strike a balance) this
kind of changes tend to make back-porting a more painful, and if
only justified by "it looks better" I would be a bit hesitant in
taking them.


>
> Introduce a v4l2_subdev_routing_xlate_streams() function which can be
> used without the entire subdev state, to avoid passing the entire state
> around when not needed.
>
> Convert all usages of v4l2_subdev_state_xlate_streams() to
> v4l2_subdev_routing_xlate_streams().
>
> Remove v4l2_subdev_state_xlate_streams().
>
> V2:
>   * Fix description of parameters
>
> Cosmin Tanislav (3):
>   media: v4l: subdev: add v4l2_subdev_routing_xlate_streams()
>   media: use v4l2_subdev_routing_xlate_streams()
>   media: v4l: subdev: remove v4l2_subdev_state_xlate_streams()
>
>  drivers/media/i2c/ds90ub913.c                 | 14 ++++++-----
>  drivers/media/i2c/ds90ub953.c                 | 14 ++++++-----
>  drivers/media/i2c/max96714.c                  | 16 ++++++-------
>  drivers/media/i2c/max96717.c                  | 23 ++++++++++---------
>  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 14 ++++++-----
>  .../platform/nxp/imx8-isi/imx8-isi-crossbar.c |  2 +-
>  drivers/media/v4l2-core/v4l2-subdev.c         |  7 +++---
>  include/media/v4l2-subdev.h                   | 10 ++++----
>  8 files changed, 53 insertions(+), 47 deletions(-)
>
> --
> 2.48.1
>
>

Re: [PATCH v2 0/3] media: add v4l2_subdev_state_xlate_streams()

[Cosmin Tanislav]

On 2/20/25 1:38 PM, Jacopo Mondi wrote:
> Hi Cosmin
> 
> On Thu, Feb 20, 2025 at 11:20:32AM +0200, Cosmin Tanislav wrote:
>> Currently, the v4l2_subdev_state_xlate_streams() function is used
>> to translate streams from one pad to another.
>> This function takes the entire subdev state as argument, but only makes
>> use of the routing.
> 
> Correct, but is this a problem ?
> 

No, it's not a problem.

> Is this the first step for a larger rework or is this a drive-by
> beautification ?
> 

Mostly a drive-by beautification to avoid passing the whole state around
where we only need the routing. I'm planning to submit drivers for more
GMSL2/3 chips and we're using this just to not pass the whole state
around. I think I can just use v4l2_subdev_state_xlate_streams(),
but I had these patches in my tree and it would have been good to get
them upstream, in preparations for submitting the GMSL2/3 drivers.

> I'm asking because (and I know it's hard to strike a balance) this
> kind of changes tend to make back-porting a more painful, and if
> only justified by "it looks better" I would be a bit hesitant in
> taking them.
> 
> 
>>
>> Introduce a v4l2_subdev_routing_xlate_streams() function which can be
>> used without the entire subdev state, to avoid passing the entire state
>> around when not needed.
>>
>> Convert all usages of v4l2_subdev_state_xlate_streams() to
>> v4l2_subdev_routing_xlate_streams().
>>
>> Remove v4l2_subdev_state_xlate_streams().
>>
>> V2:
>>    * Fix description of parameters
>>
>> Cosmin Tanislav (3):
>>    media: v4l: subdev: add v4l2_subdev_routing_xlate_streams()
>>    media: use v4l2_subdev_routing_xlate_streams()
>>    media: v4l: subdev: remove v4l2_subdev_state_xlate_streams()
>>
>>   drivers/media/i2c/ds90ub913.c                 | 14 ++++++-----
>>   drivers/media/i2c/ds90ub953.c                 | 14 ++++++-----
>>   drivers/media/i2c/max96714.c                  | 16 ++++++-------
>>   drivers/media/i2c/max96717.c                  | 23 ++++++++++---------
>>   drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 14 ++++++-----
>>   .../platform/nxp/imx8-isi/imx8-isi-crossbar.c |  2 +-
>>   drivers/media/v4l2-core/v4l2-subdev.c         |  7 +++---
>>   include/media/v4l2-subdev.h                   | 10 ++++----
>>   8 files changed, 53 insertions(+), 47 deletions(-)
>>
>> --
>> 2.48.1
>>
>>

Re: [PATCH v2 0/3] media: add v4l2_subdev_state_xlate_streams()

[Sakari Ailus]

Hi Cosmin,

On Thu, Feb 20, 2025 at 03:01:41PM +0200, Cosmin Tanislav wrote:
> 
> 
> On 2/20/25 1:38 PM, Jacopo Mondi wrote:
> > Hi Cosmin
> > 
> > On Thu, Feb 20, 2025 at 11:20:32AM +0200, Cosmin Tanislav wrote:
> > > Currently, the v4l2_subdev_state_xlate_streams() function is used
> > > to translate streams from one pad to another.
> > > This function takes the entire subdev state as argument, but only makes
> > > use of the routing.
> > 
> > Correct, but is this a problem ?
> > 
> 
> No, it's not a problem.

I think I have a slight preference to keep the pattern of referring to the
state as other functions do.

I wonder what Laurent and Hans think, too.

> 
> > Is this the first step for a larger rework or is this a drive-by
> > beautification ?
> > 
> 
> Mostly a drive-by beautification to avoid passing the whole state around
> where we only need the routing. I'm planning to submit drivers for more
> GMSL2/3 chips and we're using this just to not pass the whole state
> around. I think I can just use v4l2_subdev_state_xlate_streams(),
> but I had these patches in my tree and it would have been good to get
> them upstream, in preparations for submitting the GMSL2/3 drivers.
> 
> > I'm asking because (and I know it's hard to strike a balance) this
> > kind of changes tend to make back-porting a more painful, and if
> > only justified by "it looks better" I would be a bit hesitant in
> > taking them.
> > 
> > 
> > > 
> > > Introduce a v4l2_subdev_routing_xlate_streams() function which can be
> > > used without the entire subdev state, to avoid passing the entire state
> > > around when not needed.
> > > 
> > > Convert all usages of v4l2_subdev_state_xlate_streams() to
> > > v4l2_subdev_routing_xlate_streams().
> > > 
> > > Remove v4l2_subdev_state_xlate_streams().
> > > 
> > > V2:
> > >    * Fix description of parameters
> > > 
> > > Cosmin Tanislav (3):
> > >    media: v4l: subdev: add v4l2_subdev_routing_xlate_streams()
> > >    media: use v4l2_subdev_routing_xlate_streams()
> > >    media: v4l: subdev: remove v4l2_subdev_state_xlate_streams()
> > > 
> > >   drivers/media/i2c/ds90ub913.c                 | 14 ++++++-----
> > >   drivers/media/i2c/ds90ub953.c                 | 14 ++++++-----
> > >   drivers/media/i2c/max96714.c                  | 16 ++++++-------
> > >   drivers/media/i2c/max96717.c                  | 23 ++++++++++---------
> > >   drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 14 ++++++-----
> > >   .../platform/nxp/imx8-isi/imx8-isi-crossbar.c |  2 +-
> > >   drivers/media/v4l2-core/v4l2-subdev.c         |  7 +++---
> > >   include/media/v4l2-subdev.h                   | 10 ++++----
> > >   8 files changed, 53 insertions(+), 47 deletions(-)
> > > 
> > > --
> > > 2.48.1
> > > 
> > > 
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 0/3] media: add v4l2_subdev_state_xlate_streams()

[Laurent Pinchart]

On Thu, Feb 20, 2025 at 02:16:51PM +0000, Sakari Ailus wrote:
> On Thu, Feb 20, 2025 at 03:01:41PM +0200, Cosmin Tanislav wrote:
> > On 2/20/25 1:38 PM, Jacopo Mondi wrote:
> > > On Thu, Feb 20, 2025 at 11:20:32AM +0200, Cosmin Tanislav wrote:
> > > > Currently, the v4l2_subdev_state_xlate_streams() function is used
> > > > to translate streams from one pad to another.
> > > > This function takes the entire subdev state as argument, but only makes
> > > > use of the routing.
> > > 
> > > Correct, but is this a problem ?
> > > 
> > 
> > No, it's not a problem.
> 
> I think I have a slight preference to keep the pattern of referring to the
> state as other functions do.
> 
> I wonder what Laurent and Hans think, too.

I agree, I think the state should be passed everywhere. This lowers the
risk of subsystem-wide refactoring if a function that receives a pointer
to part of a state (such as v4l2_subdev_routing_xlate_streams()) is
later found to need more information from the state.

The situation would be different if the states were not monolithic, for
instance if the routing table could be locked separatly from other parts
of the state, but that's not the case and I don't foresee moving to
finer-grained objects.

> > > Is this the first step for a larger rework or is this a drive-by
> > > beautification ?
> > 
> > Mostly a drive-by beautification to avoid passing the whole state around
> > where we only need the routing. I'm planning to submit drivers for more
> > GMSL2/3 chips and we're using this just to not pass the whole state
> > around. I think I can just use v4l2_subdev_state_xlate_streams(),
> > but I had these patches in my tree and it would have been good to get
> > them upstream, in preparations for submitting the GMSL2/3 drivers.
> > 
> > > I'm asking because (and I know it's hard to strike a balance) this
> > > kind of changes tend to make back-porting a more painful, and if
> > > only justified by "it looks better" I would be a bit hesitant in
> > > taking them.
> > > 
> > > > Introduce a v4l2_subdev_routing_xlate_streams() function which can be
> > > > used without the entire subdev state, to avoid passing the entire state
> > > > around when not needed.
> > > > 
> > > > Convert all usages of v4l2_subdev_state_xlate_streams() to
> > > > v4l2_subdev_routing_xlate_streams().
> > > > 
> > > > Remove v4l2_subdev_state_xlate_streams().
> > > > 
> > > > V2:
> > > >    * Fix description of parameters
> > > > 
> > > > Cosmin Tanislav (3):
> > > >    media: v4l: subdev: add v4l2_subdev_routing_xlate_streams()
> > > >    media: use v4l2_subdev_routing_xlate_streams()
> > > >    media: v4l: subdev: remove v4l2_subdev_state_xlate_streams()
> > > > 
> > > >   drivers/media/i2c/ds90ub913.c                 | 14 ++++++-----
> > > >   drivers/media/i2c/ds90ub953.c                 | 14 ++++++-----
> > > >   drivers/media/i2c/max96714.c                  | 16 ++++++-------
> > > >   drivers/media/i2c/max96717.c                  | 23 ++++++++++---------
> > > >   drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 14 ++++++-----
> > > >   .../platform/nxp/imx8-isi/imx8-isi-crossbar.c |  2 +-
> > > >   drivers/media/v4l2-core/v4l2-subdev.c         |  7 +++---
> > > >   include/media/v4l2-subdev.h                   | 10 ++++----
> > > >   8 files changed, 53 insertions(+), 47 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] media: add v4l2_subdev_state_xlate_streams()

[Cosmin Tanislav]

On 2/20/25 4:39 PM, Laurent Pinchart wrote:
> On Thu, Feb 20, 2025 at 02:16:51PM +0000, Sakari Ailus wrote:
>> On Thu, Feb 20, 2025 at 03:01:41PM +0200, Cosmin Tanislav wrote:
>>> On 2/20/25 1:38 PM, Jacopo Mondi wrote:
>>>> On Thu, Feb 20, 2025 at 11:20:32AM +0200, Cosmin Tanislav wrote:
>>>>> Currently, the v4l2_subdev_state_xlate_streams() function is used
>>>>> to translate streams from one pad to another.
>>>>> This function takes the entire subdev state as argument, but only makes
>>>>> use of the routing.
>>>>
>>>> Correct, but is this a problem ?
>>>>
>>>
>>> No, it's not a problem.
>>
>> I think I have a slight preference to keep the pattern of referring to the
>> state as other functions do.
>>
>> I wonder what Laurent and Hans think, too.
> 
> I agree, I think the state should be passed everywhere. This lowers the
> risk of subsystem-wide refactoring if a function that receives a pointer
> to part of a state (such as v4l2_subdev_routing_xlate_streams()) is
> later found to need more information from the state.
> 
> The situation would be different if the states were not monolithic, for
> instance if the routing table could be locked separatly from other parts
> of the state, but that's not the case and I don't foresee moving to
> finer-grained objects.
> 

Got it. We can drop these patches then.

>>>> Is this the first step for a larger rework or is this a drive-by
>>>> beautification ?
>>>
>>> Mostly a drive-by beautification to avoid passing the whole state around
>>> where we only need the routing. I'm planning to submit drivers for more
>>> GMSL2/3 chips and we're using this just to not pass the whole state
>>> around. I think I can just use v4l2_subdev_state_xlate_streams(),
>>> but I had these patches in my tree and it would have been good to get
>>> them upstream, in preparations for submitting the GMSL2/3 drivers.
>>>
>>>> I'm asking because (and I know it's hard to strike a balance) this
>>>> kind of changes tend to make back-porting a more painful, and if
>>>> only justified by "it looks better" I would be a bit hesitant in
>>>> taking them.
>>>>
>>>>> Introduce a v4l2_subdev_routing_xlate_streams() function which can be
>>>>> used without the entire subdev state, to avoid passing the entire state
>>>>> around when not needed.
>>>>>
>>>>> Convert all usages of v4l2_subdev_state_xlate_streams() to
>>>>> v4l2_subdev_routing_xlate_streams().
>>>>>
>>>>> Remove v4l2_subdev_state_xlate_streams().
>>>>>
>>>>> V2:
>>>>>     * Fix description of parameters
>>>>>
>>>>> Cosmin Tanislav (3):
>>>>>     media: v4l: subdev: add v4l2_subdev_routing_xlate_streams()
>>>>>     media: use v4l2_subdev_routing_xlate_streams()
>>>>>     media: v4l: subdev: remove v4l2_subdev_state_xlate_streams()
>>>>>
>>>>>    drivers/media/i2c/ds90ub913.c                 | 14 ++++++-----
>>>>>    drivers/media/i2c/ds90ub953.c                 | 14 ++++++-----
>>>>>    drivers/media/i2c/max96714.c                  | 16 ++++++-------
>>>>>    drivers/media/i2c/max96717.c                  | 23 ++++++++++---------
>>>>>    drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 14 ++++++-----
>>>>>    .../platform/nxp/imx8-isi/imx8-isi-crossbar.c |  2 +-
>>>>>    drivers/media/v4l2-core/v4l2-subdev.c         |  7 +++---
>>>>>    include/media/v4l2-subdev.h                   | 10 ++++----
>>>>>    8 files changed, 53 insertions(+), 47 deletions(-)
>