Messages in this thread Patch in this message | | | Date | Thu, 11 Apr 2024 17:18:14 +0530 | Subject | Re: [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams() | From | Umang Jain <> |
| |
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; >>> } >>> >> >
| |