Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs
From: Sakari Ailus
Date: Mon Mar 25 2024 - 11:35:20 EST
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@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.
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