LinuxLists.cc - [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery

[permalink] [raw]

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

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 | 30 +++++++++++++++++++++---------
1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 012b757eac9f..13957543d153 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 &&
@@ -422,15 +439,10 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
if (!ret) {
sd->enabled_streams = enable ? BIT(0) : 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);
}

return ret;

--
2.34.1

2024-04-10 12:37:45

[permalink] [raw]

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

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 4a531c2b16c4..606a909cd778 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1533,6 +1533,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

2024-04-10 12:37:59

[permalink] [raw]

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

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 606a909cd778..6cd353d83dfc 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;

ret = sd->ops->video->s_stream(sd, enable);
@@ -437,7 +433,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;

if (enable)
v4l2_subdev_enable_privacy_led(sd);
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

2024-04-10 12:38:11

[permalink] [raw]

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

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 6cd353d83dfc..3d2c9c224b8f 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2104,37 +2104,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;
}
@@ -2221,37 +2227,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

2024-04-10 12:38:24

[permalink] [raw]

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

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 3d2c9c224b8f..20b5a00cbeeb 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2419,6 +2419,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

2024-04-10 12:38:38

[permalink] [raw]

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

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 | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 20b5a00cbeeb..f44aaa4e1fab 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2150,6 +2150,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;
@@ -2198,6 +2199,8 @@ 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);
+
/* Call the .enable_streams() operation. */
ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
streams_mask);
@@ -2216,6 +2219,9 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
cfg->enabled = true;
}

+ if (!already_streaming)
+ v4l2_subdev_enable_privacy_led(sd);
+
done:
v4l2_subdev_unlock_state(state);

@@ -2340,6 +2346,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

2024-04-10 12:39:00

[permalink] [raw]

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

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 | 109 +++++++++++++++++++---------------
1 file changed, 60 insertions(+), 49 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index f44aaa4e1fab..0d376d72ecc7 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2100,6 +2100,42 @@ 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 =
+ &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 =
+ &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)
{
@@ -2151,8 +2187,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. */
@@ -2173,22 +2209,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",
@@ -2197,6 +2220,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);
@@ -2211,13 +2241,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);

if (!already_streaming)
v4l2_subdev_enable_privacy_led(sd);
@@ -2279,8 +2303,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. */
@@ -2301,22 +2325,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",
@@ -2325,6 +2336,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. */
@@ -2336,14 +2354,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

2024-04-10 12:39:15

[permalink] [raw]

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

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 | 187 ++++++++++++++--------------------
1 file changed, 79 insertions(+), 108 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 0d376d72ecc7..4a73886741f9 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2106,6 +2106,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 =
&state->stream_configs.configs[i];
@@ -2136,51 +2151,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)
{
@@ -2189,21 +2159,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
@@ -2231,9 +2213,18 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,

already_streaming = v4l2_subdev_is_streaming(sd);

- /* Call the .enable_streams() operation. */
- ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
- streams_mask);
+ if (!use_s_stream) {
+ /* 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;
+ }
+
if (ret) {
dev_dbg(dev, "enable streams %u:%#llx failed: %d\n", pad,
streams_mask, ret);
@@ -2243,34 +2234,32 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
/* Mark the streams as enabled. */
v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, true);

- if (!already_streaming)
+ if (!use_s_stream && !already_streaming)
v4l2_subdev_enable_privacy_led(sd);

done:
- v4l2_subdev_unlock_state(state);
+ if (!use_s_stream)
+ 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;

/*
@@ -2280,46 +2269,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
@@ -2345,9 +2304,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);
@@ -2357,10 +2326,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

2024-04-10 12:42:42

[permalink] [raw]

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

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 13957543d153..4a531c2b16c4 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2133,7 +2133,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);

@@ -2250,7 +2250,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

2024-04-11 04:43:33

[permalink] [raw]

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

Hi Tomi,

Thank you for the patch.

On 10/04/24 6:05 pm, 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]>

Reviewed-by: Umang Jain <[email protected]>

> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 30 +++++++++++++++++++++---------
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 012b757eac9f..13957543d153 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 &&
> @@ -422,15 +439,10 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> if (!ret) {
> sd->enabled_streams = enable ? BIT(0) : 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);
> }
>
> return ret;
>

2024-04-11 04:43:58

[permalink] [raw]

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

Hi Tomi,

Thank you for the patch.

On 10/04/24 6:05 pm, Tomi Valkeinen wrote:
> Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams().
>
> Signed-off-by: Tomi Valkeinen <[email protected]>

Reviewed-by: Umang Jain <[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 13957543d153..4a531c2b16c4 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2133,7 +2133,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);
>
> @@ -2250,7 +2250,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);
>
>

2024-04-11 05:01:19

[permalink] [raw]

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

Hi Tomi,

Thank you for the patch.

On 10/04/24 6:05 pm, 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]>

Reviewed-by: Umang Jain <[email protected]>

> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 20b5a00cbeeb..f44aaa4e1fab 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2150,6 +2150,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;
> @@ -2198,6 +2199,8 @@ 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);
> +
> /* Call the .enable_streams() operation. */
> ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
> streams_mask);
> @@ -2216,6 +2219,9 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> cfg->enabled = true;
> }
>
> + if (!already_streaming)
> + v4l2_subdev_enable_privacy_led(sd);
> +
> done:
> v4l2_subdev_unlock_state(state);
>
> @@ -2340,6 +2346,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;
>

2024-04-11 05:16:56

[permalink] [raw]

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

Hi Tomi,

Thank you for the patch

On 10/04/24 6:05 pm, 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().

Just a suggestion, as this is only used in call_s_stream(), maybe naming
this field 's_stream_enabled' ?

Rest looks good to me,

Reviewed-by: Umang Jain <[email protected]>

>
> 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 606a909cd778..6cd353d83dfc 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;
>
> ret = sd->ops->video->s_stream(sd, enable);
> @@ -437,7 +433,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;
>
> if (enable)
> v4l2_subdev_enable_privacy_led(sd);
> 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;
> };
>
>
>

2024-04-11 05:35:07

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] media: subdev: Add checks for subdev features

Hi Tomi,

On 10/04/24 6:05 pm, Tomi Valkeinen wrote:
> 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]>

The code looks aligned with the restrictions mentioned in the commit
message.

Only one question in case 3), does the .s_stream() needs to be helper
function I think, can we (or do we) need to check that as well?

Reviewed-by: Umang Jain <[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 4a531c2b16c4..606a909cd778 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1533,6 +1533,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))
>

2024-04-11 06:18:39

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] media: subdev: Add checks for subdev features

On 11/04/2024 08:34, Umang Jain wrote:
> Hi Tomi,
>
> On 10/04/24 6:05 pm, Tomi Valkeinen wrote:
>> 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]>
>
> The code looks aligned with the restrictions mentioned in the commit
> message.
>
> Only one question in case 3), does the .s_stream() needs to be helper
> function I think, can we (or do we) need to check that as well?

Do you mean if in case 3, the s_stream should always be set to
v4l2_subdev_s_stream_helper()?

I don't think so. The helper only works for subdevices with a single
source pad. And even if the helper worked for multiple source pads, I
don't see any specific reason to prevent the drivers from having their
own implementation.

Tomi

> Reviewed-by: Umang Jain <[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 4a531c2b16c4..606a909cd778 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -1533,6 +1533,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))
>>
>

2024-04-11 06:19:18

[permalink] [raw]

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

On 11/04/2024 08:16, Umang Jain wrote:
> Hi Tomi,
>
> Thank you for the patch
>
> On 10/04/24 6:05 pm, 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().
>
> Just a suggestion, as this is only used in call_s_stream(), maybe naming
> this field 's_stream_enabled' ?

Yes, I think that's a good idea.

Tomi

> Rest looks good to me,
>
> Reviewed-by: Umang Jain <[email protected]>
>
>>
>> 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 606a909cd778..6cd353d83dfc 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;
>>       ret = sd->ops->video->s_stream(sd, enable);
>> @@ -437,7 +433,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;
>>           if (enable)
>>               v4l2_subdev_enable_privacy_led(sd);
>> 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;
>> };
>>
>

2024-04-11 11:07:52

[permalink] [raw]

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

On 11/04/2024 14:02, Umang Jain wrote:
> Hi Tomi,
>
> On 10/04/24 6:05 pm, Tomi Valkeinen wrote:
>> 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:
>
> Does this mean the patch needs to be worked upon more ?

I don't see the two issues below as blockers.

> I quickly tested the series by applying it locally with my use case of
> IMX283 .enable/disable streams and s_stream as the helper function and
> it seems I am still seeing the same behaviour as before (i.e. not being
> streamed) and have to carry the workaround as mentioned in [1] **NOTE**

Ok... Then something bugs here, as it is supposed to fix the problem.
Can you trace the code a bit to see where it goes wrong?

The execution should go to the "if (!(sd->flags &
V4L2_SUBDEV_FL_STREAMS))" blocks in v4l2_subdev_collect_streams() and
v4l2_subdev_set_streams_enabled(),

Tomi

>
> [1]
> https://lore.kernel.org/linux-media/[email protected]/
>
>>
>> - 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 | 187
>> ++++++++++++++--------------------
>> 1 file changed, 79 insertions(+), 108 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
>> b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 0d376d72ecc7..4a73886741f9 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -2106,6 +2106,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 =
>>               &state->stream_configs.configs[i];
>> @@ -2136,51 +2151,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)
>> {
>> @@ -2189,21 +2159,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
>> @@ -2231,9 +2213,18 @@ int v4l2_subdev_enable_streams(struct
>> v4l2_subdev *sd, u32 pad,
>>       already_streaming = v4l2_subdev_is_streaming(sd);
>> -    /* Call the .enable_streams() operation. */
>> -    ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
>> -                   streams_mask);
>> +    if (!use_s_stream) {
>> +        /* 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;
>> +    }
>> +
>>       if (ret) {
>>           dev_dbg(dev, "enable streams %u:%#llx failed: %d\n", pad,
>>               streams_mask, ret);
>> @@ -2243,34 +2234,32 @@ int v4l2_subdev_enable_streams(struct
>> v4l2_subdev *sd, u32 pad,
>>       /* Mark the streams as enabled. */
>>       v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask,
>> true);
>> -    if (!already_streaming)
>> +    if (!use_s_stream && !already_streaming)
>>           v4l2_subdev_enable_privacy_led(sd);
>> done:
>> -    v4l2_subdev_unlock_state(state);
>> +    if (!use_s_stream)
>> +        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;
>>       /*
>> @@ -2280,46 +2269,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
>> @@ -2345,9 +2304,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);
>> @@ -2357,10 +2326,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;
>> }
>>
>

2024-04-11 11:16:56

[permalink] [raw]

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

Hi Tomi,

On 10/04/24 6:05 pm, Tomi Valkeinen wrote:
> 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:

Does this mean the patch needs to be worked upon more ?

I quickly tested the series by applying it locally with my use case of
IMX283 .enable/disable streams and s_stream as the helper function and
it seems I am still seeing the same behaviour as before (i.e. not being
streamed) and have to carry the workaround as mentioned in [1] **NOTE**

[1]
https://lore.kernel.org/linux-media/[email protected]/

>
> - 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 | 187 ++++++++++++++--------------------
> 1 file changed, 79 insertions(+), 108 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 0d376d72ecc7..4a73886741f9 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2106,6 +2106,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 =
> &state->stream_configs.configs[i];
> @@ -2136,51 +2151,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)
> {
> @@ -2189,21 +2159,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
> @@ -2231,9 +2213,18 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>
> already_streaming = v4l2_subdev_is_streaming(sd);
>
> - /* Call the .enable_streams() operation. */
> - ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
> - streams_mask);
> + if (!use_s_stream) {
> + /* 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;
> + }
> +
> if (ret) {
> dev_dbg(dev, "enable streams %u:%#llx failed: %d\n", pad,
> streams_mask, ret);
> @@ -2243,34 +2234,32 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> /* Mark the streams as enabled. */
> v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, true);
>
> - if (!already_streaming)
> + if (!use_s_stream && !already_streaming)
> v4l2_subdev_enable_privacy_led(sd);
>
> done:
> - v4l2_subdev_unlock_state(state);
> + if (!use_s_stream)
> + 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;
>
> /*
> @@ -2280,46 +2269,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
> @@ -2345,9 +2304,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);
> @@ -2357,10 +2326,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;
> }
>

2024-04-11 11:54:33

[permalink] [raw]

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

On 11/04/2024 14:48, Umang Jain wrote:
> Hi Tomi,
>
> On 11/04/24 4:37 pm, Tomi Valkeinen wrote:
>> On 11/04/2024 14:02, Umang Jain wrote:
>>> Hi Tomi,
>>>
>>> On 10/04/24 6:05 pm, Tomi Valkeinen wrote:
>>>> 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:
>>>
>>> Does this mean the patch needs to be worked upon more ?
>>
>> I don't see the two issues below as blockers.
>>
>>> I quickly tested the series by applying it locally with my use case
>>> of IMX283 .enable/disable streams and s_stream as the helper function
>>> and it seems I am still seeing the same behaviour as before (i.e. not
>>> being streamed) and have to carry the workaround as mentioned in [1]
>>> **NOTE**
>>
>> Ok... Then something bugs here, as it is supposed to fix the problem.
>> Can you trace the code a bit to see where it goes wrong?
>>
>> The execution should go to the "if (!(sd->flags &
>> V4L2_SUBDEV_FL_STREAMS))" blocks in v4l2_subdev_collect_streams() and
>> v4l2_subdev_set_streams_enabled(),
>
> The execution is not reaching in v4l2_subdev_collect streams() even, it
> returns at
>
> if (!streams_mask)
> return 0;
>
> in v4l2_subdev_enable_streams()
>
> Refer to : https://paste.debian.net/1313760/
>
> My tree is based on v6.8 currently, but the series applies cleanly, so I
> have not introduced any rebase artifacts. If you think, v6.8 might be
> causing issues, I'll then try to test on RPi 5 with the latest media
> tree perhaps.

So who is calling the v4l2_subdev_enable_streams? I presume it comes
from v4l2_subdev_s_stream_helper(), in other words the sink side in your
pipeline is using legacy s_stream?

Indeed, that helper still needs work. It needs to detect if there's no
routing, and use the implicit stream 0. I missed that one.

Tomi

2024-04-11 12:00:33

[permalink] [raw]

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

Hi Tomi,

On 11/04/24 4:37 pm, Tomi Valkeinen wrote:
> On 11/04/2024 14:02, Umang Jain wrote:
>> Hi Tomi,
>>
>> On 10/04/24 6:05 pm, Tomi Valkeinen wrote:
>>> 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:
>>
>> Does this mean the patch needs to be worked upon more ?
>
> I don't see the two issues below as blockers.
>
>> I quickly tested the series by applying it locally with my use case
>> of IMX283 .enable/disable streams and s_stream as the helper function
>> and it seems I am still seeing the same behaviour as before (i.e. not
>> being streamed) and have to carry the workaround as mentioned in [1]
>> **NOTE**
>
> Ok... Then something bugs here, as it is supposed to fix the problem.
> Can you trace the code a bit to see where it goes wrong?
>
> The execution should go to the "if (!(sd->flags &
> V4L2_SUBDEV_FL_STREAMS))" blocks in v4l2_subdev_collect_streams() and
> v4l2_subdev_set_streams_enabled(),

The execution is not reaching in v4l2_subdev_collect streams() even, it
returns at

    if (!streams_mask)
                return 0;

in v4l2_subdev_enable_streams()

Refer to : https://paste.debian.net/1313760/

My tree is based on v6.8 currently, but the series applies cleanly, so I
have not introduced any rebase artifacts. If you think, v6.8 might be
causing issues, I'll then try to test on RPi 5 with the latest media
tree perhaps.

>
> Tomi
>
>>
>> [1]
>> https://lore.kernel.org/linux-media/[email protected]/
>>
>>>
>>> - 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 | 187
>>> ++++++++++++++--------------------
>>> 1 file changed, 79 insertions(+), 108 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
>>> b/drivers/media/v4l2-core/v4l2-subdev.c
>>> index 0d376d72ecc7..4a73886741f9 100644
>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>> @@ -2106,6 +2106,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 =
>>>               &state->stream_configs.configs[i];
>>> @@ -2136,51 +2151,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)
>>> {
>>> @@ -2189,21 +2159,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
>>> @@ -2231,9 +2213,18 @@ int v4l2_subdev_enable_streams(struct
>>> v4l2_subdev *sd, u32 pad,
>>>       already_streaming = v4l2_subdev_is_streaming(sd);
>>> -    /* Call the .enable_streams() operation. */
>>> -    ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
>>> -                   streams_mask);
>>> +    if (!use_s_stream) {
>>> +        /* 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;
>>> +    }
>>> +
>>>       if (ret) {
>>>           dev_dbg(dev, "enable streams %u:%#llx failed: %d\n", pad,
>>>               streams_mask, ret);
>>> @@ -2243,34 +2234,32 @@ int v4l2_subdev_enable_streams(struct
>>> v4l2_subdev *sd, u32 pad,
>>>       /* Mark the streams as enabled. */
>>>       v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask,
>>> true);
>>> -    if (!already_streaming)
>>> +    if (!use_s_stream && !already_streaming)
>>>           v4l2_subdev_enable_privacy_led(sd);
>>> done:
>>> -    v4l2_subdev_unlock_state(state);
>>> +    if (!use_s_stream)
>>> +        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;
>>>       /*
>>> @@ -2280,46 +2269,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
>>> @@ -2345,9 +2304,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);
>>> @@ -2357,10 +2326,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;
>>> }
>>>
>>
>

2024-04-11 12:09:14

[permalink] [raw]

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

Hi Tomi

On 11/04/24 5:23 pm, Tomi Valkeinen wrote:
> On 11/04/2024 14:48, Umang Jain wrote:
>> Hi Tomi,
>>
>> On 11/04/24 4:37 pm, Tomi Valkeinen wrote:
>>> On 11/04/2024 14:02, Umang Jain wrote:
>>>> Hi Tomi,
>>>>
>>>> On 10/04/24 6:05 pm, Tomi Valkeinen wrote:
>>>>> 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:
>>>>
>>>> Does this mean the patch needs to be worked upon more ?
>>>
>>> I don't see the two issues below as blockers.
>>>
>>>> I quickly tested the series by applying it locally with my use case
>>>> of IMX283 .enable/disable streams and s_stream as the helper
>>>> function and it seems I am still seeing the same behaviour as
>>>> before (i.e. not being streamed) and have to carry the workaround
>>>> as mentioned in [1] **NOTE**
>>>
>>> Ok... Then something bugs here, as it is supposed to fix the
>>> problem. Can you trace the code a bit to see where it goes wrong?
>>>
>>> The execution should go to the "if (!(sd->flags &
>>> V4L2_SUBDEV_FL_STREAMS))" blocks in v4l2_subdev_collect_streams()
>>> and v4l2_subdev_set_streams_enabled(),
>>
>> The execution is not reaching in v4l2_subdev_collect streams() even,
>> it returns at
>>
>>      if (!streams_mask)
>>                  return 0;
>>
>> in v4l2_subdev_enable_streams()
>>
>> Refer to : https://paste.debian.net/1313760/
>>
>> My tree is based on v6.8 currently, but the series applies cleanly,
>> so I have not introduced any rebase artifacts. If you think, v6.8
>> might be causing issues, I'll then try to test on RPi 5 with the
>> latest media tree perhaps.
>
> So who is calling the v4l2_subdev_enable_streams? I presume it comes
> from v4l2_subdev_s_stream_helper(), in other words the sink side in
> your pipeline is using legacy s_stream?

Yes it comes from the helper function

static const struct v4l2_subdev_video_ops imx283_video_ops = {
        .s_stream = v4l2_subdev_s_stream_helper,
};

>
> Indeed, that helper still needs work. It needs to detect if there's no
> routing, and use the implicit stream 0. I missed that one.

Yes, no routing in the driver.
>
> Tomi
>

2024-04-12 04:02:48

[permalink] [raw]

Subject: Re: [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming()

Hi Tomi,

Thank you for the patch

On 10/04/24 6:05 pm, Tomi Valkeinen wrote:
> 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 3d2c9c224b8f..20b5a00cbeeb 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2419,6 +2419,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;

With this field named s_stream_enabled as per the comment in one of the
previous patch,

Reviewed-by: Umang Jain <[email protected]>

> +
> + 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 */
>

2024-04-12 17:38:52

[permalink] [raw]

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

Hi Tomi,

Thank you for the patch.

On Wed, Apr 10, 2024 at 03:35:48PM +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 | 30 +++++++++++++++++++++---------
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 012b757eac9f..13957543d153 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
> +}

I would have written this as

#if IS_REACHABLE(CONFIG_LEDS_CLASS)
static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd)
{
if (!IS_ERR_OR_NULL(sd->privacy_led))
led_set_brightness(sd->privacy_led,
sd->privacy_led->max_brightness);
}

static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd)
{
if (!IS_ERR_OR_NULL(sd->privacy_led))
led_set_brightness(sd->privacy_led, 0);
}
#else
static inline void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd)
{
}

static inline void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd)
{
}
#endif /* CONFIG_LEDS_CLASS */

to avoid multipe #if but that likely makes no difference in the
generated code. Either way,

Reviewed-by: Laurent Pinchart <[email protected]>

> +
> static inline int check_which(u32 which)
> {
> if (which != V4L2_SUBDEV_FORMAT_TRY &&
> @@ -422,15 +439,10 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> if (!ret) {
> sd->enabled_streams = enable ? BIT(0) : 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);
> }
>
> return ret;
>

--
Regards,

Laurent Pinchart

2024-04-12 17:39:16

[permalink] [raw]

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

Hi Tomi,

Thank you for the patch.

On Wed, Apr 10, 2024 at 03:35:49PM +0300, Tomi Valkeinen wrote:
> Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams().

Commit messages should explain the reaon for a change. With that,

Reviewed-by: Laurent Pinchart <[email protected]>

> 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 13957543d153..4a531c2b16c4 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2133,7 +2133,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);
>
> @@ -2250,7 +2250,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);
>
>

--
Regards,

Laurent Pinchart

2024-04-12 17:53:33

[permalink] [raw]

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

Hi Tomi,

Thank you for the patch.

On Wed, Apr 10, 2024 at 03:35:51PM +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]>

Reviewed-by: Laurent Pinchart <[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 606a909cd778..6cd353d83dfc 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;
>
> ret = sd->ops->video->s_stream(sd, enable);
> @@ -437,7 +433,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;
>
> if (enable)
> v4l2_subdev_enable_privacy_led(sd);
> 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;
> };
>
>

--
Regards,

Laurent Pinchart

2024-04-12 18:02:58

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] media: subdev: Add checks for subdev features

Hi Tomi,

Thank you for the patch.

On Wed, Apr 10, 2024 at 03:35:50PM +0300, Tomi Valkeinen wrote:
> 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]>

Reviewed-by: Laurent Pinchart <[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 4a531c2b16c4..606a909cd778 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1533,6 +1533,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))
>

--
Regards,

Laurent Pinchart

2024-04-12 18:13:30

[permalink] [raw]

Subject: Re: [PATCH v3 5/9] media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback

Hi Tomi,

Thank you for the patch.

On Wed, Apr 10, 2024 at 03:35:52PM +0300, Tomi Valkeinen wrote:
> 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 6cd353d83dfc..3d2c9c224b8f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2104,37 +2104,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

s/only/the only/

> + * 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;
> }
> @@ -2221,37 +2227,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

Same here.

Reviewed-by: Laurent Pinchart <[email protected]>

> + * 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;
> };
>
>

--
Regards,

Laurent Pinchart

2024-04-12 18:16:21

[permalink] [raw]

Subject: Re: [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming()

Hi Tomi,

Thank you for the patch.

On Wed, Apr 10, 2024 at 03:35:53PM +0300, Tomi Valkeinen wrote:
> 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 3d2c9c224b8f..20b5a00cbeeb 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2419,6 +2419,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;

I think this can be moved above the
v4l2_subdev_get_locked_active_state() call.

Reviewed-by: Laurent Pinchart <[email protected]>

Any plan to convert drivers to this ?

> +
> + 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 */
>

--
Regards,

Laurent Pinchart

2024-04-12 18:20:51

[permalink] [raw]

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

Hi Tomi,

Thank you for the patch.

On Wed, Apr 10, 2024 at 03:35:54PM +0300, Tomi Valkeinen wrote:
> We support camera privacy leds with the .s_stream, in call_s_stream, but

s/the .s_stream/the .s_stream() operation/

> 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.

I wonder if that will always be the correct constraint for all devices,
but I suppose we can worry about it later.

> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 20b5a00cbeeb..f44aaa4e1fab 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2150,6 +2150,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;
> @@ -2198,6 +2199,8 @@ 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);
> +
> /* Call the .enable_streams() operation. */
> ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
> streams_mask);
> @@ -2216,6 +2219,9 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> cfg->enabled = true;
> }
>
> + if (!already_streaming)
> + v4l2_subdev_enable_privacy_led(sd);
> +
> done:
> v4l2_subdev_unlock_state(state);
>
> @@ -2340,6 +2346,9 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
> }
>
> done:
> + if (!v4l2_subdev_is_streaming(sd))

Wouldn't it be more efficient to check this while looping over the
stream configs in the loop just above ? Same for
v4l2_subdev_enable_streams().

> + v4l2_subdev_disable_privacy_led(sd);
> +
> v4l2_subdev_unlock_state(state);
>
> return ret;
>

--
Regards,

Laurent Pinchart

2024-04-12 18:52:21

[permalink] [raw]

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

Hi Tomi,

Thank you for the patch.

On Wed, Apr 10, 2024 at 03:35:55PM +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]>

Reviewed-by: Laurent Pinchart <[email protected]>

> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 109 +++++++++++++++++++---------------
> 1 file changed, 60 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index f44aaa4e1fab..0d376d72ecc7 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2100,6 +2100,42 @@ 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 =
> + &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 =
> + &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)
> {
> @@ -2151,8 +2187,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. */
> @@ -2173,22 +2209,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",
> @@ -2197,6 +2220,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);
> @@ -2211,13 +2241,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);
>
> if (!already_streaming)
> v4l2_subdev_enable_privacy_led(sd);
> @@ -2279,8 +2303,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. */
> @@ -2301,22 +2325,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",
> @@ -2325,6 +2336,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. */
> @@ -2336,14 +2354,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))
>

--
Regards,

Laurent Pinchart

2024-04-16 10:28:20

[permalink] [raw]

Subject: Re: [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming()

On 12/04/2024 21:15, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Wed, Apr 10, 2024 at 03:35:53PM +0300, Tomi Valkeinen wrote:
>> 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 3d2c9c224b8f..20b5a00cbeeb 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -2419,6 +2419,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;
>
> I think this can be moved above the
> v4l2_subdev_get_locked_active_state() call.

Yep. I think I originally thought that it'll be nice to get a warning
here if the state is not locked, but perhaps that's pointless.

> Reviewed-by: Laurent Pinchart <[email protected]>
>
> Any plan to convert drivers to this ?

No. Afaics, it will be per-driver manual work, no way to automate it.

Tomi

2024-04-16 10:34:41

[permalink] [raw]

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

On 12/04/2024 21:20, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Wed, Apr 10, 2024 at 03:35:54PM +0300, Tomi Valkeinen wrote:
>> We support camera privacy leds with the .s_stream, in call_s_stream, but
>
> s/the .s_stream/the .s_stream() operation/
>
>> 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.
>
> I wonder if that will always be the correct constraint for all devices,
> but I suppose we can worry about it later.
>
>> Signed-off-by: Tomi Valkeinen <[email protected]>
>> ---
>> drivers/media/v4l2-core/v4l2-subdev.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 20b5a00cbeeb..f44aaa4e1fab 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -2150,6 +2150,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;
>> @@ -2198,6 +2199,8 @@ 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);
>> +
>> /* Call the .enable_streams() operation. */
>> ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad,
>> streams_mask);
>> @@ -2216,6 +2219,9 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>> cfg->enabled = true;
>> }
>>
>> + if (!already_streaming)
>> + v4l2_subdev_enable_privacy_led(sd);
>> +
>> done:
>> v4l2_subdev_unlock_state(state);
>>
>> @@ -2340,6 +2346,9 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>> }
>>
>> done:
>> + if (!v4l2_subdev_is_streaming(sd))
>
> Wouldn't it be more efficient to check this while looping over the
> stream configs in the loop just above ? Same for
> v4l2_subdev_enable_streams().

It would, but it would get a lot messier to manage with "media: subdev:
Refactor v4l2_subdev_enable/disable_streams()", and we would also need
to support the non-routing case.

This is usually a loop with a couple of iterations, and only called when
enabling or enabling a subdevice, so I'm not really worried about the
performance. If it's an issue, it would probably be better to also
update the sd->enabled_pads when enabling/disabling a stream.

Tomi

2024-04-19 10:22:33