| Message ID | 20240410-enable-streams-impro-v3-0-e5e7a5da7420@ideasonboard.com |
|---|---|
| Headers | show |
| Series | media: subdev: Improve stream enable/disable machinery | expand |
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; >
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; >
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)) >
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)) >> >
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; > } >
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; >> } >> >
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; >>> } >>> >> >
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
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 >
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; >
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)) >
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; > }; > >
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; >
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.
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,