| Message ID | 20240404-enable-streams-impro-v1-4-1017a35bbe07@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: subdev: Improve stream enable/disable machinery | expand |
Hi Tomi, Thank you for the patch. On Thu, Apr 04, 2024 at 01:50:03PM +0300, Tomi Valkeinen wrote: > Currently a subdevice without streams support (V4L2_SUBDEV_FL_STREAMS), > e.g. a sensor subdevice with a single source pad, 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 subdevs without streams > support. As they don't support multiple streams, each pad must contain a > single stream, with stream ID 0, and rather than tracking enabled > streams, we can track enabled pads similarly to the > enable/disable_streams_fallback by using the sd->enabled_pads field. Could you explain in the commit message why this is desired ? > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 86 +++++++++++++++++++++++------------ > 1 file changed, 58 insertions(+), 28 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 91298bb84e6b..d86f930be2c4 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -2158,21 +2158,32 @@ 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 (sd->flags & V4L2_SUBDEV_FL_STREAMS) { > + 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; > + if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream))) > + continue; > > - found_streams |= BIT_ULL(cfg->stream); > + 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); > + if (cfg->enabled) { > + dev_dbg(dev, "stream %u already enabled on %s:%u\n", > + cfg->stream, sd->entity.name, pad); > + ret = -EALREADY; > + goto done; > + } > + } > + } else { > + if (sd->enabled_pads & BIT_ULL(pad)) { > + dev_dbg(dev, "stream 0 already enabled on %s:%u\n", > + sd->entity.name, pad); > ret = -EALREADY; > goto done; > } > + > + found_streams = BIT_ULL(0); > } > > if (found_streams != streams_mask) { > @@ -2194,12 +2205,16 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad, > } > > /* 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->flags & V4L2_SUBDEV_FL_STREAMS) { > + 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; > + } > + } else { > + sd->enabled_pads |= BIT_ULL(pad); > } > > done: > @@ -2281,21 +2296,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 (sd->flags & V4L2_SUBDEV_FL_STREAMS) { > + 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; > + if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream))) > + continue; > > - found_streams |= BIT_ULL(cfg->stream); > + 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); > + if (!cfg->enabled) { > + dev_dbg(dev, "stream %u already disabled on %s:%u\n", > + cfg->stream, sd->entity.name, pad); > + ret = -EALREADY; > + goto done; > + } > + } > + } else { > + if (!(sd->enabled_pads & BIT_ULL(pad))) { > + dev_dbg(dev, "stream 0 already disabled on %s:%u\n", > + sd->entity.name, pad); > ret = -EALREADY; > goto done; > } > + > + found_streams = BIT_ULL(0); > } > > if (found_streams != streams_mask) { > @@ -2317,12 +2343,16 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad, > } > > /* 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->flags & V4L2_SUBDEV_FL_STREAMS) { > + 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; > + } > + } else { > + sd->enabled_pads &= ~BIT_ULL(pad); > } > > done:
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 91298bb84e6b..d86f930be2c4 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -2158,21 +2158,32 @@ 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 (sd->flags & V4L2_SUBDEV_FL_STREAMS) { + 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; + if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream))) + continue; - found_streams |= BIT_ULL(cfg->stream); + 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); + if (cfg->enabled) { + dev_dbg(dev, "stream %u already enabled on %s:%u\n", + cfg->stream, sd->entity.name, pad); + ret = -EALREADY; + goto done; + } + } + } else { + if (sd->enabled_pads & BIT_ULL(pad)) { + dev_dbg(dev, "stream 0 already enabled on %s:%u\n", + sd->entity.name, pad); ret = -EALREADY; goto done; } + + found_streams = BIT_ULL(0); } if (found_streams != streams_mask) { @@ -2194,12 +2205,16 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad, } /* 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->flags & V4L2_SUBDEV_FL_STREAMS) { + 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; + } + } else { + sd->enabled_pads |= BIT_ULL(pad); } done: @@ -2281,21 +2296,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 (sd->flags & V4L2_SUBDEV_FL_STREAMS) { + 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; + if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream))) + continue; - found_streams |= BIT_ULL(cfg->stream); + 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); + if (!cfg->enabled) { + dev_dbg(dev, "stream %u already disabled on %s:%u\n", + cfg->stream, sd->entity.name, pad); + ret = -EALREADY; + goto done; + } + } + } else { + if (!(sd->enabled_pads & BIT_ULL(pad))) { + dev_dbg(dev, "stream 0 already disabled on %s:%u\n", + sd->entity.name, pad); ret = -EALREADY; goto done; } + + found_streams = BIT_ULL(0); } if (found_streams != streams_mask) { @@ -2317,12 +2343,16 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad, } /* 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->flags & V4L2_SUBDEV_FL_STREAMS) { + 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; + } + } else { + sd->enabled_pads &= ~BIT_ULL(pad); } done:
Currently a subdevice without streams support (V4L2_SUBDEV_FL_STREAMS), e.g. a sensor subdevice with a single source pad, 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 subdevs without streams support. As they don't support multiple streams, each pad must contain a single stream, with stream ID 0, and rather than tracking enabled streams, we can track enabled pads similarly to the enable/disable_streams_fallback by using the sd->enabled_pads field. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 86 +++++++++++++++++++++++------------ 1 file changed, 58 insertions(+), 28 deletions(-)