Re: [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()

[Tomi Valkeinen]

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