Message-ID: <d8c3349e-4ecc-4564-aeb4-40efcae5a43e@ideasonboard.com>
Date: Thu, 11 Apr 2024 17:26:58 +0530
From: Umang Jain <umang.jain@...asonboard.com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Hans Verkuil
 <hverkuil@...all.nl>, Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>
Subject: Re: [PATCH v3 9/9] media: subdev: Support single-stream case in
 v4l2_subdev_enable/disable_streams()

Hi Tomi

On 11/04/24 5:23 pm, Tomi Valkeinen wrote:
> 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?

Yes it comes from the helper function

static const struct v4l2_subdev_video_ops imx283_video_ops = {
         .s_stream = v4l2_subdev_s_stream_helper,
};

>
> 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.

Yes, no routing in the driver.
>
>  Tomi
>

Powered by blists - more mailing lists