* [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery
@ 2024-04-10 12:35 Tomi Valkeinen
2024-04-10 12:35 ` [PATCH v3 1/9] media: subdev: Add privacy led helpers Tomi Valkeinen
` (8 more replies)
0 siblings, 9 replies; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-10 12:35 UTC (permalink / raw
To: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Umang Jain
Cc: linux-media, linux-kernel, Tomi Valkeinen
This series works on the .s_stream, .enable_streams, .disable_streams
related code. The main feature introduced here, in the last patch, is
adding support for .enable/disable_streams() for subdevs that do not
implement full streams support.
Additionally we add support for the privacy led when
v4l2_subdev_enable/disable_streams() is used.
I have tested this on RPi5.
Tomi
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Changes in v3:
- Rebased on top of "[PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails"
- Drop extra "!!" in "media: subdev: Fix use of sd->enabled_streams in call_s_stream()"
- Enable privacy LED after a succesfull stream enable in "media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams()"
- Init 'cfg' variable when declaring in "media: subdev: Refactor v4l2_subdev_enable/disable_streams()"
- Link to v2: https://lore.kernel.org/r/20240405-enable-streams-impro-v2-0-22bca967665d@ideasonboard.com
Changes in v2:
- New patches for privacy led
- Use v4l2_subdev_has_op()
- Use BITS_PER_BYTE instead of 8
- Fix 80+ line issues
- Fix typos
- Check for pad < 64 also in the non-routing .enable/disable_streams code path
- Dropped "media: subdev: Support enable/disable_streams for non-streams
subdevs", which is implemented in a different way in new patches in this series
- Link to v1: https://lore.kernel.org/r/20240404-enable-streams-impro-v1-0-1017a35bbe07@ideasonboard.com
---
Tomi Valkeinen (9):
media: subdev: Add privacy led helpers
media: subdev: Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams()
media: subdev: Add checks for subdev features
media: subdev: Fix use of sd->enabled_streams in call_s_stream()
media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback
media: subdev: Add v4l2_subdev_is_streaming()
media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams()
media: subdev: Refactor v4l2_subdev_enable/disable_streams()
media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()
drivers/media/v4l2-core/v4l2-subdev.c | 355 ++++++++++++++++++++--------------
include/media/v4l2-subdev.h | 25 ++-
2 files changed, 229 insertions(+), 151 deletions(-)
---
base-commit: 6547a87b1ffc9b3c3a17f20f71016de98c169bbb
change-id: 20240404-enable-streams-impro-db8bcd898471
Best regards,
--
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 1/9] media: subdev: Add privacy led helpers
2024-04-10 12:35 [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery Tomi Valkeinen
@ 2024-04-10 12:35 ` Tomi Valkeinen
2024-04-11 4:43 ` Umang Jain
2024-04-12 17:36 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 2/9] media: subdev: Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
` (7 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-10 12:35 UTC (permalink / raw
To: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Umang Jain
Cc: linux-media, linux-kernel, Tomi Valkeinen
Add helper functions to enable and disable the privacy led. This moves
the #if from the call site to the privacy led functions, and makes
adding privacy led support to v4l2_subdev_enable/disable_streams()
cleaner.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 2/9] media: subdev: Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams()
2024-04-10 12:35 [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery Tomi Valkeinen
2024-04-10 12:35 ` [PATCH v3 1/9] media: subdev: Add privacy led helpers Tomi Valkeinen
@ 2024-04-10 12:35 ` Tomi Valkeinen
2024-04-11 4:43 ` Umang Jain
2024-04-12 17:38 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 3/9] media: subdev: Add checks for subdev features Tomi Valkeinen
` (6 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-10 12:35 UTC (permalink / raw
To: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Umang Jain
Cc: linux-media, linux-kernel, Tomi Valkeinen
Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams().
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 3/9] media: subdev: Add checks for subdev features
2024-04-10 12:35 [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery Tomi Valkeinen
2024-04-10 12:35 ` [PATCH v3 1/9] media: subdev: Add privacy led helpers Tomi Valkeinen
2024-04-10 12:35 ` [PATCH v3 2/9] media: subdev: Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
@ 2024-04-10 12:35 ` Tomi Valkeinen
2024-04-11 5:34 ` Umang Jain
2024-04-12 17:41 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream() Tomi Valkeinen
` (5 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-10 12:35 UTC (permalink / raw
To: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Umang Jain
Cc: linux-media, linux-kernel, Tomi Valkeinen
Add some checks to verify that the subdev driver implements required
features.
A subdevice that supports streams (V4L2_SUBDEV_FL_STREAMS) must do one
of the following:
- Implement neither .enable/disable_streams() nor .s_stream(), if the
subdev is part of a video driver that uses an internal method to
enable the subdev.
- Implement only .enable/disable_streams(), if support for legacy
sink-side subdevices is not needed.
- Implement .enable/disable_streams() and .s_stream(), if support for
legacy sink-side subdevices is needed.
At the moment the framework doesn't check this requirement. Add the
check.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream()
2024-04-10 12:35 [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery Tomi Valkeinen
` (2 preceding siblings ...)
2024-04-10 12:35 ` [PATCH v3 3/9] media: subdev: Add checks for subdev features Tomi Valkeinen
@ 2024-04-10 12:35 ` Tomi Valkeinen
2024-04-11 5:16 ` Umang Jain
2024-04-12 17:53 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 5/9] media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback Tomi Valkeinen
` (4 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-10 12:35 UTC (permalink / raw
To: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Umang Jain
Cc: linux-media, linux-kernel, Tomi Valkeinen
call_s_stream() uses sd->enabled_streams to track whether streaming has
already been enabled. However,
v4l2_subdev_enable/disable_streams_fallback(), which was the original
user of this field, already uses it, and
v4l2_subdev_enable/disable_streams_fallback() will call call_s_stream().
This leads to a conflict as both functions set the field. Afaics, both
functions set the field to the same value, so it won't cause a runtime
bug, but it's still wrong and if we, e.g., change how
v4l2_subdev_enable/disable_streams_fallback() operates we might easily
cause bugs.
Fix this by adding a new field, 'streaming_enabled', for
call_s_stream().
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------
include/media/v4l2-subdev.h | 3 +++
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 5/9] media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback
2024-04-10 12:35 [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery Tomi Valkeinen
` (3 preceding siblings ...)
2024-04-10 12:35 ` [PATCH v3 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream() Tomi Valkeinen
@ 2024-04-10 12:35 ` Tomi Valkeinen
2024-04-12 18:13 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming() Tomi Valkeinen
` (3 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-10 12:35 UTC (permalink / raw
To: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Umang Jain
Cc: linux-media, linux-kernel, Tomi Valkeinen
v4l2_subdev_enable/disable_streams_fallback() supports falling back to
.s_stream() for subdevs with a single source pad. It also tracks the
enabled streams for that one pad in the sd->enabled_streams field.
Tracking the enabled streams with sd->enabled_streams does not make
sense, as with .s_stream() there can only be a single stream per pad.
Thus, as the v4l2_subdev_enable/disable_streams_fallback() only supports
a single source pad, all we really need is a boolean which tells whether
streaming has been enabled on this pad or not.
However, as we only need a true/false state for a pad (instead of
tracking which streams have been enabled for a pad), we can easily
extend the fallback mechanism to support multiple source pads as we only
need to keep track of which pads have been enabled.
Change the sd->enabled_streams field to sd->enabled_pads, which is a
64-bit bitmask tracking the enabled source pads. With this change we can
remove the restriction that
v4l2_subdev_enable/disable_streams_fallback() only supports a single
source pad.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 68 ++++++++++++++++++++---------------
include/media/v4l2-subdev.h | 9 +++--
2 files changed, 44 insertions(+), 33 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming()
2024-04-10 12:35 [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery Tomi Valkeinen
` (4 preceding siblings ...)
2024-04-10 12:35 ` [PATCH v3 5/9] media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback Tomi Valkeinen
@ 2024-04-10 12:35 ` Tomi Valkeinen
2024-04-12 4:02 ` Umang Jain
2024-04-12 18:15 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
` (2 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-10 12:35 UTC (permalink / raw
To: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Umang Jain
Cc: linux-media, linux-kernel, Tomi Valkeinen
Add a helper function which returns whether the subdevice is streaming,
i.e. if .s_stream or .enable_streams has been called successfully.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++
include/media/v4l2-subdev.h | 13 +++++++++++++
2 files changed, 38 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams()
2024-04-10 12:35 [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery Tomi Valkeinen
` (5 preceding siblings ...)
2024-04-10 12:35 ` [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming() Tomi Valkeinen
@ 2024-04-10 12:35 ` Tomi Valkeinen
2024-04-11 4:58 ` Umang Jain
2024-04-12 18:20 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 8/9] media: subdev: Refactor v4l2_subdev_enable/disable_streams() Tomi Valkeinen
2024-04-10 12:35 ` [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
8 siblings, 2 replies; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-10 12:35 UTC (permalink / raw
To: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Umang Jain
Cc: linux-media, linux-kernel, Tomi Valkeinen
We support camera privacy leds with the .s_stream, in call_s_stream, but
we don't have that support when the subdevice implements
.enable/disable_streams.
Add the support by enabling the led when the first stream for a
subdevice is enabled, and disabling the led then the last stream is
disabled.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 8/9] media: subdev: Refactor v4l2_subdev_enable/disable_streams()
2024-04-10 12:35 [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery Tomi Valkeinen
` (6 preceding siblings ...)
2024-04-10 12:35 ` [PATCH v3 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
@ 2024-04-10 12:35 ` Tomi Valkeinen
2024-04-12 18:45 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
8 siblings, 1 reply; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-10 12:35 UTC (permalink / raw
To: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Umang Jain
Cc: linux-media, linux-kernel, Tomi Valkeinen
Add two internal helper functions, v4l2_subdev_collect_streams() and
v4l2_subdev_set_streams_enabled(), which allows us to refactor
v4l2_subdev_enable/disable_streams() functions.
This (I think) makes the code a bit easier to read, and lets us more
easily add new functionality in the helper functions in the following
patch.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()
2024-04-10 12:35 [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery Tomi Valkeinen
` (7 preceding siblings ...)
2024-04-10 12:35 ` [PATCH v3 8/9] media: subdev: Refactor v4l2_subdev_enable/disable_streams() Tomi Valkeinen
@ 2024-04-10 12:35 ` Tomi Valkeinen
2024-04-11 11:02 ` Umang Jain
8 siblings, 1 reply; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-10 12:35 UTC (permalink / raw
To: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Umang Jain
Cc: linux-media, linux-kernel, Tomi Valkeinen
At the moment the v4l2_subdev_enable/disable_streams() functions call
fallback helpers to handle the case where the subdev only implements
.s_stream(), and the main function handles the case where the subdev
implements streams (V4L2_SUBDEV_FL_STREAMS, which implies
.enable/disable_streams()).
What is missing is support for subdevs which do not implement streams
support, but do implement .enable/disable_streams(). Example cases of
these subdevices are single-stream cameras, where using
.enable/disable_streams() is not required but helps us remove the users
of the legacy .s_stream(), and subdevices with multiple source pads (but
single stream per pad), where .enable/disable_streams() allows the
subdevice to control the enable/disable state per pad.
The two single-streams cases (.s_stream() and .enable/disable_streams())
are very similar, and with small changes we can change the
v4l2_subdev_enable/disable_streams() functions to support all three
cases, without needing separate fallback functions.
A few potentially problematic details, though:
- For the single-streams cases we use sd->enabled_pads field, which
limits the number of pads for the subdevice to 64. For simplicity I
added the check for this limitation to the beginning of the function,
and it also applies to the streams case.
- The fallback functions only allowed the target pad to be a source pad.
It is not very clear to me why this check was needed, but it was not
needed in the streams case. However, I doubt the
v4l2_subdev_enable/disable_streams() code has ever been tested with
sink pads, so to be on the safe side, I added the same check
to the v4l2_subdev_enable/disable_streams() functions.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v3 1/9] media: subdev: Add privacy led helpers
2024-04-10 12:35 ` [PATCH v3 1/9] media: subdev: Add privacy led helpers Tomi Valkeinen
@ 2024-04-11 4:43 ` Umang Jain
2024-04-12 17:36 ` Laurent Pinchart
1 sibling, 0 replies; 34+ messages in thread
From: Umang Jain @ 2024-04-11 4:43 UTC (permalink / raw
To: Tomi Valkeinen, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
> 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;
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 2/9] media: subdev: Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams()
2024-04-10 12:35 ` [PATCH v3 2/9] media: subdev: Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
@ 2024-04-11 4:43 ` Umang Jain
2024-04-12 17:38 ` Laurent Pinchart
1 sibling, 0 replies; 34+ messages in thread
From: Umang Jain @ 2024-04-11 4:43 UTC (permalink / raw
To: Tomi Valkeinen, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 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);
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams()
2024-04-10 12:35 ` [PATCH v3 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
@ 2024-04-11 4:58 ` Umang Jain
2024-04-12 18:20 ` Laurent Pinchart
1 sibling, 0 replies; 34+ messages in thread
From: Umang Jain @ 2024-04-11 4:58 UTC (permalink / raw
To: Tomi Valkeinen, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
> 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;
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream()
2024-04-10 12:35 ` [PATCH v3 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream() Tomi Valkeinen
@ 2024-04-11 5:16 ` Umang Jain
2024-04-11 6:19 ` Tomi Valkeinen
2024-04-12 17:53 ` Laurent Pinchart
1 sibling, 1 reply; 34+ messages in thread
From: Umang Jain @ 2024-04-11 5:16 UTC (permalink / raw
To: Tomi Valkeinen, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: linux-media, linux-kernel
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 <umang.jain@ideasonboard.com>
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------
> include/media/v4l2-subdev.h | 3 +++
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 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;
> };
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/9] media: subdev: Add checks for subdev features
2024-04-10 12:35 ` [PATCH v3 3/9] media: subdev: Add checks for subdev features Tomi Valkeinen
@ 2024-04-11 5:34 ` Umang Jain
2024-04-11 6:18 ` Tomi Valkeinen
2024-04-12 17:41 ` Laurent Pinchart
1 sibling, 1 reply; 34+ messages in thread
From: Umang Jain @ 2024-04-11 5:34 UTC (permalink / raw
To: Tomi Valkeinen, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
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 <umang.jain@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 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))
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/9] media: subdev: Add checks for subdev features
2024-04-11 5:34 ` Umang Jain
@ 2024-04-11 6:18 ` Tomi Valkeinen
0 siblings, 0 replies; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-11 6:18 UTC (permalink / raw
To: Umang Jain
Cc: linux-media, linux-kernel, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
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 <tomi.valkeinen@ideasonboard.com>
>
> 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 <umang.jain@ideasonboard.com>
>
>> ---
>> drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
>> b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 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))
>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream()
2024-04-11 5:16 ` Umang Jain
@ 2024-04-11 6:19 ` Tomi Valkeinen
0 siblings, 0 replies; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-11 6:19 UTC (permalink / raw
To: Umang Jain
Cc: linux-media, linux-kernel, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
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 <umang.jain@ideasonboard.com>
>
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>> drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------
>> include/media/v4l2-subdev.h | 3 +++
>> 2 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
>> b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 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;
>> };
>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()
2024-04-10 12:35 ` [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
@ 2024-04-11 11:02 ` Umang Jain
2024-04-11 11:07 ` Tomi Valkeinen
0 siblings, 1 reply; 34+ messages in thread
From: Umang Jain @ 2024-04-11 11:02 UTC (permalink / raw
To: Tomi Valkeinen, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: linux-media, linux-kernel
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/20240313070705.91140-1-umang.jain@ideasonboard.com/
>
> - For the single-streams cases we use sd->enabled_pads field, which
> limits the number of pads for the subdevice to 64. For simplicity I
> added the check for this limitation to the beginning of the function,
> and it also applies to the streams case.
>
> - The fallback functions only allowed the target pad to be a source pad.
> It is not very clear to me why this check was needed, but it was not
> needed in the streams case. However, I doubt the
> v4l2_subdev_enable/disable_streams() code has ever been tested with
> sink pads, so to be on the safe side, I added the same check
> to the v4l2_subdev_enable/disable_streams() functions.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 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;
> }
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()
2024-04-11 11:02 ` Umang Jain
@ 2024-04-11 11:07 ` Tomi Valkeinen
2024-04-11 11:48 ` Umang Jain
0 siblings, 1 reply; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-11 11:07 UTC (permalink / raw
To: Umang Jain
Cc: linux-media, linux-kernel, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
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/20240313070705.91140-1-umang.jain@ideasonboard.com/
>
>>
>> - For the single-streams cases we use sd->enabled_pads field, which
>> limits the number of pads for the subdevice to 64. For simplicity I
>> added the check for this limitation to the beginning of the function,
>> and it also applies to the streams case.
>>
>> - The fallback functions only allowed the target pad to be a source pad.
>> It is not very clear to me why this check was needed, but it was not
>> needed in the streams case. However, I doubt the
>> v4l2_subdev_enable/disable_streams() code has ever been tested with
>> sink pads, so to be on the safe side, I added the same check
>> to the v4l2_subdev_enable/disable_streams() functions.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>> drivers/media/v4l2-core/v4l2-subdev.c | 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;
>> }
>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()
2024-04-11 11:07 ` Tomi Valkeinen
@ 2024-04-11 11:48 ` Umang Jain
2024-04-11 11:53 ` Tomi Valkeinen
0 siblings, 1 reply; 34+ messages in thread
From: Umang Jain @ 2024-04-11 11:48 UTC (permalink / raw
To: Tomi Valkeinen
Cc: linux-media, linux-kernel, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
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/20240313070705.91140-1-umang.jain@ideasonboard.com/
>>
>>>
>>> - For the single-streams cases we use sd->enabled_pads field, which
>>> limits the number of pads for the subdevice to 64. For simplicity I
>>> added the check for this limitation to the beginning of the
>>> function,
>>> and it also applies to the streams case.
>>>
>>> - The fallback functions only allowed the target pad to be a source
>>> pad.
>>> It is not very clear to me why this check was needed, but it was not
>>> needed in the streams case. However, I doubt the
>>> v4l2_subdev_enable/disable_streams() code has ever been tested with
>>> sink pads, so to be on the safe side, I added the same check
>>> to the v4l2_subdev_enable/disable_streams() functions.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> ---
>>> drivers/media/v4l2-core/v4l2-subdev.c | 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;
>>> }
>>>
>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()
2024-04-11 11:48 ` Umang Jain
@ 2024-04-11 11:53 ` Tomi Valkeinen
2024-04-11 11:56 ` Umang Jain
0 siblings, 1 reply; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-11 11:53 UTC (permalink / raw
To: Umang Jain
Cc: linux-media, linux-kernel, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()
2024-04-11 11:53 ` Tomi Valkeinen
@ 2024-04-11 11:56 ` Umang Jain
0 siblings, 0 replies; 34+ messages in thread
From: Umang Jain @ 2024-04-11 11:56 UTC (permalink / raw
To: Tomi Valkeinen
Cc: linux-media, linux-kernel, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
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
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming()
2024-04-10 12:35 ` [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming() Tomi Valkeinen
@ 2024-04-12 4:02 ` Umang Jain
2024-04-12 18:15 ` Laurent Pinchart
1 sibling, 0 replies; 34+ messages in thread
From: Umang Jain @ 2024-04-12 4:02 UTC (permalink / raw
To: Tomi Valkeinen, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++
> include/media/v4l2-subdev.h | 13 +++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 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 <umang.jain@ideasonboard.com>
> +
> + 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 */
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 1/9] media: subdev: Add privacy led helpers
2024-04-10 12:35 ` [PATCH v3 1/9] media: subdev: Add privacy led helpers Tomi Valkeinen
2024-04-11 4:43 ` Umang Jain
@ 2024-04-12 17:36 ` Laurent Pinchart
1 sibling, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2024-04-12 17:36 UTC (permalink / raw
To: Tomi Valkeinen
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Umang Jain,
linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
> ---
> 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 <laurent.pinchart@ideasonboard.com>
> +
> 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 2/9] media: subdev: Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams()
2024-04-10 12:35 ` [PATCH v3 2/9] media: subdev: Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
2024-04-11 4:43 ` Umang Jain
@ 2024-04-12 17:38 ` Laurent Pinchart
1 sibling, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2024-04-12 17:38 UTC (permalink / raw
To: Tomi Valkeinen
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Umang Jain,
linux-media, linux-kernel
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 <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/9] media: subdev: Add checks for subdev features
2024-04-10 12:35 ` [PATCH v3 3/9] media: subdev: Add checks for subdev features Tomi Valkeinen
2024-04-11 5:34 ` Umang Jain
@ 2024-04-12 17:41 ` Laurent Pinchart
1 sibling, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2024-04-12 17:41 UTC (permalink / raw
To: Tomi Valkeinen
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Umang Jain,
linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream()
2024-04-10 12:35 ` [PATCH v3 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream() Tomi Valkeinen
2024-04-11 5:16 ` Umang Jain
@ 2024-04-12 17:53 ` Laurent Pinchart
1 sibling, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2024-04-12 17:53 UTC (permalink / raw
To: Tomi Valkeinen
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Umang Jain,
linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------
> include/media/v4l2-subdev.h | 3 +++
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/9] media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback
2024-04-10 12:35 ` [PATCH v3 5/9] media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback Tomi Valkeinen
@ 2024-04-12 18:13 ` Laurent Pinchart
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2024-04-12 18:13 UTC (permalink / raw
To: Tomi Valkeinen
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Umang Jain,
linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 68 ++++++++++++++++++++---------------
> include/media/v4l2-subdev.h | 9 +++--
> 2 files changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 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 <laurent.pinchart@ideasonboard.com>
> + * 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming()
2024-04-10 12:35 ` [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming() Tomi Valkeinen
2024-04-12 4:02 ` Umang Jain
@ 2024-04-12 18:15 ` Laurent Pinchart
2024-04-16 10:27 ` Tomi Valkeinen
1 sibling, 1 reply; 34+ messages in thread
From: Laurent Pinchart @ 2024-04-12 18:15 UTC (permalink / raw
To: Tomi Valkeinen
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Umang Jain,
linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++
> include/media/v4l2-subdev.h | 13 +++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 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 <laurent.pinchart@ideasonboard.com>
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams()
2024-04-10 12:35 ` [PATCH v3 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
2024-04-11 4:58 ` Umang Jain
@ 2024-04-12 18:20 ` Laurent Pinchart
2024-04-16 10:34 ` Tomi Valkeinen
1 sibling, 1 reply; 34+ messages in thread
From: Laurent Pinchart @ 2024-04-12 18:20 UTC (permalink / raw
To: Tomi Valkeinen
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Umang Jain,
linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
> ---
> 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 8/9] media: subdev: Refactor v4l2_subdev_enable/disable_streams()
2024-04-10 12:35 ` [PATCH v3 8/9] media: subdev: Refactor v4l2_subdev_enable/disable_streams() Tomi Valkeinen
@ 2024-04-12 18:45 ` Laurent Pinchart
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2024-04-12 18:45 UTC (permalink / raw
To: Tomi Valkeinen
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Umang Jain,
linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming()
2024-04-12 18:15 ` Laurent Pinchart
@ 2024-04-16 10:27 ` Tomi Valkeinen
0 siblings, 0 replies; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-16 10:27 UTC (permalink / raw
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Umang Jain,
linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
>> ---
>> drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++
>> include/media/v4l2-subdev.h | 13 +++++++++++++
>> 2 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 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 <laurent.pinchart@ideasonboard.com>
>
> Any plan to convert drivers to this ?
No. Afaics, it will be per-driver manual work, no way to automate it.
Tomi
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams()
2024-04-12 18:20 ` Laurent Pinchart
@ 2024-04-16 10:34 ` Tomi Valkeinen
2024-04-19 10:22 ` Laurent Pinchart
0 siblings, 1 reply; 34+ messages in thread
From: Tomi Valkeinen @ 2024-04-16 10:34 UTC (permalink / raw
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Umang Jain,
linux-media, linux-kernel
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 <tomi.valkeinen@ideasonboard.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams()
2024-04-16 10:34 ` Tomi Valkeinen
@ 2024-04-19 10:22 ` Laurent Pinchart
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2024-04-19 10:22 UTC (permalink / raw
To: Tomi Valkeinen
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Umang Jain,
linux-media, linux-kernel
On Tue, Apr 16, 2024 at 01:34:22PM +0300, Tomi Valkeinen wrote:
> 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 <tomi.valkeinen@ideasonboard.com>
> >> ---
> >> 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.
True.
> 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.
OK, I can live with that for now.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2024-04-19 10:22 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-10 12:35 [PATCH v3 0/9] media: subdev: Improve stream enable/disable machinery Tomi Valkeinen
2024-04-10 12:35 ` [PATCH v3 1/9] media: subdev: Add privacy led helpers Tomi Valkeinen
2024-04-11 4:43 ` Umang Jain
2024-04-12 17:36 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 2/9] media: subdev: Use v4l2_subdev_has_op() in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
2024-04-11 4:43 ` Umang Jain
2024-04-12 17:38 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 3/9] media: subdev: Add checks for subdev features Tomi Valkeinen
2024-04-11 5:34 ` Umang Jain
2024-04-11 6:18 ` Tomi Valkeinen
2024-04-12 17:41 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 4/9] media: subdev: Fix use of sd->enabled_streams in call_s_stream() Tomi Valkeinen
2024-04-11 5:16 ` Umang Jain
2024-04-11 6:19 ` Tomi Valkeinen
2024-04-12 17:53 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 5/9] media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback Tomi Valkeinen
2024-04-12 18:13 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 6/9] media: subdev: Add v4l2_subdev_is_streaming() Tomi Valkeinen
2024-04-12 4:02 ` Umang Jain
2024-04-12 18:15 ` Laurent Pinchart
2024-04-16 10:27 ` Tomi Valkeinen
2024-04-10 12:35 ` [PATCH v3 7/9] media: subdev: Support privacy led in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
2024-04-11 4:58 ` Umang Jain
2024-04-12 18:20 ` Laurent Pinchart
2024-04-16 10:34 ` Tomi Valkeinen
2024-04-19 10:22 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 8/9] media: subdev: Refactor v4l2_subdev_enable/disable_streams() Tomi Valkeinen
2024-04-12 18:45 ` Laurent Pinchart
2024-04-10 12:35 ` [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams() Tomi Valkeinen
2024-04-11 11:02 ` Umang Jain
2024-04-11 11:07 ` Tomi Valkeinen
2024-04-11 11:48 ` Umang Jain
2024-04-11 11:53 ` Tomi Valkeinen
2024-04-11 11:56 ` Umang Jain
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).