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@xxxxxxxxxxxxxxxx>
> ---
> 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
