On 27/03/2024 15:32, Sakari Ailus wrote:
Heissulivei,
On Wed, Mar 27, 2024 at 01:06:42PM +0200, Tomi Valkeinen wrote:
On 27/03/2024 12:46, Sakari Ailus wrote:
Heippa,
On Mon, Mar 25, 2024 at 07:56:46PM +0200, Tomi Valkeinen wrote:
On 25/03/2024 19:52, Sakari Ailus wrote:
Moi,
On Mon, Mar 25, 2024 at 03:43:01PM +0200, Tomi Valkeinen wrote:
On 25/03/2024 15:02, Sakari Ailus wrote:
Moi,
Thanks for the patch.
On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
Hi Tomi,
On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
Currently a subdevice with a single pad, e.g. a sensor subdevice, must
use the v4l2_subdev_video_ops.s_stream op, instead of
v4l2_subdev_pad_ops.enable/disable_streams. This is because the
enable/disable_streams machinery requires a routing table which a subdev
cannot have with a single pad.
Implement enable/disable_streams support for these single-pad subdevices
by assuming an implicit stream 0 when the subdevice has only one pad.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
---
Even though I did send this patch, I'm not sure if this is necessary.
s_stream works fine for the subdevs with a single pad. With the upcoming
internal pads, adding an internal pad to the subdev will create a
routing table, and enable/disable_streams would get "fixed" that way.
I'd like to get rid of a redundant way to control streaming.
We can't get rid of it anyway, can we? We're not going to convert old
drivers to streams.
I'd expect to do that but it'd take a long time. That being said, I think
we need to consider devices without pads (VCMs) so it may well be this
would remain after all.
For new drivers, yes, we shouldn't use s_stream. But is the answer for new
sensor drivers this patch, or requiring an internal pad?
For new drivers I'd like to see an internal pad in fact.
{enable,disable}_streams is still internal to the kernel.
So, you think this patch should be dropped?
No, no. Not all sub-device drivers with pads are camera sensor drivers. :-)
Hmm, alright. So we want to support enable/disable_streams for sub-devices
with multiple source pads but no routing (so probably no sink pads)?
That should be allowed indeed, in order to move from s_stream() to
{enable,disable}_streams().
So perhaps the question is, do we want to support single-pad subdevs in
the future, in which case something like this patch is necessary, or
will all modern source subdev drivers have internal pads, in which
case this is not needed...
I think the latter would be best. I however can't guarantee we won't
have valid use cases for (enable|disable)_streams on single-pad subdevs
though, so you patch could still be interesting.
Instead of the number of pads, could we use instead the
V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
determine the need for this?
Maybe, but are they better? Do you see some issue with checking for the
number of pads? I considered a few options, but then thought that the most
safest test for this case is 1) one pad 2) enable/disable_streams
implemented.
I think I'd actually prefer {enable,disable}_streams in fact.
Hmm, sorry, now I'm confused =). What do you mean with that?
I'd use V4L2_SUBDEV_FL_STREAMS flag instead of the number of pads. The
number of pads is less related to routing.
Well, with one pad you cannot have routing =).
In this patch I used sd->enabled_streams to track the enabled streams, but
if we need to support multiple pads, I'll have to invent something new for
that.
What exactly do you think needs to be changed? This is just about starting
and stopping streaming using a different sent of callbacks, right?
The helpers track which streams are enabled, so we need some place to
store the enabled streams.
For V4L2_SUBDEV_FL_STREAMS we have that in state->stream_configs for
each stream. For the one-source-pad case we have a subdev wide
sd->enabled_streams to store that. But we don't have any place at the
moment to store if a pad is enabled.
Tomi
