[PATCH v2 0/3] media: Make v4l2_subdev_stream_config private

[PATCH v2 0/3] media: Make v4l2_subdev_stream_config private

[Laurent Pinchart]

Hello,

This small patch series attempts to avoid usage of the
v4l2_subdev_stream_config structure in drivers. The rationale is that
the structure was meant to be an implementation detail. Recent
discussions about how we store stream config led to considering a need
to rework the internals, and usage of the structure in drivers makes
this more difficult.

Patch 1/3 and 2/3 drop usage of the structure in the ds90ub913 and
ipu7-isys drivers, and patch 3/3 then makes the structure private to
v4l2-subdev.c.

Ideally we should also make v4l2_subdev_pad_config private, but it has a
few more users:

drivers/media/pci/saa7134/saa7134-empress.c
drivers/media/platform/atmel/atmel-isi.c
drivers/media/platform/intel/pxa_camera.c
drivers/media/platform/marvell/mcam-core.c
drivers/media/platform/renesas/renesas-ceu.c
drivers/media/platform/via/via-camera.c
drivers/staging/media/deprecated/atmel/atmel-isc-base.c

All those drivers are video-centric drivers that use
v4l2_subdev_pad_config to implement TRY_FMT. I think we could use
v4l2_subdev_call_state_try() in those drivers, like done in

commit f076057f0107c3ef890bfad8d6658387504e7f11
Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Date:   Fri Jul 1 14:15:59 2022 +0100

    media: stm32: dcmi: Fix subdev op call with uninitialized state

I however lack the ability to test that beside compile testing.

Laurent Pinchart (3):
  media: i2c: ds90ub913: Stop accessing streams configs directly
  media: staging/ipu7: Disallow source multiplexing
  media: v4l2-subdev: Make struct v4l2_subdev_stream_config private

 drivers/media/i2c/ds90ub913.c                 | 17 ++++-----
 drivers/media/v4l2-core/v4l2-subdev.c         | 24 +++++++++++++
 drivers/staging/media/ipu7/ipu7-isys-queue.c  |  3 +-
 drivers/staging/media/ipu7/ipu7-isys-subdev.c | 35 ++++++-------------
 drivers/staging/media/ipu7/ipu7-isys-subdev.h |  1 -
 drivers/staging/media/ipu7/ipu7-isys-video.c  | 35 ++-----------------
 include/media/v4l2-subdev.h                   | 25 +------------
 7 files changed, 48 insertions(+), 92 deletions(-)


base-commit: d968e50b5c26642754492dea23cbd3592bde62d8
-- 
Regards,

Laurent Pinchart

[PATCH v2 1/3] media: i2c: ds90ub913: Stop accessing streams configs directly

[Laurent Pinchart]

The v4l2_subdev_stream_config structure will be made private. Stop
accessing it directly, iterate over routes instead to initialize
formats.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/ds90ub913.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index bc74499b0a96..29c486c114c4 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -333,8 +333,7 @@ static int _ub913_set_routing(struct v4l2_subdev *sd,
 		.quantization = V4L2_QUANTIZATION_LIM_RANGE,
 		.xfer_func = V4L2_XFER_FUNC_SRGB,
 	};
-	struct v4l2_subdev_stream_configs *stream_configs;
-	unsigned int i;
+	struct v4l2_subdev_route *route;
 	int ret;
 
 	ret = v4l2_subdev_routing_validate(sd, routing,
@@ -346,13 +345,15 @@ static int _ub913_set_routing(struct v4l2_subdev *sd,
 	if (ret)
 		return ret;
 
-	stream_configs = &state->stream_configs;
+	for_each_active_route(&state->routing, route) {
+		struct v4l2_mbus_framefmt *fmt;
 
-	for (i = 0; i < stream_configs->num_configs; i++) {
-		if (stream_configs->configs[i].pad == UB913_PAD_SINK)
-			stream_configs->configs[i].fmt = in_format;
-		else
-			stream_configs->configs[i].fmt = out_format;
+		fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
+						   route->sink_stream);
+		*fmt = in_format;
+		fmt = v4l2_subdev_state_get_format(state, route->source_pad,
+						   route->source_stream);
+		*fmt = out_format;
 	}
 
 	return 0;
-- 
Regards,

Laurent Pinchart

[PATCH v2 2/3] media: staging/ipu7: Disallow source multiplexing

[Laurent Pinchart]

The IPU7 ISYS driver can't capture multiple streams on the same video
device. Disallow source multiplexing in the routes of the internal
subdev to reflect that limitation. As a result we can hardcode the
source stream to 0, simplifying the driver.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/ipu7/ipu7-isys-queue.c  |  3 +-
 drivers/staging/media/ipu7/ipu7-isys-subdev.c | 35 ++++++-------------
 drivers/staging/media/ipu7/ipu7-isys-subdev.h |  1 -
 drivers/staging/media/ipu7/ipu7-isys-video.c  | 35 ++-----------------
 4 files changed, 14 insertions(+), 60 deletions(-)

diff --git a/drivers/staging/media/ipu7/ipu7-isys-queue.c b/drivers/staging/media/ipu7/ipu7-isys-queue.c
index 7046c29141f8..434d9d9c7158 100644
--- a/drivers/staging/media/ipu7/ipu7-isys-queue.c
+++ b/drivers/staging/media/ipu7/ipu7-isys-queue.c
@@ -442,14 +442,13 @@ static int ipu7_isys_link_fmt_validate(struct ipu7_isys_queue *aq)
 		media_pad_remote_pad_first(av->vdev.entity.pads);
 	struct v4l2_mbus_framefmt format;
 	struct v4l2_subdev *sd;
-	u32 r_stream, code;
+	u32 r_stream = 0, code;
 	int ret;
 
 	if (!remote_pad)
 		return -ENOTCONN;
 
 	sd = media_entity_to_v4l2_subdev(remote_pad->entity);
-	r_stream = ipu7_isys_get_src_stream_by_src_pad(sd, remote_pad->index);
 
 	ret = ipu7_isys_get_stream_pad_fmt(sd, remote_pad->index, r_stream,
 					   &format);
diff --git a/drivers/staging/media/ipu7/ipu7-isys-subdev.c b/drivers/staging/media/ipu7/ipu7-isys-subdev.c
index 98b6ef6a2f21..67a776033d5b 100644
--- a/drivers/staging/media/ipu7/ipu7-isys-subdev.c
+++ b/drivers/staging/media/ipu7/ipu7-isys-subdev.c
@@ -194,13 +194,22 @@ static int subdev_set_routing(struct v4l2_subdev *sd,
 		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
 		.field = V4L2_FIELD_NONE,
 	};
+	struct v4l2_subdev_route *route;
 	int ret;
 
 	ret = v4l2_subdev_routing_validate(sd, routing,
-					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
+					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1 |
+					   V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING);
 	if (ret)
 		return ret;
 
+	/*
+	 * The device doesn't support source multiplexing, set all source
+	 * streams to 0 to simplify stream handling through the driver.
+	 */
+	for_each_active_route(routing, route)
+		route->source_stream = 0;
+
 	return v4l2_subdev_set_routing_with_fmt(sd, state, routing, &fmt);
 }
 
@@ -222,30 +231,6 @@ int ipu7_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
 	return fmt ? 0 : -EINVAL;
 }
 
-u32 ipu7_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad)
-{
-	struct v4l2_subdev_state *state;
-	struct v4l2_subdev_route *routes;
-	u32 source_stream = 0;
-	unsigned int i;
-
-	state = v4l2_subdev_lock_and_get_active_state(sd);
-	if (!state)
-		return 0;
-
-	routes = state->routing.routes;
-	for (i = 0; i < state->routing.num_routes; i++) {
-		if (routes[i].source_pad == pad) {
-			source_stream = routes[i].source_stream;
-			break;
-		}
-	}
-
-	v4l2_subdev_unlock_state(state);
-
-	return source_stream;
-}
-
 static int ipu7_isys_subdev_init_state(struct v4l2_subdev *sd,
 				       struct v4l2_subdev_state *state)
 {
diff --git a/drivers/staging/media/ipu7/ipu7-isys-subdev.h b/drivers/staging/media/ipu7/ipu7-isys-subdev.h
index 1057ec39ae39..faa50031cf24 100644
--- a/drivers/staging/media/ipu7/ipu7-isys-subdev.h
+++ b/drivers/staging/media/ipu7/ipu7-isys-subdev.h
@@ -37,7 +37,6 @@ int ipu7_isys_subdev_enum_mbus_code(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_state *state,
 				    struct v4l2_subdev_mbus_code_enum
 				    *code);
-u32 ipu7_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad);
 int ipu7_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
 				 struct v4l2_mbus_framefmt *format);
 int ipu7_isys_subdev_set_routing(struct v4l2_subdev *sd,
diff --git a/drivers/staging/media/ipu7/ipu7-isys-video.c b/drivers/staging/media/ipu7/ipu7-isys-video.c
index 8756da3a8fb0..3d52634683d6 100644
--- a/drivers/staging/media/ipu7/ipu7-isys-video.c
+++ b/drivers/staging/media/ipu7/ipu7-isys-video.c
@@ -291,7 +291,7 @@ static int link_validate(struct media_link *link)
 	struct v4l2_mbus_framefmt *s_fmt;
 	struct v4l2_subdev *s_sd;
 	struct media_pad *s_pad;
-	u32 s_stream, code;
+	u32 s_stream = 0, code;
 	int ret = -EPIPE;
 
 	if (!link->source->entity)
@@ -307,7 +307,6 @@ static int link_validate(struct media_link *link)
 		link->sink->entity->name);
 
 	s_pad = media_pad_remote_pad_first(&av->pad);
-	s_stream = ipu7_isys_get_src_stream_by_src_pad(s_sd, s_pad->index);
 
 	v4l2_subdev_lock_state(s_state);
 
@@ -370,10 +369,9 @@ static int ipu7_isys_fw_pin_cfg(struct ipu7_isys_video *av,
 	struct device *dev = &isys->adev->auxdev.dev;
 	struct v4l2_mbus_framefmt fmt;
 	int output_pins;
-	u32 src_stream;
+	u32 src_stream = 0;
 	int ret;
 
-	src_stream = ipu7_isys_get_src_stream_by_src_pad(sd, src_pad->index);
 	ret = ipu7_isys_get_stream_pad_fmt(sd, src_pad->index, src_stream,
 					   &fmt);
 	if (ret < 0) {
@@ -781,32 +779,6 @@ ipu7_isys_query_stream_by_source(struct ipu7_isys *isys, int source, u8 vc)
 	return stream;
 }
 
-static u32 get_remote_pad_stream(struct media_pad *r_pad)
-{
-	struct v4l2_subdev_state *state;
-	struct v4l2_subdev *sd;
-	u32 stream_id = 0;
-	unsigned int i;
-
-	sd = media_entity_to_v4l2_subdev(r_pad->entity);
-	state = v4l2_subdev_lock_and_get_active_state(sd);
-	if (!state)
-		return 0;
-
-	for (i = 0; i < state->stream_configs.num_configs; i++) {
-		struct v4l2_subdev_stream_config *cfg =
-			&state->stream_configs.configs[i];
-		if (cfg->pad == r_pad->index) {
-			stream_id = cfg->stream;
-			break;
-		}
-	}
-
-	v4l2_subdev_unlock_state(state);
-
-	return stream_id;
-}
-
 int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
 				  struct ipu7_isys_buffer_list *bl)
 {
@@ -814,7 +786,7 @@ int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
 	struct device *dev = &av->isys->adev->auxdev.dev;
 	struct media_pad *r_pad;
 	struct v4l2_subdev *sd;
-	u32 r_stream;
+	u32 r_stream = 0;
 	int ret = 0;
 
 	dev_dbg(dev, "set stream: %d\n", state);
@@ -824,7 +796,6 @@ int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
 
 	sd = &stream->asd->sd;
 	r_pad = media_pad_remote_pad_first(&av->pad);
-	r_stream = get_remote_pad_stream(r_pad);
 	if (!state) {
 		stop_streaming_firmware(av);
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 3/3] media: v4l2-subdev: Make struct v4l2_subdev_stream_config private

[Laurent Pinchart]

The v4l2_subdev_stream_config structure holds configuration data for a
stream. It was meant to be used internally only, but already found its
way into the ds90ub913 driver. Now that the driver has been fixed, make
the structure private to v4l2-subdev.c to avoid using it by accident.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 24 ++++++++++++++++++++++++
 include/media/v4l2-subdev.h           | 25 +------------------------
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 4fd25fea3b58..7be28e573d3f 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -26,6 +26,30 @@
 #include <media/v4l2-fh.h>
 #include <media/v4l2-ioctl.h>
 
+/**
+ * struct v4l2_subdev_stream_config - Used for storing stream configuration.
+ *
+ * @pad: pad number
+ * @stream: stream number
+ * @enabled: has the stream been enabled with v4l2_subdev_enable_streams()
+ * @fmt: &struct v4l2_mbus_framefmt
+ * @crop: &struct v4l2_rect to be used for crop
+ * @compose: &struct v4l2_rect to be used for compose
+ * @interval: frame interval
+ *
+ * This structure stores configuration for a stream.
+ */
+struct v4l2_subdev_stream_config {
+	u32 pad;
+	u32 stream;
+	bool enabled;
+
+	struct v4l2_mbus_framefmt fmt;
+	struct v4l2_rect crop;
+	struct v4l2_rect compose;
+	struct v4l2_fract interval;
+};
+
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 /*
  * The Streams API is an experimental feature. To use the Streams API, set
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 5dcf4065708f..8f54fd0d90ad 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -36,6 +36,7 @@ struct v4l2_event_subscription;
 struct v4l2_fh;
 struct v4l2_subdev;
 struct v4l2_subdev_fh;
+struct v4l2_subdev_stream_config;
 struct tuner_setup;
 struct v4l2_mbus_frame_desc;
 struct led_classdev;
@@ -683,30 +684,6 @@ struct v4l2_subdev_pad_config {
 	struct v4l2_fract interval;
 };
 
-/**
- * struct v4l2_subdev_stream_config - Used for storing stream configuration.
- *
- * @pad: pad number
- * @stream: stream number
- * @enabled: has the stream been enabled with v4l2_subdev_enable_streams()
- * @fmt: &struct v4l2_mbus_framefmt
- * @crop: &struct v4l2_rect to be used for crop
- * @compose: &struct v4l2_rect to be used for compose
- * @interval: frame interval
- *
- * This structure stores configuration for a stream.
- */
-struct v4l2_subdev_stream_config {
-	u32 pad;
-	u32 stream;
-	bool enabled;
-
-	struct v4l2_mbus_framefmt fmt;
-	struct v4l2_rect crop;
-	struct v4l2_rect compose;
-	struct v4l2_fract interval;
-};
-
 /**
  * struct v4l2_subdev_stream_configs - A collection of stream configs.
  *
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] media: staging/ipu7: Disallow source multiplexing

[Laurent Pinchart]

On Tue, Jul 29, 2025 at 02:50:09AM +0300, Laurent Pinchart wrote:
> The IPU7 ISYS driver can't capture multiple streams on the same video
> device. Disallow source multiplexing in the routes of the internal
> subdev to reflect that limitation. As a result we can hardcode the
> source stream to 0, simplifying the driver.

This is the first time I look at the IPU7 ISYS driver internals.
Apparently, it started as a copy of the ipu6-isys, and then diverged.
The two drivers share a very similar architecture for the
hardware-independent parts, and I'm very concerned about the amount of
code duplication this causes. There's also lots of cargo-cult code
constructs like the ones removed/simplified in this patch, or just code
that needs to be cleaned up.

Give all this, and the size of the two drivers, I would like to see the
IPU6 and IPU7 ISYS being supported by the same driver. Anything else
will become absolutely unmaintainable. I don't want to see IPU6 and IPU7
repeating the IPU3 story.

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/ipu7/ipu7-isys-queue.c  |  3 +-
>  drivers/staging/media/ipu7/ipu7-isys-subdev.c | 35 ++++++-------------
>  drivers/staging/media/ipu7/ipu7-isys-subdev.h |  1 -
>  drivers/staging/media/ipu7/ipu7-isys-video.c  | 35 ++-----------------
>  4 files changed, 14 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu7/ipu7-isys-queue.c b/drivers/staging/media/ipu7/ipu7-isys-queue.c
> index 7046c29141f8..434d9d9c7158 100644
> --- a/drivers/staging/media/ipu7/ipu7-isys-queue.c
> +++ b/drivers/staging/media/ipu7/ipu7-isys-queue.c
> @@ -442,14 +442,13 @@ static int ipu7_isys_link_fmt_validate(struct ipu7_isys_queue *aq)
>  		media_pad_remote_pad_first(av->vdev.entity.pads);
>  	struct v4l2_mbus_framefmt format;
>  	struct v4l2_subdev *sd;
> -	u32 r_stream, code;
> +	u32 r_stream = 0, code;
>  	int ret;
>  
>  	if (!remote_pad)
>  		return -ENOTCONN;
>  
>  	sd = media_entity_to_v4l2_subdev(remote_pad->entity);
> -	r_stream = ipu7_isys_get_src_stream_by_src_pad(sd, remote_pad->index);
>  
>  	ret = ipu7_isys_get_stream_pad_fmt(sd, remote_pad->index, r_stream,
>  					   &format);
> diff --git a/drivers/staging/media/ipu7/ipu7-isys-subdev.c b/drivers/staging/media/ipu7/ipu7-isys-subdev.c
> index 98b6ef6a2f21..67a776033d5b 100644
> --- a/drivers/staging/media/ipu7/ipu7-isys-subdev.c
> +++ b/drivers/staging/media/ipu7/ipu7-isys-subdev.c
> @@ -194,13 +194,22 @@ static int subdev_set_routing(struct v4l2_subdev *sd,
>  		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
>  		.field = V4L2_FIELD_NONE,
>  	};
> +	struct v4l2_subdev_route *route;
>  	int ret;
>  
>  	ret = v4l2_subdev_routing_validate(sd, routing,
> -					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
> +					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1 |
> +					   V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING);
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * The device doesn't support source multiplexing, set all source
> +	 * streams to 0 to simplify stream handling through the driver.
> +	 */
> +	for_each_active_route(routing, route)
> +		route->source_stream = 0;
> +
>  	return v4l2_subdev_set_routing_with_fmt(sd, state, routing, &fmt);
>  }
>  
> @@ -222,30 +231,6 @@ int ipu7_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
>  	return fmt ? 0 : -EINVAL;
>  }
>  
> -u32 ipu7_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad)
> -{
> -	struct v4l2_subdev_state *state;
> -	struct v4l2_subdev_route *routes;
> -	u32 source_stream = 0;
> -	unsigned int i;
> -
> -	state = v4l2_subdev_lock_and_get_active_state(sd);
> -	if (!state)
> -		return 0;
> -
> -	routes = state->routing.routes;
> -	for (i = 0; i < state->routing.num_routes; i++) {
> -		if (routes[i].source_pad == pad) {
> -			source_stream = routes[i].source_stream;
> -			break;
> -		}
> -	}
> -
> -	v4l2_subdev_unlock_state(state);
> -
> -	return source_stream;
> -}
> -
>  static int ipu7_isys_subdev_init_state(struct v4l2_subdev *sd,
>  				       struct v4l2_subdev_state *state)
>  {
> diff --git a/drivers/staging/media/ipu7/ipu7-isys-subdev.h b/drivers/staging/media/ipu7/ipu7-isys-subdev.h
> index 1057ec39ae39..faa50031cf24 100644
> --- a/drivers/staging/media/ipu7/ipu7-isys-subdev.h
> +++ b/drivers/staging/media/ipu7/ipu7-isys-subdev.h
> @@ -37,7 +37,6 @@ int ipu7_isys_subdev_enum_mbus_code(struct v4l2_subdev *sd,
>  				    struct v4l2_subdev_state *state,
>  				    struct v4l2_subdev_mbus_code_enum
>  				    *code);
> -u32 ipu7_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad);
>  int ipu7_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
>  				 struct v4l2_mbus_framefmt *format);
>  int ipu7_isys_subdev_set_routing(struct v4l2_subdev *sd,
> diff --git a/drivers/staging/media/ipu7/ipu7-isys-video.c b/drivers/staging/media/ipu7/ipu7-isys-video.c
> index 8756da3a8fb0..3d52634683d6 100644
> --- a/drivers/staging/media/ipu7/ipu7-isys-video.c
> +++ b/drivers/staging/media/ipu7/ipu7-isys-video.c
> @@ -291,7 +291,7 @@ static int link_validate(struct media_link *link)
>  	struct v4l2_mbus_framefmt *s_fmt;
>  	struct v4l2_subdev *s_sd;
>  	struct media_pad *s_pad;
> -	u32 s_stream, code;
> +	u32 s_stream = 0, code;
>  	int ret = -EPIPE;
>  
>  	if (!link->source->entity)
> @@ -307,7 +307,6 @@ static int link_validate(struct media_link *link)
>  		link->sink->entity->name);
>  
>  	s_pad = media_pad_remote_pad_first(&av->pad);
> -	s_stream = ipu7_isys_get_src_stream_by_src_pad(s_sd, s_pad->index);
>  
>  	v4l2_subdev_lock_state(s_state);
>  
> @@ -370,10 +369,9 @@ static int ipu7_isys_fw_pin_cfg(struct ipu7_isys_video *av,
>  	struct device *dev = &isys->adev->auxdev.dev;
>  	struct v4l2_mbus_framefmt fmt;
>  	int output_pins;
> -	u32 src_stream;
> +	u32 src_stream = 0;
>  	int ret;
>  
> -	src_stream = ipu7_isys_get_src_stream_by_src_pad(sd, src_pad->index);
>  	ret = ipu7_isys_get_stream_pad_fmt(sd, src_pad->index, src_stream,
>  					   &fmt);
>  	if (ret < 0) {
> @@ -781,32 +779,6 @@ ipu7_isys_query_stream_by_source(struct ipu7_isys *isys, int source, u8 vc)
>  	return stream;
>  }
>  
> -static u32 get_remote_pad_stream(struct media_pad *r_pad)
> -{
> -	struct v4l2_subdev_state *state;
> -	struct v4l2_subdev *sd;
> -	u32 stream_id = 0;
> -	unsigned int i;
> -
> -	sd = media_entity_to_v4l2_subdev(r_pad->entity);
> -	state = v4l2_subdev_lock_and_get_active_state(sd);
> -	if (!state)
> -		return 0;
> -
> -	for (i = 0; i < state->stream_configs.num_configs; i++) {
> -		struct v4l2_subdev_stream_config *cfg =
> -			&state->stream_configs.configs[i];
> -		if (cfg->pad == r_pad->index) {
> -			stream_id = cfg->stream;
> -			break;
> -		}
> -	}
> -
> -	v4l2_subdev_unlock_state(state);
> -
> -	return stream_id;
> -}
> -
>  int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
>  				  struct ipu7_isys_buffer_list *bl)
>  {
> @@ -814,7 +786,7 @@ int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
>  	struct device *dev = &av->isys->adev->auxdev.dev;
>  	struct media_pad *r_pad;
>  	struct v4l2_subdev *sd;
> -	u32 r_stream;
> +	u32 r_stream = 0;
>  	int ret = 0;
>  
>  	dev_dbg(dev, "set stream: %d\n", state);
> @@ -824,7 +796,6 @@ int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
>  
>  	sd = &stream->asd->sd;
>  	r_pad = media_pad_remote_pad_first(&av->pad);
> -	r_stream = get_remote_pad_stream(r_pad);
>  	if (!state) {
>  		stop_streaming_firmware(av);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] media: staging/ipu7: Disallow source multiplexing

[Jacopo Mondi]

Hi Laurent

On Tue, Jul 29, 2025 at 02:50:09AM +0300, Laurent Pinchart wrote:
> The IPU7 ISYS driver can't capture multiple streams on the same video
> device. Disallow source multiplexing in the routes of the internal
> subdev to reflect that limitation. As a result we can hardcode the
> source stream to 0, simplifying the driver.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/ipu7/ipu7-isys-queue.c  |  3 +-
>  drivers/staging/media/ipu7/ipu7-isys-subdev.c | 35 ++++++-------------
>  drivers/staging/media/ipu7/ipu7-isys-subdev.h |  1 -
>  drivers/staging/media/ipu7/ipu7-isys-video.c  | 35 ++-----------------
>  4 files changed, 14 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/staging/media/ipu7/ipu7-isys-queue.c b/drivers/staging/media/ipu7/ipu7-isys-queue.c
> index 7046c29141f8..434d9d9c7158 100644
> --- a/drivers/staging/media/ipu7/ipu7-isys-queue.c
> +++ b/drivers/staging/media/ipu7/ipu7-isys-queue.c
> @@ -442,14 +442,13 @@ static int ipu7_isys_link_fmt_validate(struct ipu7_isys_queue *aq)
>  		media_pad_remote_pad_first(av->vdev.entity.pads);
>  	struct v4l2_mbus_framefmt format;
>  	struct v4l2_subdev *sd;
> -	u32 r_stream, code;
> +	u32 r_stream = 0, code;
>  	int ret;
>
>  	if (!remote_pad)
>  		return -ENOTCONN;
>
>  	sd = media_entity_to_v4l2_subdev(remote_pad->entity);
> -	r_stream = ipu7_isys_get_src_stream_by_src_pad(sd, remote_pad->index);
>
>  	ret = ipu7_isys_get_stream_pad_fmt(sd, remote_pad->index, r_stream,
>  					   &format);
> diff --git a/drivers/staging/media/ipu7/ipu7-isys-subdev.c b/drivers/staging/media/ipu7/ipu7-isys-subdev.c
> index 98b6ef6a2f21..67a776033d5b 100644
> --- a/drivers/staging/media/ipu7/ipu7-isys-subdev.c
> +++ b/drivers/staging/media/ipu7/ipu7-isys-subdev.c
> @@ -194,13 +194,22 @@ static int subdev_set_routing(struct v4l2_subdev *sd,
>  		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
>  		.field = V4L2_FIELD_NONE,
>  	};
> +	struct v4l2_subdev_route *route;
>  	int ret;
>
>  	ret = v4l2_subdev_routing_validate(sd, routing,
> -					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
> +					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1 |
> +					   V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING);
>  	if (ret)
>  		return ret;
>
> +	/*
> +	 * The device doesn't support source multiplexing, set all source
> +	 * streams to 0 to simplify stream handling through the driver.
> +	 */
> +	for_each_active_route(routing, route)
> +		route->source_stream = 0;
> +
>  	return v4l2_subdev_set_routing_with_fmt(sd, state, routing, &fmt);
>  }
>
> @@ -222,30 +231,6 @@ int ipu7_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
>  	return fmt ? 0 : -EINVAL;
>  }
>
> -u32 ipu7_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad)
> -{
> -	struct v4l2_subdev_state *state;
> -	struct v4l2_subdev_route *routes;
> -	u32 source_stream = 0;
> -	unsigned int i;
> -
> -	state = v4l2_subdev_lock_and_get_active_state(sd);
> -	if (!state)
> -		return 0;
> -
> -	routes = state->routing.routes;
> -	for (i = 0; i < state->routing.num_routes; i++) {
> -		if (routes[i].source_pad == pad) {
> -			source_stream = routes[i].source_stream;
> -			break;
> -		}
> -	}
> -
> -	v4l2_subdev_unlock_state(state);
> -
> -	return source_stream;
> -}
> -
>  static int ipu7_isys_subdev_init_state(struct v4l2_subdev *sd,
>  				       struct v4l2_subdev_state *state)
>  {
> diff --git a/drivers/staging/media/ipu7/ipu7-isys-subdev.h b/drivers/staging/media/ipu7/ipu7-isys-subdev.h
> index 1057ec39ae39..faa50031cf24 100644
> --- a/drivers/staging/media/ipu7/ipu7-isys-subdev.h
> +++ b/drivers/staging/media/ipu7/ipu7-isys-subdev.h
> @@ -37,7 +37,6 @@ int ipu7_isys_subdev_enum_mbus_code(struct v4l2_subdev *sd,
>  				    struct v4l2_subdev_state *state,
>  				    struct v4l2_subdev_mbus_code_enum
>  				    *code);
> -u32 ipu7_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad);
>  int ipu7_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
>  				 struct v4l2_mbus_framefmt *format);
>  int ipu7_isys_subdev_set_routing(struct v4l2_subdev *sd,
> diff --git a/drivers/staging/media/ipu7/ipu7-isys-video.c b/drivers/staging/media/ipu7/ipu7-isys-video.c
> index 8756da3a8fb0..3d52634683d6 100644
> --- a/drivers/staging/media/ipu7/ipu7-isys-video.c
> +++ b/drivers/staging/media/ipu7/ipu7-isys-video.c
> @@ -291,7 +291,7 @@ static int link_validate(struct media_link *link)
>  	struct v4l2_mbus_framefmt *s_fmt;
>  	struct v4l2_subdev *s_sd;
>  	struct media_pad *s_pad;
> -	u32 s_stream, code;
> +	u32 s_stream = 0, code;
>  	int ret = -EPIPE;
>
>  	if (!link->source->entity)
> @@ -307,7 +307,6 @@ static int link_validate(struct media_link *link)
>  		link->sink->entity->name);
>
>  	s_pad = media_pad_remote_pad_first(&av->pad);
> -	s_stream = ipu7_isys_get_src_stream_by_src_pad(s_sd, s_pad->index);
>
>  	v4l2_subdev_lock_state(s_state);
>
> @@ -370,10 +369,9 @@ static int ipu7_isys_fw_pin_cfg(struct ipu7_isys_video *av,
>  	struct device *dev = &isys->adev->auxdev.dev;
>  	struct v4l2_mbus_framefmt fmt;
>  	int output_pins;
> -	u32 src_stream;
> +	u32 src_stream = 0;

very minor nit: this can now be moved up

>  	int ret;
>
> -	src_stream = ipu7_isys_get_src_stream_by_src_pad(sd, src_pad->index);
>  	ret = ipu7_isys_get_stream_pad_fmt(sd, src_pad->index, src_stream,
>  					   &fmt);
>  	if (ret < 0) {
> @@ -781,32 +779,6 @@ ipu7_isys_query_stream_by_source(struct ipu7_isys *isys, int source, u8 vc)
>  	return stream;
>  }
>
> -static u32 get_remote_pad_stream(struct media_pad *r_pad)
> -{
> -	struct v4l2_subdev_state *state;
> -	struct v4l2_subdev *sd;
> -	u32 stream_id = 0;
> -	unsigned int i;
> -
> -	sd = media_entity_to_v4l2_subdev(r_pad->entity);
> -	state = v4l2_subdev_lock_and_get_active_state(sd);
> -	if (!state)
> -		return 0;
> -
> -	for (i = 0; i < state->stream_configs.num_configs; i++) {
> -		struct v4l2_subdev_stream_config *cfg =
> -			&state->stream_configs.configs[i];
> -		if (cfg->pad == r_pad->index) {
> -			stream_id = cfg->stream;
> -			break;
> -		}
> -	}
> -
> -	v4l2_subdev_unlock_state(state);
> -
> -	return stream_id;
> -}
> -
>  int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
>  				  struct ipu7_isys_buffer_list *bl)
>  {
> @@ -814,7 +786,7 @@ int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
>  	struct device *dev = &av->isys->adev->auxdev.dev;
>  	struct media_pad *r_pad;
>  	struct v4l2_subdev *sd;
> -	u32 r_stream;
> +	u32 r_stream = 0;
>  	int ret = 0;
>
>  	dev_dbg(dev, "set stream: %d\n", state);
> @@ -824,7 +796,6 @@ int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
>
>  	sd = &stream->asd->sd;
>  	r_pad = media_pad_remote_pad_first(&av->pad);
> -	r_stream = get_remote_pad_stream(r_pad);
>  	if (!state) {
>  		stop_streaming_firmware(av);
>

Here and in the other occurencies where s_stream/r_stream gets
hardcoded to 0, I wonder if using 0 directly instead of going through
a variable would make things easier to follow.

Either way:
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 2/3] media: staging/ipu7: Disallow source multiplexing

[Laurent Pinchart]

Hi Jacopo,

On Tue, Jul 29, 2025 at 09:16:07AM +0200, Jacopo Mondi wrote:
> On Tue, Jul 29, 2025 at 02:50:09AM +0300, Laurent Pinchart wrote:
> > The IPU7 ISYS driver can't capture multiple streams on the same video
> > device. Disallow source multiplexing in the routes of the internal
> > subdev to reflect that limitation. As a result we can hardcode the
> > source stream to 0, simplifying the driver.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/staging/media/ipu7/ipu7-isys-queue.c  |  3 +-
> >  drivers/staging/media/ipu7/ipu7-isys-subdev.c | 35 ++++++-------------
> >  drivers/staging/media/ipu7/ipu7-isys-subdev.h |  1 -
> >  drivers/staging/media/ipu7/ipu7-isys-video.c  | 35 ++-----------------
> >  4 files changed, 14 insertions(+), 60 deletions(-)
> >
> > diff --git a/drivers/staging/media/ipu7/ipu7-isys-queue.c b/drivers/staging/media/ipu7/ipu7-isys-queue.c
> > index 7046c29141f8..434d9d9c7158 100644
> > --- a/drivers/staging/media/ipu7/ipu7-isys-queue.c
> > +++ b/drivers/staging/media/ipu7/ipu7-isys-queue.c
> > @@ -442,14 +442,13 @@ static int ipu7_isys_link_fmt_validate(struct ipu7_isys_queue *aq)
> >  		media_pad_remote_pad_first(av->vdev.entity.pads);
> >  	struct v4l2_mbus_framefmt format;
> >  	struct v4l2_subdev *sd;
> > -	u32 r_stream, code;
> > +	u32 r_stream = 0, code;
> >  	int ret;
> >
> >  	if (!remote_pad)
> >  		return -ENOTCONN;
> >
> >  	sd = media_entity_to_v4l2_subdev(remote_pad->entity);
> > -	r_stream = ipu7_isys_get_src_stream_by_src_pad(sd, remote_pad->index);
> >
> >  	ret = ipu7_isys_get_stream_pad_fmt(sd, remote_pad->index, r_stream,
> >  					   &format);
> > diff --git a/drivers/staging/media/ipu7/ipu7-isys-subdev.c b/drivers/staging/media/ipu7/ipu7-isys-subdev.c
> > index 98b6ef6a2f21..67a776033d5b 100644
> > --- a/drivers/staging/media/ipu7/ipu7-isys-subdev.c
> > +++ b/drivers/staging/media/ipu7/ipu7-isys-subdev.c
> > @@ -194,13 +194,22 @@ static int subdev_set_routing(struct v4l2_subdev *sd,
> >  		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
> >  		.field = V4L2_FIELD_NONE,
> >  	};
> > +	struct v4l2_subdev_route *route;
> >  	int ret;
> >
> >  	ret = v4l2_subdev_routing_validate(sd, routing,
> > -					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
> > +					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1 |
> > +					   V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING);
> >  	if (ret)
> >  		return ret;
> >
> > +	/*
> > +	 * The device doesn't support source multiplexing, set all source
> > +	 * streams to 0 to simplify stream handling through the driver.
> > +	 */
> > +	for_each_active_route(routing, route)
> > +		route->source_stream = 0;
> > +
> >  	return v4l2_subdev_set_routing_with_fmt(sd, state, routing, &fmt);
> >  }
> >
> > @@ -222,30 +231,6 @@ int ipu7_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
> >  	return fmt ? 0 : -EINVAL;
> >  }
> >
> > -u32 ipu7_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad)
> > -{
> > -	struct v4l2_subdev_state *state;
> > -	struct v4l2_subdev_route *routes;
> > -	u32 source_stream = 0;
> > -	unsigned int i;
> > -
> > -	state = v4l2_subdev_lock_and_get_active_state(sd);
> > -	if (!state)
> > -		return 0;
> > -
> > -	routes = state->routing.routes;
> > -	for (i = 0; i < state->routing.num_routes; i++) {
> > -		if (routes[i].source_pad == pad) {
> > -			source_stream = routes[i].source_stream;
> > -			break;
> > -		}
> > -	}
> > -
> > -	v4l2_subdev_unlock_state(state);
> > -
> > -	return source_stream;
> > -}
> > -
> >  static int ipu7_isys_subdev_init_state(struct v4l2_subdev *sd,
> >  				       struct v4l2_subdev_state *state)
> >  {
> > diff --git a/drivers/staging/media/ipu7/ipu7-isys-subdev.h b/drivers/staging/media/ipu7/ipu7-isys-subdev.h
> > index 1057ec39ae39..faa50031cf24 100644
> > --- a/drivers/staging/media/ipu7/ipu7-isys-subdev.h
> > +++ b/drivers/staging/media/ipu7/ipu7-isys-subdev.h
> > @@ -37,7 +37,6 @@ int ipu7_isys_subdev_enum_mbus_code(struct v4l2_subdev *sd,
> >  				    struct v4l2_subdev_state *state,
> >  				    struct v4l2_subdev_mbus_code_enum
> >  				    *code);
> > -u32 ipu7_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad);
> >  int ipu7_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
> >  				 struct v4l2_mbus_framefmt *format);
> >  int ipu7_isys_subdev_set_routing(struct v4l2_subdev *sd,
> > diff --git a/drivers/staging/media/ipu7/ipu7-isys-video.c b/drivers/staging/media/ipu7/ipu7-isys-video.c
> > index 8756da3a8fb0..3d52634683d6 100644
> > --- a/drivers/staging/media/ipu7/ipu7-isys-video.c
> > +++ b/drivers/staging/media/ipu7/ipu7-isys-video.c
> > @@ -291,7 +291,7 @@ static int link_validate(struct media_link *link)
> >  	struct v4l2_mbus_framefmt *s_fmt;
> >  	struct v4l2_subdev *s_sd;
> >  	struct media_pad *s_pad;
> > -	u32 s_stream, code;
> > +	u32 s_stream = 0, code;
> >  	int ret = -EPIPE;
> >
> >  	if (!link->source->entity)
> > @@ -307,7 +307,6 @@ static int link_validate(struct media_link *link)
> >  		link->sink->entity->name);
> >
> >  	s_pad = media_pad_remote_pad_first(&av->pad);
> > -	s_stream = ipu7_isys_get_src_stream_by_src_pad(s_sd, s_pad->index);
> >
> >  	v4l2_subdev_lock_state(s_state);
> >
> > @@ -370,10 +369,9 @@ static int ipu7_isys_fw_pin_cfg(struct ipu7_isys_video *av,
> >  	struct device *dev = &isys->adev->auxdev.dev;
> >  	struct v4l2_mbus_framefmt fmt;
> >  	int output_pins;
> > -	u32 src_stream;
> > +	u32 src_stream = 0;
> 
> very minor nit: this can now be moved up
> 
> >  	int ret;
> >
> > -	src_stream = ipu7_isys_get_src_stream_by_src_pad(sd, src_pad->index);
> >  	ret = ipu7_isys_get_stream_pad_fmt(sd, src_pad->index, src_stream,
> >  					   &fmt);
> >  	if (ret < 0) {
> > @@ -781,32 +779,6 @@ ipu7_isys_query_stream_by_source(struct ipu7_isys *isys, int source, u8 vc)
> >  	return stream;
> >  }
> >
> > -static u32 get_remote_pad_stream(struct media_pad *r_pad)
> > -{
> > -	struct v4l2_subdev_state *state;
> > -	struct v4l2_subdev *sd;
> > -	u32 stream_id = 0;
> > -	unsigned int i;
> > -
> > -	sd = media_entity_to_v4l2_subdev(r_pad->entity);
> > -	state = v4l2_subdev_lock_and_get_active_state(sd);
> > -	if (!state)
> > -		return 0;
> > -
> > -	for (i = 0; i < state->stream_configs.num_configs; i++) {
> > -		struct v4l2_subdev_stream_config *cfg =
> > -			&state->stream_configs.configs[i];
> > -		if (cfg->pad == r_pad->index) {
> > -			stream_id = cfg->stream;
> > -			break;
> > -		}
> > -	}
> > -
> > -	v4l2_subdev_unlock_state(state);
> > -
> > -	return stream_id;
> > -}
> > -
> >  int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
> >  				  struct ipu7_isys_buffer_list *bl)
> >  {
> > @@ -814,7 +786,7 @@ int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
> >  	struct device *dev = &av->isys->adev->auxdev.dev;
> >  	struct media_pad *r_pad;
> >  	struct v4l2_subdev *sd;
> > -	u32 r_stream;
> > +	u32 r_stream = 0;
> >  	int ret = 0;
> >
> >  	dev_dbg(dev, "set stream: %d\n", state);
> > @@ -824,7 +796,6 @@ int ipu7_isys_video_set_streaming(struct ipu7_isys_video *av, int state,
> >
> >  	sd = &stream->asd->sd;
> >  	r_pad = media_pad_remote_pad_first(&av->pad);
> > -	r_stream = get_remote_pad_stream(r_pad);
> >  	if (!state) {
> >  		stop_streaming_firmware(av);
> >
> 
> Here and in the other occurencies where s_stream/r_stream gets
> hardcoded to 0, I wonder if using 0 directly instead of going through
> a variable would make things easier to follow.

I considered that, and thought it may be better left for future cleanup
in the driver. Bingbu, what's your preference ?

> Either way:
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

-- 
Regards,

Laurent Pinchart