linux-kernel - Re: [PATCH v2 0/6] media: uvcvideo: Implement the Privacy GPIO as a subdevice

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: <20241125125834.GC32280@pendragon.ideasonboard.com>
Date: Mon, 25 Nov 2024 14:58:34 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Ricardo Ribalda <ribalda@...omium.org>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	Yunke Cao <yunkec@...omium.org>, Hans Verkuil <hverkuil@...all.nl>
Subject: Re: [PATCH v2 0/6] media: uvcvideo: Implement the Privacy GPIO as a
 subdevice

On Mon, Nov 25, 2024 at 01:39:05PM +0100, Hans de Goede wrote:
> Hi Ricardo,
> 
> On 10-Nov-24 5:07 PM, Ricardo Ribalda wrote:
> > On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart
> > <laurent.pinchart@...asonboard.com> wrote:
> 
> <snip>
> 
> >>>> Here is what I have in mind for this:
> >>>>
> >>>> 1. Assume that the results of trying a specific fmt do not change over time.
> >>>>
> >>>> 2. Only allow userspace to request fmts which match one of the enum-fmts ->
> >>>>    enum-frame-sizes -> enum-frame-rates tripplet results
> >>>>    (constrain what userspace requests to these)
> >>>>
> >>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
> >>>>    3 levels nested loop for this) on probe() and cache the results
> >>>>
> >>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain
> >>>>    the requested fmt to one from our cached fmts
> >>>>
> >>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get
> >>>>    what we expect based on the cache, and otherwise return -EIO.
> >>>
> >>> Can we start powering up the device during try/set fmt and then
> >>> implement the format caching as an improvement?
> >>
> >> This sounds worth trying. We'll need to test it on a wide range of
> >> devices though, both internal and external.
> > 
> > For what is worth, we have been running something similar to
> > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
> > in ChromeOS and it has worked fine....
> > 
> > But I am pretty sure that it has issues with async controls :S
> 
> Interesting that is actually a lot more aggressive (as in doing a
> usb_autopm_put_interface() often) then what I expected when you said:
> 
> "Can we start powering up the device during try/set fmt"
> 
> As I mentioned in my other emails what I think would worth nicely
> is delay the initial usb_autopm_get_interface() till we need it
> and then just leave the camera on till /dev/video# gets closed.
> 
> That idea is based on dividing apps in 2 groups:
> 
> 1. Apps just temporarily opening /dev/video# nodes for discovery,
> where we ideally would not power-up the camera.
> 
> 2. Apps (could even be the same app) opening /dev/video# for
> a longer time because it actually want to use the camera
> at the moment of opening and which close the /dev/video# node
> when done with the camera.
> 
> Your code seems to also covers a 3th group of apps:
> 
> 3. Apps opening /dev/video# for a long time, while not using
> it all the time.
> 
> Although it would be nice to also cover those, IMHO those are
> not well behaved apps and I'm not sure if we need to cover those.

Is that right ? Isn't it better for an application to keep the device
open to avoid open delays or even open failures when it wants to use the
device ?

> Although looking back at the libcamera uvc pipeline handler issue
> I fixed recently, that was actually a case of 3.

-- 
Regards,

Laurent Pinchart