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 <[email protected]>
---
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/[email protected]
---
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 <[email protected]>
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 <[email protected]>
---
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
Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams().
Signed-off-by: Tomi Valkeinen <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> ---
>> 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))
>>
>
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 <[email protected]>
>> ---
>> 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;
>> };
>>
>>
>>
>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > > ---
> > > 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