Received-SPF: pass (google.com: domain of linux-kernel+bounces-117028-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Date: Mon, 25 Mar 2024 14:43:23 +0200
Subject: [PATCH] media: v4l2-subdev: Support enable/disable_streams for
 single-pad subdevs
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Message-Id: <20240325-single-pad-enable-streams-v1-1-142e19896a72@ideasonboard.com>
To: Mauro Carvalho Chehab <mchehab@kernel.org>, 
 Hans Verkuil <hverkuil@xs4all.nl>, 
 Laurent Pinchart <laurent.pinchart@ideasonboard.com>, 
 Sakari Ailus <sakari.ailus@linux.intel.com>, 
 Umang Jain <umang.jain@ideasonboard.com>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, 
 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

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(-)

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,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>