linux-kernel - Re: [PATCH v3 9/9] media: subdev: Support single-stream case in v4l2_subdev_enable/disable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <da30ee6a-d8b1-44ad-8a29-fc9ac84aba9d@ideasonboard.com>
Date: Thu, 11 Apr 2024 14:53:55 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Umang Jain <umang.jain@...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()

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