| Message ID | 20240416-enable-streams-impro-v4-10-1d370c9c2b6d@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: subdev: Improve stream enable/disable machinery | expand |
Hi, On 16/04/2024 13:40, Tomi Valkeinen wrote: > 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. I forgot to mention, I have not tested this patch as I don't have a HW setup. And, of course, I now see that it has a bug. The BIT_ULL(1) should be BIT_ULL(0). Umang, can you try a fixed one on your side? If it works, I'll send a v5. Tomi > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > 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 1c6b305839a1..83ebcde54a34 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -2360,15 +2360,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(1); > + } > > if (enable) > return v4l2_subdev_enable_streams(sd, pad_index, source_mask); >
Hi Tomi On 16/04/24 4:13 pm, Tomi Valkeinen wrote: > Hi, > > On 16/04/2024 13:40, Tomi Valkeinen wrote: >> 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. > > I forgot to mention, I have not tested this patch as I don't have a HW > setup. And, of course, I now see that it has a bug. The BIT_ULL(1) > should be BIT_ULL(0). > > Umang, can you try a fixed one on your side? If it works, I'll send a v5. This doesn't work. Streaming fails as : [ 132.108845] rkisp1 32e10000.isp: streams 0xffff8000801fef88 already enabled on imx283 1-001a:0 [ 133.140906] rkisp1 32e10000.isp: streams 0xffff8000801fef88 already enabled on imx283 1-001a:0 With locally applied: diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 04d85b5f23f5..4684e4e1984c 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -2203,7 +2203,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad, } if (enabled_streams) { - dev_dbg(dev, "streams 0x%llx already enabled on %s:%u\n", + dev_err(dev, "streams 0x%llx already enabled on %s:%u\n", enabled_streams, sd->entity.name, pad); ret = -EINVAL; goto done; @@ -2376,7 +2376,7 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable) * For non-streams subdevices, there's a single implicit stream * per pad. */ - source_mask = BIT_ULL(1); + source_mask = BIT_ULL(0); } > > Tomi > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> 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 1c6b305839a1..83ebcde54a34 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -2360,15 +2360,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(1); >> + } >> if (enable) >> return v4l2_subdev_enable_streams(sd, pad_index, source_mask); >> >
On 16/04/2024 16:28, Umang Jain wrote: > Hi Tomi > > On 16/04/24 4:13 pm, Tomi Valkeinen wrote: >> Hi, >> >> On 16/04/2024 13:40, Tomi Valkeinen wrote: >>> 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. >> >> I forgot to mention, I have not tested this patch as I don't have a HW >> setup. And, of course, I now see that it has a bug. The BIT_ULL(1) >> should be BIT_ULL(0). >> >> Umang, can you try a fixed one on your side? If it works, I'll send a v5. > > This doesn't work. Streaming fails as : > > [ 132.108845] rkisp1 32e10000.isp: streams 0xffff8000801fef88 already > enabled on imx283 1-001a:0 > [ 133.140906] rkisp1 32e10000.isp: streams 0xffff8000801fef88 already > enabled on imx283 1-001a:0 Indeed, there's a bug in v4l2_subdev_collect_streams() too: @@ -2108,8 +2108,8 @@ static void v4l2_subdev_collect_streams(struct v4l2_subdev *sd, { if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) { *found_streams = BIT_ULL(0); - if (sd->enabled_pads & BIT_ULL(pad)) - *enabled_streams = BIT_ULL(0); + *enabled_streams = + (sd->enabled_pads & BIT_ULL(pad)) ? BIT_ULL(0) : 0; return; } Tomi > > With locally applied: > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c > b/drivers/media/v4l2-core/v4l2-subdev.c > index 04d85b5f23f5..4684e4e1984c 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -2203,7 +2203,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev > *sd, u32 pad, > } > > if (enabled_streams) { > - dev_dbg(dev, "streams 0x%llx already enabled on %s:%u\n", > + dev_err(dev, "streams 0x%llx already enabled on %s:%u\n", > enabled_streams, sd->entity.name, pad); > ret = -EINVAL; > goto done; > @@ -2376,7 +2376,7 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev > *sd, int enable) > * For non-streams subdevices, there's a single > implicit stream > * per pad. > */ > - source_mask = BIT_ULL(1); > + source_mask = BIT_ULL(0); > } > >> >> Tomi >> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> --- >>> 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 1c6b305839a1..83ebcde54a34 100644 >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>> @@ -2360,15 +2360,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(1); >>> + } >>> if (enable) >>> return v4l2_subdev_enable_streams(sd, pad_index, source_mask); >>> >> >
Hi Tomi, On 16/04/24 7:14 pm, Tomi Valkeinen wrote: > On 16/04/2024 16:28, Umang Jain wrote: >> Hi Tomi >> >> On 16/04/24 4:13 pm, Tomi Valkeinen wrote: >>> Hi, >>> >>> On 16/04/2024 13:40, Tomi Valkeinen wrote: >>>> 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. >>> >>> I forgot to mention, I have not tested this patch as I don't have a >>> HW setup. And, of course, I now see that it has a bug. The >>> BIT_ULL(1) should be BIT_ULL(0). >>> >>> Umang, can you try a fixed one on your side? If it works, I'll send >>> a v5. >> >> This doesn't work. Streaming fails as : >> >> [ 132.108845] rkisp1 32e10000.isp: streams 0xffff8000801fef88 >> already enabled on imx283 1-001a:0 >> [ 133.140906] rkisp1 32e10000.isp: streams 0xffff8000801fef88 >> already enabled on imx283 1-001a:0 > > Indeed, there's a bug in v4l2_subdev_collect_streams() too: > > @@ -2108,8 +2108,8 @@ static void v4l2_subdev_collect_streams(struct > v4l2_subdev *sd, > { > if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) { > *found_streams = BIT_ULL(0); > - if (sd->enabled_pads & BIT_ULL(pad)) > - *enabled_streams = BIT_ULL(0); > + *enabled_streams = > + (sd->enabled_pads & BIT_ULL(pad)) ? BIT_ULL(0) > : 0; > return; > } > > Tomi Yep, works now \o/ Locally applied: diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 04d85b5f23f5..8c591309df24 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -2108,6 +2108,8 @@ static void v4l2_subdev_collect_streams(struct v4l2_subdev *sd, { if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) { *found_streams = BIT_ULL(0); + *enabled_streams = + (sd->enabled_pads & BIT_ULL(pad)) ? BIT_ULL(0) : 0; if (sd->enabled_pads & BIT_ULL(pad)) *enabled_streams = BIT_ULL(0); return; @@ -2203,7 +2205,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad, } if (enabled_streams) { - dev_dbg(dev, "streams 0x%llx already enabled on %s:%u\n", + dev_err(dev, "streams 0x%llx already enabled on %s:%u\n", enabled_streams, sd->entity.name, pad); ret = -EINVAL; goto done; @@ -2376,7 +2378,7 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable) * For non-streams subdevices, there's a single implicit stream * per pad. */ - source_mask = BIT_ULL(1); + source_mask = BIT_ULL(0); } I will provide a Tested-by in v5. > >> >> With locally applied: >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c >> b/drivers/media/v4l2-core/v4l2-subdev.c >> index 04d85b5f23f5..4684e4e1984c 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -2203,7 +2203,7 @@ int v4l2_subdev_enable_streams(struct >> v4l2_subdev *sd, u32 pad, >> } >> >> if (enabled_streams) { >> - dev_dbg(dev, "streams 0x%llx already enabled on >> %s:%u\n", >> + dev_err(dev, "streams 0x%llx already enabled on >> %s:%u\n", >> enabled_streams, sd->entity.name, pad); >> ret = -EINVAL; >> goto done; >> @@ -2376,7 +2376,7 @@ int v4l2_subdev_s_stream_helper(struct >> v4l2_subdev *sd, int enable) >> * For non-streams subdevices, there's a single >> implicit stream >> * per pad. >> */ >> - source_mask = BIT_ULL(1); >> + source_mask = BIT_ULL(0); >> } >> >>> >>> Tomi >>> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>> --- >>>> 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 1c6b305839a1..83ebcde54a34 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>>> @@ -2360,15 +2360,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(1); >>>> + } >>>> if (enable) >>>> return v4l2_subdev_enable_streams(sd, pad_index, >>>> source_mask); >>>> >>> >> >
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 1c6b305839a1..83ebcde54a34 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -2360,15 +2360,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(1); + } if (enable) return v4l2_subdev_enable_streams(sd, pad_index, source_mask);
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. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)