Received-SPF: pass (google.com: domain of linux-kernel+bounces-140389-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <d8c3349e-4ecc-4564-aeb4-40efcae5a43e@ideasonboard.com>
Date: Thu, 11 Apr 2024 17:26:58 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 9/9] media: subdev: Support single-stream case in
 v4l2_subdev_enable/disable_streams()
Content-Language: en-US
To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
 Mauro Carvalho Chehab <mchehab@kernel.org>, Hans Verkuil
 <hverkuil@xs4all.nl>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
 Sakari Ailus <sakari.ailus@linux.intel.com>
References: <20240410-enable-streams-impro-v3-0-e5e7a5da7420@ideasonboard.com>
 <20240410-enable-streams-impro-v3-9-e5e7a5da7420@ideasonboard.com>
 <f8e293b7-6a06-4477-9c7e-d1b83163f8e1@ideasonboard.com>
 <72940e89-0384-4fd3-8a10-42d6db44fdf0@ideasonboard.com>
 <155bb2c2-21b9-48d5-9615-7a44d4b6a590@ideasonboard.com>
 <da30ee6a-d8b1-44ad-8a29-fc9ac84aba9d@ideasonboard.com>
From: Umang Jain <umang.jain@ideasonboard.com>
In-Reply-To: <da30ee6a-d8b1-44ad-8a29-fc9ac84aba9d@ideasonboard.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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
>