| Message ID | 20240325-single-pad-enable-streams-v1-1-142e19896a72@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs | expand |
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
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 >
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
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.
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
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,
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.
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
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?
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
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?
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
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?
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.
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(-) --- base-commit: e8f897f4afef0031fe618a8e94127a0934896aba change-id: 20240325-single-pad-enable-streams-32a9a746ac5b Best regards,