[PATCH v2 0/9] media: subdev: Improve stream enable/disable machinery

[PATCH v2 0/9] media: subdev: Improve stream enable/disable machinery

[Tomi Valkeinen]

This series works on the .s_stream, .enable_streams, .disable_streams
related code. The main feature introduced here, in the last patch, is
adding support for .enable/disable_streams() for subdevs that do not
implement full streams support.

Additionally we add support for the privacy led when
v4l2_subdev_enable/disable_streams() is used.

I have tested this on RPi5.

 Tomi

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Changes in v2:
- New patches for privacy led
- Use v4l2_subdev_has_op()
- Use BITS_PER_BYTE instead of 8
- Fix 80+ line issues
- Fix typos
- Check for pad < 64 also in the non-routing .enable/disable_streams code path
- Dropped "media: subdev: Support enable/disable_streams for non-streams
  subdevs", which is implemented in a different way in new patches in this series
- Link to v1: https://lore.kernel.org/r/20240404-enable-streams-impro-v1-0-1017a35bbe07@ideasonboard.com

---
Tomi Valkeinen (9):
      media: subdev: Add privacy led helpers
      media: subdev: Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams()
      media: subdev: Add checks for subdev features
      media: subdev: Fix use of sd->enabled_streams in call_s_stream()
      media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback
      media: subdev: Add v4l2_subdev_is_streaming()
      media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams()
      media: subdev: Refactor v4l2_subdev_enable/disable_streams()
      media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()

 drivers/media/v4l2-core/v4l2-subdev.c | 364 ++++++++++++++++++++--------------
 include/media/v4l2-subdev.h           |  25 ++-
 2 files changed, 237 insertions(+), 152 deletions(-)
---
base-commit: b82779648dfd3814df4e381f086326ec70fd791f
change-id: 20240404-enable-streams-impro-db8bcd898471

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH v2 1/9] media: subdev: Add privacy led helpers

[Tomi Valkeinen]

Add helper functions to enable and disable the privacy led. This moves
the #if from the call site to the privacy led functions, and makes
adding privacy led support to v4l2_subdev_enable/disable_streams()
cleaner.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 4c6198c48dd6..942c7a644033 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -148,6 +148,23 @@ static int subdev_close(struct file *file)
 }
 #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
 
+static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd)
+{
+#if IS_REACHABLE(CONFIG_LEDS_CLASS)
+	if (!IS_ERR_OR_NULL(sd->privacy_led))
+		led_set_brightness(sd->privacy_led,
+				   sd->privacy_led->max_brightness);
+#endif
+}
+
+static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd)
+{
+#if IS_REACHABLE(CONFIG_LEDS_CLASS)
+	if (!IS_ERR_OR_NULL(sd->privacy_led))
+		led_set_brightness(sd->privacy_led, 0);
+#endif
+}
+
 static inline int check_which(u32 which)
 {
 	if (which != V4L2_SUBDEV_FORMAT_TRY &&
@@ -412,15 +429,11 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
 	if (WARN_ON(!!sd->enabled_streams == !!enable))
 		return 0;
 
-#if IS_REACHABLE(CONFIG_LEDS_CLASS)
-	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
-		if (enable)
-			led_set_brightness(sd->privacy_led,
-					   sd->privacy_led->max_brightness);
-		else
-			led_set_brightness(sd->privacy_led, 0);
-	}
-#endif
+	if (enable)
+		v4l2_subdev_enable_privacy_led(sd);
+	else
+		v4l2_subdev_disable_privacy_led(sd);
+
 	ret = sd->ops->video->s_stream(sd, enable);
 
 	if (!enable && ret < 0) {

-- 
2.34.1

[PATCH v2 2/9] media: subdev: Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams()

[Tomi Valkeinen]

Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 942c7a644033..c8124919b92c 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2132,7 +2132,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 		return 0;
 
 	/* Fallback on .s_stream() if .enable_streams() isn't available. */
-	if (!sd->ops->pad || !sd->ops->pad->enable_streams)
+	if (!v4l2_subdev_has_op(sd, pad, enable_streams))
 		return v4l2_subdev_enable_streams_fallback(sd, pad,
 							   streams_mask);
 
@@ -2249,7 +2249,7 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
 		return 0;
 
 	/* Fallback on .s_stream() if .disable_streams() isn't available. */
-	if (!sd->ops->pad || !sd->ops->pad->disable_streams)
+	if (!v4l2_subdev_has_op(sd, pad, disable_streams))
 		return v4l2_subdev_disable_streams_fallback(sd, pad,
 							    streams_mask);
 

-- 
2.34.1

[PATCH v2 3/9] media: subdev: Add checks for subdev features

[Tomi Valkeinen]

Add some checks to verify that the subdev driver implements required
features.

A subdevice that supports streams (V4L2_SUBDEV_FL_STREAMS) must do one
of the following:

- Implement neither .enable/disable_streams() nor .s_stream(), if the
  subdev is part of a video driver that uses an internal method to
  enable the subdev.
- Implement only .enable/disable_streams(), if support for legacy
  sink-side subdevices is not needed.
- Implement .enable/disable_streams() and .s_stream(), if support for
  legacy sink-side subdevices is needed.

At the moment the framework doesn't check this requirement. Add the
check.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index c8124919b92c..a15a41576918 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1532,6 +1532,33 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
 				struct lock_class_key *key)
 {
 	struct v4l2_subdev_state *state;
+	struct device *dev = sd->dev;
+	bool has_disable_streams;
+	bool has_enable_streams;
+	bool has_s_stream;
+
+	/* Check that the subdevice implements the required features */
+
+	has_s_stream = v4l2_subdev_has_op(sd, video, s_stream);
+	has_enable_streams = v4l2_subdev_has_op(sd, pad, enable_streams);
+	has_disable_streams = v4l2_subdev_has_op(sd, pad, disable_streams);
+
+	if (has_enable_streams != has_disable_streams) {
+		dev_err(dev,
+			"subdev '%s' must implement both or neither of .enable_streams() and .disable_streams()\n",
+			sd->name);
+		return -EINVAL;
+	}
+
+	if (sd->flags & V4L2_SUBDEV_FL_STREAMS) {
+		if (has_s_stream && !has_enable_streams) {
+			dev_err(dev,
+				"subdev '%s' must implement .enable/disable_streams()\n",
+				sd->name);
+
+			return -EINVAL;
+		}
+	}
 
 	state = __v4l2_subdev_state_alloc(sd, name, key);
 	if (IS_ERR(state))

-- 
2.34.1

[PATCH v2 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream()

[Tomi Valkeinen]

call_s_stream() uses sd->enabled_streams to track whether streaming has
already been enabled. However,
v4l2_subdev_enable/disable_streams_fallback(), which was the original
user of this field, already uses it, and
v4l2_subdev_enable/disable_streams_fallback() will call call_s_stream().

This leads to a conflict as both functions set the field. Afaics, both
functions set the field to the same value, so it won't cause a runtime
bug, but it's still wrong and if we, e.g., change how
v4l2_subdev_enable/disable_streams_fallback() operates we might easily
cause bugs.

Fix this by adding a new field, 'streaming_enabled', for
call_s_stream().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------
 include/media/v4l2-subdev.h           | 3 +++
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index a15a41576918..94483b238f2a 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -421,12 +421,8 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
 	 * The .s_stream() operation must never be called to start or stop an
 	 * already started or stopped subdev. Catch offenders but don't return
 	 * an error yet to avoid regressions.
-	 *
-	 * As .s_stream() is mutually exclusive with the .enable_streams() and
-	 * .disable_streams() operation, we can use the enabled_streams field
-	 * to store the subdev streaming state.
 	 */
-	if (WARN_ON(!!sd->enabled_streams == !!enable))
+	if (WARN_ON(!!sd->streaming_enabled == !!enable))
 		return 0;
 
 	if (enable)
@@ -442,7 +438,7 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
 	}
 
 	if (!ret)
-		sd->enabled_streams = enable ? BIT(0) : 0;
+		sd->streaming_enabled = !!enable;
 
 	return ret;
 }
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index a9e6b8146279..f55d03e0acc1 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1043,6 +1043,8 @@ struct v4l2_subdev_platform_data {
  *		     v4l2_subdev_enable_streams() and
  *		     v4l2_subdev_disable_streams() helper functions for fallback
  *		     cases.
+ * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
+ *                     This is only for call_s_stream() internal use.
  *
  * Each instance of a subdev driver should create this struct, either
  * stand-alone or embedded in a larger struct.
@@ -1091,6 +1093,7 @@ struct v4l2_subdev {
 	 */
 	struct v4l2_subdev_state *active_state;
 	u64 enabled_streams;
+	bool streaming_enabled;
 };
 
 

-- 
2.34.1

[PATCH v2 5/9] media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback

[Tomi Valkeinen]

v4l2_subdev_enable/disable_streams_fallback() supports falling back to
.s_stream() for subdevs with a single source pad. It also tracks the
enabled streams for that one pad in the sd->enabled_streams field.

Tracking the enabled streams with sd->enabled_streams does not make
sense, as with .s_stream() there can only be a single stream per pad.
Thus, as the v4l2_subdev_enable/disable_streams_fallback() only supports
a single source pad, all we really need is a boolean which tells whether
streaming has been enabled on this pad or not.

However, as we only need a true/false state for a pad (instead of
tracking which streams have been enabled for a pad), we can easily
extend the fallback mechanism to support multiple source pads as we only
need to keep track of which pads have been enabled.

Change the sd->enabled_streams field to sd->enabled_pads, which is a
64-bit bitmask tracking the enabled source pads. With this change we can
remove the restriction that
v4l2_subdev_enable/disable_streams_fallback() only supports a single
source pad.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 68 ++++++++++++++++++++---------------
 include/media/v4l2-subdev.h           |  9 +++--
 2 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 94483b238f2a..764d61636765 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2103,37 +2103,43 @@ static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
 					       u64 streams_mask)
 {
 	struct device *dev = sd->entity.graph_obj.mdev->dev;
-	unsigned int i;
 	int ret;
 
 	/*
 	 * The subdev doesn't implement pad-based stream enable, fall back
-	 * on the .s_stream() operation. This can only be done for subdevs that
-	 * have a single source pad, as sd->enabled_streams is global to the
-	 * subdev.
+	 * to the .s_stream() operation.
 	 */
 	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
 		return -EOPNOTSUPP;
 
-	for (i = 0; i < sd->entity.num_pads; ++i) {
-		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
-			return -EOPNOTSUPP;
-	}
+	/*
+	 * .s_stream() means there is no streams support, so only allowed stream
+	 * is the implicit stream 0.
+	 */
+	if (streams_mask != BIT_ULL(0))
+		return -EOPNOTSUPP;
+
+	/*
+	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
+	 * with 64 pads or less can be supported.
+	 */
+	if (pad >= sizeof(sd->enabled_pads) * BITS_PER_BYTE)
+		return -EOPNOTSUPP;
 
-	if (sd->enabled_streams & streams_mask) {
-		dev_dbg(dev, "set of streams %#llx already enabled on %s:%u\n",
-			streams_mask, sd->entity.name, pad);
+	if (sd->enabled_pads & BIT_ULL(pad)) {
+		dev_dbg(dev, "pad %u already enabled on %s\n",
+			pad, sd->entity.name);
 		return -EALREADY;
 	}
 
-	/* Start streaming when the first streams are enabled. */
-	if (!sd->enabled_streams) {
+	/* Start streaming when the first pad is enabled. */
+	if (!sd->enabled_pads) {
 		ret = v4l2_subdev_call(sd, video, s_stream, 1);
 		if (ret)
 			return ret;
 	}
 
-	sd->enabled_streams |= streams_mask;
+	sd->enabled_pads |= BIT_ULL(pad);
 
 	return 0;
 }
@@ -2220,37 +2226,43 @@ static int v4l2_subdev_disable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
 						u64 streams_mask)
 {
 	struct device *dev = sd->entity.graph_obj.mdev->dev;
-	unsigned int i;
 	int ret;
 
 	/*
-	 * If the subdev doesn't implement pad-based stream enable, fall  back
-	 * on the .s_stream() operation. This can only be done for subdevs that
-	 * have a single source pad, as sd->enabled_streams is global to the
-	 * subdev.
+	 * If the subdev doesn't implement pad-based stream enable, fall back
+	 * to the .s_stream() operation.
 	 */
 	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
 		return -EOPNOTSUPP;
 
-	for (i = 0; i < sd->entity.num_pads; ++i) {
-		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
-			return -EOPNOTSUPP;
-	}
+	/*
+	 * .s_stream() means there is no streams support, so only allowed stream
+	 * is the implicit stream 0.
+	 */
+	if (streams_mask != BIT_ULL(0))
+		return -EOPNOTSUPP;
+
+	/*
+	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
+	 * with 64 pads or less can be supported.
+	 */
+	if (pad >= sizeof(sd->enabled_pads) * BITS_PER_BYTE)
+		return -EOPNOTSUPP;
 
-	if ((sd->enabled_streams & streams_mask) != streams_mask) {
-		dev_dbg(dev, "set of streams %#llx already disabled on %s:%u\n",
-			streams_mask, sd->entity.name, pad);
+	if (!(sd->enabled_pads & BIT_ULL(pad))) {
+		dev_dbg(dev, "pad %u already disabled on %s\n",
+			pad, sd->entity.name);
 		return -EALREADY;
 	}
 
 	/* Stop streaming when the last streams are disabled. */
-	if (!(sd->enabled_streams & ~streams_mask)) {
+	if (!(sd->enabled_pads & ~BIT_ULL(pad))) {
 		ret = v4l2_subdev_call(sd, video, s_stream, 0);
 		if (ret)
 			return ret;
 	}
 
-	sd->enabled_streams &= ~streams_mask;
+	sd->enabled_pads &= ~BIT_ULL(pad);
 
 	return 0;
 }
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index f55d03e0acc1..d6867511e9cf 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1039,10 +1039,9 @@ struct v4l2_subdev_platform_data {
  * @active_state: Active state for the subdev (NULL for subdevs tracking the
  *		  state internally). Initialized by calling
  *		  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.
+ * @enabled_pads: Bitmask of enabled pads used by v4l2_subdev_enable_streams()
+ *		  and v4l2_subdev_disable_streams() helper functions for
+ *		  fallback cases.
  * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
  *                     This is only for call_s_stream() internal use.
  *
@@ -1092,7 +1091,7 @@ struct v4l2_subdev {
 	 * doesn't support it.
 	 */
 	struct v4l2_subdev_state *active_state;
-	u64 enabled_streams;
+	u64 enabled_pads;
 	bool streaming_enabled;
 };
 

-- 
2.34.1

[PATCH v2 6/9] media: subdev: Add v4l2_subdev_is_streaming()

[Tomi Valkeinen]

Add a helper function which returns whether the subdevice is streaming,
i.e. if .s_stream or .enable_streams has been called successfully.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++
 include/media/v4l2-subdev.h           | 13 +++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 764d61636765..b4981447961a 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2418,6 +2418,31 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
 
+bool v4l2_subdev_is_streaming(struct v4l2_subdev *sd)
+{
+	struct v4l2_subdev_state *state;
+
+	if (!v4l2_subdev_has_op(sd, pad, enable_streams))
+		return sd->streaming_enabled;
+
+	state = v4l2_subdev_get_locked_active_state(sd);
+
+	if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS))
+		return !!sd->enabled_pads;
+
+	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
+		const struct v4l2_subdev_stream_config *cfg;
+
+		cfg = &state->stream_configs.configs[i];
+
+		if (cfg->enabled)
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_is_streaming);
+
 int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd)
 {
 #if IS_REACHABLE(CONFIG_LEDS_CLASS)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d6867511e9cf..270a4dfa5663 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1914,4 +1914,17 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
 void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
 			      const struct v4l2_event *ev);
 
+/**
+ * v4l2_subdev_is_streaming() - Returns if the subdevice is streaming
+ * @sd: The subdevice
+ *
+ * v4l2_subdev_is_streaming() tells if the subdevice is currently streaming.
+ * "Streaming" here means whether .s_stream() or .enable_streams() has been
+ * successfully called, and the streaming has not yet been disabled.
+ *
+ * If the subdevice implements .enable_streams() this function must be called
+ * while holding the active state lock.
+ */
+bool v4l2_subdev_is_streaming(struct v4l2_subdev *sd);
+
 #endif /* _V4L2_SUBDEV_H */

-- 
2.34.1

[PATCH v2 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams()

[Tomi Valkeinen]

We support camera privacy leds with the .s_stream, in call_s_stream, but
we don't have that support when the subdevice implements
.enable/disable_streams.

Add the support by enabling the led when the first stream for a
subdevice is enabled, and disabling the led then the last stream is
disabled.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index b4981447961a..015f2fb423c9 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2149,6 +2149,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 {
 	struct device *dev = sd->entity.graph_obj.mdev->dev;
 	struct v4l2_subdev_state *state;
+	bool already_streaming;
 	u64 found_streams = 0;
 	unsigned int i;
 	int ret;
@@ -2197,6 +2198,11 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 
 	dev_dbg(dev, "enable streams %u:%#llx\n", pad, streams_mask);
 
+	already_streaming = v4l2_subdev_is_streaming(sd);
+
+	if (!already_streaming)
+		v4l2_subdev_enable_privacy_led(sd);
+
 	/* Call the .enable_streams() operation. */
 	ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
 			       streams_mask);
@@ -2216,6 +2222,9 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 	}
 
 done:
+	if (ret && !already_streaming)
+		v4l2_subdev_disable_privacy_led(sd);
+
 	v4l2_subdev_unlock_state(state);
 
 	return ret;
@@ -2339,6 +2348,9 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
 	}
 
 done:
+	if (!v4l2_subdev_is_streaming(sd))
+		v4l2_subdev_disable_privacy_led(sd);
+
 	v4l2_subdev_unlock_state(state);
 
 	return ret;

-- 
2.34.1

[PATCH v2 8/9] media: subdev: Refactor v4l2_subdev_enable/disable_streams()

[Tomi Valkeinen]

Add two internal helper functions, v4l2_subdev_collect_streams() and
v4l2_subdev_set_streams_enabled(), which allows us to refactor
v4l2_subdev_enable/disable_streams() functions.

This (I think) makes the code a bit easier to read, and lets us more
easily add new functionality in the helper functions in the following
patch.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 111 +++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 49 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 015f2fb423c9..6c3c9069f1e2 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2099,6 +2099,44 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd,
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_routing_validate);
 
+static void v4l2_subdev_collect_streams(struct v4l2_subdev *sd,
+					struct v4l2_subdev_state *state,
+					u32 pad, u64 streams_mask,
+					u64 *found_streams,
+					u64 *enabled_streams)
+{
+	*found_streams = 0;
+	*enabled_streams = 0;
+
+	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
+		const struct v4l2_subdev_stream_config *cfg;
+
+		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)
+			*enabled_streams |= BIT_ULL(cfg->stream);
+	}
+}
+
+static void v4l2_subdev_set_streams_enabled(struct v4l2_subdev *sd,
+					    struct v4l2_subdev_state *state,
+					    u32 pad, u64 streams_mask,
+					    bool enabled)
+{
+	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
+		struct v4l2_subdev_stream_config *cfg;
+
+		cfg = &state->stream_configs.configs[i];
+
+		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
+			cfg->enabled = enabled;
+	}
+}
+
 static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
 					       u64 streams_mask)
 {
@@ -2150,8 +2188,8 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 	struct device *dev = sd->entity.graph_obj.mdev->dev;
 	struct v4l2_subdev_state *state;
 	bool already_streaming;
-	u64 found_streams = 0;
-	unsigned int i;
+	u64 enabled_streams;
+	u64 found_streams;
 	int ret;
 
 	/* A few basic sanity checks first. */
@@ -2172,22 +2210,9 @@ 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) {
-			dev_dbg(dev, "stream %u already enabled on %s:%u\n",
-				cfg->stream, sd->entity.name, pad);
-			ret = -EALREADY;
-			goto done;
-		}
-	}
+	v4l2_subdev_collect_streams(sd, state, pad, streams_mask,
+				    &found_streams, &enabled_streams);
 
 	if (found_streams != streams_mask) {
 		dev_dbg(dev, "streams 0x%llx not found on %s:%u\n",
@@ -2196,6 +2221,13 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 		goto done;
 	}
 
+	if (enabled_streams) {
+		dev_dbg(dev, "streams 0x%llx already enabled on %s:%u\n",
+			enabled_streams, sd->entity.name, pad);
+		ret = -EINVAL;
+		goto done;
+	}
+
 	dev_dbg(dev, "enable streams %u:%#llx\n", pad, streams_mask);
 
 	already_streaming = v4l2_subdev_is_streaming(sd);
@@ -2213,13 +2245,7 @@ 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 (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
-			cfg->enabled = true;
-	}
+	v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, true);
 
 done:
 	if (ret && !already_streaming)
@@ -2281,8 +2307,8 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
 {
 	struct device *dev = sd->entity.graph_obj.mdev->dev;
 	struct v4l2_subdev_state *state;
-	u64 found_streams = 0;
-	unsigned int i;
+	u64 enabled_streams;
+	u64 found_streams;
 	int ret;
 
 	/* A few basic sanity checks first. */
@@ -2303,22 +2329,9 @@ 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) {
-			dev_dbg(dev, "stream %u already disabled on %s:%u\n",
-				cfg->stream, sd->entity.name, pad);
-			ret = -EALREADY;
-			goto done;
-		}
-	}
+	v4l2_subdev_collect_streams(sd, state, pad, streams_mask,
+				    &found_streams, &enabled_streams);
 
 	if (found_streams != streams_mask) {
 		dev_dbg(dev, "streams 0x%llx not found on %s:%u\n",
@@ -2327,6 +2340,13 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
 		goto done;
 	}
 
+	if (enabled_streams != streams_mask) {
+		dev_dbg(dev, "streams 0x%llx already disabled on %s:%u\n",
+			streams_mask & ~enabled_streams, sd->entity.name, pad);
+		ret = -EINVAL;
+		goto done;
+	}
+
 	dev_dbg(dev, "disable streams %u:%#llx\n", pad, streams_mask);
 
 	/* Call the .disable_streams() operation. */
@@ -2338,14 +2358,7 @@ 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 (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
-			cfg->enabled = false;
-	}
+	v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, false);
 
 done:
 	if (!v4l2_subdev_is_streaming(sd))

-- 
2.34.1

[PATCH v2 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()

[Tomi Valkeinen]

At the moment the v4l2_subdev_enable/disable_streams() functions call
fallback helpers to handle the case where the subdev only implements
.s_stream(), and the main function handles the case where the subdev
implements streams (V4L2_SUBDEV_FL_STREAMS, which implies
.enable/disable_streams()).

What is missing is support for subdevs which do not implement streams
support, but do implement .enable/disable_streams(). Example cases of
these subdevices are single-stream cameras, where using
.enable/disable_streams() is not required but helps us remove the users
of the legacy .s_stream(), and subdevices with multiple source pads (but
single stream per pad), where .enable/disable_streams() allows the
subdevice to control the enable/disable state per pad.

The two single-streams cases (.s_stream() and .enable/disable_streams())
are very similar, and with small changes we can change the
v4l2_subdev_enable/disable_streams() functions to support all three
cases, without needing separate fallback functions.

A few potentially problematic details, though:

- For the single-streams cases we use sd->enabled_pads field, which
  limits the number of pads for the subdevice to 64. For simplicity I
  added the check for this limitation to the beginning of the function,
  and it also applies to the streams case.

- The fallback functions only allowed the target pad to be a source pad.
  It is not very clear to me why this check was needed, but it was not
  needed in the streams case. However, I doubt the
  v4l2_subdev_enable/disable_streams() code has ever been tested with
  sink pads, so to be on the safe side, I added the same check
  to the v4l2_subdev_enable/disable_streams() functions.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 194 +++++++++++++++-------------------
 1 file changed, 83 insertions(+), 111 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 6c3c9069f1e2..12b90a1cdf6f 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2105,6 +2105,13 @@ static void v4l2_subdev_collect_streams(struct v4l2_subdev *sd,
 					u64 *found_streams,
 					u64 *enabled_streams)
 {
+	if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) {
+		*found_streams = BIT_ULL(0);
+		if (sd->enabled_pads & BIT_ULL(pad))
+			*enabled_streams = BIT_ULL(0);
+		return;
+	}
+
 	*found_streams = 0;
 	*enabled_streams = 0;
 
@@ -2127,6 +2134,14 @@ static void v4l2_subdev_set_streams_enabled(struct v4l2_subdev *sd,
 					    u32 pad, u64 streams_mask,
 					    bool enabled)
 {
+	if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) {
+		if (enabled)
+			sd->enabled_pads |= BIT_ULL(pad);
+		else
+			sd->enabled_pads &= ~BIT_ULL(pad);
+		return;
+	}
+
 	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
 		struct v4l2_subdev_stream_config *cfg;
 
@@ -2137,51 +2152,6 @@ static void v4l2_subdev_set_streams_enabled(struct v4l2_subdev *sd,
 	}
 }
 
-static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
-					       u64 streams_mask)
-{
-	struct device *dev = sd->entity.graph_obj.mdev->dev;
-	int ret;
-
-	/*
-	 * The subdev doesn't implement pad-based stream enable, fall back
-	 * to the .s_stream() operation.
-	 */
-	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
-		return -EOPNOTSUPP;
-
-	/*
-	 * .s_stream() means there is no streams support, so only allowed stream
-	 * is the implicit stream 0.
-	 */
-	if (streams_mask != BIT_ULL(0))
-		return -EOPNOTSUPP;
-
-	/*
-	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
-	 * with 64 pads or less can be supported.
-	 */
-	if (pad >= sizeof(sd->enabled_pads) * BITS_PER_BYTE)
-		return -EOPNOTSUPP;
-
-	if (sd->enabled_pads & BIT_ULL(pad)) {
-		dev_dbg(dev, "pad %u already enabled on %s\n",
-			pad, sd->entity.name);
-		return -EALREADY;
-	}
-
-	/* Start streaming when the first pad is enabled. */
-	if (!sd->enabled_pads) {
-		ret = v4l2_subdev_call(sd, video, s_stream, 1);
-		if (ret)
-			return ret;
-	}
-
-	sd->enabled_pads |= BIT_ULL(pad);
-
-	return 0;
-}
-
 int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 			       u64 streams_mask)
 {
@@ -2190,21 +2160,33 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 	bool already_streaming;
 	u64 enabled_streams;
 	u64 found_streams;
+	bool use_s_stream;
 	int ret;
 
 	/* A few basic sanity checks first. */
 	if (pad >= sd->entity.num_pads)
 		return -EINVAL;
 
+	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
+		return -EOPNOTSUPP;
+
+	/*
+	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
+	 * with 64 pads or less can be supported.
+	 */
+	if (pad >= sizeof(sd->enabled_pads) * BITS_PER_BYTE)
+		return -EOPNOTSUPP;
+
 	if (!streams_mask)
 		return 0;
 
 	/* Fallback on .s_stream() if .enable_streams() isn't available. */
-	if (!v4l2_subdev_has_op(sd, pad, enable_streams))
-		return v4l2_subdev_enable_streams_fallback(sd, pad,
-							   streams_mask);
+	use_s_stream = !v4l2_subdev_has_op(sd, pad, enable_streams);
 
-	state = v4l2_subdev_lock_and_get_active_state(sd);
+	if (!use_s_stream)
+		state = v4l2_subdev_lock_and_get_active_state(sd);
+	else
+		state = NULL;
 
 	/*
 	 * Verify that the requested streams exist and that they are not
@@ -2232,12 +2214,21 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 
 	already_streaming = v4l2_subdev_is_streaming(sd);
 
-	if (!already_streaming)
-		v4l2_subdev_enable_privacy_led(sd);
+	if (!use_s_stream) {
+		if (!already_streaming)
+			v4l2_subdev_enable_privacy_led(sd);
+
+		/* Call the .enable_streams() operation. */
+		ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
+				       streams_mask);
+	} else {
+		/* Start streaming when the first pad is enabled. */
+		if (!already_streaming)
+			ret = v4l2_subdev_call(sd, video, s_stream, 1);
+		else
+			ret = 0;
+	}
 
-	/* Call the .enable_streams() operation. */
-	ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
-			       streams_mask);
 	if (ret) {
 		dev_dbg(dev, "enable streams %u:%#llx failed: %d\n", pad,
 			streams_mask, ret);
@@ -2248,33 +2239,32 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 	v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, true);
 
 done:
-	if (ret && !already_streaming)
-		v4l2_subdev_disable_privacy_led(sd);
+	if (!use_s_stream) {
+		if (ret && !already_streaming)
+			v4l2_subdev_disable_privacy_led(sd);
 
-	v4l2_subdev_unlock_state(state);
+		v4l2_subdev_unlock_state(state);
+	}
 
 	return ret;
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_enable_streams);
 
-static int v4l2_subdev_disable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
-						u64 streams_mask)
+int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
+				u64 streams_mask)
 {
 	struct device *dev = sd->entity.graph_obj.mdev->dev;
+	struct v4l2_subdev_state *state;
+	u64 enabled_streams;
+	u64 found_streams;
+	bool use_s_stream;
 	int ret;
 
-	/*
-	 * If the subdev doesn't implement pad-based stream enable, fall back
-	 * to the .s_stream() operation.
-	 */
-	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
-		return -EOPNOTSUPP;
+	/* A few basic sanity checks first. */
+	if (pad >= sd->entity.num_pads)
+		return -EINVAL;
 
-	/*
-	 * .s_stream() means there is no streams support, so only allowed stream
-	 * is the implicit stream 0.
-	 */
-	if (streams_mask != BIT_ULL(0))
+	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
 		return -EOPNOTSUPP;
 
 	/*
@@ -2284,46 +2274,16 @@ static int v4l2_subdev_disable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
 	if (pad >= sizeof(sd->enabled_pads) * BITS_PER_BYTE)
 		return -EOPNOTSUPP;
 
-	if (!(sd->enabled_pads & BIT_ULL(pad))) {
-		dev_dbg(dev, "pad %u already disabled on %s\n",
-			pad, sd->entity.name);
-		return -EALREADY;
-	}
-
-	/* Stop streaming when the last streams are disabled. */
-	if (!(sd->enabled_pads & ~BIT_ULL(pad))) {
-		ret = v4l2_subdev_call(sd, video, s_stream, 0);
-		if (ret)
-			return ret;
-	}
-
-	sd->enabled_pads &= ~BIT_ULL(pad);
-
-	return 0;
-}
-
-int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
-				u64 streams_mask)
-{
-	struct device *dev = sd->entity.graph_obj.mdev->dev;
-	struct v4l2_subdev_state *state;
-	u64 enabled_streams;
-	u64 found_streams;
-	int ret;
-
-	/* A few basic sanity checks first. */
-	if (pad >= sd->entity.num_pads)
-		return -EINVAL;
-
 	if (!streams_mask)
 		return 0;
 
 	/* Fallback on .s_stream() if .disable_streams() isn't available. */
-	if (!v4l2_subdev_has_op(sd, pad, disable_streams))
-		return v4l2_subdev_disable_streams_fallback(sd, pad,
-							    streams_mask);
+	use_s_stream = !v4l2_subdev_has_op(sd, pad, disable_streams);
 
-	state = v4l2_subdev_lock_and_get_active_state(sd);
+	if (!use_s_stream)
+		state = v4l2_subdev_lock_and_get_active_state(sd);
+	else
+		state = NULL;
 
 	/*
 	 * Verify that the requested streams exist and that they are not
@@ -2349,9 +2309,19 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
 
 	dev_dbg(dev, "disable streams %u:%#llx\n", pad, streams_mask);
 
-	/* Call the .disable_streams() operation. */
-	ret = v4l2_subdev_call(sd, pad, disable_streams, state, pad,
-			       streams_mask);
+	if (!use_s_stream) {
+		/* Call the .disable_streams() operation. */
+		ret = v4l2_subdev_call(sd, pad, disable_streams, state, pad,
+				       streams_mask);
+	} else {
+		/* Stop streaming when the last streams are disabled. */
+
+		if (!(sd->enabled_pads & ~BIT_ULL(pad)))
+			ret = v4l2_subdev_call(sd, video, s_stream, 0);
+		else
+			ret = 0;
+	}
+
 	if (ret) {
 		dev_dbg(dev, "disable streams %u:%#llx failed: %d\n", pad,
 			streams_mask, ret);
@@ -2361,10 +2331,12 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
 	v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, false);
 
 done:
-	if (!v4l2_subdev_is_streaming(sd))
-		v4l2_subdev_disable_privacy_led(sd);
+	if (!use_s_stream) {
+		if (!v4l2_subdev_is_streaming(sd))
+			v4l2_subdev_disable_privacy_led(sd);
 
-	v4l2_subdev_unlock_state(state);
+		v4l2_subdev_unlock_state(state);
+	}
 
 	return ret;
 }

-- 
2.34.1

Re: [PATCH v2 1/9] media: subdev: Add privacy led helpers

[Sakari Ailus]

Moi,

On Fri, Apr 05, 2024 at 12:14:19PM +0300, Tomi Valkeinen wrote:
> Add helper functions to enable and disable the privacy led. This moves
> the #if from the call site to the privacy led functions, and makes
> adding privacy led support to v4l2_subdev_enable/disable_streams()
> cleaner.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4c6198c48dd6..942c7a644033 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -148,6 +148,23 @@ static int subdev_close(struct file *file)
>  }
>  #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>  
> +static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd)
> +{
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	if (!IS_ERR_OR_NULL(sd->privacy_led))
> +		led_set_brightness(sd->privacy_led,
> +				   sd->privacy_led->max_brightness);
> +#endif
> +}
> +
> +static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd)
> +{
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	if (!IS_ERR_OR_NULL(sd->privacy_led))
> +		led_set_brightness(sd->privacy_led, 0);
> +#endif
> +}
> +
>  static inline int check_which(u32 which)
>  {
>  	if (which != V4L2_SUBDEV_FORMAT_TRY &&
> @@ -412,15 +429,11 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (WARN_ON(!!sd->enabled_streams == !!enable))
>  		return 0;
>  
> -#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> -	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> -		if (enable)
> -			led_set_brightness(sd->privacy_led,
> -					   sd->privacy_led->max_brightness);
> -		else
> -			led_set_brightness(sd->privacy_led, 0);
> -	}
> -#endif
> +	if (enable)
> +		v4l2_subdev_enable_privacy_led(sd);
> +	else
> +		v4l2_subdev_disable_privacy_led(sd);
> +
>  	ret = sd->ops->video->s_stream(sd, enable);

I see that you're only making changes before the s_stream call which also
reveals a bug here: if streaming on fails, the LED will remain lit. It
should be fixed before this set.

I cc'd Hans de Goede.

>  
>  	if (!enable && ret < 0) {
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 1/9] media: subdev: Add privacy led helpers

[Hans de Goede]

Hi,

On 4/10/24 11:07 AM, Sakari Ailus wrote:
> Moi,
> 
> On Fri, Apr 05, 2024 at 12:14:19PM +0300, Tomi Valkeinen wrote:
>> Add helper functions to enable and disable the privacy led. This moves
>> the #if from the call site to the privacy led functions, and makes
>> adding privacy led support to v4l2_subdev_enable/disable_streams()
>> cleaner.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-subdev.c | 31 ++++++++++++++++++++++---------
>>  1 file changed, 22 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 4c6198c48dd6..942c7a644033 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -148,6 +148,23 @@ static int subdev_close(struct file *file)
>>  }
>>  #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>>  
>> +static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd)
>> +{
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +	if (!IS_ERR_OR_NULL(sd->privacy_led))
>> +		led_set_brightness(sd->privacy_led,
>> +				   sd->privacy_led->max_brightness);
>> +#endif
>> +}
>> +
>> +static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd)
>> +{
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +	if (!IS_ERR_OR_NULL(sd->privacy_led))
>> +		led_set_brightness(sd->privacy_led, 0);
>> +#endif
>> +}
>> +
>>  static inline int check_which(u32 which)
>>  {
>>  	if (which != V4L2_SUBDEV_FORMAT_TRY &&
>> @@ -412,15 +429,11 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>  	if (WARN_ON(!!sd->enabled_streams == !!enable))
>>  		return 0;
>>  
>> -#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> -	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> -		if (enable)
>> -			led_set_brightness(sd->privacy_led,
>> -					   sd->privacy_led->max_brightness);
>> -		else
>> -			led_set_brightness(sd->privacy_led, 0);
>> -	}
>> -#endif
>> +	if (enable)
>> +		v4l2_subdev_enable_privacy_led(sd);
>> +	else
>> +		v4l2_subdev_disable_privacy_led(sd);
>> +
>>  	ret = sd->ops->video->s_stream(sd, enable);
> 
> I see that you're only making changes before the s_stream call which also
> reveals a bug here: if streaming on fails, the LED will remain lit. It
> should be fixed before this set.
> 
> I cc'd Hans de Goede.

Right that seems like a bug which I introduced. I think it would be
best to move the setting of the LED on/off to after the s_stream()
call in the:

       if (!ret)
                sd->enabled_streams = enable ? BIT(0) : 0;

block, so that the LED is not touched when s_stream(sd, 1) fails.

This delay enabling the LED slightly on stream start but IMHO
that is not a bit issue.

Regards,

Hans

Re: [PATCH v2 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream()

[Sakari Ailus]

Moi,

On Fri, Apr 05, 2024 at 12:14:22PM +0300, Tomi Valkeinen wrote:
> call_s_stream() uses sd->enabled_streams to track whether streaming has
> already been enabled. However,
> v4l2_subdev_enable/disable_streams_fallback(), which was the original
> user of this field, already uses it, and
> v4l2_subdev_enable/disable_streams_fallback() will call call_s_stream().
> 
> This leads to a conflict as both functions set the field. Afaics, both
> functions set the field to the same value, so it won't cause a runtime
> bug, but it's still wrong and if we, e.g., change how
> v4l2_subdev_enable/disable_streams_fallback() operates we might easily
> cause bugs.
> 
> Fix this by adding a new field, 'streaming_enabled', for
> call_s_stream().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------
>  include/media/v4l2-subdev.h           | 3 +++
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index a15a41576918..94483b238f2a 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -421,12 +421,8 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>  	 * The .s_stream() operation must never be called to start or stop an
>  	 * already started or stopped subdev. Catch offenders but don't return
>  	 * an error yet to avoid regressions.
> -	 *
> -	 * As .s_stream() is mutually exclusive with the .enable_streams() and
> -	 * .disable_streams() operation, we can use the enabled_streams field
> -	 * to store the subdev streaming state.
>  	 */
> -	if (WARN_ON(!!sd->enabled_streams == !!enable))
> +	if (WARN_ON(!!sd->streaming_enabled == !!enable))

A single ! is enough on both sides.

>  		return 0;
>  
>  	if (enable)
> @@ -442,7 +438,7 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>  	}
>  
>  	if (!ret)
> -		sd->enabled_streams = enable ? BIT(0) : 0;
> +		sd->streaming_enabled = !!enable;

No need for !! unless you think it needs to be very loud. Casting a
non-boolean value to boolean results in true if it's non-zero.

>  
>  	return ret;
>  }
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a9e6b8146279..f55d03e0acc1 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1043,6 +1043,8 @@ struct v4l2_subdev_platform_data {
>   *		     v4l2_subdev_enable_streams() and
>   *		     v4l2_subdev_disable_streams() helper functions for fallback
>   *		     cases.
> + * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
> + *                     This is only for call_s_stream() internal use.
>   *
>   * Each instance of a subdev driver should create this struct, either
>   * stand-alone or embedded in a larger struct.
> @@ -1091,6 +1093,7 @@ struct v4l2_subdev {
>  	 */
>  	struct v4l2_subdev_state *active_state;
>  	u64 enabled_streams;
> +	bool streaming_enabled;
>  };
>  
>  
> 

-- 
Terveisin,

Sakari Ailus

Re: [PATCH v2 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams()

[Sakari Ailus]

Moi,

On Fri, Apr 05, 2024 at 12:14:25PM +0300, Tomi Valkeinen wrote:
> We support camera privacy leds with the .s_stream, in call_s_stream, but
> we don't have that support when the subdevice implements
> .enable/disable_streams.
> 
> Add the support by enabling the led when the first stream for a
> subdevice is enabled, and disabling the led then the last stream is
> disabled.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index b4981447961a..015f2fb423c9 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2149,6 +2149,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>  {
>  	struct device *dev = sd->entity.graph_obj.mdev->dev;
>  	struct v4l2_subdev_state *state;
> +	bool already_streaming;
>  	u64 found_streams = 0;
>  	unsigned int i;
>  	int ret;
> @@ -2197,6 +2198,11 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>  
>  	dev_dbg(dev, "enable streams %u:%#llx\n", pad, streams_mask);
>  
> +	already_streaming = v4l2_subdev_is_streaming(sd);
> +
> +	if (!already_streaming)
> +		v4l2_subdev_enable_privacy_led(sd);
> +
>  	/* Call the .enable_streams() operation. */
>  	ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
>  			       streams_mask);
> @@ -2216,6 +2222,9 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>  	}
>  
>  done:
> +	if (ret && !already_streaming)
> +		v4l2_subdev_disable_privacy_led(sd);

I think you could also lit the LED only if enabling streaming succeeds.

> +
>  	v4l2_subdev_unlock_state(state);
>  
>  	return ret;
> @@ -2339,6 +2348,9 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>  	}
>  
>  done:
> +	if (!v4l2_subdev_is_streaming(sd))
> +		v4l2_subdev_disable_privacy_led(sd);
> +
>  	v4l2_subdev_unlock_state(state);
>  
>  	return ret;
> 

-- 
Terveisin,

Sakari Ailus

Re: [PATCH v2 8/9] media: subdev: Refactor v4l2_subdev_enable/disable_streams()

[Sakari Ailus]

Moi,

Thank you for working on this.

On Fri, Apr 05, 2024 at 12:14:26PM +0300, Tomi Valkeinen wrote:
> Add two internal helper functions, v4l2_subdev_collect_streams() and
> v4l2_subdev_set_streams_enabled(), which allows us to refactor
> v4l2_subdev_enable/disable_streams() functions.
> 
> This (I think) makes the code a bit easier to read, and lets us more
> easily add new functionality in the helper functions in the following
> patch.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 111 +++++++++++++++++++---------------
>  1 file changed, 62 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 015f2fb423c9..6c3c9069f1e2 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2099,6 +2099,44 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_routing_validate);
>  
> +static void v4l2_subdev_collect_streams(struct v4l2_subdev *sd,
> +					struct v4l2_subdev_state *state,
> +					u32 pad, u64 streams_mask,
> +					u64 *found_streams,
> +					u64 *enabled_streams)
> +{
> +	*found_streams = 0;
> +	*enabled_streams = 0;
> +
> +	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
> +		const struct v4l2_subdev_stream_config *cfg;
> +
> +		cfg = &state->stream_configs.configs[i];

You can do the assignment in declaration.

Same for streams_enabled() below.

> +
> +		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> +			continue;
> +
> +		*found_streams |= BIT_ULL(cfg->stream);
> +		if (cfg->enabled)
> +			*enabled_streams |= BIT_ULL(cfg->stream);
> +	}
> +}
> +
> +static void v4l2_subdev_set_streams_enabled(struct v4l2_subdev *sd,
> +					    struct v4l2_subdev_state *state,
> +					    u32 pad, u64 streams_mask,
> +					    bool enabled)
> +{
> +	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
> +		struct v4l2_subdev_stream_config *cfg;
> +
> +		cfg = &state->stream_configs.configs[i];
> +
> +		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> +			cfg->enabled = enabled;
> +	}
> +}
> +
>  static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
>  					       u64 streams_mask)
>  {
> @@ -2150,8 +2188,8 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>  	struct device *dev = sd->entity.graph_obj.mdev->dev;
>  	struct v4l2_subdev_state *state;
>  	bool already_streaming;
> -	u64 found_streams = 0;
> -	unsigned int i;
> +	u64 enabled_streams;
> +	u64 found_streams;
>  	int ret;
>  
>  	/* A few basic sanity checks first. */
> @@ -2172,22 +2210,9 @@ 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) {
> -			dev_dbg(dev, "stream %u already enabled on %s:%u\n",
> -				cfg->stream, sd->entity.name, pad);
> -			ret = -EALREADY;
> -			goto done;
> -		}
> -	}
> +	v4l2_subdev_collect_streams(sd, state, pad, streams_mask,
> +				    &found_streams, &enabled_streams);
>  
>  	if (found_streams != streams_mask) {
>  		dev_dbg(dev, "streams 0x%llx not found on %s:%u\n",
> @@ -2196,6 +2221,13 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>  		goto done;
>  	}
>  
> +	if (enabled_streams) {
> +		dev_dbg(dev, "streams 0x%llx already enabled on %s:%u\n",
> +			enabled_streams, sd->entity.name, pad);
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +
>  	dev_dbg(dev, "enable streams %u:%#llx\n", pad, streams_mask);
>  
>  	already_streaming = v4l2_subdev_is_streaming(sd);
> @@ -2213,13 +2245,7 @@ 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 (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> -			cfg->enabled = true;
> -	}
> +	v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, true);
>  
>  done:
>  	if (ret && !already_streaming)
> @@ -2281,8 +2307,8 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>  {
>  	struct device *dev = sd->entity.graph_obj.mdev->dev;
>  	struct v4l2_subdev_state *state;
> -	u64 found_streams = 0;
> -	unsigned int i;
> +	u64 enabled_streams;
> +	u64 found_streams;
>  	int ret;
>  
>  	/* A few basic sanity checks first. */
> @@ -2303,22 +2329,9 @@ 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) {
> -			dev_dbg(dev, "stream %u already disabled on %s:%u\n",
> -				cfg->stream, sd->entity.name, pad);
> -			ret = -EALREADY;
> -			goto done;
> -		}
> -	}
> +	v4l2_subdev_collect_streams(sd, state, pad, streams_mask,
> +				    &found_streams, &enabled_streams);
>  
>  	if (found_streams != streams_mask) {
>  		dev_dbg(dev, "streams 0x%llx not found on %s:%u\n",
> @@ -2327,6 +2340,13 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>  		goto done;
>  	}
>  
> +	if (enabled_streams != streams_mask) {
> +		dev_dbg(dev, "streams 0x%llx already disabled on %s:%u\n",
> +			streams_mask & ~enabled_streams, sd->entity.name, pad);
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +
>  	dev_dbg(dev, "disable streams %u:%#llx\n", pad, streams_mask);
>  
>  	/* Call the .disable_streams() operation. */
> @@ -2338,14 +2358,7 @@ 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 (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> -			cfg->enabled = false;
> -	}
> +	v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, false);
>  
>  done:
>  	if (!v4l2_subdev_is_streaming(sd))
> 

-- 
Terveisin,

Sakari Ailus

Re: [PATCH v2 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams()

[Tomi Valkeinen]

On 10/04/2024 13:11, Sakari Ailus wrote:
> Moi,
> 
> On Fri, Apr 05, 2024 at 12:14:25PM +0300, Tomi Valkeinen wrote:
>> We support camera privacy leds with the .s_stream, in call_s_stream, but
>> we don't have that support when the subdevice implements
>> .enable/disable_streams.
>>
>> Add the support by enabling the led when the first stream for a
>> subdevice is enabled, and disabling the led then the last stream is
>> disabled.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index b4981447961a..015f2fb423c9 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -2149,6 +2149,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>>   {
>>   	struct device *dev = sd->entity.graph_obj.mdev->dev;
>>   	struct v4l2_subdev_state *state;
>> +	bool already_streaming;
>>   	u64 found_streams = 0;
>>   	unsigned int i;
>>   	int ret;
>> @@ -2197,6 +2198,11 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>>   
>>   	dev_dbg(dev, "enable streams %u:%#llx\n", pad, streams_mask);
>>   
>> +	already_streaming = v4l2_subdev_is_streaming(sd);
>> +
>> +	if (!already_streaming)
>> +		v4l2_subdev_enable_privacy_led(sd);
>> +
>>   	/* Call the .enable_streams() operation. */
>>   	ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
>>   			       streams_mask);
>> @@ -2216,6 +2222,9 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>>   	}
>>   
>>   done:
>> +	if (ret && !already_streaming)
>> +		v4l2_subdev_disable_privacy_led(sd);
> 
> I think you could also lit the LED only if enabling streaming succeeds.

This matches the implementation in s_stream. I'll change it to match the 
new one being discussed.

  Tomi

Re: [PATCH v2 8/9] media: subdev: Refactor v4l2_subdev_enable/disable_streams()

[Tomi Valkeinen]

On 10/04/2024 13:13, Sakari Ailus wrote:
> Moi,
> 
> Thank you for working on this.
> 
> On Fri, Apr 05, 2024 at 12:14:26PM +0300, Tomi Valkeinen wrote:
>> Add two internal helper functions, v4l2_subdev_collect_streams() and
>> v4l2_subdev_set_streams_enabled(), which allows us to refactor
>> v4l2_subdev_enable/disable_streams() functions.
>>
>> This (I think) makes the code a bit easier to read, and lets us more
>> easily add new functionality in the helper functions in the following
>> patch.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-subdev.c | 111 +++++++++++++++++++---------------
>>   1 file changed, 62 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 015f2fb423c9..6c3c9069f1e2 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -2099,6 +2099,44 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd,
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_subdev_routing_validate);
>>   
>> +static void v4l2_subdev_collect_streams(struct v4l2_subdev *sd,
>> +					struct v4l2_subdev_state *state,
>> +					u32 pad, u64 streams_mask,
>> +					u64 *found_streams,
>> +					u64 *enabled_streams)
>> +{
>> +	*found_streams = 0;
>> +	*enabled_streams = 0;
>> +
>> +	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
>> +		const struct v4l2_subdev_stream_config *cfg;
>> +
>> +		cfg = &state->stream_configs.configs[i];
> 
> You can do the assignment in declaration.

I can, but you want the lines to be split at 80, so that'll end up being 
in two lines, which, I think, looks messier than the one above.

Usually I think doing initialization when declaring the variable is best 
done if it fits into one line.

  Tomi

> 
> Same for streams_enabled() below.
> 
>> +
>> +		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
>> +			continue;
>> +
>> +		*found_streams |= BIT_ULL(cfg->stream);
>> +		if (cfg->enabled)
>> +			*enabled_streams |= BIT_ULL(cfg->stream);
>> +	}
>> +}
>> +
>> +static void v4l2_subdev_set_streams_enabled(struct v4l2_subdev *sd,
>> +					    struct v4l2_subdev_state *state,
>> +					    u32 pad, u64 streams_mask,
>> +					    bool enabled)
>> +{
>> +	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
>> +		struct v4l2_subdev_stream_config *cfg;
>> +
>> +		cfg = &state->stream_configs.configs[i];
>> +
>> +		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
>> +			cfg->enabled = enabled;
>> +	}
>> +}
>> +
>>   static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
>>   					       u64 streams_mask)
>>   {
>> @@ -2150,8 +2188,8 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>>   	struct device *dev = sd->entity.graph_obj.mdev->dev;
>>   	struct v4l2_subdev_state *state;
>>   	bool already_streaming;
>> -	u64 found_streams = 0;
>> -	unsigned int i;
>> +	u64 enabled_streams;
>> +	u64 found_streams;
>>   	int ret;
>>   
>>   	/* A few basic sanity checks first. */
>> @@ -2172,22 +2210,9 @@ 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) {
>> -			dev_dbg(dev, "stream %u already enabled on %s:%u\n",
>> -				cfg->stream, sd->entity.name, pad);
>> -			ret = -EALREADY;
>> -			goto done;
>> -		}
>> -	}
>> +	v4l2_subdev_collect_streams(sd, state, pad, streams_mask,
>> +				    &found_streams, &enabled_streams);
>>   
>>   	if (found_streams != streams_mask) {
>>   		dev_dbg(dev, "streams 0x%llx not found on %s:%u\n",
>> @@ -2196,6 +2221,13 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>>   		goto done;
>>   	}
>>   
>> +	if (enabled_streams) {
>> +		dev_dbg(dev, "streams 0x%llx already enabled on %s:%u\n",
>> +			enabled_streams, sd->entity.name, pad);
>> +		ret = -EINVAL;
>> +		goto done;
>> +	}
>> +
>>   	dev_dbg(dev, "enable streams %u:%#llx\n", pad, streams_mask);
>>   
>>   	already_streaming = v4l2_subdev_is_streaming(sd);
>> @@ -2213,13 +2245,7 @@ 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 (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
>> -			cfg->enabled = true;
>> -	}
>> +	v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, true);
>>   
>>   done:
>>   	if (ret && !already_streaming)
>> @@ -2281,8 +2307,8 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>>   {
>>   	struct device *dev = sd->entity.graph_obj.mdev->dev;
>>   	struct v4l2_subdev_state *state;
>> -	u64 found_streams = 0;
>> -	unsigned int i;
>> +	u64 enabled_streams;
>> +	u64 found_streams;
>>   	int ret;
>>   
>>   	/* A few basic sanity checks first. */
>> @@ -2303,22 +2329,9 @@ 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) {
>> -			dev_dbg(dev, "stream %u already disabled on %s:%u\n",
>> -				cfg->stream, sd->entity.name, pad);
>> -			ret = -EALREADY;
>> -			goto done;
>> -		}
>> -	}
>> +	v4l2_subdev_collect_streams(sd, state, pad, streams_mask,
>> +				    &found_streams, &enabled_streams);
>>   
>>   	if (found_streams != streams_mask) {
>>   		dev_dbg(dev, "streams 0x%llx not found on %s:%u\n",
>> @@ -2327,6 +2340,13 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>>   		goto done;
>>   	}
>>   
>> +	if (enabled_streams != streams_mask) {
>> +		dev_dbg(dev, "streams 0x%llx already disabled on %s:%u\n",
>> +			streams_mask & ~enabled_streams, sd->entity.name, pad);
>> +		ret = -EINVAL;
>> +		goto done;
>> +	}
>> +
>>   	dev_dbg(dev, "disable streams %u:%#llx\n", pad, streams_mask);
>>   
>>   	/* Call the .disable_streams() operation. */
>> @@ -2338,14 +2358,7 @@ 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 (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
>> -			cfg->enabled = false;
>> -	}
>> +	v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, false);
>>   
>>   done:
>>   	if (!v4l2_subdev_is_streaming(sd))
>>
> 

Re: [PATCH v2 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream()

[Tomi Valkeinen]

On 10/04/2024 13:05, Sakari Ailus wrote:
> Moi,
> 
> On Fri, Apr 05, 2024 at 12:14:22PM +0300, Tomi Valkeinen wrote:
>> call_s_stream() uses sd->enabled_streams to track whether streaming has
>> already been enabled. However,
>> v4l2_subdev_enable/disable_streams_fallback(), which was the original
>> user of this field, already uses it, and
>> v4l2_subdev_enable/disable_streams_fallback() will call call_s_stream().
>>
>> This leads to a conflict as both functions set the field. Afaics, both
>> functions set the field to the same value, so it won't cause a runtime
>> bug, but it's still wrong and if we, e.g., change how
>> v4l2_subdev_enable/disable_streams_fallback() operates we might easily
>> cause bugs.
>>
>> Fix this by adding a new field, 'streaming_enabled', for
>> call_s_stream().
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------
>>   include/media/v4l2-subdev.h           | 3 +++
>>   2 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index a15a41576918..94483b238f2a 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -421,12 +421,8 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>   	 * The .s_stream() operation must never be called to start or stop an
>>   	 * already started or stopped subdev. Catch offenders but don't return
>>   	 * an error yet to avoid regressions.
>> -	 *
>> -	 * As .s_stream() is mutually exclusive with the .enable_streams() and
>> -	 * .disable_streams() operation, we can use the enabled_streams field
>> -	 * to store the subdev streaming state.
>>   	 */
>> -	if (WARN_ON(!!sd->enabled_streams == !!enable))
>> +	if (WARN_ON(!!sd->streaming_enabled == !!enable))
> 
> A single ! is enough on both sides.

Or perhaps (sd->streaming_enabled == !!enable).

> 
>>   		return 0;
>>   
>>   	if (enable)
>> @@ -442,7 +438,7 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>   	}
>>   
>>   	if (!ret)
>> -		sd->enabled_streams = enable ? BIT(0) : 0;
>> +		sd->streaming_enabled = !!enable;
> 
> No need for !! unless you think it needs to be very loud. Casting a
> non-boolean value to boolean results in true if it's non-zero.

Indeed. My coding style for booleans seems to be from times long past.

  Tomi

> 
>>   
>>   	return ret;
>>   }
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index a9e6b8146279..f55d03e0acc1 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -1043,6 +1043,8 @@ struct v4l2_subdev_platform_data {
>>    *		     v4l2_subdev_enable_streams() and
>>    *		     v4l2_subdev_disable_streams() helper functions for fallback
>>    *		     cases.
>> + * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
>> + *                     This is only for call_s_stream() internal use.
>>    *
>>    * Each instance of a subdev driver should create this struct, either
>>    * stand-alone or embedded in a larger struct.
>> @@ -1091,6 +1093,7 @@ struct v4l2_subdev {
>>   	 */
>>   	struct v4l2_subdev_state *active_state;
>>   	u64 enabled_streams;
>> +	bool streaming_enabled;
>>   };
>>   
>>   
>>
> 

Re: [PATCH v2 8/9] media: subdev: Refactor v4l2_subdev_enable/disable_streams()

[Sakari Ailus]

On Wed, Apr 10, 2024 at 01:35:41PM +0300, Tomi Valkeinen wrote:
> On 10/04/2024 13:13, Sakari Ailus wrote:
> > Moi,
> > 
> > Thank you for working on this.
> > 
> > On Fri, Apr 05, 2024 at 12:14:26PM +0300, Tomi Valkeinen wrote:
> > > Add two internal helper functions, v4l2_subdev_collect_streams() and
> > > v4l2_subdev_set_streams_enabled(), which allows us to refactor
> > > v4l2_subdev_enable/disable_streams() functions.
> > > 
> > > This (I think) makes the code a bit easier to read, and lets us more
> > > easily add new functionality in the helper functions in the following
> > > patch.
> > > 
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > ---
> > >   drivers/media/v4l2-core/v4l2-subdev.c | 111 +++++++++++++++++++---------------
> > >   1 file changed, 62 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 015f2fb423c9..6c3c9069f1e2 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -2099,6 +2099,44 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd,
> > >   }
> > >   EXPORT_SYMBOL_GPL(v4l2_subdev_routing_validate);
> > > +static void v4l2_subdev_collect_streams(struct v4l2_subdev *sd,
> > > +					struct v4l2_subdev_state *state,
> > > +					u32 pad, u64 streams_mask,
> > > +					u64 *found_streams,
> > > +					u64 *enabled_streams)
> > > +{
> > > +	*found_streams = 0;
> > > +	*enabled_streams = 0;
> > > +
> > > +	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
> > > +		const struct v4l2_subdev_stream_config *cfg;
> > > +
> > > +		cfg = &state->stream_configs.configs[i];
> > 
> > You can do the assignment in declaration.
> 
> I can, but you want the lines to be split at 80, so that'll end up being in
> two lines, which, I think, looks messier than the one above.
> 
> Usually I think doing initialization when declaring the variable is best
> done if it fits into one line.

I don't consider a line break being an issue here (or almost anywhere else,
except possibly for arrays of definitions).

-- 
Sakari Ailus