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@xxxxxxxxxxxxxxxx>
> ---
> 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:
--
Regards,
Laurent Pinchart
