This series works on the .s_stream, .enable_streams, .disable_streams
related code. The main feature introduced here, in the last two patchs,
is adding support for .enable/disable_streams() for subdevs that do not
implement full streams support.
Additionally we add support for the privacy led when
v4l2_subdev_enable/disable_streams() is used.
I have tested this on RPi5.
Tomi
Signed-off-by: Tomi Valkeinen <[email protected]>
---
Changes in v6:
- v4l2_subdev_enable_streams() and v4l2_subdev_disable_streams()
accidentally returned -EINVAL when a stream was already
enabled/disabled. Change it back to -EALREADY.
- Add TODO note about privacy leds to "media: subdev: Support
single-stream case in v4l2_subdev_enable/disable_streams()"
- New commit "media: subdev: Improve s_stream documentation"
- Link to v5: https://lore.kernel.org/r/[email protected]
Changes in v5:
- Fix issues with non-routing case:
- Always set 'found_streams' variable instead of leaving it
uninitialized in "Support single-stream case in
v4l2_subdev_enable/disable_streams()"
- Fix the "implicit" stream from bit 1 to bit 0 in "Support
non-routing subdevs in v4l2_subdev_s_stream_helper()"
- Link to v4: https://lore.kernel.org/r/[email protected]
Changes in v4:
- Added Rb tags
- Rename 'streaming_enabled' to 's_stream_enabled'
- Cosmetic changes (comments / patch descs)
- Added new patch "media: subdev: Support non-routing subdevs in v4l2_subdev_s_stream_helper()".
- Link to v3: https://lore.kernel.org/r/[email protected]
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/[email protected]
Changes in v2:
- New patches for privacy led
- Use v4l2_subdev_has_op()
- Use BITS_PER_BYTE instead of 8
- Fix 80+ line issues
- Fix typos
- Check for pad < 64 also in the non-routing .enable/disable_streams code path
- Dropped "media: subdev: Support enable/disable_streams for non-streams
subdevs", which is implemented in a different way in new patches in this series
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Tomi Valkeinen (11):
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()
media: subdev: Support non-routing subdevs in v4l2_subdev_s_stream_helper()
media: subdev: Improve s_stream documentation
drivers/media/v4l2-core/v4l2-subdev.c | 385 ++++++++++++++++++++--------------
include/media/v4l2-subdev.h | 34 ++-
2 files changed, 261 insertions(+), 158 deletions(-)
---
base-commit: e42a204f0519a2540f1507ac2798be2aeaa76bee
change-id: 20240404-enable-streams-impro-db8bcd898471
Best regards,
--
Tomi Valkeinen <[email protected]>
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.
Reviewed-by: Laurent Pinchart <[email protected]>
Tested-by: Umang Jain <[email protected]>
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/media/v4l2-core/v4l2-subdev.c | 109 +++++++++++++++++++---------------
1 file changed, 60 insertions(+), 49 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 7d0343e8480b..98629fa6de49 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2135,6 +2135,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)
{
@@ -2186,8 +2222,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. */
@@ -2208,22 +2244,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",
@@ -2232,6 +2255,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 = -EALREADY;
+ goto done;
+ }
+
dev_dbg(dev, "enable streams %u:%#llx\n", pad, streams_mask);
already_streaming = v4l2_subdev_is_streaming(sd);
@@ -2246,13 +2276,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);
@@ -2314,8 +2338,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. */
@@ -2336,22 +2360,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",
@@ -2360,6 +2371,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 = -EALREADY;
+ goto done;
+ }
+
dev_dbg(dev, "disable streams %u:%#llx\n", pad, streams_mask);
/* Call the .disable_streams() operation. */
@@ -2371,14 +2389,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
At the moment v4l2_subdev_s_stream_helper() only works for subdevices
that support routing. As enable/disable_streams now also works for
subdevices without routing, improve v4l2_subdev_s_stream_helper() to do
the same.
Reviewed-by: Laurent Pinchart <[email protected]>
Tested-by: Umang Jain <[email protected]>
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/media/v4l2-core/v4l2-subdev.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 72085027e213..88cfa5e0157a 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2402,15 +2402,24 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
if (WARN_ON(pad_index == -1))
return -EINVAL;
- /*
- * As there's a single source pad, just collect all the source streams.
- */
- state = v4l2_subdev_lock_and_get_active_state(sd);
+ if (sd->flags & V4L2_SUBDEV_FL_STREAMS) {
+ /*
+ * As there's a single source pad, just collect all the source
+ * streams.
+ */
+ state = v4l2_subdev_lock_and_get_active_state(sd);
- for_each_active_route(&state->routing, route)
- source_mask |= BIT_ULL(route->source_stream);
+ for_each_active_route(&state->routing, route)
+ source_mask |= BIT_ULL(route->source_stream);
- v4l2_subdev_unlock_state(state);
+ v4l2_subdev_unlock_state(state);
+ } else {
+ /*
+ * For non-streams subdevices, there's a single implicit stream
+ * per pad.
+ */
+ source_mask = BIT_ULL(0);
+ }
if (enable)
return v4l2_subdev_enable_streams(sd, pad_index, source_mask);
--
2.34.1
Now that enable/disable_streams operations are available for
single-stream subdevices too, there's no reason to use the old s_stream
operation on new drivers. Extend the documentation reflecting this.
Signed-off-by: Tomi Valkeinen <[email protected]>
---
include/media/v4l2-subdev.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 14a3c91cce93..99564a2ef71c 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -450,6 +450,15 @@ enum v4l2_subdev_pre_streamon_flags {
* already started or stopped subdev. Also see call_s_stream wrapper in
* v4l2-subdev.c.
*
+ * New drivers should instead implement &v4l2_subdev_pad_ops.enable_streams
+ * and &v4l2_subdev_pad_ops.disable_streams operations, and use
+ * v4l2_subdev_s_stream_helper for the &v4l2_subdev_video_ops.s_stream
+ * operation to support legacy users.
+ *
+ * Drivers should also not call the .s_stream() subdev operation directly,
+ * but use the v4l2_subdev_enable_streams() and
+ * v4l2_subdev_disable_streams() helpers.
+ *
* @g_pixelaspect: callback to return the pixelaspect ratio.
*
* @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev
--
2.34.1
Hi Tomi,
Thank you for the patch.
On Wed, Apr 24, 2024 at 06:39:14PM +0300, Tomi Valkeinen wrote:
> Now that enable/disable_streams operations are available for
> single-stream subdevices too, there's no reason to use the old s_stream
> operation on new drivers. Extend the documentation reflecting this.
>
> Signed-off-by: Tomi Valkeinen <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> include/media/v4l2-subdev.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 14a3c91cce93..99564a2ef71c 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -450,6 +450,15 @@ enum v4l2_subdev_pre_streamon_flags {
> * already started or stopped subdev. Also see call_s_stream wrapper in
> * v4l2-subdev.c.
> *
> + * New drivers should instead implement &v4l2_subdev_pad_ops.enable_streams
> + * and &v4l2_subdev_pad_ops.disable_streams operations, and use
> + * v4l2_subdev_s_stream_helper for the &v4l2_subdev_video_ops.s_stream
> + * operation to support legacy users.
> + *
> + * Drivers should also not call the .s_stream() subdev operation directly,
> + * but use the v4l2_subdev_enable_streams() and
> + * v4l2_subdev_disable_streams() helpers.
> + *
> * @g_pixelaspect: callback to return the pixelaspect ratio.
> *
> * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev
--
Regards,
Laurent Pinchart
Hi Laurent, others,
On Thu, Apr 25, 2024 at 02:08:54PM +0300, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Wed, Apr 24, 2024 at 06:39:14PM +0300, Tomi Valkeinen wrote:
> > Now that enable/disable_streams operations are available for
> > single-stream subdevices too, there's no reason to use the old s_stream
> > operation on new drivers. Extend the documentation reflecting this.
> >
> > Signed-off-by: Tomi Valkeinen <[email protected]>
>
> Reviewed-by: Laurent Pinchart <[email protected]>
Thanks!
If there are no further comments requiring changes, I'll send a PR on these
with Umang's imx335 and my CCS driver patches tomorrow.
--
Kind regards,
Sakari Ailus