Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for single-pad subdevs

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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@ideasonboard.com>
>>>>>>>>> ---
>>>>>>>>> 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