[PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Tomi Valkeinen]

Currently a subdevice with a single pad, e.g. a sensor subdevice, must
use the v4l2_subdev_video_ops.s_stream op, instead of
v4l2_subdev_pad_ops.enable/disable_streams. This is because the
enable/disable_streams machinery requires a routing table which a subdev
cannot have with a single pad.

Implement enable/disable_streams support for these single-pad subdevices
by assuming an implicit stream 0 when the subdevice has only one pad.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Even though I did send this patch, I'm not sure if this is necessary.
s_stream works fine for the subdevs with a single pad. With the upcoming
internal pads, adding an internal pad to the subdev will create a
routing table, and enable/disable_streams would get "fixed" that way.

So perhaps the question is, do we want to support single-pad subdevs in
the future, in which case something like this patch is necessary, or
will all modern source subdev drivers have internal pads, in which
case this is not needed...
---
 drivers/media/v4l2-core/v4l2-subdev.c | 105 ++++++++++++++++++++++------------
 include/media/v4l2-subdev.h           |   4 +-
 2 files changed, 72 insertions(+), 37 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 4c6198c48dd6..ddc7ed69421c 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2129,21 +2129,33 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 	 * Verify that the requested streams exist and that they are not
 	 * already enabled.
 	 */
-	for (i = 0; i < state->stream_configs.num_configs; ++i) {
-		struct v4l2_subdev_stream_config *cfg =
-			&state->stream_configs.configs[i];
 
-		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
-			continue;
-
-		found_streams |= BIT_ULL(cfg->stream);
-
-		if (cfg->enabled) {
+	if (sd->entity.num_pads == 1) {
+		if (sd->enabled_streams) {
 			dev_dbg(dev, "stream %u already enabled on %s:%u\n",
-				cfg->stream, sd->entity.name, pad);
+				0, sd->entity.name, pad);
 			ret = -EALREADY;
 			goto done;
 		}
+
+		found_streams = BIT_ULL(0);
+	} else {
+		for (i = 0; i < state->stream_configs.num_configs; ++i) {
+			struct v4l2_subdev_stream_config *cfg =
+				&state->stream_configs.configs[i];
+
+			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
+				continue;
+
+			found_streams |= BIT_ULL(cfg->stream);
+
+			if (cfg->enabled) {
+				dev_dbg(dev, "stream %u already enabled on %s:%u\n",
+					cfg->stream, sd->entity.name, pad);
+				ret = -EALREADY;
+				goto done;
+			}
+		}
 	}
 
 	if (found_streams != streams_mask) {
@@ -2164,13 +2176,17 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 		goto done;
 	}
 
-	/* Mark the streams as enabled. */
-	for (i = 0; i < state->stream_configs.num_configs; ++i) {
-		struct v4l2_subdev_stream_config *cfg =
-			&state->stream_configs.configs[i];
+	if (sd->entity.num_pads == 1) {
+		sd->enabled_streams |= streams_mask;
+	} else {
+		/* Mark the streams as enabled. */
+		for (i = 0; i < state->stream_configs.num_configs; ++i) {
+			struct v4l2_subdev_stream_config *cfg =
+				&state->stream_configs.configs[i];
 
-		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
-			cfg->enabled = true;
+			if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
+				cfg->enabled = true;
+		}
 	}
 
 done:
@@ -2246,21 +2262,32 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
 	 * Verify that the requested streams exist and that they are not
 	 * already disabled.
 	 */
-	for (i = 0; i < state->stream_configs.num_configs; ++i) {
-		struct v4l2_subdev_stream_config *cfg =
-			&state->stream_configs.configs[i];
-
-		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
-			continue;
-
-		found_streams |= BIT_ULL(cfg->stream);
-
-		if (!cfg->enabled) {
+	if (sd->entity.num_pads == 1) {
+		if (!sd->enabled_streams) {
 			dev_dbg(dev, "stream %u already disabled on %s:%u\n",
-				cfg->stream, sd->entity.name, pad);
+				0, sd->entity.name, pad);
 			ret = -EALREADY;
 			goto done;
 		}
+
+		found_streams = BIT_ULL(0);
+	} else {
+		for (i = 0; i < state->stream_configs.num_configs; ++i) {
+			struct v4l2_subdev_stream_config *cfg =
+				&state->stream_configs.configs[i];
+
+			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
+				continue;
+
+			found_streams |= BIT_ULL(cfg->stream);
+
+			if (!cfg->enabled) {
+				dev_dbg(dev, "stream %u already disabled on %s:%u\n",
+					cfg->stream, sd->entity.name, pad);
+				ret = -EALREADY;
+				goto done;
+			}
+		}
 	}
 
 	if (found_streams != streams_mask) {
@@ -2281,13 +2308,17 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
 		goto done;
 	}
 
-	/* Mark the streams as disabled. */
-	for (i = 0; i < state->stream_configs.num_configs; ++i) {
-		struct v4l2_subdev_stream_config *cfg =
-			&state->stream_configs.configs[i];
+	if (sd->entity.num_pads == 1) {
+		sd->enabled_streams &= ~streams_mask;
+	} else {
+		/* Mark the streams as disabled. */
+		for (i = 0; i < state->stream_configs.num_configs; ++i) {
+			struct v4l2_subdev_stream_config *cfg =
+				&state->stream_configs.configs[i];
 
-		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
-			cfg->enabled = false;
+			if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
+				cfg->enabled = false;
+		}
 	}
 
 done:
@@ -2325,8 +2356,12 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
 	 */
 	state = v4l2_subdev_lock_and_get_active_state(sd);
 
-	for_each_active_route(&state->routing, route)
-		source_mask |= BIT_ULL(route->source_stream);
+	if (sd->entity.num_pads == 1) {
+		source_mask = BIT_ULL(0);
+	} else {
+		for_each_active_route(&state->routing, route)
+			source_mask |= BIT_ULL(route->source_stream);
+	}
 
 	v4l2_subdev_unlock_state(state);
 
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index a9e6b8146279..39b230f7b3c8 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1041,8 +1041,8 @@ struct v4l2_subdev_platform_data {
  *		  v4l2_subdev_init_finalize().
  * @enabled_streams: Bitmask of enabled streams used by
  *		     v4l2_subdev_enable_streams() and
- *		     v4l2_subdev_disable_streams() helper functions for fallback
- *		     cases.
+ *		     v4l2_subdev_disable_streams() helper functions. This is
+ *		     for fallback cases and for subdevs with single pads.
  *
  * Each instance of a subdev driver should create this struct, either
  * stand-alone or embedded in a larger struct.

---
base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
change-id: 20240325-single-pad-enable-streams-32a9a746ac5b

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Laurent Pinchart]

Hi Tomi,

On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
> Currently a subdevice with a single pad, e.g. a sensor subdevice, must
> use the v4l2_subdev_video_ops.s_stream op, instead of
> v4l2_subdev_pad_ops.enable/disable_streams. This is because the
> enable/disable_streams machinery requires a routing table which a subdev
> cannot have with a single pad.
> 
> Implement enable/disable_streams support for these single-pad subdevices
> by assuming an implicit stream 0 when the subdevice has only one pad.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> Even though I did send this patch, I'm not sure if this is necessary.
> s_stream works fine for the subdevs with a single pad. With the upcoming
> internal pads, adding an internal pad to the subdev will create a
> routing table, and enable/disable_streams would get "fixed" that way.
> 
> So perhaps the question is, do we want to support single-pad subdevs in
> the future, in which case something like this patch is necessary, or
> will all modern source subdev drivers have internal pads, in which
> case this is not needed...

I think the latter would be best. I however can't guarantee we won't
have valid use cases for (enable|disable)_streams on single-pad subdevs
though, so you patch could still be interesting.

> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 105 ++++++++++++++++++++++------------
>  include/media/v4l2-subdev.h           |   4 +-
>  2 files changed, 72 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4c6198c48dd6..ddc7ed69421c 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2129,21 +2129,33 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>  	 * Verify that the requested streams exist and that they are not
>  	 * already enabled.
>  	 */
> -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> -		struct v4l2_subdev_stream_config *cfg =
> -			&state->stream_configs.configs[i];
>  
> -		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> -			continue;
> -
> -		found_streams |= BIT_ULL(cfg->stream);
> -
> -		if (cfg->enabled) {
> +	if (sd->entity.num_pads == 1) {
> +		if (sd->enabled_streams) {
>  			dev_dbg(dev, "stream %u already enabled on %s:%u\n",
> -				cfg->stream, sd->entity.name, pad);
> +				0, sd->entity.name, pad);
>  			ret = -EALREADY;
>  			goto done;
>  		}
> +
> +		found_streams = BIT_ULL(0);
> +	} else {
> +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> +			struct v4l2_subdev_stream_config *cfg =
> +				&state->stream_configs.configs[i];
> +
> +			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> +				continue;
> +
> +			found_streams |= BIT_ULL(cfg->stream);
> +
> +			if (cfg->enabled) {
> +				dev_dbg(dev, "stream %u already enabled on %s:%u\n",
> +					cfg->stream, sd->entity.name, pad);
> +				ret = -EALREADY;
> +				goto done;
> +			}
> +		}
>  	}
>  
>  	if (found_streams != streams_mask) {
> @@ -2164,13 +2176,17 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>  		goto done;
>  	}
>  
> -	/* Mark the streams as enabled. */
> -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> -		struct v4l2_subdev_stream_config *cfg =
> -			&state->stream_configs.configs[i];
> +	if (sd->entity.num_pads == 1) {
> +		sd->enabled_streams |= streams_mask;
> +	} else {
> +		/* Mark the streams as enabled. */
> +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> +			struct v4l2_subdev_stream_config *cfg =
> +				&state->stream_configs.configs[i];
>  
> -		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> -			cfg->enabled = true;
> +			if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> +				cfg->enabled = true;
> +		}
>  	}
>  
>  done:
> @@ -2246,21 +2262,32 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>  	 * Verify that the requested streams exist and that they are not
>  	 * already disabled.
>  	 */
> -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> -		struct v4l2_subdev_stream_config *cfg =
> -			&state->stream_configs.configs[i];
> -
> -		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> -			continue;
> -
> -		found_streams |= BIT_ULL(cfg->stream);
> -
> -		if (!cfg->enabled) {
> +	if (sd->entity.num_pads == 1) {
> +		if (!sd->enabled_streams) {
>  			dev_dbg(dev, "stream %u already disabled on %s:%u\n",
> -				cfg->stream, sd->entity.name, pad);
> +				0, sd->entity.name, pad);
>  			ret = -EALREADY;
>  			goto done;
>  		}
> +
> +		found_streams = BIT_ULL(0);
> +	} else {
> +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> +			struct v4l2_subdev_stream_config *cfg =
> +				&state->stream_configs.configs[i];
> +
> +			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> +				continue;
> +
> +			found_streams |= BIT_ULL(cfg->stream);
> +
> +			if (!cfg->enabled) {
> +				dev_dbg(dev, "stream %u already disabled on %s:%u\n",
> +					cfg->stream, sd->entity.name, pad);
> +				ret = -EALREADY;
> +				goto done;
> +			}
> +		}
>  	}
>  
>  	if (found_streams != streams_mask) {
> @@ -2281,13 +2308,17 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>  		goto done;
>  	}
>  
> -	/* Mark the streams as disabled. */
> -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> -		struct v4l2_subdev_stream_config *cfg =
> -			&state->stream_configs.configs[i];
> +	if (sd->entity.num_pads == 1) {
> +		sd->enabled_streams &= ~streams_mask;
> +	} else {
> +		/* Mark the streams as disabled. */
> +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> +			struct v4l2_subdev_stream_config *cfg =
> +				&state->stream_configs.configs[i];
>  
> -		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> -			cfg->enabled = false;
> +			if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> +				cfg->enabled = false;
> +		}
>  	}
>  
>  done:
> @@ -2325,8 +2356,12 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
>  	 */
>  	state = v4l2_subdev_lock_and_get_active_state(sd);
>  
> -	for_each_active_route(&state->routing, route)
> -		source_mask |= BIT_ULL(route->source_stream);
> +	if (sd->entity.num_pads == 1) {
> +		source_mask = BIT_ULL(0);
> +	} else {
> +		for_each_active_route(&state->routing, route)
> +			source_mask |= BIT_ULL(route->source_stream);
> +	}
>  
>  	v4l2_subdev_unlock_state(state);
>  
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a9e6b8146279..39b230f7b3c8 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1041,8 +1041,8 @@ struct v4l2_subdev_platform_data {
>   *		  v4l2_subdev_init_finalize().
>   * @enabled_streams: Bitmask of enabled streams used by
>   *		     v4l2_subdev_enable_streams() and
> - *		     v4l2_subdev_disable_streams() helper functions for fallback
> - *		     cases.
> + *		     v4l2_subdev_disable_streams() helper functions. This is
> + *		     for fallback cases and for subdevs with single pads.
>   *
>   * Each instance of a subdev driver should create this struct, either
>   * stand-alone or embedded in a larger struct.
> 
> ---
> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> change-id: 20240325-single-pad-enable-streams-32a9a746ac5b

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Sakari Ailus]

Moi,

Thanks for the patch.

On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
> > Currently a subdevice with a single pad, e.g. a sensor subdevice, must
> > use the v4l2_subdev_video_ops.s_stream op, instead of
> > v4l2_subdev_pad_ops.enable/disable_streams. This is because the
> > enable/disable_streams machinery requires a routing table which a subdev
> > cannot have with a single pad.
> > 
> > Implement enable/disable_streams support for these single-pad subdevices
> > by assuming an implicit stream 0 when the subdevice has only one pad.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> > Even though I did send this patch, I'm not sure if this is necessary.
> > s_stream works fine for the subdevs with a single pad. With the upcoming
> > internal pads, adding an internal pad to the subdev will create a
> > routing table, and enable/disable_streams would get "fixed" that way.

I'd like to get rid of a redundant way to control streaming.

> > 
> > So perhaps the question is, do we want to support single-pad subdevs in
> > the future, in which case something like this patch is necessary, or
> > will all modern source subdev drivers have internal pads, in which
> > case this is not needed...
> 
> I think the latter would be best. I however can't guarantee we won't
> have valid use cases for (enable|disable)_streams on single-pad subdevs
> though, so you patch could still be interesting.

Instead of the number of pads, could we use instead the
V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
determine the need for this?

> 
> > ---
> >  drivers/media/v4l2-core/v4l2-subdev.c | 105 ++++++++++++++++++++++------------
> >  include/media/v4l2-subdev.h           |   4 +-
> >  2 files changed, 72 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 4c6198c48dd6..ddc7ed69421c 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -2129,21 +2129,33 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> >  	 * Verify that the requested streams exist and that they are not
> >  	 * already enabled.
> >  	 */
> > -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > -		struct v4l2_subdev_stream_config *cfg =
> > -			&state->stream_configs.configs[i];
> >  
> > -		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> > -			continue;
> > -
> > -		found_streams |= BIT_ULL(cfg->stream);
> > -
> > -		if (cfg->enabled) {
> > +	if (sd->entity.num_pads == 1) {
> > +		if (sd->enabled_streams) {
> >  			dev_dbg(dev, "stream %u already enabled on %s:%u\n",
> > -				cfg->stream, sd->entity.name, pad);
> > +				0, sd->entity.name, pad);
> >  			ret = -EALREADY;
> >  			goto done;
> >  		}
> > +
> > +		found_streams = BIT_ULL(0);
> > +	} else {
> > +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > +			struct v4l2_subdev_stream_config *cfg =
> > +				&state->stream_configs.configs[i];
> > +
> > +			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> > +				continue;
> > +
> > +			found_streams |= BIT_ULL(cfg->stream);
> > +
> > +			if (cfg->enabled) {
> > +				dev_dbg(dev, "stream %u already enabled on %s:%u\n",
> > +					cfg->stream, sd->entity.name, pad);
> > +				ret = -EALREADY;
> > +				goto done;
> > +			}
> > +		}
> >  	}
> >  
> >  	if (found_streams != streams_mask) {
> > @@ -2164,13 +2176,17 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> >  		goto done;
> >  	}
> >  
> > -	/* Mark the streams as enabled. */
> > -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > -		struct v4l2_subdev_stream_config *cfg =
> > -			&state->stream_configs.configs[i];
> > +	if (sd->entity.num_pads == 1) {
> > +		sd->enabled_streams |= streams_mask;
> > +	} else {
> > +		/* Mark the streams as enabled. */
> > +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > +			struct v4l2_subdev_stream_config *cfg =
> > +				&state->stream_configs.configs[i];
> >  
> > -		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> > -			cfg->enabled = true;
> > +			if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> > +				cfg->enabled = true;
> > +		}
> >  	}
> >  
> >  done:
> > @@ -2246,21 +2262,32 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
> >  	 * Verify that the requested streams exist and that they are not
> >  	 * already disabled.
> >  	 */
> > -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > -		struct v4l2_subdev_stream_config *cfg =
> > -			&state->stream_configs.configs[i];
> > -
> > -		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> > -			continue;
> > -
> > -		found_streams |= BIT_ULL(cfg->stream);
> > -
> > -		if (!cfg->enabled) {
> > +	if (sd->entity.num_pads == 1) {
> > +		if (!sd->enabled_streams) {
> >  			dev_dbg(dev, "stream %u already disabled on %s:%u\n",
> > -				cfg->stream, sd->entity.name, pad);
> > +				0, sd->entity.name, pad);
> >  			ret = -EALREADY;
> >  			goto done;
> >  		}
> > +
> > +		found_streams = BIT_ULL(0);
> > +	} else {
> > +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > +			struct v4l2_subdev_stream_config *cfg =
> > +				&state->stream_configs.configs[i];
> > +
> > +			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> > +				continue;
> > +
> > +			found_streams |= BIT_ULL(cfg->stream);
> > +
> > +			if (!cfg->enabled) {
> > +				dev_dbg(dev, "stream %u already disabled on %s:%u\n",
> > +					cfg->stream, sd->entity.name, pad);
> > +				ret = -EALREADY;
> > +				goto done;
> > +			}
> > +		}
> >  	}
> >  
> >  	if (found_streams != streams_mask) {
> > @@ -2281,13 +2308,17 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
> >  		goto done;
> >  	}
> >  
> > -	/* Mark the streams as disabled. */
> > -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > -		struct v4l2_subdev_stream_config *cfg =
> > -			&state->stream_configs.configs[i];
> > +	if (sd->entity.num_pads == 1) {
> > +		sd->enabled_streams &= ~streams_mask;
> > +	} else {
> > +		/* Mark the streams as disabled. */
> > +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > +			struct v4l2_subdev_stream_config *cfg =
> > +				&state->stream_configs.configs[i];
> >  
> > -		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> > -			cfg->enabled = false;
> > +			if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> > +				cfg->enabled = false;
> > +		}
> >  	}
> >  
> >  done:
> > @@ -2325,8 +2356,12 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
> >  	 */
> >  	state = v4l2_subdev_lock_and_get_active_state(sd);
> >  
> > -	for_each_active_route(&state->routing, route)
> > -		source_mask |= BIT_ULL(route->source_stream);
> > +	if (sd->entity.num_pads == 1) {
> > +		source_mask = BIT_ULL(0);
> > +	} else {
> > +		for_each_active_route(&state->routing, route)
> > +			source_mask |= BIT_ULL(route->source_stream);
> > +	}
> >  
> >  	v4l2_subdev_unlock_state(state);
> >  
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index a9e6b8146279..39b230f7b3c8 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -1041,8 +1041,8 @@ struct v4l2_subdev_platform_data {
> >   *		  v4l2_subdev_init_finalize().
> >   * @enabled_streams: Bitmask of enabled streams used by
> >   *		     v4l2_subdev_enable_streams() and
> > - *		     v4l2_subdev_disable_streams() helper functions for fallback
> > - *		     cases.
> > + *		     v4l2_subdev_disable_streams() helper functions. This is
> > + *		     for fallback cases and for subdevs with single pads.
> >   *
> >   * Each instance of a subdev driver should create this struct, either
> >   * stand-alone or embedded in a larger struct.
> > 
> > ---
> > base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> > change-id: 20240325-single-pad-enable-streams-32a9a746ac5b
> 

-- 
Terveisin,

Sakari Ailus

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Tomi Valkeinen]

On 25/03/2024 15:02, Sakari Ailus wrote:
> Moi,
> 
> Thanks for the patch.
> 
> On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
>> Hi Tomi,
>>
>> On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
>>> Currently a subdevice with a single pad, e.g. a sensor subdevice, must
>>> use the v4l2_subdev_video_ops.s_stream op, instead of
>>> v4l2_subdev_pad_ops.enable/disable_streams. This is because the
>>> enable/disable_streams machinery requires a routing table which a subdev
>>> cannot have with a single pad.
>>>
>>> Implement enable/disable_streams support for these single-pad subdevices
>>> by assuming an implicit stream 0 when the subdevice has only one pad.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> ---
>>> Even though I did send this patch, I'm not sure if this is necessary.
>>> s_stream works fine for the subdevs with a single pad. With the upcoming
>>> internal pads, adding an internal pad to the subdev will create a
>>> routing table, and enable/disable_streams would get "fixed" that way.
> 
> I'd like to get rid of a redundant way to control streaming.

We can't get rid of it anyway, can we? We're not going to convert old 
drivers to streams.

For new drivers, yes, we shouldn't use s_stream. But is the answer for 
new sensor drivers this patch, or requiring an internal pad?

>>> So perhaps the question is, do we want to support single-pad subdevs in
>>> the future, in which case something like this patch is necessary, or
>>> will all modern source subdev drivers have internal pads, in which
>>> case this is not needed...
>>
>> I think the latter would be best. I however can't guarantee we won't
>> have valid use cases for (enable|disable)_streams on single-pad subdevs
>> though, so you patch could still be interesting.
> 
> Instead of the number of pads, could we use instead the
> V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
> determine the need for this?

Maybe, but are they better? Do you see some issue with checking for the 
number of pads? I considered a few options, but then thought that the 
most safest test for this case is 1) one pad 2) enable/disable_streams 
implemented.

  Tomi

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Sakari Ailus]

Moi,

On Mon, Mar 25, 2024 at 03:43:01PM +0200, Tomi Valkeinen wrote:
> On 25/03/2024 15:02, Sakari Ailus wrote:
> > Moi,
> > 
> > Thanks for the patch.
> > 
> > On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
> > > Hi Tomi,
> > > 
> > > On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
> > > > Currently a subdevice with a single pad, e.g. a sensor subdevice, must
> > > > use the v4l2_subdev_video_ops.s_stream op, instead of
> > > > v4l2_subdev_pad_ops.enable/disable_streams. This is because the
> > > > enable/disable_streams machinery requires a routing table which a subdev
> > > > cannot have with a single pad.
> > > > 
> > > > Implement enable/disable_streams support for these single-pad subdevices
> > > > by assuming an implicit stream 0 when the subdevice has only one pad.
> > > > 
> > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > > ---
> > > > Even though I did send this patch, I'm not sure if this is necessary.
> > > > s_stream works fine for the subdevs with a single pad. With the upcoming
> > > > internal pads, adding an internal pad to the subdev will create a
> > > > routing table, and enable/disable_streams would get "fixed" that way.
> > 
> > I'd like to get rid of a redundant way to control streaming.
> 
> We can't get rid of it anyway, can we? We're not going to convert old
> drivers to streams.

I'd expect to do that but it'd take a long time. That being said, I think
we need to consider devices without pads (VCMs) so it may well be this
would remain after all.

> 
> For new drivers, yes, we shouldn't use s_stream. But is the answer for new
> sensor drivers this patch, or requiring an internal pad?

For new drivers I'd like to see an internal pad in fact.
{enable,disable}_streams is still internal to the kernel.

> 
> > > > So perhaps the question is, do we want to support single-pad subdevs in
> > > > the future, in which case something like this patch is necessary, or
> > > > will all modern source subdev drivers have internal pads, in which
> > > > case this is not needed...
> > > 
> > > I think the latter would be best. I however can't guarantee we won't
> > > have valid use cases for (enable|disable)_streams on single-pad subdevs
> > > though, so you patch could still be interesting.
> > 
> > Instead of the number of pads, could we use instead the
> > V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
> > determine the need for this?
> 
> Maybe, but are they better? Do you see some issue with checking for the
> number of pads? I considered a few options, but then thought that the most
> safest test for this case is 1) one pad 2) enable/disable_streams
> implemented.

I think I'd actually prefer {enable,disable}_streams in fact.

-- 
Terveisin,

Sakari Ailus

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Tomi Valkeinen]

On 25/03/2024 19:52, Sakari Ailus wrote:
> Moi,
> 
> On Mon, Mar 25, 2024 at 03:43:01PM +0200, Tomi Valkeinen wrote:
>> On 25/03/2024 15:02, Sakari Ailus wrote:
>>> Moi,
>>>
>>> Thanks for the patch.
>>>
>>> On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
>>>> Hi Tomi,
>>>>
>>>> On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
>>>>> Currently a subdevice with a single pad, e.g. a sensor subdevice, must
>>>>> use the v4l2_subdev_video_ops.s_stream op, instead of
>>>>> v4l2_subdev_pad_ops.enable/disable_streams. This is because the
>>>>> enable/disable_streams machinery requires a routing table which a subdev
>>>>> cannot have with a single pad.
>>>>>
>>>>> Implement enable/disable_streams support for these single-pad subdevices
>>>>> by assuming an implicit stream 0 when the subdevice has only one pad.
>>>>>
>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>> ---
>>>>> Even though I did send this patch, I'm not sure if this is necessary.
>>>>> s_stream works fine for the subdevs with a single pad. With the upcoming
>>>>> internal pads, adding an internal pad to the subdev will create a
>>>>> routing table, and enable/disable_streams would get "fixed" that way.
>>>
>>> I'd like to get rid of a redundant way to control streaming.
>>
>> We can't get rid of it anyway, can we? We're not going to convert old
>> drivers to streams.
> 
> I'd expect to do that but it'd take a long time. That being said, I think
> we need to consider devices without pads (VCMs) so it may well be this
> would remain after all.
> 
>>
>> For new drivers, yes, we shouldn't use s_stream. But is the answer for new
>> sensor drivers this patch, or requiring an internal pad?
> 
> For new drivers I'd like to see an internal pad in fact.
> {enable,disable}_streams is still internal to the kernel.

So, you think this patch should be dropped?

>>>>> So perhaps the question is, do we want to support single-pad subdevs in
>>>>> the future, in which case something like this patch is necessary, or
>>>>> will all modern source subdev drivers have internal pads, in which
>>>>> case this is not needed...
>>>>
>>>> I think the latter would be best. I however can't guarantee we won't
>>>> have valid use cases for (enable|disable)_streams on single-pad subdevs
>>>> though, so you patch could still be interesting.
>>>
>>> Instead of the number of pads, could we use instead the
>>> V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
>>> determine the need for this?
>>
>> Maybe, but are they better? Do you see some issue with checking for the
>> number of pads? I considered a few options, but then thought that the most
>> safest test for this case is 1) one pad 2) enable/disable_streams
>> implemented.
> 
> I think I'd actually prefer {enable,disable}_streams in fact.

Hmm, sorry, now I'm confused =). What do you mean with that?

  Tomi

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Umang Jain]

Hi Tomi,

On 25/03/24 6:13 pm, Tomi Valkeinen wrote:
> Currently a subdevice with a single pad, e.g. a sensor subdevice, must
> use the v4l2_subdev_video_ops.s_stream op, instead of
> v4l2_subdev_pad_ops.enable/disable_streams. This is because the
> enable/disable_streams machinery requires a routing table which a subdev
> cannot have with a single pad.
>
> Implement enable/disable_streams support for these single-pad subdevices
> by assuming an implicit stream 0 when the subdevice has only one pad.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

fwiw,

Tested-by: Umang Jain <umang.jain@ideasonboard.com>

with [1]

[1]: 
https://lore.kernel.org/linux-media/4bb01eb0-bf53-43f2-a488-7959aadacc3b@ideasonboard.com/
> ---
> Even though I did send this patch, I'm not sure if this is necessary.
> s_stream works fine for the subdevs with a single pad. With the upcoming
> internal pads, adding an internal pad to the subdev will create a
> routing table, and enable/disable_streams would get "fixed" that way.
>
> So perhaps the question is, do we want to support single-pad subdevs in
> the future, in which case something like this patch is necessary, or
> will all modern source subdev drivers have internal pads, in which
> case this is not needed...
> ---
>   drivers/media/v4l2-core/v4l2-subdev.c | 105 ++++++++++++++++++++++------------
>   include/media/v4l2-subdev.h           |   4 +-
>   2 files changed, 72 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4c6198c48dd6..ddc7ed69421c 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2129,21 +2129,33 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>   	 * Verify that the requested streams exist and that they are not
>   	 * already enabled.
>   	 */
> -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> -		struct v4l2_subdev_stream_config *cfg =
> -			&state->stream_configs.configs[i];
>   
> -		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> -			continue;
> -
> -		found_streams |= BIT_ULL(cfg->stream);
> -
> -		if (cfg->enabled) {
> +	if (sd->entity.num_pads == 1) {
> +		if (sd->enabled_streams) {
>   			dev_dbg(dev, "stream %u already enabled on %s:%u\n",
> -				cfg->stream, sd->entity.name, pad);
> +				0, sd->entity.name, pad);
>   			ret = -EALREADY;
>   			goto done;
>   		}
> +
> +		found_streams = BIT_ULL(0);
> +	} else {
> +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> +			struct v4l2_subdev_stream_config *cfg =
> +				&state->stream_configs.configs[i];
> +
> +			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> +				continue;
> +
> +			found_streams |= BIT_ULL(cfg->stream);
> +
> +			if (cfg->enabled) {
> +				dev_dbg(dev, "stream %u already enabled on %s:%u\n",
> +					cfg->stream, sd->entity.name, pad);
> +				ret = -EALREADY;
> +				goto done;
> +			}
> +		}
>   	}
>   
>   	if (found_streams != streams_mask) {
> @@ -2164,13 +2176,17 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>   		goto done;
>   	}
>   
> -	/* Mark the streams as enabled. */
> -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> -		struct v4l2_subdev_stream_config *cfg =
> -			&state->stream_configs.configs[i];
> +	if (sd->entity.num_pads == 1) {
> +		sd->enabled_streams |= streams_mask;
> +	} else {
> +		/* Mark the streams as enabled. */
> +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> +			struct v4l2_subdev_stream_config *cfg =
> +				&state->stream_configs.configs[i];
>   
> -		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> -			cfg->enabled = true;
> +			if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> +				cfg->enabled = true;
> +		}
>   	}
>   
>   done:
> @@ -2246,21 +2262,32 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>   	 * Verify that the requested streams exist and that they are not
>   	 * already disabled.
>   	 */
> -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> -		struct v4l2_subdev_stream_config *cfg =
> -			&state->stream_configs.configs[i];
> -
> -		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> -			continue;
> -
> -		found_streams |= BIT_ULL(cfg->stream);
> -
> -		if (!cfg->enabled) {
> +	if (sd->entity.num_pads == 1) {
> +		if (!sd->enabled_streams) {
>   			dev_dbg(dev, "stream %u already disabled on %s:%u\n",
> -				cfg->stream, sd->entity.name, pad);
> +				0, sd->entity.name, pad);
>   			ret = -EALREADY;
>   			goto done;
>   		}
> +
> +		found_streams = BIT_ULL(0);
> +	} else {
> +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> +			struct v4l2_subdev_stream_config *cfg =
> +				&state->stream_configs.configs[i];
> +
> +			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> +				continue;
> +
> +			found_streams |= BIT_ULL(cfg->stream);
> +
> +			if (!cfg->enabled) {
> +				dev_dbg(dev, "stream %u already disabled on %s:%u\n",
> +					cfg->stream, sd->entity.name, pad);
> +				ret = -EALREADY;
> +				goto done;
> +			}
> +		}
>   	}
>   
>   	if (found_streams != streams_mask) {
> @@ -2281,13 +2308,17 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>   		goto done;
>   	}
>   
> -	/* Mark the streams as disabled. */
> -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> -		struct v4l2_subdev_stream_config *cfg =
> -			&state->stream_configs.configs[i];
> +	if (sd->entity.num_pads == 1) {
> +		sd->enabled_streams &= ~streams_mask;
> +	} else {
> +		/* Mark the streams as disabled. */
> +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> +			struct v4l2_subdev_stream_config *cfg =
> +				&state->stream_configs.configs[i];
>   
> -		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> -			cfg->enabled = false;
> +			if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> +				cfg->enabled = false;
> +		}
>   	}
>   
>   done:
> @@ -2325,8 +2356,12 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
>   	 */
>   	state = v4l2_subdev_lock_and_get_active_state(sd);
>   
> -	for_each_active_route(&state->routing, route)
> -		source_mask |= BIT_ULL(route->source_stream);
> +	if (sd->entity.num_pads == 1) {
> +		source_mask = BIT_ULL(0);
> +	} else {
> +		for_each_active_route(&state->routing, route)
> +			source_mask |= BIT_ULL(route->source_stream);
> +	}
>   
>   	v4l2_subdev_unlock_state(state);
>   
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a9e6b8146279..39b230f7b3c8 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1041,8 +1041,8 @@ struct v4l2_subdev_platform_data {
>    *		  v4l2_subdev_init_finalize().
>    * @enabled_streams: Bitmask of enabled streams used by
>    *		     v4l2_subdev_enable_streams() and
> - *		     v4l2_subdev_disable_streams() helper functions for fallback
> - *		     cases.
> + *		     v4l2_subdev_disable_streams() helper functions. This is
> + *		     for fallback cases and for subdevs with single pads.
>    *
>    * Each instance of a subdev driver should create this struct, either
>    * stand-alone or embedded in a larger struct.
>
> ---
> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> change-id: 20240325-single-pad-enable-streams-32a9a746ac5b
>
> Best regards,

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Sakari Ailus]

Heippa,

On Mon, Mar 25, 2024 at 07:56:46PM +0200, Tomi Valkeinen wrote:
> On 25/03/2024 19:52, Sakari Ailus wrote:
> > Moi,
> > 
> > On Mon, Mar 25, 2024 at 03:43:01PM +0200, Tomi Valkeinen wrote:
> > > On 25/03/2024 15:02, Sakari Ailus wrote:
> > > > Moi,
> > > > 
> > > > Thanks for the patch.
> > > > 
> > > > On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
> > > > > Hi Tomi,
> > > > > 
> > > > > On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
> > > > > > Currently a subdevice with a single pad, e.g. a sensor subdevice, must
> > > > > > use the v4l2_subdev_video_ops.s_stream op, instead of
> > > > > > v4l2_subdev_pad_ops.enable/disable_streams. This is because the
> > > > > > enable/disable_streams machinery requires a routing table which a subdev
> > > > > > cannot have with a single pad.
> > > > > > 
> > > > > > Implement enable/disable_streams support for these single-pad subdevices
> > > > > > by assuming an implicit stream 0 when the subdevice has only one pad.
> > > > > > 
> > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > > > > ---
> > > > > > Even though I did send this patch, I'm not sure if this is necessary.
> > > > > > s_stream works fine for the subdevs with a single pad. With the upcoming
> > > > > > internal pads, adding an internal pad to the subdev will create a
> > > > > > routing table, and enable/disable_streams would get "fixed" that way.
> > > > 
> > > > I'd like to get rid of a redundant way to control streaming.
> > > 
> > > We can't get rid of it anyway, can we? We're not going to convert old
> > > drivers to streams.
> > 
> > I'd expect to do that but it'd take a long time. That being said, I think
> > we need to consider devices without pads (VCMs) so it may well be this
> > would remain after all.
> > 
> > > 
> > > For new drivers, yes, we shouldn't use s_stream. But is the answer for new
> > > sensor drivers this patch, or requiring an internal pad?
> > 
> > For new drivers I'd like to see an internal pad in fact.
> > {enable,disable}_streams is still internal to the kernel.
> 
> So, you think this patch should be dropped?

No, no. Not all sub-device drivers with pads are camera sensor drivers. :-)

> 
> > > > > > So perhaps the question is, do we want to support single-pad subdevs in
> > > > > > the future, in which case something like this patch is necessary, or
> > > > > > will all modern source subdev drivers have internal pads, in which
> > > > > > case this is not needed...
> > > > > 
> > > > > I think the latter would be best. I however can't guarantee we won't
> > > > > have valid use cases for (enable|disable)_streams on single-pad subdevs
> > > > > though, so you patch could still be interesting.
> > > > 
> > > > Instead of the number of pads, could we use instead the
> > > > V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
> > > > determine the need for this?
> > > 
> > > Maybe, but are they better? Do you see some issue with checking for the
> > > number of pads? I considered a few options, but then thought that the most
> > > safest test for this case is 1) one pad 2) enable/disable_streams
> > > implemented.
> > 
> > I think I'd actually prefer {enable,disable}_streams in fact.
> 
> Hmm, sorry, now I'm confused =). What do you mean with that?

I'd use V4L2_SUBDEV_FL_STREAMS flag instead of the number of pads. The
number of pads is less related to routing.

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Tomi Valkeinen]

On 27/03/2024 12:46, Sakari Ailus wrote:
> Heippa,
> 
> On Mon, Mar 25, 2024 at 07:56:46PM +0200, Tomi Valkeinen wrote:
>> On 25/03/2024 19:52, Sakari Ailus wrote:
>>> Moi,
>>>
>>> On Mon, Mar 25, 2024 at 03:43:01PM +0200, Tomi Valkeinen wrote:
>>>> On 25/03/2024 15:02, Sakari Ailus wrote:
>>>>> Moi,
>>>>>
>>>>> Thanks for the patch.
>>>>>
>>>>> On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
>>>>>> Hi Tomi,
>>>>>>
>>>>>> On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
>>>>>>> Currently a subdevice with a single pad, e.g. a sensor subdevice, must
>>>>>>> use the v4l2_subdev_video_ops.s_stream op, instead of
>>>>>>> v4l2_subdev_pad_ops.enable/disable_streams. This is because the
>>>>>>> enable/disable_streams machinery requires a routing table which a subdev
>>>>>>> cannot have with a single pad.
>>>>>>>
>>>>>>> Implement enable/disable_streams support for these single-pad subdevices
>>>>>>> by assuming an implicit stream 0 when the subdevice has only one pad.
>>>>>>>
>>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>>> ---
>>>>>>> Even though I did send this patch, I'm not sure if this is necessary.
>>>>>>> s_stream works fine for the subdevs with a single pad. With the upcoming
>>>>>>> internal pads, adding an internal pad to the subdev will create a
>>>>>>> routing table, and enable/disable_streams would get "fixed" that way.
>>>>>
>>>>> I'd like to get rid of a redundant way to control streaming.
>>>>
>>>> We can't get rid of it anyway, can we? We're not going to convert old
>>>> drivers to streams.
>>>
>>> I'd expect to do that but it'd take a long time. That being said, I think
>>> we need to consider devices without pads (VCMs) so it may well be this
>>> would remain after all.
>>>
>>>>
>>>> For new drivers, yes, we shouldn't use s_stream. But is the answer for new
>>>> sensor drivers this patch, or requiring an internal pad?
>>>
>>> For new drivers I'd like to see an internal pad in fact.
>>> {enable,disable}_streams is still internal to the kernel.
>>
>> So, you think this patch should be dropped?
> 
> No, no. Not all sub-device drivers with pads are camera sensor drivers. :-)

Hmm, alright. So we want to support enable/disable_streams for 
sub-devices with multiple source pads but no routing (so probably no 
sink pads)?

>>>>>>> So perhaps the question is, do we want to support single-pad subdevs in
>>>>>>> the future, in which case something like this patch is necessary, or
>>>>>>> will all modern source subdev drivers have internal pads, in which
>>>>>>> case this is not needed...
>>>>>>
>>>>>> I think the latter would be best. I however can't guarantee we won't
>>>>>> have valid use cases for (enable|disable)_streams on single-pad subdevs
>>>>>> though, so you patch could still be interesting.
>>>>>
>>>>> Instead of the number of pads, could we use instead the
>>>>> V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
>>>>> determine the need for this?
>>>>
>>>> Maybe, but are they better? Do you see some issue with checking for the
>>>> number of pads? I considered a few options, but then thought that the most
>>>> safest test for this case is 1) one pad 2) enable/disable_streams
>>>> implemented.
>>>
>>> I think I'd actually prefer {enable,disable}_streams in fact.
>>
>> Hmm, sorry, now I'm confused =). What do you mean with that?
> 
> I'd use V4L2_SUBDEV_FL_STREAMS flag instead of the number of pads. The
> number of pads is less related to routing.

Well, with one pad you cannot have routing =).

In this patch I used sd->enabled_streams to track the enabled streams, 
but if we need to support multiple pads, I'll have to invent something 
new for that.

  Tomi

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Sakari Ailus]

Heissulivei,

On Wed, Mar 27, 2024 at 01:06:42PM +0200, Tomi Valkeinen wrote:
> On 27/03/2024 12:46, Sakari Ailus wrote:
> > Heippa,
> > 
> > On Mon, Mar 25, 2024 at 07:56:46PM +0200, Tomi Valkeinen wrote:
> > > On 25/03/2024 19:52, Sakari Ailus wrote:
> > > > Moi,
> > > > 
> > > > On Mon, Mar 25, 2024 at 03:43:01PM +0200, Tomi Valkeinen wrote:
> > > > > On 25/03/2024 15:02, Sakari Ailus wrote:
> > > > > > Moi,
> > > > > > 
> > > > > > Thanks for the patch.
> > > > > > 
> > > > > > On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
> > > > > > > Hi Tomi,
> > > > > > > 
> > > > > > > On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
> > > > > > > > Currently a subdevice with a single pad, e.g. a sensor subdevice, must
> > > > > > > > use the v4l2_subdev_video_ops.s_stream op, instead of
> > > > > > > > v4l2_subdev_pad_ops.enable/disable_streams. This is because the
> > > > > > > > enable/disable_streams machinery requires a routing table which a subdev
> > > > > > > > cannot have with a single pad.
> > > > > > > > 
> > > > > > > > Implement enable/disable_streams support for these single-pad subdevices
> > > > > > > > by assuming an implicit stream 0 when the subdevice has only one pad.
> > > > > > > > 
> > > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > > > > > > ---
> > > > > > > > Even though I did send this patch, I'm not sure if this is necessary.
> > > > > > > > s_stream works fine for the subdevs with a single pad. With the upcoming
> > > > > > > > internal pads, adding an internal pad to the subdev will create a
> > > > > > > > routing table, and enable/disable_streams would get "fixed" that way.
> > > > > > 
> > > > > > I'd like to get rid of a redundant way to control streaming.
> > > > > 
> > > > > We can't get rid of it anyway, can we? We're not going to convert old
> > > > > drivers to streams.
> > > > 
> > > > I'd expect to do that but it'd take a long time. That being said, I think
> > > > we need to consider devices without pads (VCMs) so it may well be this
> > > > would remain after all.
> > > > 
> > > > > 
> > > > > For new drivers, yes, we shouldn't use s_stream. But is the answer for new
> > > > > sensor drivers this patch, or requiring an internal pad?
> > > > 
> > > > For new drivers I'd like to see an internal pad in fact.
> > > > {enable,disable}_streams is still internal to the kernel.
> > > 
> > > So, you think this patch should be dropped?
> > 
> > No, no. Not all sub-device drivers with pads are camera sensor drivers. :-)
> 
> Hmm, alright. So we want to support enable/disable_streams for sub-devices
> with multiple source pads but no routing (so probably no sink pads)?

That should be allowed indeed, in order to move from s_stream() to
{enable,disable}_streams().

> 
> > > > > > > > So perhaps the question is, do we want to support single-pad subdevs in
> > > > > > > > the future, in which case something like this patch is necessary, or
> > > > > > > > will all modern source subdev drivers have internal pads, in which
> > > > > > > > case this is not needed...
> > > > > > > 
> > > > > > > I think the latter would be best. I however can't guarantee we won't
> > > > > > > have valid use cases for (enable|disable)_streams on single-pad subdevs
> > > > > > > though, so you patch could still be interesting.
> > > > > > 
> > > > > > Instead of the number of pads, could we use instead the
> > > > > > V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
> > > > > > determine the need for this?
> > > > > 
> > > > > Maybe, but are they better? Do you see some issue with checking for the
> > > > > number of pads? I considered a few options, but then thought that the most
> > > > > safest test for this case is 1) one pad 2) enable/disable_streams
> > > > > implemented.
> > > > 
> > > > I think I'd actually prefer {enable,disable}_streams in fact.
> > > 
> > > Hmm, sorry, now I'm confused =). What do you mean with that?
> > 
> > I'd use V4L2_SUBDEV_FL_STREAMS flag instead of the number of pads. The
> > number of pads is less related to routing.
> 
> Well, with one pad you cannot have routing =).
> 
> In this patch I used sd->enabled_streams to track the enabled streams, but
> if we need to support multiple pads, I'll have to invent something new for
> that.

What exactly do you think needs to be changed? This is just about starting
and stopping streaming using a different sent of callbacks, right?

-- 
Terveisin,

Sakari Ailus

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Tomi Valkeinen]

On 27/03/2024 15:32, Sakari Ailus wrote:
> Heissulivei,
> 
> On Wed, Mar 27, 2024 at 01:06:42PM +0200, Tomi Valkeinen wrote:
>> On 27/03/2024 12:46, Sakari Ailus wrote:
>>> Heippa,
>>>
>>> On Mon, Mar 25, 2024 at 07:56:46PM +0200, Tomi Valkeinen wrote:
>>>> On 25/03/2024 19:52, Sakari Ailus wrote:
>>>>> Moi,
>>>>>
>>>>> On Mon, Mar 25, 2024 at 03:43:01PM +0200, Tomi Valkeinen wrote:
>>>>>> On 25/03/2024 15:02, Sakari Ailus wrote:
>>>>>>> Moi,
>>>>>>>
>>>>>>> Thanks for the patch.
>>>>>>>
>>>>>>> On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
>>>>>>>> Hi Tomi,
>>>>>>>>
>>>>>>>> On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
>>>>>>>>> Currently a subdevice with a single pad, e.g. a sensor subdevice, must
>>>>>>>>> use the v4l2_subdev_video_ops.s_stream op, instead of
>>>>>>>>> v4l2_subdev_pad_ops.enable/disable_streams. This is because the
>>>>>>>>> enable/disable_streams machinery requires a routing table which a subdev
>>>>>>>>> cannot have with a single pad.
>>>>>>>>>
>>>>>>>>> Implement enable/disable_streams support for these single-pad subdevices
>>>>>>>>> by assuming an implicit stream 0 when the subdevice has only one pad.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>>>>> ---
>>>>>>>>> Even though I did send this patch, I'm not sure if this is necessary.
>>>>>>>>> s_stream works fine for the subdevs with a single pad. With the upcoming
>>>>>>>>> internal pads, adding an internal pad to the subdev will create a
>>>>>>>>> routing table, and enable/disable_streams would get "fixed" that way.
>>>>>>>
>>>>>>> I'd like to get rid of a redundant way to control streaming.
>>>>>>
>>>>>> We can't get rid of it anyway, can we? We're not going to convert old
>>>>>> drivers to streams.
>>>>>
>>>>> I'd expect to do that but it'd take a long time. That being said, I think
>>>>> we need to consider devices without pads (VCMs) so it may well be this
>>>>> would remain after all.
>>>>>
>>>>>>
>>>>>> For new drivers, yes, we shouldn't use s_stream. But is the answer for new
>>>>>> sensor drivers this patch, or requiring an internal pad?
>>>>>
>>>>> For new drivers I'd like to see an internal pad in fact.
>>>>> {enable,disable}_streams is still internal to the kernel.
>>>>
>>>> So, you think this patch should be dropped?
>>>
>>> No, no. Not all sub-device drivers with pads are camera sensor drivers. :-)
>>
>> Hmm, alright. So we want to support enable/disable_streams for sub-devices
>> with multiple source pads but no routing (so probably no sink pads)?
> 
> That should be allowed indeed, in order to move from s_stream() to
> {enable,disable}_streams().
> 
>>
>>>>>>>>> So perhaps the question is, do we want to support single-pad subdevs in
>>>>>>>>> the future, in which case something like this patch is necessary, or
>>>>>>>>> will all modern source subdev drivers have internal pads, in which
>>>>>>>>> case this is not needed...
>>>>>>>>
>>>>>>>> I think the latter would be best. I however can't guarantee we won't
>>>>>>>> have valid use cases for (enable|disable)_streams on single-pad subdevs
>>>>>>>> though, so you patch could still be interesting.
>>>>>>>
>>>>>>> Instead of the number of pads, could we use instead the
>>>>>>> V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
>>>>>>> determine the need for this?
>>>>>>
>>>>>> Maybe, but are they better? Do you see some issue with checking for the
>>>>>> number of pads? I considered a few options, but then thought that the most
>>>>>> safest test for this case is 1) one pad 2) enable/disable_streams
>>>>>> implemented.
>>>>>
>>>>> I think I'd actually prefer {enable,disable}_streams in fact.
>>>>
>>>> Hmm, sorry, now I'm confused =). What do you mean with that?
>>>
>>> I'd use V4L2_SUBDEV_FL_STREAMS flag instead of the number of pads. The
>>> number of pads is less related to routing.
>>
>> Well, with one pad you cannot have routing =).
>>
>> In this patch I used sd->enabled_streams to track the enabled streams, but
>> if we need to support multiple pads, I'll have to invent something new for
>> that.
> 
> What exactly do you think needs to be changed? This is just about starting
> and stopping streaming using a different sent of callbacks, right?

The helpers track which streams are enabled, so we need some place to 
store the enabled streams.

For V4L2_SUBDEV_FL_STREAMS we have that in state->stream_configs for 
each stream. For the one-source-pad case we have a subdev wide 
sd->enabled_streams to store that. But we don't have any place at the 
moment to store if a pad is enabled.

  Tomi

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Sakari Ailus]

On Wed, Mar 27, 2024 at 03:39:31PM +0200, Tomi Valkeinen wrote:
> On 27/03/2024 15:32, Sakari Ailus wrote:
> > Heissulivei,
> > 
> > On Wed, Mar 27, 2024 at 01:06:42PM +0200, Tomi Valkeinen wrote:
> > > On 27/03/2024 12:46, Sakari Ailus wrote:
> > > > Heippa,
> > > > 
> > > > On Mon, Mar 25, 2024 at 07:56:46PM +0200, Tomi Valkeinen wrote:
> > > > > On 25/03/2024 19:52, Sakari Ailus wrote:
> > > > > > Moi,
> > > > > > 
> > > > > > On Mon, Mar 25, 2024 at 03:43:01PM +0200, Tomi Valkeinen wrote:
> > > > > > > On 25/03/2024 15:02, Sakari Ailus wrote:
> > > > > > > > Moi,
> > > > > > > > 
> > > > > > > > Thanks for the patch.
> > > > > > > > 
> > > > > > > > On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
> > > > > > > > > Hi Tomi,
> > > > > > > > > 
> > > > > > > > > On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
> > > > > > > > > > Currently a subdevice with a single pad, e.g. a sensor subdevice, must
> > > > > > > > > > use the v4l2_subdev_video_ops.s_stream op, instead of
> > > > > > > > > > v4l2_subdev_pad_ops.enable/disable_streams. This is because the
> > > > > > > > > > enable/disable_streams machinery requires a routing table which a subdev
> > > > > > > > > > cannot have with a single pad.
> > > > > > > > > > 
> > > > > > > > > > Implement enable/disable_streams support for these single-pad subdevices
> > > > > > > > > > by assuming an implicit stream 0 when the subdevice has only one pad.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > > > > > > > > ---
> > > > > > > > > > Even though I did send this patch, I'm not sure if this is necessary.
> > > > > > > > > > s_stream works fine for the subdevs with a single pad. With the upcoming
> > > > > > > > > > internal pads, adding an internal pad to the subdev will create a
> > > > > > > > > > routing table, and enable/disable_streams would get "fixed" that way.
> > > > > > > > 
> > > > > > > > I'd like to get rid of a redundant way to control streaming.
> > > > > > > 
> > > > > > > We can't get rid of it anyway, can we? We're not going to convert old
> > > > > > > drivers to streams.
> > > > > > 
> > > > > > I'd expect to do that but it'd take a long time. That being said, I think
> > > > > > we need to consider devices without pads (VCMs) so it may well be this
> > > > > > would remain after all.
> > > > > > 
> > > > > > > 
> > > > > > > For new drivers, yes, we shouldn't use s_stream. But is the answer for new
> > > > > > > sensor drivers this patch, or requiring an internal pad?
> > > > > > 
> > > > > > For new drivers I'd like to see an internal pad in fact.
> > > > > > {enable,disable}_streams is still internal to the kernel.
> > > > > 
> > > > > So, you think this patch should be dropped?
> > > > 
> > > > No, no. Not all sub-device drivers with pads are camera sensor drivers. :-)
> > > 
> > > Hmm, alright. So we want to support enable/disable_streams for sub-devices
> > > with multiple source pads but no routing (so probably no sink pads)?
> > 
> > That should be allowed indeed, in order to move from s_stream() to
> > {enable,disable}_streams().
> > 
> > > 
> > > > > > > > > > So perhaps the question is, do we want to support single-pad subdevs in
> > > > > > > > > > the future, in which case something like this patch is necessary, or
> > > > > > > > > > will all modern source subdev drivers have internal pads, in which
> > > > > > > > > > case this is not needed...
> > > > > > > > > 
> > > > > > > > > I think the latter would be best. I however can't guarantee we won't
> > > > > > > > > have valid use cases for (enable|disable)_streams on single-pad subdevs
> > > > > > > > > though, so you patch could still be interesting.
> > > > > > > > 
> > > > > > > > Instead of the number of pads, could we use instead the
> > > > > > > > V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
> > > > > > > > determine the need for this?
> > > > > > > 
> > > > > > > Maybe, but are they better? Do you see some issue with checking for the
> > > > > > > number of pads? I considered a few options, but then thought that the most
> > > > > > > safest test for this case is 1) one pad 2) enable/disable_streams
> > > > > > > implemented.
> > > > > > 
> > > > > > I think I'd actually prefer {enable,disable}_streams in fact.
> > > > > 
> > > > > Hmm, sorry, now I'm confused =). What do you mean with that?
> > > > 
> > > > I'd use V4L2_SUBDEV_FL_STREAMS flag instead of the number of pads. The
> > > > number of pads is less related to routing.
> > > 
> > > Well, with one pad you cannot have routing =).
> > > 
> > > In this patch I used sd->enabled_streams to track the enabled streams, but
> > > if we need to support multiple pads, I'll have to invent something new for
> > > that.
> > 
> > What exactly do you think needs to be changed? This is just about starting
> > and stopping streaming using a different sent of callbacks, right?
> 
> The helpers track which streams are enabled, so we need some place to store
> the enabled streams.
> 
> For V4L2_SUBDEV_FL_STREAMS we have that in state->stream_configs for each
> stream. For the one-source-pad case we have a subdev wide
> sd->enabled_streams to store that. But we don't have any place at the moment
> to store if a pad is enabled.

If there are is no support for routing, isn't streaming either enabled or
disabled on all of them?

-- 
Sakari Ailus

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Tomi Valkeinen]

On 02/04/2024 15:05, Sakari Ailus wrote:
> On Wed, Mar 27, 2024 at 03:39:31PM +0200, Tomi Valkeinen wrote:
>> On 27/03/2024 15:32, Sakari Ailus wrote:
>>> Heissulivei,
>>>
>>> On Wed, Mar 27, 2024 at 01:06:42PM +0200, Tomi Valkeinen wrote:
>>>> On 27/03/2024 12:46, Sakari Ailus wrote:
>>>>> Heippa,
>>>>>
>>>>> On Mon, Mar 25, 2024 at 07:56:46PM +0200, Tomi Valkeinen wrote:
>>>>>> On 25/03/2024 19:52, Sakari Ailus wrote:
>>>>>>> Moi,
>>>>>>>
>>>>>>> On Mon, Mar 25, 2024 at 03:43:01PM +0200, Tomi Valkeinen wrote:
>>>>>>>> On 25/03/2024 15:02, Sakari Ailus wrote:
>>>>>>>>> Moi,
>>>>>>>>>
>>>>>>>>> Thanks for the patch.
>>>>>>>>>
>>>>>>>>> On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
>>>>>>>>>> Hi Tomi,
>>>>>>>>>>
>>>>>>>>>> On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
>>>>>>>>>>> Currently a subdevice with a single pad, e.g. a sensor subdevice, must
>>>>>>>>>>> use the v4l2_subdev_video_ops.s_stream op, instead of
>>>>>>>>>>> v4l2_subdev_pad_ops.enable/disable_streams. This is because the
>>>>>>>>>>> enable/disable_streams machinery requires a routing table which a subdev
>>>>>>>>>>> cannot have with a single pad.
>>>>>>>>>>>
>>>>>>>>>>> Implement enable/disable_streams support for these single-pad subdevices
>>>>>>>>>>> by assuming an implicit stream 0 when the subdevice has only one pad.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>>>>>>> ---
>>>>>>>>>>> Even though I did send this patch, I'm not sure if this is necessary.
>>>>>>>>>>> s_stream works fine for the subdevs with a single pad. With the upcoming
>>>>>>>>>>> internal pads, adding an internal pad to the subdev will create a
>>>>>>>>>>> routing table, and enable/disable_streams would get "fixed" that way.
>>>>>>>>>
>>>>>>>>> I'd like to get rid of a redundant way to control streaming.
>>>>>>>>
>>>>>>>> We can't get rid of it anyway, can we? We're not going to convert old
>>>>>>>> drivers to streams.
>>>>>>>
>>>>>>> I'd expect to do that but it'd take a long time. That being said, I think
>>>>>>> we need to consider devices without pads (VCMs) so it may well be this
>>>>>>> would remain after all.
>>>>>>>
>>>>>>>>
>>>>>>>> For new drivers, yes, we shouldn't use s_stream. But is the answer for new
>>>>>>>> sensor drivers this patch, or requiring an internal pad?
>>>>>>>
>>>>>>> For new drivers I'd like to see an internal pad in fact.
>>>>>>> {enable,disable}_streams is still internal to the kernel.
>>>>>>
>>>>>> So, you think this patch should be dropped?
>>>>>
>>>>> No, no. Not all sub-device drivers with pads are camera sensor drivers. :-)
>>>>
>>>> Hmm, alright. So we want to support enable/disable_streams for sub-devices
>>>> with multiple source pads but no routing (so probably no sink pads)?
>>>
>>> That should be allowed indeed, in order to move from s_stream() to
>>> {enable,disable}_streams().
>>>
>>>>
>>>>>>>>>>> So perhaps the question is, do we want to support single-pad subdevs in
>>>>>>>>>>> the future, in which case something like this patch is necessary, or
>>>>>>>>>>> will all modern source subdev drivers have internal pads, in which
>>>>>>>>>>> case this is not needed...
>>>>>>>>>>
>>>>>>>>>> I think the latter would be best. I however can't guarantee we won't
>>>>>>>>>> have valid use cases for (enable|disable)_streams on single-pad subdevs
>>>>>>>>>> though, so you patch could still be interesting.
>>>>>>>>>
>>>>>>>>> Instead of the number of pads, could we use instead the
>>>>>>>>> V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
>>>>>>>>> determine the need for this?
>>>>>>>>
>>>>>>>> Maybe, but are they better? Do you see some issue with checking for the
>>>>>>>> number of pads? I considered a few options, but then thought that the most
>>>>>>>> safest test for this case is 1) one pad 2) enable/disable_streams
>>>>>>>> implemented.
>>>>>>>
>>>>>>> I think I'd actually prefer {enable,disable}_streams in fact.
>>>>>>
>>>>>> Hmm, sorry, now I'm confused =). What do you mean with that?
>>>>>
>>>>> I'd use V4L2_SUBDEV_FL_STREAMS flag instead of the number of pads. The
>>>>> number of pads is less related to routing.
>>>>
>>>> Well, with one pad you cannot have routing =).
>>>>
>>>> In this patch I used sd->enabled_streams to track the enabled streams, but
>>>> if we need to support multiple pads, I'll have to invent something new for
>>>> that.
>>>
>>> What exactly do you think needs to be changed? This is just about starting
>>> and stopping streaming using a different sent of callbacks, right?
>>
>> The helpers track which streams are enabled, so we need some place to store
>> the enabled streams.
>>
>> For V4L2_SUBDEV_FL_STREAMS we have that in state->stream_configs for each
>> stream. For the one-source-pad case we have a subdev wide
>> sd->enabled_streams to store that. But we don't have any place at the moment
>> to store if a pad is enabled.
> 
> If there are is no support for routing, isn't streaming either enabled or
> disabled on all of them?

Hmm, no, I don't see why that would be the case. If a subdev has two 
source pads, and it gets an enable_streams() call on the first source 
pad, why would the second source pad be enabled too?

  Tomi

Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Sakari Ailus]

On Wed, Apr 03, 2024 at 12:25:44PM +0300, Tomi Valkeinen wrote:
> On 02/04/2024 15:05, Sakari Ailus wrote:
> > On Wed, Mar 27, 2024 at 03:39:31PM +0200, Tomi Valkeinen wrote:
> > > On 27/03/2024 15:32, Sakari Ailus wrote:
> > > > Heissulivei,
> > > > 
> > > > On Wed, Mar 27, 2024 at 01:06:42PM +0200, Tomi Valkeinen wrote:
> > > > > On 27/03/2024 12:46, Sakari Ailus wrote:
> > > > > > Heippa,
> > > > > > 
> > > > > > On Mon, Mar 25, 2024 at 07:56:46PM +0200, Tomi Valkeinen wrote:
> > > > > > > On 25/03/2024 19:52, Sakari Ailus wrote:
> > > > > > > > Moi,
> > > > > > > > 
> > > > > > > > On Mon, Mar 25, 2024 at 03:43:01PM +0200, Tomi Valkeinen wrote:
> > > > > > > > > On 25/03/2024 15:02, Sakari Ailus wrote:
> > > > > > > > > > Moi,
> > > > > > > > > > 
> > > > > > > > > > Thanks for the patch.
> > > > > > > > > > 
> > > > > > > > > > On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
> > > > > > > > > > > Hi Tomi,
> > > > > > > > > > > 
> > > > > > > > > > > On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
> > > > > > > > > > > > Currently a subdevice with a single pad, e.g. a sensor subdevice, must
> > > > > > > > > > > > use the v4l2_subdev_video_ops.s_stream op, instead of
> > > > > > > > > > > > v4l2_subdev_pad_ops.enable/disable_streams. This is because the
> > > > > > > > > > > > enable/disable_streams machinery requires a routing table which a subdev
> > > > > > > > > > > > cannot have with a single pad.
> > > > > > > > > > > > 
> > > > > > > > > > > > Implement enable/disable_streams support for these single-pad subdevices
> > > > > > > > > > > > by assuming an implicit stream 0 when the subdevice has only one pad.
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > > Even though I did send this patch, I'm not sure if this is necessary.
> > > > > > > > > > > > s_stream works fine for the subdevs with a single pad. With the upcoming
> > > > > > > > > > > > internal pads, adding an internal pad to the subdev will create a
> > > > > > > > > > > > routing table, and enable/disable_streams would get "fixed" that way.
> > > > > > > > > > 
> > > > > > > > > > I'd like to get rid of a redundant way to control streaming.
> > > > > > > > > 
> > > > > > > > > We can't get rid of it anyway, can we? We're not going to convert old
> > > > > > > > > drivers to streams.
> > > > > > > > 
> > > > > > > > I'd expect to do that but it'd take a long time. That being said, I think
> > > > > > > > we need to consider devices without pads (VCMs) so it may well be this
> > > > > > > > would remain after all.
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > For new drivers, yes, we shouldn't use s_stream. But is the answer for new
> > > > > > > > > sensor drivers this patch, or requiring an internal pad?
> > > > > > > > 
> > > > > > > > For new drivers I'd like to see an internal pad in fact.
> > > > > > > > {enable,disable}_streams is still internal to the kernel.
> > > > > > > 
> > > > > > > So, you think this patch should be dropped?
> > > > > > 
> > > > > > No, no. Not all sub-device drivers with pads are camera sensor drivers. :-)
> > > > > 
> > > > > Hmm, alright. So we want to support enable/disable_streams for sub-devices
> > > > > with multiple source pads but no routing (so probably no sink pads)?
> > > > 
> > > > That should be allowed indeed, in order to move from s_stream() to
> > > > {enable,disable}_streams().
> > > > 
> > > > > 
> > > > > > > > > > > > So perhaps the question is, do we want to support single-pad subdevs in
> > > > > > > > > > > > the future, in which case something like this patch is necessary, or
> > > > > > > > > > > > will all modern source subdev drivers have internal pads, in which
> > > > > > > > > > > > case this is not needed...
> > > > > > > > > > > 
> > > > > > > > > > > I think the latter would be best. I however can't guarantee we won't
> > > > > > > > > > > have valid use cases for (enable|disable)_streams on single-pad subdevs
> > > > > > > > > > > though, so you patch could still be interesting.
> > > > > > > > > > 
> > > > > > > > > > Instead of the number of pads, could we use instead the
> > > > > > > > > > V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
> > > > > > > > > > determine the need for this?
> > > > > > > > > 
> > > > > > > > > Maybe, but are they better? Do you see some issue with checking for the
> > > > > > > > > number of pads? I considered a few options, but then thought that the most
> > > > > > > > > safest test for this case is 1) one pad 2) enable/disable_streams
> > > > > > > > > implemented.
> > > > > > > > 
> > > > > > > > I think I'd actually prefer {enable,disable}_streams in fact.
> > > > > > > 
> > > > > > > Hmm, sorry, now I'm confused =). What do you mean with that?
> > > > > > 
> > > > > > I'd use V4L2_SUBDEV_FL_STREAMS flag instead of the number of pads. The
> > > > > > number of pads is less related to routing.
> > > > > 
> > > > > Well, with one pad you cannot have routing =).
> > > > > 
> > > > > In this patch I used sd->enabled_streams to track the enabled streams, but
> > > > > if we need to support multiple pads, I'll have to invent something new for
> > > > > that.
> > > > 
> > > > What exactly do you think needs to be changed? This is just about starting
> > > > and stopping streaming using a different sent of callbacks, right?
> > > 
> > > The helpers track which streams are enabled, so we need some place to store
> > > the enabled streams.
> > > 
> > > For V4L2_SUBDEV_FL_STREAMS we have that in state->stream_configs for each
> > > stream. For the one-source-pad case we have a subdev wide
> > > sd->enabled_streams to store that. But we don't have any place at the moment
> > > to store if a pad is enabled.
> > 
> > If there are is no support for routing, isn't streaming either enabled or
> > disabled on all of them?
> 
> Hmm, no, I don't see why that would be the case. If a subdev has two source
> pads, and it gets an enable_streams() call on the first source pad, why
> would the second source pad be enabled too?

Because when there are no routes, all pads are interconnected so streaming
starts or stops on all pads at the same time?

-- 
Sakari Ailus