| Message ID | 20241112-uvc-subdev-v3-0-0ea573d41a18@chromium.org |
|---|---|
| Headers | show |
| Series | media: uvcvideo: Implement the Privacy GPIO as a evdev | expand |
Hi Ricardo, On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote: > Some notebooks have a button to disable the camera (not to be mistaken > with the mechanical cover). This is a standard GPIO linked to the > camera via the ACPI table. > > 4 years ago we added support for this button in UVC via the Privacy control. > This has three issues: > - If the camera has its own privacy control, it will be masked. > - We need to power-up the camera to read the privacy control gpio. > - Other drivers have not followed this approach and have used evdev. > > We tried to fix the power-up issues implementing "granular power > saving" but it has been more complicated than anticipated... > > This patchset implements the Privacy GPIO as a evdev. > > The first patch of this set is already in Laurent's tree... but I > include it to get some CI coverage. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > Changes in v3: > - CodeStyle (Thanks Sakari) > - Re-implement as input device Thank you for your enthusiasm for my suggestion to implement this as an input device. As I mentioned in my reply in the v2 thread, the goal of my enumeration of various way camera privacy-controls are exposed to userspace today is to try and get everyone to agree on a single userspace API for this. Except for this v3 patch-set, which I take as an implied vote from you (Ricardo) for the evdev SW_CAMERA_LENS_COVER approach, we have not heard anything on this subject from Sakari or Laurent yet. So for now I would like to first focus on / circle back to the userspace API discussion and then once we have a plan for the userspace API we can implement that for uvcvideo. First lets look at the API question top down, iow what use-cases do we expect there to be for information about the camera-privacy switch state: a) Having an app which is using (trying to use) the camera show a notification to the user that the camera is turned-off by a privacy switch. Ricardo, AFAICT this is the main use-case for chrome-os, do I have this right ? b) Showing on on-screen-display (OSD) with a camera / crossed-out-camera icon when the switch is toggled, similar to how muting speakers/mic show an OSD. Laptop vendor Windows add-on software does this and I know that some users have been asking for this. Then lets look at the question bottom-up which hardware interfaces do we have exposing this information: 1. Internal UVC camera with an input privacy GPIO resource in the ACPI fwnode for the UVC camera, with the GPIO reporting the privacy-switch state. Found on some chrome-books 2. Laptop firmware (EC/ACPI/WMI) which reports privacy-switch state, without a clear 1:1 relation between the reported state and which camera it applies to. In this case sometimes the whole UVC camera module (if it is UVC) is simply dropped of the bus when the camera is disabled through the privacy switch, removing the entire /dev/video# node for the camera. Found on many windows laptops. 3. UVC cameras which report privacy-switch status through a UVC_CT_PRIVACY_CONTROL. Found on ... ? Note this will only work while the camera is streaming and even then may require polling of the ctrl because not all cameras reliably send UVC status messages when it changes. This renders this hardware interface as not usable Currently there are 2 ways this info is being communicated to userspace, hw-interfaces 1. + 3. are exposed as a v4l2 privacy-ctrl where as hw-if 2. uses and input evdev device. The advantage of the v4l2 privacy-ctrl is that it makes it very clear which camera is controlled by the camera privacy-switch. The disadvantage is that it will not work for hw-if 2, because the ACPI / WMI drivers have no v4l2 device to report the control on. We could try to add some magic glue code, but even then with e.g. IPU6 cameras it would still be unclear which v4l2(sub)device we should put the control on and if a UVC camera is just dropped from the bus there is no /dev/video# device at all. Using an input device does not has this disadvantage and it has the advantage of not requiring to power-up the camera as currently happens with a v4l2 ctrl on a UVC camera. But using an input device makes it harder to determine which camera the privacy-switch applies to. We can specify that SW_CAMERA_LENS_COVER only applies to device internal cameras, but then it is up to userspace to determine which cameras that are. Another problem with using an input device is that it will not work for "UVC cameras which report privacy-switch status through a UVC_CT_PRIVACY_CONTROL." since those need the camera on and even then need to be polled to get a reliable reading. Taking this all into account my proposal would be to go with an input device and document that SW_CAMERA_LENS_COVER only applies to device internal cameras. This should work well for both use-cases a) and b) described above and also be easy to support for both hw interfaces 1. and 2. My proposal for hw-if 3. (UVC_CT_PRIVACY_CONTROL) would be to keep reporting this as V4L2_CID_PRIVACY. This means it will not work out of the box for userspace which expects the input device method, but giving the limitations of this hw interface I think that requiring userspace to have to explicitly support this use-case (and e.g. poll the control) is a good thing rather then a bad thing. Still before moving forward with switching the hw-if 1. case to an input device as this patch-series does I would like to hear input from others. Sakari, Laurent, any comments ? Regards, Hans > - Make the code depend on UVC_INPUT_EVDEV > - Link to v2: https://lore.kernel.org/r/20241108-uvc-subdev-v2-0-85d8a051a3d3@chromium.org > > Changes in v2: > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > - Create uvc_gpio_cleanup and uvc_gpio_deinit > - Refactor quirk: do not disable irq > - Change define number for MEDIA_ENT_F_GPIO > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > --- > Ricardo Ribalda (8): > media: uvcvideo: Fix crash during unbind if gpio unit is in use > media: uvcvideo: Factor out gpio functions to its own file > media: uvcvideo: Re-implement privacy GPIO as an input device > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > media: uvcvideo: Create ancillary link for GPIO subdevice > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > media: uvcvideo: Introduce UVC_QUIRK_PRIVACY_DURING_STREAM > > .../userspace-api/media/mediactl/media-types.rst | 4 + > drivers/media/usb/uvc/Kconfig | 2 +- > drivers/media/usb/uvc/Makefile | 3 + > drivers/media/usb/uvc/uvc_ctrl.c | 40 +----- > drivers/media/usb/uvc/uvc_driver.c | 112 +--------------- > drivers/media/usb/uvc/uvc_entity.c | 21 ++- > drivers/media/usb/uvc/uvc_gpio.c | 144 +++++++++++++++++++++ > drivers/media/usb/uvc/uvc_status.c | 13 +- > drivers/media/usb/uvc/uvc_video.c | 4 + > drivers/media/usb/uvc/uvcvideo.h | 31 +++-- > drivers/media/v4l2-core/v4l2-async.c | 3 +- > include/uapi/linux/media.h | 1 + > 12 files changed, 223 insertions(+), 155 deletions(-) > --- > base-commit: 1b3bb4d69f20be5931abc18a6dbc24ff687fa780 > change-id: 20241030-uvc-subdev-89f4467a00b5 > > Best regards,
Hi Hans Thanks for the great summary. On Wed, 13 Nov 2024 at 18:57, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Ricardo, > > On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote: > > Some notebooks have a button to disable the camera (not to be mistaken > > with the mechanical cover). This is a standard GPIO linked to the > > camera via the ACPI table. > > > > 4 years ago we added support for this button in UVC via the Privacy control. > > This has three issues: > > - If the camera has its own privacy control, it will be masked. > > - We need to power-up the camera to read the privacy control gpio. > > - Other drivers have not followed this approach and have used evdev. > > > > We tried to fix the power-up issues implementing "granular power > > saving" but it has been more complicated than anticipated... > > > > This patchset implements the Privacy GPIO as a evdev. > > > > The first patch of this set is already in Laurent's tree... but I > > include it to get some CI coverage. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > Changes in v3: > > - CodeStyle (Thanks Sakari) > > - Re-implement as input device > > Thank you for your enthusiasm for my suggestion to implement this > as an input device. I wanted to give it a try... and it turned out to be quite simple to implement. I thought it could be a good idea to share it, so we can have something tangible to talk about ;). > > As I mentioned in my reply in the v2 thread, the goal of my > enumeration of various way camera privacy-controls are exposed to > userspace today is to try and get everyone to agree on a single > userspace API for this. > > Except for this v3 patch-set, which I take as an implied vote > from you (Ricardo) for the evdev SW_CAMERA_LENS_COVER approach, > we have not heard anything on this subject from Sakari or Laurent > yet. So for now I would like to first focus on / circle back to > the userspace API discussion and then once we have a plan for > the userspace API we can implement that for uvcvideo. > > First lets look at the API question top down, iow what use-cases > do we expect there to be for information about the camera-privacy > switch state: > > a) Having an app which is using (trying to use) the camera show > a notification to the user that the camera is turned-off by > a privacy switch . > > Ricardo, AFAICT this is the main use-case for chrome-os, do I have > this right ? b) is as important as a) for us. If you do not give instant feedback to the user when they change the status of the camera they might not be able to find the button later on :) > > b) Showing on on-screen-display (OSD) with a camera / > crossed-out-camera icon when the switch is toggled, similar to how > muting speakers/mic show an OSD . Laptop vendor Windows add-on > software does this and I know that some users have been asking > for this. > > Then lets look at the question bottom-up which hardware interfaces > do we have exposing this information: > > 1. Internal UVC camera with an input privacy GPIO resource in > the ACPI fwnode for the UVC camera, with the GPIO reporting > the privacy-switch state. Found on some chrome-books > > 2. Laptop firmware (EC/ACPI/WMI) which reports privacy-switch > state, without a clear 1:1 relation between the reported state and > which camera it applies to. In this case sometimes the whole UVC > camera module (if it is UVC) is simply dropped of the bus when > the camera is disabled through the privacy switch, removing > the entire /dev/video# node for the camera. Found on many windows > laptops. > > 3. UVC cameras which report privacy-switch status through > a UVC_CT_PRIVACY_CONTROL. Found on ... ? Some logitech cameras and also internal ones. > > Note this will only work while the camera is streaming and > even then may require polling of the ctrl because not all > cameras reliably send UVC status messages when it changes. > This renders this hardware interface as not usable > > > Currently there are 2 ways this info is being communicated > to userspace, hw-interfaces 1. + 3. are exposed as a v4l2 > privacy-ctrl where as hw-if 2. uses and input evdev device. > > The advantage of the v4l2 privacy-ctrl is that it makes it > very clear which camera is controlled by the camera > privacy-switch. > > The disadvantage is that it will not work for hw-if 2, > because the ACPI / WMI drivers have no v4l2 device to report > the control on. We could try to add some magic glue code, > but even then with e.g. IPU6 cameras it would still be > unclear which v4l2(sub)device we should put the control on > and if a UVC camera is just dropped from the bus there is > no /dev/video# device at all. > > Using an input device does not has this disadvantage and > it has the advantage of not requiring to power-up the camera > as currently happens with a v4l2 ctrl on a UVC camera. > > But using an input device makes it harder to determine > which camera the privacy-switch applies to. We can specify > that SW_CAMERA_LENS_COVER only applies to device internal > cameras, but then it is up to userspace to determine which > cameras that are. I am working on wiring up this to userspace right now.. I will report back if it cannot do it. > > Another problem with using an input device is that it will > not work for "UVC cameras which report privacy-switch status > through a UVC_CT_PRIVACY_CONTROL." since those need the camera > on and even then need to be polled to get a reliable reading. > > Taking this all into account my proposal would be to go > with an input device and document that SW_CAMERA_LENS_COVER > only applies to device internal cameras. > > This should work well for both use-cases a) and b) described > above and also be easy to support for both hw interfaces > 1. and 2. > > My proposal for hw-if 3. (UVC_CT_PRIVACY_CONTROL) would be > to keep reporting this as V4L2_CID_PRIVACY. This means it > will not work out of the box for userspace which expects > the input device method, but giving the limitations of > this hw interface I think that requiring userspace to have > to explicitly support this use-case (and e.g. poll the > control) is a good thing rather then a bad thing. > > Still before moving forward with switching the hw-if 1. > case to an input device as this patch-series does I would > like to hear input from others. > > Sakari, Laurent, any comments ? > > Regards, > > Hans > > > > > > > > > > > > > > > > > - Make the code depend on UVC_INPUT_EVDEV > > - Link to v2: https://lore.kernel.org/r/20241108-uvc-subdev-v2-0-85d8a051a3d3@chromium.org > > > > Changes in v2: > > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > > - Create uvc_gpio_cleanup and uvc_gpio_deinit > > - Refactor quirk: do not disable irq > > - Change define number for MEDIA_ENT_F_GPIO > > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > > > --- > > Ricardo Ribalda (8): > > media: uvcvideo: Fix crash during unbind if gpio unit is in use > > media: uvcvideo: Factor out gpio functions to its own file > > media: uvcvideo: Re-implement privacy GPIO as an input device > > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > > media: uvcvideo: Create ancillary link for GPIO subdevice > > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > > media: uvcvideo: Introduce UVC_QUIRK_PRIVACY_DURING_STREAM > > > > .../userspace-api/media/mediactl/media-types.rst | 4 + > > drivers/media/usb/uvc/Kconfig | 2 +- > > drivers/media/usb/uvc/Makefile | 3 + > > drivers/media/usb/uvc/uvc_ctrl.c | 40 +----- > > drivers/media/usb/uvc/uvc_driver.c | 112 +--------------- > > drivers/media/usb/uvc/uvc_entity.c | 21 ++- > > drivers/media/usb/uvc/uvc_gpio.c | 144 +++++++++++++++++++++ > > drivers/media/usb/uvc/uvc_status.c | 13 +- > > drivers/media/usb/uvc/uvc_video.c | 4 + > > drivers/media/usb/uvc/uvcvideo.h | 31 +++-- > > drivers/media/v4l2-core/v4l2-async.c | 3 +- > > include/uapi/linux/media.h | 1 + > > 12 files changed, 223 insertions(+), 155 deletions(-) > > --- > > base-commit: 1b3bb4d69f20be5931abc18a6dbc24ff687fa780 > > change-id: 20241030-uvc-subdev-89f4467a00b5 > > > > Best regards, >
Hello, On Thu, Nov 14, 2024 at 08:21:26PM +0100, Ricardo Ribalda wrote: > On Wed, 13 Nov 2024 at 18:57, Hans de Goede wrote: > > On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote: > > > Some notebooks have a button to disable the camera (not to be mistaken > > > with the mechanical cover). This is a standard GPIO linked to the > > > camera via the ACPI table. > > > > > > 4 years ago we added support for this button in UVC via the Privacy control. > > > This has three issues: > > > - If the camera has its own privacy control, it will be masked. > > > - We need to power-up the camera to read the privacy control gpio. > > > - Other drivers have not followed this approach and have used evdev. > > > > > > We tried to fix the power-up issues implementing "granular power > > > saving" but it has been more complicated than anticipated... > > > > > > This patchset implements the Privacy GPIO as a evdev. > > > > > > The first patch of this set is already in Laurent's tree... but I > > > include it to get some CI coverage. > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > Changes in v3: > > > - CodeStyle (Thanks Sakari) > > > - Re-implement as input device > > > > Thank you for your enthusiasm for my suggestion to implement this > > as an input device. > > I wanted to give it a try... and it turned out to be quite simple to > implement. I thought it could be a good idea to share it, so we can > have something tangible to talk about ;). > > > As I mentioned in my reply in the v2 thread, the goal of my > > enumeration of various way camera privacy-controls are exposed to > > userspace today is to try and get everyone to agree on a single > > userspace API for this. > > > > Except for this v3 patch-set, which I take as an implied vote > > from you (Ricardo) for the evdev SW_CAMERA_LENS_COVER approach, > > we have not heard anything on this subject from Sakari or Laurent > > yet. So for now I would like to first focus on / circle back to > > the userspace API discussion and then once we have a plan for > > the userspace API we can implement that for uvcvideo. > > > > First lets look at the API question top down, iow what use-cases > > do we expect there to be for information about the camera-privacy > > switch state: > > > > a) Having an app which is using (trying to use) the camera show > > a notification to the user that the camera is turned-off by > > a privacy switch . > > > > Ricardo, AFAICT this is the main use-case for chrome-os, do I have > > this right ? > > b) is as important as a) for us. If you do not give instant feedback > to the user when they change the status of the camera they might not > be able to find the button later on :) How do you handle cameras that suffer from UVC_QUIRK_PRIVACY_DURING_STREAM ? > > b) Showing on on-screen-display (OSD) with a camera / > > crossed-out-camera icon when the switch is toggled, similar to how > > muting speakers/mic show an OSD . Laptop vendor Windows add-on > > software does this and I know that some users have been asking > > for this. > > > > Then lets look at the question bottom-up which hardware interfaces > > do we have exposing this information: > > > > 1. Internal UVC camera with an input privacy GPIO resource in > > the ACPI fwnode for the UVC camera, with the GPIO reporting > > the privacy-switch state. Found on some chrome-books Ricardo, is this found only in ACPI-based (x86) chromebooks, or also in DT-based chromebooks ? Can we assume that the UVC module will not be powered off (and therefore disappear from USB) when the privacy switch is toggled to disable the camera ? > > 2. Laptop firmware (EC/ACPI/WMI) which reports privacy-switch > > state, without a clear 1:1 relation between the reported state and > > which camera it applies to. In this case sometimes the whole UVC > > camera module (if it is UVC) is simply dropped of the bus when > > the camera is disabled through the privacy switch, removing > > the entire /dev/video# node for the camera. Found on many windows > > laptops. > > > > 3. UVC cameras which report privacy-switch status through > > a UVC_CT_PRIVACY_CONTROL. Found on ... ? > > Some logitech cameras and also internal ones. > > > Note this will only work while the camera is streaming and > > even then may require polling of the ctrl because not all > > cameras reliably send UVC status messages when it changes. > > This renders this hardware interface as not usable In general I agree, but maybe the situation is better with the UVC cameras that implement UVC_CT_PRIVACY_CONTROL ? Note that, in theory, and as far as I understand, it should be possible to get the UVC_CT_PRIVACY_CONTROL events when the camera is not streaming, if the device implement remote wakeup. In practice that's hardly ever the case, among the ~450 sets of USB descriptors I've collected over time, only 8 report support for remote wakeup in the configuration descriptor's bmAttributes field, and I'm not even sure we could trust those devices to implement this feature correctly. Ricardo, do you know if the internal UVC cameras used in chromebooks that implement UVC_CT_PRIVACY_CONTROL support remote wakeup to notify changes in the privacy control when the camera is suspended ? > > Currently there are 2 ways this info is being communicated > > to userspace, hw-interfaces 1. + 3. are exposed as a v4l2 > > privacy-ctrl where as hw-if 2. uses and input evdev device. > > > > The advantage of the v4l2 privacy-ctrl is that it makes it > > very clear which camera is controlled by the camera > > privacy-switch. > > > > The disadvantage is that it will not work for hw-if 2, > > because the ACPI / WMI drivers have no v4l2 device to report > > the control on. We could try to add some magic glue code, > > but even then with e.g. IPU6 cameras it would still be > > unclear which v4l2(sub)device we should put the control on > > and if a UVC camera is just dropped from the bus there is > > no /dev/video# device at all. Is there any ACPI- or WMI-provided information that could assist with associating a privacy GPIO with a camera ? > > Using an input device does not has this disadvantage and > > it has the advantage of not requiring to power-up the camera > > as currently happens with a v4l2 ctrl on a UVC camera. API-wise, and with the current uvcvideo implementation, I agree. We could of course also try to improve the uvcvideo driver to not power the device unless it is streaming (depending on whether or not the known drawbacks are considered acceptable). Devices in the 3rd category will still need to be powered up to report the status of the privacy control, as well as some devices in the 1st category (see patch 8/8 in this series that introduces UVC_QUIRK_PRIVACY_DURING_STREAM). > > But using an input device makes it harder to determine > > which camera the privacy-switch applies to. We could include the evdev in the MC graph. That will of course only be possible if the kernel knows about that association in the first place. At least the 1st category of devices would benefit from this. > > We can specify > > that SW_CAMERA_LENS_COVER only applies to device internal > > cameras, but then it is up to userspace to determine which > > cameras that are. > > I am working on wiring up this to userspace right now.. I will report > back if it cannot do it. > > > Another problem with using an input device is that it will > > not work for "UVC cameras which report privacy-switch status > > through a UVC_CT_PRIVACY_CONTROL." since those need the camera > > on and even then need to be polled to get a reliable reading. > > > > Taking this all into account my proposal would be to go > > with an input device and document that SW_CAMERA_LENS_COVER > > only applies to device internal cameras. > > > > This should work well for both use-cases a) and b) described > > above and also be easy to support for both hw interfaces > > 1. and 2. > > > > My proposal for hw-if 3. (UVC_CT_PRIVACY_CONTROL) would be > > to keep reporting this as V4L2_CID_PRIVACY. This means it > > will not work out of the box for userspace which expects > > the input device method, but giving the limitations of > > this hw interface I think that requiring userspace to have > > to explicitly support this use-case (and e.g. poll the > > control) is a good thing rather then a bad thing. > > > > Still before moving forward with switching the hw-if 1. > > case to an input device as this patch-series does I would > > like to hear input from others. > > > > Sakari, Laurent, any comments ? Assuming the kernel could report the association between an evdev and camera, we would need to report which evdev SW_CAMERA_LENS_COVER originates from all the way from the evdev to the consumer of the event. How well is that supported in standard Linux system architectures ? If I'm not mistaken libinput will report the originating device, but how far up the stack is it propagated ? And which component would we expect to consume those events, should the camera evdev be managed by e.g. libcamera ? > > > - Make the code depend on UVC_INPUT_EVDEV > > > - Link to v2: https://lore.kernel.org/r/20241108-uvc-subdev-v2-0-85d8a051a3d3@chromium.org > > > > > > Changes in v2: > > > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > > > - Create uvc_gpio_cleanup and uvc_gpio_deinit > > > - Refactor quirk: do not disable irq > > > - Change define number for MEDIA_ENT_F_GPIO > > > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > > > > > --- > > > Ricardo Ribalda (8): > > > media: uvcvideo: Fix crash during unbind if gpio unit is in use > > > media: uvcvideo: Factor out gpio functions to its own file > > > media: uvcvideo: Re-implement privacy GPIO as an input device > > > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > > > media: uvcvideo: Create ancillary link for GPIO subdevice > > > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > > > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > > > media: uvcvideo: Introduce UVC_QUIRK_PRIVACY_DURING_STREAM > > > > > > .../userspace-api/media/mediactl/media-types.rst | 4 + > > > drivers/media/usb/uvc/Kconfig | 2 +- > > > drivers/media/usb/uvc/Makefile | 3 + > > > drivers/media/usb/uvc/uvc_ctrl.c | 40 +----- > > > drivers/media/usb/uvc/uvc_driver.c | 112 +--------------- > > > drivers/media/usb/uvc/uvc_entity.c | 21 ++- > > > drivers/media/usb/uvc/uvc_gpio.c | 144 +++++++++++++++++++++ > > > drivers/media/usb/uvc/uvc_status.c | 13 +- > > > drivers/media/usb/uvc/uvc_video.c | 4 + > > > drivers/media/usb/uvc/uvcvideo.h | 31 +++-- > > > drivers/media/v4l2-core/v4l2-async.c | 3 +- > > > include/uapi/linux/media.h | 1 + > > > 12 files changed, 223 insertions(+), 155 deletions(-) > > > --- > > > base-commit: 1b3bb4d69f20be5931abc18a6dbc24ff687fa780 > > > change-id: 20241030-uvc-subdev-89f4467a00b5
On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > On Thu, Nov 14, 2024 at 08:21:26PM +0100, Ricardo Ribalda wrote: > > On Wed, 13 Nov 2024 at 18:57, Hans de Goede wrote: > > > On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote: > > > > Some notebooks have a button to disable the camera (not to be mistaken > > > > with the mechanical cover). This is a standard GPIO linked to the > > > > camera via the ACPI table. > > > > > > > > 4 years ago we added support for this button in UVC via the Privacy control. > > > > This has three issues: > > > > - If the camera has its own privacy control, it will be masked. > > > > - We need to power-up the camera to read the privacy control gpio. > > > > - Other drivers have not followed this approach and have used evdev. > > > > > > > > We tried to fix the power-up issues implementing "granular power > > > > saving" but it has been more complicated than anticipated... > > > > > > > > This patchset implements the Privacy GPIO as a evdev. > > > > > > > > The first patch of this set is already in Laurent's tree... but I > > > > include it to get some CI coverage. > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > --- > > > > Changes in v3: > > > > - CodeStyle (Thanks Sakari) > > > > - Re-implement as input device > > > > > > Thank you for your enthusiasm for my suggestion to implement this > > > as an input device. > > > > I wanted to give it a try... and it turned out to be quite simple to > > implement. I thought it could be a good idea to share it, so we can > > have something tangible to talk about ;). > > > > > As I mentioned in my reply in the v2 thread, the goal of my > > > enumeration of various way camera privacy-controls are exposed to > > > userspace today is to try and get everyone to agree on a single > > > userspace API for this. > > > > > > Except for this v3 patch-set, which I take as an implied vote > > > from you (Ricardo) for the evdev SW_CAMERA_LENS_COVER approach, > > > we have not heard anything on this subject from Sakari or Laurent > > > yet. So for now I would like to first focus on / circle back to > > > the userspace API discussion and then once we have a plan for > > > the userspace API we can implement that for uvcvideo. > > > > > > First lets look at the API question top down, iow what use-cases > > > do we expect there to be for information about the camera-privacy > > > switch state: > > > > > > a) Having an app which is using (trying to use) the camera show > > > a notification to the user that the camera is turned-off by > > > a privacy switch . > > > > > > Ricardo, AFAICT this is the main use-case for chrome-os, do I have > > > this right ? > > > > b) is as important as a) for us. If you do not give instant feedback > > to the user when they change the status of the camera they might not > > be able to find the button later on :) > > How do you handle cameras that suffer from > UVC_QUIRK_PRIVACY_DURING_STREAM ? For those b) does not work. > > > > b) Showing on on-screen-display (OSD) with a camera / > > > crossed-out-camera icon when the switch is toggled, similar to how > > > muting speakers/mic show an OSD . Laptop vendor Windows add-on > > > software does this and I know that some users have been asking > > > for this. > > > > > > Then lets look at the question bottom-up which hardware interfaces > > > do we have exposing this information: > > > > > > 1. Internal UVC camera with an input privacy GPIO resource in > > > the ACPI fwnode for the UVC camera, with the GPIO reporting > > > the privacy-switch state. Found on some chrome-books > > Ricardo, is this found only in ACPI-based (x86) chromebooks, or also in > DT-based chromebooks ? I am only aware of ACPI models using this feature today. But there might be DT devices in the future that will use this feature. AFAIK the code should support ACPI and DT. > > Can we assume that the UVC module will not be powered off (and therefore > disappear from USB) when the privacy switch is toggled to disable the > camera ? That is true today, but I cannot be sure that some vendor becomes creative and wire things in a weird way. We usually catch this things early in the process and solve them, but I cannot predict the future (yet :P) > > > > 2. Laptop firmware (EC/ACPI/WMI) which reports privacy-switch > > > state, without a clear 1:1 relation between the reported state and > > > which camera it applies to. In this case sometimes the whole UVC > > > camera module (if it is UVC) is simply dropped of the bus when > > > the camera is disabled through the privacy switch, removing > > > the entire /dev/video# node for the camera. Found on many windows > > > laptops. > > > > > > 3. UVC cameras which report privacy-switch status through > > > a UVC_CT_PRIVACY_CONTROL. Found on ... ? > > > > Some logitech cameras and also internal ones. > > > > > Note this will only work while the camera is streaming and > > > even then may require polling of the ctrl because not all > > > cameras reliably send UVC status messages when it changes. > > > This renders this hardware interface as not usable > > In general I agree, but maybe the situation is better with the UVC > cameras that implement UVC_CT_PRIVACY_CONTROL ? > > Note that, in theory, and as far as I understand, it should be possible > to get the UVC_CT_PRIVACY_CONTROL events when the camera is not > streaming, if the device implement remote wakeup. In practice that's > hardly ever the case, among the ~450 sets of USB descriptors I've > collected over time, only 8 report support for remote wakeup in the > configuration descriptor's bmAttributes field, and I'm not even sure we > could trust those devices to implement this feature correctly. I would bet that they simply copied the descriptor from another project and did not test it. > > Ricardo, do you know if the internal UVC cameras used in chromebooks > that implement UVC_CT_PRIVACY_CONTROL support remote wakeup to notify > changes in the privacy control when the camera is suspended ? Today we only rely on the gpio privacy. Some camera vendors even emulate the control: Instead of having a gpio and a sensor, they look at the frame and if it is very dark, they zero it out completely and set UVC_CT_PRIVACY_CONTROL to 1. > > > > Currently there are 2 ways this info is being communicated > > > to userspace, hw-interfaces 1. + 3. are exposed as a v4l2 > > > privacy-ctrl where as hw-if 2. uses and input evdev device. > > > > > > The advantage of the v4l2 privacy-ctrl is that it makes it > > > very clear which camera is controlled by the camera > > > privacy-switch. > > > > > > The disadvantage is that it will not work for hw-if 2, > > > because the ACPI / WMI drivers have no v4l2 device to report > > > the control on. We could try to add some magic glue code, > > > but even then with e.g. IPU6 cameras it would still be > > > unclear which v4l2(sub)device we should put the control on > > > and if a UVC camera is just dropped from the bus there is > > > no /dev/video# device at all. > > Is there any ACPI- or WMI-provided information that could assist with > associating a privacy GPIO with a camera ? > > > > Using an input device does not has this disadvantage and > > > it has the advantage of not requiring to power-up the camera > > > as currently happens with a v4l2 ctrl on a UVC camera. > > API-wise, and with the current uvcvideo implementation, I agree. We > could of course also try to improve the uvcvideo driver to not power the > device unless it is streaming (depending on whether or not the known > drawbacks are considered acceptable). > > Devices in the 3rd category will still need to be powered up to report > the status of the privacy control, as well as some devices in the 1st > category (see patch 8/8 in this series that introduces > UVC_QUIRK_PRIVACY_DURING_STREAM). > > > > But using an input device makes it harder to determine > > > which camera the privacy-switch applies to. > > We could include the evdev in the MC graph. That will of course only be > possible if the kernel knows about that association in the first place. > At least the 1st category of devices would benefit from this. > > > > We can specify > > > that SW_CAMERA_LENS_COVER only applies to device internal > > > cameras, but then it is up to userspace to determine which > > > cameras that are. > > > > I am working on wiring up this to userspace right now.. I will report > > back if it cannot do it. > > > > > Another problem with using an input device is that it will > > > not work for "UVC cameras which report privacy-switch status > > > through a UVC_CT_PRIVACY_CONTROL." since those need the camera > > > on and even then need to be polled to get a reliable reading. > > > > > > Taking this all into account my proposal would be to go > > > with an input device and document that SW_CAMERA_LENS_COVER > > > only applies to device internal cameras. > > > > > > This should work well for both use-cases a) and b) described > > > above and also be easy to support for both hw interfaces > > > 1. and 2. > > > > > > My proposal for hw-if 3. (UVC_CT_PRIVACY_CONTROL) would be > > > to keep reporting this as V4L2_CID_PRIVACY. This means it > > > will not work out of the box for userspace which expects > > > the input device method, but giving the limitations of > > > this hw interface I think that requiring userspace to have > > > to explicitly support this use-case (and e.g. poll the > > > control) is a good thing rather then a bad thing. > > > > > > Still before moving forward with switching the hw-if 1. > > > case to an input device as this patch-series does I would > > > like to hear input from others. > > > > > > Sakari, Laurent, any comments ? > > Assuming the kernel could report the association between an evdev and > camera, we would need to report which evdev SW_CAMERA_LENS_COVER > originates from all the way from the evdev to the consumer of the event. > How well is that supported in standard Linux system architectures ? If > I'm not mistaken libinput will report the originating device, but how > far up the stack is it propagated ? And which component would we expect > to consume those events, should the camera evdev be managed by e.g. > libcamera ? > > > > > - Make the code depend on UVC_INPUT_EVDEV > > > > - Link to v2: https://lore.kernel.org/r/20241108-uvc-subdev-v2-0-85d8a051a3d3@chromium.org > > > > > > > > Changes in v2: > > > > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > > > > - Create uvc_gpio_cleanup and uvc_gpio_deinit > > > > - Refactor quirk: do not disable irq > > > > - Change define number for MEDIA_ENT_F_GPIO > > > > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > > > > > > > --- > > > > Ricardo Ribalda (8): > > > > media: uvcvideo: Fix crash during unbind if gpio unit is in use > > > > media: uvcvideo: Factor out gpio functions to its own file > > > > media: uvcvideo: Re-implement privacy GPIO as an input device > > > > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > > > > media: uvcvideo: Create ancillary link for GPIO subdevice > > > > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > > > > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > > > > media: uvcvideo: Introduce UVC_QUIRK_PRIVACY_DURING_STREAM > > > > > > > > .../userspace-api/media/mediactl/media-types.rst | 4 + > > > > drivers/media/usb/uvc/Kconfig | 2 +- > > > > drivers/media/usb/uvc/Makefile | 3 + > > > > drivers/media/usb/uvc/uvc_ctrl.c | 40 +----- > > > > drivers/media/usb/uvc/uvc_driver.c | 112 +--------------- > > > > drivers/media/usb/uvc/uvc_entity.c | 21 ++- > > > > drivers/media/usb/uvc/uvc_gpio.c | 144 +++++++++++++++++++++ > > > > drivers/media/usb/uvc/uvc_status.c | 13 +- > > > > drivers/media/usb/uvc/uvc_video.c | 4 + > > > > drivers/media/usb/uvc/uvcvideo.h | 31 +++-- > > > > drivers/media/v4l2-core/v4l2-async.c | 3 +- > > > > include/uapi/linux/media.h | 1 + > > > > 12 files changed, 223 insertions(+), 155 deletions(-) > > > > --- > > > > base-commit: 1b3bb4d69f20be5931abc18a6dbc24ff687fa780 > > > > change-id: 20241030-uvc-subdev-89f4467a00b5 > > -- > Regards, > > Laurent Pinchart
Hi All, On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote: > On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> >> Hello, >> >> On Thu, Nov 14, 2024 at 08:21:26PM +0100, Ricardo Ribalda wrote: >>> On Wed, 13 Nov 2024 at 18:57, Hans de Goede wrote: >>>> On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote: >>>>> Some notebooks have a button to disable the camera (not to be mistaken >>>>> with the mechanical cover). This is a standard GPIO linked to the >>>>> camera via the ACPI table. >>>>> >>>>> 4 years ago we added support for this button in UVC via the Privacy control. >>>>> This has three issues: >>>>> - If the camera has its own privacy control, it will be masked. >>>>> - We need to power-up the camera to read the privacy control gpio. >>>>> - Other drivers have not followed this approach and have used evdev. >>>>> >>>>> We tried to fix the power-up issues implementing "granular power >>>>> saving" but it has been more complicated than anticipated... >>>>> >>>>> This patchset implements the Privacy GPIO as a evdev. >>>>> >>>>> The first patch of this set is already in Laurent's tree... but I >>>>> include it to get some CI coverage. >>>>> >>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >>>>> --- >>>>> Changes in v3: >>>>> - CodeStyle (Thanks Sakari) >>>>> - Re-implement as input device >>>> >>>> Thank you for your enthusiasm for my suggestion to implement this >>>> as an input device. >>> >>> I wanted to give it a try... and it turned out to be quite simple to >>> implement. I thought it could be a good idea to share it, so we can >>> have something tangible to talk about ;). >>> >>>> As I mentioned in my reply in the v2 thread, the goal of my >>>> enumeration of various way camera privacy-controls are exposed to >>>> userspace today is to try and get everyone to agree on a single >>>> userspace API for this. >>>> >>>> Except for this v3 patch-set, which I take as an implied vote >>>> from you (Ricardo) for the evdev SW_CAMERA_LENS_COVER approach, >>>> we have not heard anything on this subject from Sakari or Laurent >>>> yet. So for now I would like to first focus on / circle back to >>>> the userspace API discussion and then once we have a plan for >>>> the userspace API we can implement that for uvcvideo. >>>> >>>> First lets look at the API question top down, iow what use-cases >>>> do we expect there to be for information about the camera-privacy >>>> switch state: >>>> >>>> a) Having an app which is using (trying to use) the camera show >>>> a notification to the user that the camera is turned-off by >>>> a privacy switch . >>>> >>>> Ricardo, AFAICT this is the main use-case for chrome-os, do I have >>>> this right ? >>> >>> b) is as important as a) for us. If you do not give instant feedback >>> to the user when they change the status of the camera they might not >>> be able to find the button later on :) >> >> How do you handle cameras that suffer from >> UVC_QUIRK_PRIVACY_DURING_STREAM ? > > For those b) does not work. I already suspected as much, but it is good to have this confirmed. I'm afraid that from a userspace API pov cameras with a GPIO which only works when powered-on need to be treated the same as cameras which only have UVC_CT_PRIVACY_CONTROL IOW in this case keep exporting V4L2_CID_PRIVACY instead of switching to evdev with SW_CAMERA_LENS_COVER. Unfortunately this will make the GPIO handling code in the UVC driver somewhat more involved since now we have both uAPI-s for GPIOs depending on UVC_QUIRK_PRIVACY_DURING_STREAM. But I think that this makes sense, this way we end up offering 2 uAPIs depending on the hw capabilities: 1. evdev with SW_CAMERA_LENS_COVER which always reports a reliable state + events on the state changing without needing to power-up the camera. 2. V4L2_CID_PRIVACY for the case where the camera needs to be powered-on (/dev/video opened) and where the ctrl possibly needs to be polled. Assuming we can all agree on this split based on hw capabilities I think that we must document this somewhere in the media subsystem documentation. We can then also write down there that SW_CAMERA_LENS_COVER only applies to internal cameras. >>>> b) Showing on on-screen-display (OSD) with a camera / >>>> crossed-out-camera icon when the switch is toggled, similar to how >>>> muting speakers/mic show an OSD . Laptop vendor Windows add-on >>>> software does this and I know that some users have been asking >>>> for this. >>>> >>>> Then lets look at the question bottom-up which hardware interfaces >>>> do we have exposing this information: >>>> >>>> 1. Internal UVC camera with an input privacy GPIO resource in >>>> the ACPI fwnode for the UVC camera, with the GPIO reporting >>>> the privacy-switch state. Found on some chrome-books >> >> Ricardo, is this found only in ACPI-based (x86) chromebooks, or also in >> DT-based chromebooks ? > > I am only aware of ACPI models using this feature today. But there > might be DT devices in the future that will use this feature. > AFAIK the code should support ACPI and DT. > >> >> Can we assume that the UVC module will not be powered off (and therefore >> disappear from USB) when the privacy switch is toggled to disable the >> camera ? > > That is true today, but I cannot be sure that some vendor becomes > creative and wire things in a weird way. We usually catch this things > early in the process and solve them, but I cannot predict the future > (yet :P) FWIW note that dropping the UVC module of the bus is definitely a thing on Windows laptops, but there the camera on/off events are handled by the embedded-controller and reported through some vendor WMI/ACPI interface rather then being handled by the UVC driver. So not really relevant to the discussion wrt the UVC driver, but still good to keep in mind. >>>> 2. Laptop firmware (EC/ACPI/WMI) which reports privacy-switch >>>> state, without a clear 1:1 relation between the reported state and >>>> which camera it applies to. In this case sometimes the whole UVC >>>> camera module (if it is UVC) is simply dropped of the bus when >>>> the camera is disabled through the privacy switch, removing >>>> the entire /dev/video# node for the camera. Found on many windows >>>> laptops. >>>> >>>> 3. UVC cameras which report privacy-switch status through >>>> a UVC_CT_PRIVACY_CONTROL. Found on ... ? >>> >>> Some logitech cameras and also internal ones. >>> >>>> Note this will only work while the camera is streaming and >>>> even then may require polling of the ctrl because not all >>>> cameras reliably send UVC status messages when it changes. >>>> This renders this hardware interface as not usable >> >> In general I agree, but maybe the situation is better with the UVC >> cameras that implement UVC_CT_PRIVACY_CONTROL ? >> >> Note that, in theory, and as far as I understand, it should be possible >> to get the UVC_CT_PRIVACY_CONTROL events when the camera is not >> streaming, if the device implement remote wakeup. In practice that's >> hardly ever the case, among the ~450 sets of USB descriptors I've >> collected over time, only 8 report support for remote wakeup in the >> configuration descriptor's bmAttributes field, and I'm not even sure we >> could trust those devices to implement this feature correctly. > > I would bet that they simply copied the descriptor from another > project and did not test it. > >> >> Ricardo, do you know if the internal UVC cameras used in chromebooks >> that implement UVC_CT_PRIVACY_CONTROL support remote wakeup to notify >> changes in the privacy control when the camera is suspended ? > > Today we only rely on the gpio privacy. > > Some camera vendors even emulate the control: > Instead of having a gpio and a sensor, they look at the frame and if > it is very dark, they zero it out completely and set > UVC_CT_PRIVACY_CONTROL to 1. My 2 cents here are that given the wide variety of hardware that even if some hw reliably provides status interrupts for UVC_CT_PRIVACY_CONTROL we cannot rely on that and we certainly cannot rely on remote wakeup being present *and* reliabe. So I really think that for UVC_CT_PRIVACY_CONTROL we should stick with V4L2_CID_PRIVACY. >>>> Currently there are 2 ways this info is being communicated >>>> to userspace, hw-interfaces 1. + 3. are exposed as a v4l2 >>>> privacy-ctrl where as hw-if 2. uses and input evdev device. >>>> >>>> The advantage of the v4l2 privacy-ctrl is that it makes it >>>> very clear which camera is controlled by the camera >>>> privacy-switch. >>>> >>>> The disadvantage is that it will not work for hw-if 2, >>>> because the ACPI / WMI drivers have no v4l2 device to report >>>> the control on. We could try to add some magic glue code, >>>> but even then with e.g. IPU6 cameras it would still be >>>> unclear which v4l2(sub)device we should put the control on >>>> and if a UVC camera is just dropped from the bus there is >>>> no /dev/video# device at all. >> >> Is there any ACPI- or WMI-provided information that could assist with >> associating a privacy GPIO with a camera ? >> >>>> Using an input device does not has this disadvantage and >>>> it has the advantage of not requiring to power-up the camera >>>> as currently happens with a v4l2 ctrl on a UVC camera. >> >> API-wise, and with the current uvcvideo implementation, I agree. We >> could of course also try to improve the uvcvideo driver to not power the >> device unless it is streaming (depending on whether or not the known >> drawbacks are considered acceptable). >> >> Devices in the 3rd category will still need to be powered up to report >> the status of the privacy control, as well as some devices in the 1st >> category (see patch 8/8 in this series that introduces >> UVC_QUIRK_PRIVACY_DURING_STREAM). >> >>>> But using an input device makes it harder to determine >>>> which camera the privacy-switch applies to. >> >> We could include the evdev in the MC graph. That will of course only be >> possible if the kernel knows about that association in the first place. >> At least the 1st category of devices would benefit from this. Yes I was thinking about adding a link to the MC graph for this too. Ricardo I notice that in this v3 series you still create a v4l2-subdev for the GPIO handling and then add an ancillary link for the GPIO subdev to the mc-graph. But I'm not sure how that is helpful. Userspace would still need to do parent matching, but then match the evdev parent to the subdev after getting the subdev from the mc. In that case it might as well look at the physical (USB-interface) parent of the MC/video node and do parent matching on that avoiding the need to go through the MC at all. I think using the MC could still be useful by adding a new type of ancillary link to the MC API which provides a file-path as info to userspace rather then a mc-link and then just directly provide the /dev/input/event# path through this new API? I guess that extending the MC API like this might be a bit of a discussion. But it would already make sense to have this for the existing input device for the snapshot button. >>>> We can specify >>>> that SW_CAMERA_LENS_COVER only applies to device internal >>>> cameras, but then it is up to userspace to determine which >>>> cameras that are. >>> >>> I am working on wiring up this to userspace right now.. I will report >>> back if it cannot do it. Ricardo, great, thank you! >>>> Another problem with using an input device is that it will >>>> not work for "UVC cameras which report privacy-switch status >>>> through a UVC_CT_PRIVACY_CONTROL." since those need the camera >>>> on and even then need to be polled to get a reliable reading. >>>> >>>> Taking this all into account my proposal would be to go >>>> with an input device and document that SW_CAMERA_LENS_COVER >>>> only applies to device internal cameras. >>>> >>>> This should work well for both use-cases a) and b) described >>>> above and also be easy to support for both hw interfaces >>>> 1. and 2. >>>> >>>> My proposal for hw-if 3. (UVC_CT_PRIVACY_CONTROL) would be >>>> to keep reporting this as V4L2_CID_PRIVACY. This means it >>>> will not work out of the box for userspace which expects >>>> the input device method, but giving the limitations of >>>> this hw interface I think that requiring userspace to have >>>> to explicitly support this use-case (and e.g. poll the >>>> control) is a good thing rather then a bad thing. >>>> >>>> Still before moving forward with switching the hw-if 1. >>>> case to an input device as this patch-series does I would >>>> like to hear input from others. >>>> >>>> Sakari, Laurent, any comments ? >> >> Assuming the kernel could report the association between an evdev and >> camera, we would need to report which evdev SW_CAMERA_LENS_COVER >> originates from all the way from the evdev to the consumer of the event. >> How well is that supported in standard Linux system architectures ? If >> I'm not mistaken libinput will report the originating device, but how >> far up the stack is it propagated ? And which component would we expect >> to consume those events, should the camera evdev be managed by e.g. >> libcamera ? Good questions. Looking back at our 2 primary use-cases: a) Having an app which is using (trying to use) the camera show a notification to the user that the camera is turned-off by a privacy switch. b) Showing on on-screen-display (OSD) with a camera / crossed-out-camera icon when the switch is toggled, similar to how muting speakers/mic show an OSD. Laptop vendor Windows add-on software does this and I know that some users have been asking for this. I think we have everything to do b) in current compositors like gnome-shell. Using an evdev with SW_CAMERA_LENS_COVER would even be a lot easier for b) then the current V4L2_CID_PRIVACY API. a) though is a lot harder. We could open up access to the relevant /dev/input/event# node using a udev uaccess tag so that users who can access /dev/video# nodes also get raw access to that /dev/input/event# node and then libcamera could indeed provide this information that way. I think that is probably the best option. At least for the cases where the camera on/off switch does not simply make the camera completely disappear. That case is harder. atm that case is not handled at all though. So even just getting b) to work for that case would be nice / an improvement. Eventually if we give libcamera access to event# nodes which advertise SW_CAMERA_LENS_COVER (and no other privacy sensitive information) then libcamera could even separately offer some API for apps to just get that value if there is no camera to associate it with. Actually thinking more about it libcamera probably might be the right place for some sort of "no cameras found have you tried hitting your camera privacy-switch" API. That is some API to query if such a message should be shown to the user. But that is very much future work. Regards, Hans
Hi Hans On Mon, 18 Nov 2024 at 16:43, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi All, > > On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote: > > On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > >> > >> Hello, > >> > >> On Thu, Nov 14, 2024 at 08:21:26PM +0100, Ricardo Ribalda wrote: > >>> On Wed, 13 Nov 2024 at 18:57, Hans de Goede wrote: > >>>> On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote: > >>>>> Some notebooks have a button to disable the camera (not to be mistaken > >>>>> with the mechanical cover). This is a standard GPIO linked to the > >>>>> camera via the ACPI table. > >>>>> > >>>>> 4 years ago we added support for this button in UVC via the Privacy control. > >>>>> This has three issues: > >>>>> - If the camera has its own privacy control, it will be masked. > >>>>> - We need to power-up the camera to read the privacy control gpio. > >>>>> - Other drivers have not followed this approach and have used evdev. > >>>>> > >>>>> We tried to fix the power-up issues implementing "granular power > >>>>> saving" but it has been more complicated than anticipated... > >>>>> > >>>>> This patchset implements the Privacy GPIO as a evdev. > >>>>> > >>>>> The first patch of this set is already in Laurent's tree... but I > >>>>> include it to get some CI coverage. > >>>>> > >>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > >>>>> --- > >>>>> Changes in v3: > >>>>> - CodeStyle (Thanks Sakari) > >>>>> - Re-implement as input device > >>>> > >>>> Thank you for your enthusiasm for my suggestion to implement this > >>>> as an input device. > >>> > >>> I wanted to give it a try... and it turned out to be quite simple to > >>> implement. I thought it could be a good idea to share it, so we can > >>> have something tangible to talk about ;). > >>> > >>>> As I mentioned in my reply in the v2 thread, the goal of my > >>>> enumeration of various way camera privacy-controls are exposed to > >>>> userspace today is to try and get everyone to agree on a single > >>>> userspace API for this. > >>>> > >>>> Except for this v3 patch-set, which I take as an implied vote > >>>> from you (Ricardo) for the evdev SW_CAMERA_LENS_COVER approach, > >>>> we have not heard anything on this subject from Sakari or Laurent > >>>> yet. So for now I would like to first focus on / circle back to > >>>> the userspace API discussion and then once we have a plan for > >>>> the userspace API we can implement that for uvcvideo. > >>>> > >>>> First lets look at the API question top down, iow what use-cases > >>>> do we expect there to be for information about the camera-privacy > >>>> switch state: > >>>> > >>>> a) Having an app which is using (trying to use) the camera show > >>>> a notification to the user that the camera is turned-off by > >>>> a privacy switch . > >>>> > >>>> Ricardo, AFAICT this is the main use-case for chrome-os, do I have > >>>> this right ? > >>> > >>> b) is as important as a) for us. If you do not give instant feedback > >>> to the user when they change the status of the camera they might not > >>> be able to find the button later on :) > >> > >> How do you handle cameras that suffer from > >> UVC_QUIRK_PRIVACY_DURING_STREAM ? > > > > For those b) does not work. > > I already suspected as much, but it is good to have this > confirmed. > > I'm afraid that from a userspace API pov cameras with a GPIO > which only works when powered-on need to be treated the same as > cameras which only have UVC_CT_PRIVACY_CONTROL IOW in this case > keep exporting V4L2_CID_PRIVACY instead of switching to evdev > with SW_CAMERA_LENS_COVER. > > Unfortunately this will make the GPIO handling code in the UVC > driver somewhat more involved since now we have both uAPI-s for > GPIOs depending on UVC_QUIRK_PRIVACY_DURING_STREAM. > > But I think that this makes sense, this way we end up offering > 2 uAPIs depending on the hw capabilities: > > 1. evdev with SW_CAMERA_LENS_COVER which always reports a reliable > state + events on the state changing without needing to power-up > the camera. > > 2. V4L2_CID_PRIVACY for the case where the camera needs to be > powered-on (/dev/video opened) and where the ctrl possibly needs > to be polled. > > Assuming we can all agree on this split based on hw capabilities > I think that we must document this somewhere in the media subsystem > documentation. We can then also write down there that > SW_CAMERA_LENS_COVER only applies to internal cameras. I do not think that it is worth it to keep UVC_CT_PRIVACY_CONTROL for the two devices that have connected the GPIO's pull up to the wrong power rail. Now that the GPIO can be used from userspace, I expect that those errors will be found early in the design process and never reach production stage. If we use UVC_CT_PRIVACY_CONTROL for thes two devices: - userspace will have to implement two different APIs - the driver will have to duplicate the code. - all that code will be very difficult to test: there are only 2 devices affected and it requires manual intervention to properly test it. I think that UVC_QUIRK_PRIVACY_DURING_STREAM is a good compromise and the main user handles it properly. > > >>>> b) Showing on on-screen-display (OSD) with a camera / > >>>> crossed-out-camera icon when the switch is toggled, similar to how > >>>> muting speakers/mic show an OSD . Laptop vendor Windows add-on > >>>> software does this and I know that some users have been asking > >>>> for this. > >>>> > >>>> Then lets look at the question bottom-up which hardware interfaces > >>>> do we have exposing this information: > >>>> > >>>> 1. Internal UVC camera with an input privacy GPIO resource in > >>>> the ACPI fwnode for the UVC camera, with the GPIO reporting > >>>> the privacy-switch state. Found on some chrome-books > >> > >> Ricardo, is this found only in ACPI-based (x86) chromebooks, or also in > >> DT-based chromebooks ? > > > > I am only aware of ACPI models using this feature today. But there > > might be DT devices in the future that will use this feature. > > AFAIK the code should support ACPI and DT. > > > >> > >> Can we assume that the UVC module will not be powered off (and therefore > >> disappear from USB) when the privacy switch is toggled to disable the > >> camera ? > > > > That is true today, but I cannot be sure that some vendor becomes > > creative and wire things in a weird way. We usually catch this things > > early in the process and solve them, but I cannot predict the future > > (yet :P) > > FWIW note that dropping the UVC module of the bus is definitely > a thing on Windows laptops, but there the camera on/off events > are handled by the embedded-controller and reported through > some vendor WMI/ACPI interface rather then being handled by > the UVC driver. > > So not really relevant to the discussion wrt the UVC driver, > but still good to keep in mind. > > >>>> 2. Laptop firmware (EC/ACPI/WMI) which reports privacy-switch > >>>> state, without a clear 1:1 relation between the reported state and > >>>> which camera it applies to. In this case sometimes the whole UVC > >>>> camera module (if it is UVC) is simply dropped of the bus when > >>>> the camera is disabled through the privacy switch, removing > >>>> the entire /dev/video# node for the camera. Found on many windows > >>>> laptops. > >>>> > >>>> 3. UVC cameras which report privacy-switch status through > >>>> a UVC_CT_PRIVACY_CONTROL. Found on ... ? > >>> > >>> Some logitech cameras and also internal ones. > >>> > >>>> Note this will only work while the camera is streaming and > >>>> even then may require polling of the ctrl because not all > >>>> cameras reliably send UVC status messages when it changes. > >>>> This renders this hardware interface as not usable > >> > >> In general I agree, but maybe the situation is better with the UVC > >> cameras that implement UVC_CT_PRIVACY_CONTROL ? > >> > >> Note that, in theory, and as far as I understand, it should be possible > >> to get the UVC_CT_PRIVACY_CONTROL events when the camera is not > >> streaming, if the device implement remote wakeup. In practice that's > >> hardly ever the case, among the ~450 sets of USB descriptors I've > >> collected over time, only 8 report support for remote wakeup in the > >> configuration descriptor's bmAttributes field, and I'm not even sure we > >> could trust those devices to implement this feature correctly. > > > > I would bet that they simply copied the descriptor from another > > project and did not test it. > > > >> > >> Ricardo, do you know if the internal UVC cameras used in chromebooks > >> that implement UVC_CT_PRIVACY_CONTROL support remote wakeup to notify > >> changes in the privacy control when the camera is suspended ? > > > > Today we only rely on the gpio privacy. > > > > Some camera vendors even emulate the control: > > Instead of having a gpio and a sensor, they look at the frame and if > > it is very dark, they zero it out completely and set > > UVC_CT_PRIVACY_CONTROL to 1. > > My 2 cents here are that given the wide variety of hardware that > even if some hw reliably provides status interrupts for > UVC_CT_PRIVACY_CONTROL we cannot rely on that and we certainly > cannot rely on remote wakeup being present *and* reliabe. > > So I really think that for UVC_CT_PRIVACY_CONTROL we should > stick with V4L2_CID_PRIVACY. > > >>>> Currently there are 2 ways this info is being communicated > >>>> to userspace, hw-interfaces 1. + 3. are exposed as a v4l2 > >>>> privacy-ctrl where as hw-if 2. uses and input evdev device. > >>>> > >>>> The advantage of the v4l2 privacy-ctrl is that it makes it > >>>> very clear which camera is controlled by the camera > >>>> privacy-switch. > >>>> > >>>> The disadvantage is that it will not work for hw-if 2, > >>>> because the ACPI / WMI drivers have no v4l2 device to report > >>>> the control on. We could try to add some magic glue code, > >>>> but even then with e.g. IPU6 cameras it would still be > >>>> unclear which v4l2(sub)device we should put the control on > >>>> and if a UVC camera is just dropped from the bus there is > >>>> no /dev/video# device at all. > >> > >> Is there any ACPI- or WMI-provided information that could assist with > >> associating a privacy GPIO with a camera ? > >> > >>>> Using an input device does not has this disadvantage and > >>>> it has the advantage of not requiring to power-up the camera > >>>> as currently happens with a v4l2 ctrl on a UVC camera. > >> > >> API-wise, and with the current uvcvideo implementation, I agree. We > >> could of course also try to improve the uvcvideo driver to not power the > >> device unless it is streaming (depending on whether or not the known > >> drawbacks are considered acceptable). > >> > >> Devices in the 3rd category will still need to be powered up to report > >> the status of the privacy control, as well as some devices in the 1st > >> category (see patch 8/8 in this series that introduces > >> UVC_QUIRK_PRIVACY_DURING_STREAM). > >> > >>>> But using an input device makes it harder to determine > >>>> which camera the privacy-switch applies to. > >> > >> We could include the evdev in the MC graph. That will of course only be > >> possible if the kernel knows about that association in the first place. > >> At least the 1st category of devices would benefit from this. > > Yes I was thinking about adding a link to the MC graph for this too. > > Ricardo I notice that in this v3 series you still create a v4l2-subdev > for the GPIO handling and then add an ancillary link for the GPIO subdev > to the mc-graph. But I'm not sure how that is helpful. Userspace would > still need to do parent matching, but then match the evdev parent to > the subdev after getting the subdev from the mc. In that case it might > as well look at the physical (USB-interface) parent of the MC/video > node and do parent matching on that avoiding the need to go through > the MC at all. > > I think using the MC could still be useful by adding a new type of > ancillary link to the MC API which provides a file-path as info to > userspace rather then a mc-link and then just directly provide > the /dev/input/event# path through this new API? > > I guess that extending the MC API like this might be a bit of > a discussion. But it would already make sense to have this for > the existing input device for the snapshot button. The driver creates a v4l2-subdevice for every entity, and the gpio today is modeled as an entity. The patchset just adds an ancillary link as Sakari suggested. I am not against removing the gpio entity all together if it is not needed. Now that we are brainstorming here... what about adding a control that contains the name of the input device (eventX)? Is that a horrible idea? > > >>>> We can specify > >>>> that SW_CAMERA_LENS_COVER only applies to device internal > >>>> cameras, but then it is up to userspace to determine which > >>>> cameras that are. > >>> > >>> I am working on wiring up this to userspace right now.. I will report > >>> back if it cannot do it. > > Ricardo, great, thank you! > > >>>> Another problem with using an input device is that it will > >>>> not work for "UVC cameras which report privacy-switch status > >>>> through a UVC_CT_PRIVACY_CONTROL." since those need the camera > >>>> on and even then need to be polled to get a reliable reading. > >>>> > >>>> Taking this all into account my proposal would be to go > >>>> with an input device and document that SW_CAMERA_LENS_COVER > >>>> only applies to device internal cameras. > >>>> > >>>> This should work well for both use-cases a) and b) described > >>>> above and also be easy to support for both hw interfaces > >>>> 1. and 2. > >>>> > >>>> My proposal for hw-if 3. (UVC_CT_PRIVACY_CONTROL) would be > >>>> to keep reporting this as V4L2_CID_PRIVACY. This means it > >>>> will not work out of the box for userspace which expects > >>>> the input device method, but giving the limitations of > >>>> this hw interface I think that requiring userspace to have > >>>> to explicitly support this use-case (and e.g. poll the > >>>> control) is a good thing rather then a bad thing. > >>>> > >>>> Still before moving forward with switching the hw-if 1. > >>>> case to an input device as this patch-series does I would > >>>> like to hear input from others. > >>>> > >>>> Sakari, Laurent, any comments ? > >> > >> Assuming the kernel could report the association between an evdev and > >> camera, we would need to report which evdev SW_CAMERA_LENS_COVER > >> originates from all the way from the evdev to the consumer of the event. > >> How well is that supported in standard Linux system architectures ? If > >> I'm not mistaken libinput will report the originating device, but how > >> far up the stack is it propagated ? And which component would we expect > >> to consume those events, should the camera evdev be managed by e.g. > >> libcamera ? > > Good questions. Looking back at our 2 primary use-cases: > > a) Having an app which is using (trying to use) the camera show > a notification to the user that the camera is turned-off by > a privacy switch . > > b) Showing on on-screen-display (OSD) with a camera / > crossed-out-camera icon when the switch is toggled, similar to how > muting speakers/mic show an OSD . Laptop vendor Windows add-on > software does this and I know that some users have been asking > for this. > > I think we have everything to do b) in current compositors > like gnome-shell. Using an evdev with SW_CAMERA_LENS_COVER > would even be a lot easier for b) then the current > V4L2_CID_PRIVACY API. > > a) though is a lot harder. We could open up access to > the relevant /dev/input/event# node using a udev uaccess > tag so that users who can access /dev/video# nodes also > get raw access to that /dev/input/event# node and then > libcamera could indeed provide this information that way. > I think that is probably the best option. > > At least for the cases where the camera on/off switch > does not simply make the camera completely disappear. > > That case is harder. atm that case is not handled at all > though. So even just getting b) to work for that case > would be nice / an improvement. > > Eventually if we give libcamera access to event# > nodes which advertise SW_CAMERA_LENS_COVER (and no other > privacy sensitive information) then libcamera could even > separately offer some API for apps to just get that value > if there is no camera to associate it with. > > Actually thinking more about it libcamera probably might > be the right place for some sort of "no cameras found > have you tried hitting your camera privacy-switch" API. > That is some API to query if such a message should be > shown to the user. But that is very much future work. Are standard apps expected to use libcamera directly or they should use pipewire? Maybe a) Should be pipewire's task? > > Regards, > > Hans > > > -- Ricardo Ribalda
Hi Ricardo, On 18-Nov-24 5:47 PM, Ricardo Ribalda wrote: > Hi Hans > > On Mon, 18 Nov 2024 at 16:43, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi All, >> >> On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote: >>> On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart >>> <laurent.pinchart@ideasonboard.com> wrote: <snip> >>>> How do you handle cameras that suffer from >>>> UVC_QUIRK_PRIVACY_DURING_STREAM ? >>> >>> For those b) does not work. >> >> I already suspected as much, but it is good to have this >> confirmed. >> >> I'm afraid that from a userspace API pov cameras with a GPIO >> which only works when powered-on need to be treated the same as >> cameras which only have UVC_CT_PRIVACY_CONTROL IOW in this case >> keep exporting V4L2_CID_PRIVACY instead of switching to evdev >> with SW_CAMERA_LENS_COVER. >> >> Unfortunately this will make the GPIO handling code in the UVC >> driver somewhat more involved since now we have both uAPI-s for >> GPIOs depending on UVC_QUIRK_PRIVACY_DURING_STREAM. >> >> But I think that this makes sense, this way we end up offering >> 2 uAPIs depending on the hw capabilities: >> >> 1. evdev with SW_CAMERA_LENS_COVER which always reports a reliable >> state + events on the state changing without needing to power-up >> the camera. >> >> 2. V4L2_CID_PRIVACY for the case where the camera needs to be >> powered-on (/dev/video opened) and where the ctrl possibly needs >> to be polled. >> >> Assuming we can all agree on this split based on hw capabilities >> I think that we must document this somewhere in the media subsystem >> documentation. We can then also write down there that >> SW_CAMERA_LENS_COVER only applies to internal cameras. > > I do not think that it is worth it to keep UVC_CT_PRIVACY_CONTROL for > the two devices that have connected the GPIO's pull up to the wrong > power rail. > Now that the GPIO can be used from userspace, I expect that those > errors will be found early in the design process and never reach > production stage. > > > If we use UVC_CT_PRIVACY_CONTROL for thes two devices: > - userspace will have to implement two different APIs > - the driver will have to duplicate the code. > - all that code will be very difficult to test: there are only 2 > devices affected and it requires manual intervention to properly test > it. > > I think that UVC_QUIRK_PRIVACY_DURING_STREAM is a good compromise and > the main user handles it properly. Ok, as you wish. Lets go with using SW_CAMERA_LENS_COVER for the 2 models with UVC_QUIRK_PRIVACY_DURING_STREAM too. <snip> >>>> Is there any ACPI- or WMI-provided information that could assist with >>>> associating a privacy GPIO with a camera ? I just realized I did not answer this question from Laurent in my previous reply. No unfortunately there is no ACPI- or WMI-provided information that could assist with associating ACPI/WMI camera privacy controls with a specific camera. Note that these are typically not exposed as a GPIO, but rather as some vendor firmware interface. Thinking more about this I'm starting to believe more and more that the privacy-control stuff should be handled by libcamera and then specifically by the pipeline-handler, with some helper code to share functionality where possible. E.g. on IPU6 equipped Windows laptops there may be some ACPI/WMI driver which provides a /dev/input/event# SW_CAMERA_LENS_COVER node. So I would expect the IPU6 pipeline-handler to search for such a /dev/input/event# node and then expose that to users of the camera through a to-be-defined API (I'm thinking a read-only control). The code to find the event node can be shared, because this would e.g. likely also apply to some IPU3 designs as well as upcoming IPU7 designs. <snip> >>>> We could include the evdev in the MC graph. That will of course only be >>>> possible if the kernel knows about that association in the first place. >>>> At least the 1st category of devices would benefit from this. >> >> Yes I was thinking about adding a link to the MC graph for this too. >> >> Ricardo I notice that in this v3 series you still create a v4l2-subdev >> for the GPIO handling and then add an ancillary link for the GPIO subdev >> to the mc-graph. But I'm not sure how that is helpful. Userspace would >> still need to do parent matching, but then match the evdev parent to >> the subdev after getting the subdev from the mc. In that case it might >> as well look at the physical (USB-interface) parent of the MC/video >> node and do parent matching on that avoiding the need to go through >> the MC at all. >> >> I think using the MC could still be useful by adding a new type of >> ancillary link to the MC API which provides a file-path as info to >> userspace rather then a mc-link and then just directly provide >> the /dev/input/event# path through this new API? >> >> I guess that extending the MC API like this might be a bit of >> a discussion. But it would already make sense to have this for >> the existing input device for the snapshot button. > > The driver creates a v4l2-subdevice for every entity, and the gpio > today is modeled as an entity. Ok I see that explains why the subdevice is there, thank you. > The patchset just adds an ancillary link as Sakari suggested. > I am not against removing the gpio entity all together if it is not needed. Right unlike other entities which are really part of the UVC specification, the GPIO is not a "real" UVC entity. So I wonder if, after switching to SW_CAMERA_LENS_COVER, having this as a v4l2-subdevice buys us anything ? If not I think removing it might be a good idea. As for the ancillary link, that was useful to have when the API was a v4l2-ctrl on the subdevice. Just like I doubt if having the subdevice at all gives us any added value, I also doubt if having the ancillary link gives us any added value. > Now that we are brainstorming here... what about adding a control that > contains the name of the input device (eventX)? Is that a horrible > idea? I don't know, my initial reaction is that does not feel right to me. >>>>>> We can specify >>>>>> that SW_CAMERA_LENS_COVER only applies to device internal >>>>>> cameras, but then it is up to userspace to determine which >>>>>> cameras that are. >>>>> >>>>> I am working on wiring up this to userspace right now.. I will report >>>>> back if it cannot do it. >> >> Ricardo, great, thank you! Ricardo, any status update on this ? <snip> >>>> Assuming the kernel could report the association between an evdev and >>>> camera, we would need to report which evdev SW_CAMERA_LENS_COVER >>>> originates from all the way from the evdev to the consumer of the event. >>>> How well is that supported in standard Linux system architectures ? If >>>> I'm not mistaken libinput will report the originating device, but how >>>> far up the stack is it propagated ? And which component would we expect >>>> to consume those events, should the camera evdev be managed by e.g. >>>> libcamera ? >> >> Good questions. Looking back at our 2 primary use-cases: >> >> a) Having an app which is using (trying to use) the camera show >> a notification to the user that the camera is turned-off by >> a privacy switch . >> >> b) Showing on on-screen-display (OSD) with a camera / >> crossed-out-camera icon when the switch is toggled, similar to how >> muting speakers/mic show an OSD . Laptop vendor Windows add-on >> software does this and I know that some users have been asking >> for this. >> >> I think we have everything to do b) in current compositors >> like gnome-shell. Using an evdev with SW_CAMERA_LENS_COVER >> would even be a lot easier for b) then the current >> V4L2_CID_PRIVACY API. >> >> a) though is a lot harder. We could open up access to >> the relevant /dev/input/event# node using a udev uaccess >> tag so that users who can access /dev/video# nodes also >> get raw access to that /dev/input/event# node and then >> libcamera could indeed provide this information that way. >> I think that is probably the best option. >> >> At least for the cases where the camera on/off switch >> does not simply make the camera completely disappear. >> >> That case is harder. atm that case is not handled at all >> though. So even just getting b) to work for that case >> would be nice / an improvement. >> >> Eventually if we give libcamera access to event# >> nodes which advertise SW_CAMERA_LENS_COVER (and no other >> privacy sensitive information) then libcamera could even >> separately offer some API for apps to just get that value >> if there is no camera to associate it with. >> >> Actually thinking more about it libcamera probably might >> be the right place for some sort of "no cameras found >> have you tried hitting your camera privacy-switch" API. >> That is some API to query if such a message should be >> shown to the user. But that is very much future work. > > Are standard apps expected to use libcamera directly or they should > use pipewire? > Maybe a) Should be pipewire's task? Standard apps are supposed to use pipewire, but IMHO this is really too low-level for pipewire to handle itself. Also see my remarks above about how I think this needs to be part of the pipeline handler. Since e.g. associating a /dev/input/event# SW_CAMERA_LENS_COVER node with a specific UVC camera is going to be UVC specific solution. For other pipeline-handlers combined with vendor fw-interfaces offering SW_CAMERA_LENS_COVER support I do not think that there is going to be a way to actually associate the 2. So we will likely simply have the pipeline handler for e.g. IPU6 simply associate any SW_CAMERA_LENS_COVER with the normal (non IR) user facing camera. Since we need this different ways to map a /dev/input/event# SW_CAMERA_LENS_COVER node to a specific camera this really needs to be done in libcamera IMHO. And I think this also solves the question about needing a kernel API to associate the /dev/input/event# with a specific /dev/video#. At least for now I think we don't need an API and instead we can simply walk sysfs to find the common USB-interface parent to associate the 2. See how xawtv associates the alsa and /dev/video# parts of tv-grabber cards for an example. Regards, Hans
On Mon, Nov 25, 2024 at 01:01:14PM +0100, Hans de Goede wrote: > On 18-Nov-24 5:47 PM, Ricardo Ribalda wrote: > > On Mon, 18 Nov 2024 at 16:43, Hans de Goede wrote: > >> On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote: > >>> On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart wrote: > > <snip> > > >>>> How do you handle cameras that suffer from > >>>> UVC_QUIRK_PRIVACY_DURING_STREAM ? > >>> > >>> For those b) does not work. > >> > >> I already suspected as much, but it is good to have this > >> confirmed. > >> > >> I'm afraid that from a userspace API pov cameras with a GPIO > >> which only works when powered-on need to be treated the same as > >> cameras which only have UVC_CT_PRIVACY_CONTROL IOW in this case > >> keep exporting V4L2_CID_PRIVACY instead of switching to evdev > >> with SW_CAMERA_LENS_COVER. > >> > >> Unfortunately this will make the GPIO handling code in the UVC > >> driver somewhat more involved since now we have both uAPI-s for > >> GPIOs depending on UVC_QUIRK_PRIVACY_DURING_STREAM. > >> > >> But I think that this makes sense, this way we end up offering > >> 2 uAPIs depending on the hw capabilities: > >> > >> 1. evdev with SW_CAMERA_LENS_COVER which always reports a reliable > >> state + events on the state changing without needing to power-up > >> the camera. > >> > >> 2. V4L2_CID_PRIVACY for the case where the camera needs to be > >> powered-on (/dev/video opened) and where the ctrl possibly needs > >> to be polled. > >> > >> Assuming we can all agree on this split based on hw capabilities > >> I think that we must document this somewhere in the media subsystem > >> documentation. We can then also write down there that > >> SW_CAMERA_LENS_COVER only applies to internal cameras. > > > > I do not think that it is worth it to keep UVC_CT_PRIVACY_CONTROL for > > the two devices that have connected the GPIO's pull up to the wrong > > power rail. > > Now that the GPIO can be used from userspace, I expect that those > > errors will be found early in the design process and never reach > > production stage. > > > > > > If we use UVC_CT_PRIVACY_CONTROL for thes two devices: > > - userspace will have to implement two different APIs > > - the driver will have to duplicate the code. > > - all that code will be very difficult to test: there are only 2 > > devices affected and it requires manual intervention to properly test > > it. > > > > I think that UVC_QUIRK_PRIVACY_DURING_STREAM is a good compromise and > > the main user handles it properly. > > Ok, as you wish. Lets go with using SW_CAMERA_LENS_COVER for the 2 models with > UVC_QUIRK_PRIVACY_DURING_STREAM too. > > <snip> > > >>>> Is there any ACPI- or WMI-provided information that could assist with > >>>> associating a privacy GPIO with a camera ? > > I just realized I did not answer this question from Laurent > in my previous reply. > > No unfortunately there is no ACPI- or WMI-provided information that > could assist with associating ACPI/WMI camera privacy controls with > a specific camera. Note that these are typically not exposed as a GPIO, > but rather as some vendor firmware interface. > > Thinking more about this I'm starting to believe more and more > that the privacy-control stuff should be handled by libcamera > and then specifically by the pipeline-handler, with some helper > code to share functionality where possible. > > E.g. on IPU6 equipped Windows laptops there may be some ACPI/WMI > driver which provides a /dev/input/event# SW_CAMERA_LENS_COVER node. Using an event device means that the user would need permissions to access it. Would distributions be able to tell the device apart from other event devices such as mouse/keyboard, where a logged user may not have permission to access all event devices in a multi-seat system ? Would compositors be able to ignore the device to let libcamera handle it ? > So I would expect the IPU6 pipeline-handler to search for such a > /dev/input/event# node and then expose that to users of the camera > through a to-be-defined API (I'm thinking a read-only control). > > The code to find the event node can be shared, because this would > e.g. likely also apply to some IPU3 designs as well as upcoming > IPU7 designs. > > <snip> > > >>>> We could include the evdev in the MC graph. That will of course only be > >>>> possible if the kernel knows about that association in the first place. > >>>> At least the 1st category of devices would benefit from this. > >> > >> Yes I was thinking about adding a link to the MC graph for this too. > >> > >> Ricardo I notice that in this v3 series you still create a v4l2-subdev > >> for the GPIO handling and then add an ancillary link for the GPIO subdev > >> to the mc-graph. But I'm not sure how that is helpful. Userspace would > >> still need to do parent matching, but then match the evdev parent to > >> the subdev after getting the subdev from the mc. In that case it might > >> as well look at the physical (USB-interface) parent of the MC/video > >> node and do parent matching on that avoiding the need to go through > >> the MC at all. > >> > >> I think using the MC could still be useful by adding a new type of > >> ancillary link to the MC API which provides a file-path as info to > >> userspace rather then a mc-link and then just directly provide > >> the /dev/input/event# path through this new API? I don't think we need that. MC can model any type of entity and report the device major:minor. That plus ancillary links should give us most of what we need, the only required addition should be a new MC entity function. > >> I guess that extending the MC API like this might be a bit of > >> a discussion. But it would already make sense to have this for > >> the existing input device for the snapshot button. > > > > The driver creates a v4l2-subdevice for every entity, and the gpio > > today is modeled as an entity. > > Ok I see that explains why the subdevice is there, thank you. > > > The patchset just adds an ancillary link as Sakari suggested. > > I am not against removing the gpio entity all together if it is not needed. > > Right unlike other entities which are really part of the UVC > specification, the GPIO is not a "real" UVC entity. > > So I wonder if, after switching to SW_CAMERA_LENS_COVER, having > this as a v4l2-subdevice buys us anything ? If not I think removing > it might be a good idea. > > As for the ancillary link, that was useful to have when the API > was a v4l2-ctrl on the subdevice. Just like I doubt if having > the subdevice at all gives us any added value, I also doubt if > having the ancillary link gives us any added value. > > > Now that we are brainstorming here... what about adding a control that > > contains the name of the input device (eventX)? Is that a horrible > > idea? > > I don't know, my initial reaction is that does not feel right to me. > > >>>>>> We can specify > >>>>>> that SW_CAMERA_LENS_COVER only applies to device internal > >>>>>> cameras, but then it is up to userspace to determine which > >>>>>> cameras that are. > >>>>> > >>>>> I am working on wiring up this to userspace right now.. I will report > >>>>> back if it cannot do it. > >> > >> Ricardo, great, thank you! > > Ricardo, any status update on this ? > > <snip> > > >>>> Assuming the kernel could report the association between an evdev and > >>>> camera, we would need to report which evdev SW_CAMERA_LENS_COVER > >>>> originates from all the way from the evdev to the consumer of the event. > >>>> How well is that supported in standard Linux system architectures ? If > >>>> I'm not mistaken libinput will report the originating device, but how > >>>> far up the stack is it propagated ? And which component would we expect > >>>> to consume those events, should the camera evdev be managed by e.g. > >>>> libcamera ? > >> > >> Good questions. Looking back at our 2 primary use-cases: > >> > >> a) Having an app which is using (trying to use) the camera show > >> a notification to the user that the camera is turned-off by > >> a privacy switch . > >> > >> b) Showing on on-screen-display (OSD) with a camera / > >> crossed-out-camera icon when the switch is toggled, similar to how > >> muting speakers/mic show an OSD . Laptop vendor Windows add-on > >> software does this and I know that some users have been asking > >> for this. > >> > >> I think we have everything to do b) in current compositors > >> like gnome-shell. Using an evdev with SW_CAMERA_LENS_COVER > >> would even be a lot easier for b) then the current > >> V4L2_CID_PRIVACY API. > >> > >> a) though is a lot harder. We could open up access to > >> the relevant /dev/input/event# node using a udev uaccess > >> tag so that users who can access /dev/video# nodes also > >> get raw access to that /dev/input/event# node and then > >> libcamera could indeed provide this information that way. > >> I think that is probably the best option. > >> > >> At least for the cases where the camera on/off switch > >> does not simply make the camera completely disappear. > >> > >> That case is harder. atm that case is not handled at all > >> though. So even just getting b) to work for that case > >> would be nice / an improvement. > >> > >> Eventually if we give libcamera access to event# > >> nodes which advertise SW_CAMERA_LENS_COVER (and no other > >> privacy sensitive information) then libcamera could even > >> separately offer some API for apps to just get that value > >> if there is no camera to associate it with. > >> > >> Actually thinking more about it libcamera probably might > >> be the right place for some sort of "no cameras found > >> have you tried hitting your camera privacy-switch" API. > >> That is some API to query if such a message should be > >> shown to the user. But that is very much future work. > > > > Are standard apps expected to use libcamera directly or they should > > use pipewire? > > Maybe a) Should be pipewire's task? > > Standard apps are supposed to use pipewire, but IMHO this is > really too low-level for pipewire to handle itself. > > Also see my remarks above about how I think this needs to > be part of the pipeline handler. Since e.g. associating > a /dev/input/event# SW_CAMERA_LENS_COVER node with a specific > UVC camera is going to be UVC specific solution. > > For other pipeline-handlers combined with vendor fw-interfaces > offering SW_CAMERA_LENS_COVER support I do not think that there > is going to be a way to actually associate the 2. So we will > likely simply have the pipeline handler for e.g. IPU6 simply > associate any SW_CAMERA_LENS_COVER with the normal (non IR) > user facing camera. > > Since we need this different ways to map a /dev/input/event# > SW_CAMERA_LENS_COVER node to a specific camera this really > needs to be done in libcamera IMHO. > > And I think this also solves the question about needing > a kernel API to associate the /dev/input/event# with > a specific /dev/video#. At least for now I think we don't > need an API and instead we can simply walk sysfs to find > the common USB-interface parent to associate the 2. > > See how xawtv associates the alsa and /dev/video# parts > of tv-grabber cards for an example.
Hi, On 25-Nov-24 2:14 PM, Laurent Pinchart wrote: > On Mon, Nov 25, 2024 at 01:01:14PM +0100, Hans de Goede wrote: >> On 18-Nov-24 5:47 PM, Ricardo Ribalda wrote: >>> On Mon, 18 Nov 2024 at 16:43, Hans de Goede wrote: >>>> On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote: >>>>> On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart wrote: <snip> >>>>>> Is there any ACPI- or WMI-provided information that could assist with >>>>>> associating a privacy GPIO with a camera ? >> >> I just realized I did not answer this question from Laurent >> in my previous reply. >> >> No unfortunately there is no ACPI- or WMI-provided information that >> could assist with associating ACPI/WMI camera privacy controls with >> a specific camera. Note that these are typically not exposed as a GPIO, >> but rather as some vendor firmware interface. >> >> Thinking more about this I'm starting to believe more and more >> that the privacy-control stuff should be handled by libcamera >> and then specifically by the pipeline-handler, with some helper >> code to share functionality where possible. >> >> E.g. on IPU6 equipped Windows laptops there may be some ACPI/WMI >> driver which provides a /dev/input/event# SW_CAMERA_LENS_COVER node. > > Using an event device means that the user would need permissions to > access it. Would distributions be able to tell the device apart from > other event devices such as mouse/keyboard, where a logged user may not > have permission to access all event devices in a multi-seat system ? input events modaliases contain a lot of info, including what sort of events they report, e.g. : [hans@shalem uvc]$ cat /sys/class/input/input36/modalias input:b0003v046Dp405Ee0111-e0,1,2,3,4,11,14,k71,72,73,74,75,77,78,79,7A,7B,7C,7D,7E,7F,80,81,82,83,84,85,86,87,88,89,8A,8B,8C,8E,8F,90,96,98,9B,9C,9E,9F,A1,A3,A4,A5,A6,A7,A8,A9,AB,AC,AD,AE,B0,B1,B2,B3,B4,B5,B6,B7,B8,B9,BA,BB,BC,BD,BE,BF,C0,C1,C2,CC,CE,CF,D0,D1,D2,D4,D8,D9,DB,DF,E0,E1,E4,E5,E6,E7,E8,E9,EA,EB,F0,F1,F4,100,110,111,112,113,114,115,116,117,118,119,11A,11B,11C,11D,11E,11F,161,162,166,16A,16E,172,174,176,177,178,179,17A,17B,17C,17D,17F,180,182,183,185,188,189,18C,18D,18E,18F,190,191,192,193,195,197,198,199,19A,19C,1A0,1A1,1A2,1A3,1A4,1A5,1A6,1A7,1A8,1A9,1AA,1AB,1AC,1AD,1AE,1AF,1B0,1B1,1B7,1BA,240,241,242,243,244,245,246,247,248,249,24A,24B,24C,24D,250,251,260,261,262,263,264,265,r0,1,6,8,B,C,a20,m4,l0,1,2,3,4,sfw So I believe that we can create a udev rule which matches on input devices with SW_CAMERA_LENS_COVER functionality and set a uaccess tag on those just like it is done for /dev/video# nodes. Or we can just use a specific input-device-name (sub) string and match on that. This may require using a separate input_device with just the SW_CAMERA_LENS_COVER functionality in some of the laptop ACPI / WMI drivers, but that is an acceptable compromise IMHO. (we don't want to report privacy sensitive input events on these nodes to avoid keylogging). > Would compositors be able to ignore the device to let libcamera handle > it ? input devices can be opened multiple times and we want the compositor to also open it to show camera on/off OSD icons / messages. If opened multiple times all listeners will get the events. >>>>>> We could include the evdev in the MC graph. That will of course only be >>>>>> possible if the kernel knows about that association in the first place. >>>>>> At least the 1st category of devices would benefit from this. >>>> >>>> Yes I was thinking about adding a link to the MC graph for this too. >>>> >>>> Ricardo I notice that in this v3 series you still create a v4l2-subdev >>>> for the GPIO handling and then add an ancillary link for the GPIO subdev >>>> to the mc-graph. But I'm not sure how that is helpful. Userspace would >>>> still need to do parent matching, but then match the evdev parent to >>>> the subdev after getting the subdev from the mc. In that case it might >>>> as well look at the physical (USB-interface) parent of the MC/video >>>> node and do parent matching on that avoiding the need to go through >>>> the MC at all. >>>> >>>> I think using the MC could still be useful by adding a new type of >>>> ancillary link to the MC API which provides a file-path as info to >>>> userspace rather then a mc-link and then just directly provide >>>> the /dev/input/event# path through this new API? > > I don't think we need that. MC can model any type of entity and report > the device major:minor. That plus ancillary links should give us most of > what we need, the only required addition should be a new MC entity > function. Ah interesting yes that should work nicely. Regards, Hans
On Mon, 25 Nov 2024 at 13:01, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Ricardo, > > On 18-Nov-24 5:47 PM, Ricardo Ribalda wrote: > > Hi Hans > > > > On Mon, 18 Nov 2024 at 16:43, Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi All, > >> > >> On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote: > >>> On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart > >>> <laurent.pinchart@ideasonboard.com> wrote: > > <snip> > > >>>> How do you handle cameras that suffer from > >>>> UVC_QUIRK_PRIVACY_DURING_STREAM ? > >>> > >>> For those b) does not work. > >> > >> I already suspected as much, but it is good to have this > >> confirmed. > >> > >> I'm afraid that from a userspace API pov cameras with a GPIO > >> which only works when powered-on need to be treated the same as > >> cameras which only have UVC_CT_PRIVACY_CONTROL IOW in this case > >> keep exporting V4L2_CID_PRIVACY instead of switching to evdev > >> with SW_CAMERA_LENS_COVER. > >> > >> Unfortunately this will make the GPIO handling code in the UVC > >> driver somewhat more involved since now we have both uAPI-s for > >> GPIOs depending on UVC_QUIRK_PRIVACY_DURING_STREAM. > >> > >> But I think that this makes sense, this way we end up offering > >> 2 uAPIs depending on the hw capabilities: > >> > >> 1. evdev with SW_CAMERA_LENS_COVER which always reports a reliable > >> state + events on the state changing without needing to power-up > >> the camera. > >> > >> 2. V4L2_CID_PRIVACY for the case where the camera needs to be > >> powered-on (/dev/video opened) and where the ctrl possibly needs > >> to be polled. > >> > >> Assuming we can all agree on this split based on hw capabilities > >> I think that we must document this somewhere in the media subsystem > >> documentation. We can then also write down there that > >> SW_CAMERA_LENS_COVER only applies to internal cameras. > > > > I do not think that it is worth it to keep UVC_CT_PRIVACY_CONTROL for > > the two devices that have connected the GPIO's pull up to the wrong > > power rail. > > Now that the GPIO can be used from userspace, I expect that those > > errors will be found early in the design process and never reach > > production stage. > > > > > > If we use UVC_CT_PRIVACY_CONTROL for thes two devices: > > - userspace will have to implement two different APIs > > - the driver will have to duplicate the code. > > - all that code will be very difficult to test: there are only 2 > > devices affected and it requires manual intervention to properly test > > it. > > > > I think that UVC_QUIRK_PRIVACY_DURING_STREAM is a good compromise and > > the main user handles it properly. > > Ok, as you wish. Lets go with using SW_CAMERA_LENS_COVER for the 2 models with > UVC_QUIRK_PRIVACY_DURING_STREAM too. > > <snip> > > >>>> Is there any ACPI- or WMI-provided information that could assist with > >>>> associating a privacy GPIO with a camera ? > > I just realized I did not answer this question from Laurent > in my previous reply. > > No unfortunately there is no ACPI- or WMI-provided information that > could assist with associating ACPI/WMI camera privacy controls with > a specific camera. Note that these are typically not exposed as a GPIO, > but rather as some vendor firmware interface. > > Thinking more about this I'm starting to believe more and more > that the privacy-control stuff should be handled by libcamera > and then specifically by the pipeline-handler, with some helper > code to share functionality where possible. > > E.g. on IPU6 equipped Windows laptops there may be some ACPI/WMI > driver which provides a /dev/input/event# SW_CAMERA_LENS_COVER node. > > So I would expect the IPU6 pipeline-handler to search for such a > /dev/input/event# node and then expose that to users of the camera > through a to-be-defined API (I'm thinking a read-only control). > > The code to find the event node can be shared, because this would > e.g. likely also apply to some IPU3 designs as well as upcoming > IPU7 designs. > > <snip> > > >>>> We could include the evdev in the MC graph. That will of course only be > >>>> possible if the kernel knows about that association in the first place. > >>>> At least the 1st category of devices would benefit from this. > >> > >> Yes I was thinking about adding a link to the MC graph for this too. > >> > >> Ricardo I notice that in this v3 series you still create a v4l2-subdev > >> for the GPIO handling and then add an ancillary link for the GPIO subdev > >> to the mc-graph. But I'm not sure how that is helpful. Userspace would > >> still need to do parent matching, but then match the evdev parent to > >> the subdev after getting the subdev from the mc. In that case it might > >> as well look at the physical (USB-interface) parent of the MC/video > >> node and do parent matching on that avoiding the need to go through > >> the MC at all. > >> > >> I think using the MC could still be useful by adding a new type of > >> ancillary link to the MC API which provides a file-path as info to > >> userspace rather then a mc-link and then just directly provide > >> the /dev/input/event# path through this new API? > >> > >> I guess that extending the MC API like this might be a bit of > >> a discussion. But it would already make sense to have this for > >> the existing input device for the snapshot button. > > > > The driver creates a v4l2-subdevice for every entity, and the gpio > > today is modeled as an entity. > > Ok I see that explains why the subdevice is there, thank you. > > > The patchset just adds an ancillary link as Sakari suggested. > > I am not against removing the gpio entity all together if it is not needed. > > Right unlike other entities which are really part of the UVC > specification, the GPIO is not a "real" UVC entity. > > So I wonder if, after switching to SW_CAMERA_LENS_COVER, having > this as a v4l2-subdevice buys us anything ? If not I think removing > it might be a good idea. > > As for the ancillary link, that was useful to have when the API > was a v4l2-ctrl on the subdevice. Just like I doubt if having > the subdevice at all gives us any added value, I also doubt if > having the ancillary link gives us any added value. > > > Now that we are brainstorming here... what about adding a control that > > contains the name of the input device (eventX)? Is that a horrible > > idea? > > I don't know, my initial reaction is that does not feel right to me. > > >>>>>> We can specify > >>>>>> that SW_CAMERA_LENS_COVER only applies to device internal > >>>>>> cameras, but then it is up to userspace to determine which > >>>>>> cameras that are. > >>>>> > >>>>> I am working on wiring up this to userspace right now.. I will report > >>>>> back if it cannot do it. > >> > >> Ricardo, great, thank you! > > Ricardo, any status update on this ? I still have not wired it to ChromeOS. But I do not expect to have any issues. it is relative simple to go from vdev to evdev and the other way around # ls -la /sys/class/video4linux/video0/device/input/input*/ drwxr-xr-x. 3 root root 0 Nov 25 06:56 event11 # ls -la /sys/class/input/event11/device/device/video4linux/ drwxr-xr-x. 3 root root 0 Nov 25 06:56 video0 drwxr-xr-x. 3 root root 0 Nov 25 06:56 video1 > > <snip> > > >>>> Assuming the kernel could report the association between an evdev and > >>>> camera, we would need to report which evdev SW_CAMERA_LENS_COVER > >>>> originates from all the way from the evdev to the consumer of the event. > >>>> How well is that supported in standard Linux system architectures ? If > >>>> I'm not mistaken libinput will report the originating device, but how > >>>> far up the stack is it propagated ? And which component would we expect > >>>> to consume those events, should the camera evdev be managed by e.g. > >>>> libcamera ? > >> > >> Good questions. Looking back at our 2 primary use-cases: > >> > >> a) Having an app which is using (trying to use) the camera show > >> a notification to the user that the camera is turned-off by > >> a privacy switch . > >> > >> b) Showing on on-screen-display (OSD) with a camera / > >> crossed-out-camera icon when the switch is toggled, similar to how > >> muting speakers/mic show an OSD . Laptop vendor Windows add-on > >> software does this and I know that some users have been asking > >> for this. > >> > >> I think we have everything to do b) in current compositors > >> like gnome-shell. Using an evdev with SW_CAMERA_LENS_COVER > >> would even be a lot easier for b) then the current > >> V4L2_CID_PRIVACY API. > >> > >> a) though is a lot harder. We could open up access to > >> the relevant /dev/input/event# node using a udev uaccess > >> tag so that users who can access /dev/video# nodes also > >> get raw access to that /dev/input/event# node and then > >> libcamera could indeed provide this information that way. > >> I think that is probably the best option. > >> > >> At least for the cases where the camera on/off switch > >> does not simply make the camera completely disappear. > >> > >> That case is harder. atm that case is not handled at all > >> though. So even just getting b) to work for that case > >> would be nice / an improvement. > >> > >> Eventually if we give libcamera access to event# > >> nodes which advertise SW_CAMERA_LENS_COVER (and no other > >> privacy sensitive information) then libcamera could even > >> separately offer some API for apps to just get that value > >> if there is no camera to associate it with. > >> > >> Actually thinking more about it libcamera probably might > >> be the right place for some sort of "no cameras found > >> have you tried hitting your camera privacy-switch" API. > >> That is some API to query if such a message should be > >> shown to the user. But that is very much future work. > > > > Are standard apps expected to use libcamera directly or they should > > use pipewire? > > Maybe a) Should be pipewire's task? > > Standard apps are supposed to use pipewire, but IMHO this is > really too low-level for pipewire to handle itself. > > Also see my remarks above about how I think this needs to > be part of the pipeline handler. Since e.g. associating > a /dev/input/event# SW_CAMERA_LENS_COVER node with a specific > UVC camera is going to be UVC specific solution. > > For other pipeline-handlers combined with vendor fw-interfaces > offering SW_CAMERA_LENS_COVER support I do not think that there > is going to be a way to actually associate the 2. So we will > likely simply have the pipeline handler for e.g. IPU6 simply > associate any SW_CAMERA_LENS_COVER with the normal (non IR) > user facing camera. > > Since we need this different ways to map a /dev/input/event# > SW_CAMERA_LENS_COVER node to a specific camera this really > needs to be done in libcamera IMHO. > > And I think this also solves the question about needing > a kernel API to associate the /dev/input/event# with > a specific /dev/video#. At least for now I think we don't > need an API and instead we can simply walk sysfs to find > the common USB-interface parent to associate the 2. > > See how xawtv associates the alsa and /dev/video# parts > of tv-grabber cards for an example. > > Regards, > > Hans > > >
On Mon, Nov 25, 2024 at 03:41:19PM +0100, Hans de Goede wrote: > Hi, > > On 25-Nov-24 2:14 PM, Laurent Pinchart wrote: > > On Mon, Nov 25, 2024 at 01:01:14PM +0100, Hans de Goede wrote: > >> On 18-Nov-24 5:47 PM, Ricardo Ribalda wrote: > >>> On Mon, 18 Nov 2024 at 16:43, Hans de Goede wrote: > >>>> On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote: > >>>>> On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart wrote: > > <snip> > > >>>>>> Is there any ACPI- or WMI-provided information that could assist with > >>>>>> associating a privacy GPIO with a camera ? > >> > >> I just realized I did not answer this question from Laurent > >> in my previous reply. > >> > >> No unfortunately there is no ACPI- or WMI-provided information that > >> could assist with associating ACPI/WMI camera privacy controls with > >> a specific camera. Note that these are typically not exposed as a GPIO, > >> but rather as some vendor firmware interface. > >> > >> Thinking more about this I'm starting to believe more and more > >> that the privacy-control stuff should be handled by libcamera > >> and then specifically by the pipeline-handler, with some helper > >> code to share functionality where possible. > >> > >> E.g. on IPU6 equipped Windows laptops there may be some ACPI/WMI > >> driver which provides a /dev/input/event# SW_CAMERA_LENS_COVER node. > > > > Using an event device means that the user would need permissions to > > access it. Would distributions be able to tell the device apart from > > other event devices such as mouse/keyboard, where a logged user may not > > have permission to access all event devices in a multi-seat system ? > > input events modaliases contain a lot of info, including what sort > of events they report, e.g. : > > [hans@shalem uvc]$ cat /sys/class/input/input36/modalias > input:b0003v046Dp405Ee0111-e0,1,2,3,4,11,14,k71,72,73,74,75,77,78,79,7A,7B,7C,7D,7E,7F,80,81,82,83,84,85,86,87,88,89,8A,8B,8C,8E,8F,90,96,98,9B,9C,9E,9F,A1,A3,A4,A5,A6,A7,A8,A9,AB,AC,AD,AE,B0,B1,B2,B3,B4,B5,B6,B7,B8,B9,BA,BB,BC,BD,BE,BF,C0,C1,C2,CC,CE,CF,D0,D1,D2,D4,D8,D9,DB,DF,E0,E1,E4,E5,E6,E7,E8,E9,EA,EB,F0,F1,F4,100,110,111,112,113,114,115,116,117,118,119,11A,11B,11C,11D,11E,11F,161,162,166,16A,16E,172,174,176,177,178,179,17A,17B,17C,17D,17F,180,182,183,185,188,189,18C,18D,18E,18F,190,191,192,193,195,197,198,199,19A,19C,1A0,1A1,1A2,1A3,1A4,1A5,1A6,1A7,1A8,1A9,1AA,1AB,1AC,1AD,1AE,1AF,1B0,1B1,1B7,1BA,240,241,242,243,244,245,246,247,248,249,24A,24B,24C,24D,250,251,260,261,262,263,264,265,r0,1,6,8,B,C,a20,m4,l0,1,2,3,4,sfw > > So I believe that we can create a udev rule which matches on input > devices with SW_CAMERA_LENS_COVER functionality and set a uaccess > tag on those just like it is done for /dev/video# nodes. > > Or we can just use a specific input-device-name (sub) string > and match on that. > > This may require using a separate input_device with just > the SW_CAMERA_LENS_COVER functionality in some of the laptop > ACPI / WMI drivers, but that is an acceptable compromise IMHO. As long as it's doable I'm OK with it. > (we don't want to report privacy sensitive input events on > these nodes to avoid keylogging). > > > Would compositors be able to ignore the device to let libcamera handle > > it ? > > input devices can be opened multiple times and we want the compositor > to also open it to show camera on/off OSD icons / messages. I'm not sure we want that though, as the event should be associated with a particular camera in messages. It would be better if it still went through libcamera and pipewire. > If opened multiple times all listeners will get the events. > > >>>>>> We could include the evdev in the MC graph. That will of course only be > >>>>>> possible if the kernel knows about that association in the first place. > >>>>>> At least the 1st category of devices would benefit from this. > >>>> > >>>> Yes I was thinking about adding a link to the MC graph for this too. > >>>> > >>>> Ricardo I notice that in this v3 series you still create a v4l2-subdev > >>>> for the GPIO handling and then add an ancillary link for the GPIO subdev > >>>> to the mc-graph. But I'm not sure how that is helpful. Userspace would > >>>> still need to do parent matching, but then match the evdev parent to > >>>> the subdev after getting the subdev from the mc. In that case it might > >>>> as well look at the physical (USB-interface) parent of the MC/video > >>>> node and do parent matching on that avoiding the need to go through > >>>> the MC at all. > >>>> > >>>> I think using the MC could still be useful by adding a new type of > >>>> ancillary link to the MC API which provides a file-path as info to > >>>> userspace rather then a mc-link and then just directly provide > >>>> the /dev/input/event# path through this new API? > > > > I don't think we need that. MC can model any type of entity and report > > the device major:minor. That plus ancillary links should give us most of > > what we need, the only required addition should be a new MC entity > > function. > > Ah interesting yes that should work nicely.
On Mon, 25 Nov 2024 at 22:35, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Mon, Nov 25, 2024 at 03:41:19PM +0100, Hans de Goede wrote: > > Hi, > > > > On 25-Nov-24 2:14 PM, Laurent Pinchart wrote: > > > On Mon, Nov 25, 2024 at 01:01:14PM +0100, Hans de Goede wrote: > > >> On 18-Nov-24 5:47 PM, Ricardo Ribalda wrote: > > >>> On Mon, 18 Nov 2024 at 16:43, Hans de Goede wrote: > > >>>> On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote: > > >>>>> On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart wrote: > > > > <snip> > > > > >>>>>> Is there any ACPI- or WMI-provided information that could assist with > > >>>>>> associating a privacy GPIO with a camera ? > > >> > > >> I just realized I did not answer this question from Laurent > > >> in my previous reply. > > >> > > >> No unfortunately there is no ACPI- or WMI-provided information that > > >> could assist with associating ACPI/WMI camera privacy controls with > > >> a specific camera. Note that these are typically not exposed as a GPIO, > > >> but rather as some vendor firmware interface. > > >> > > >> Thinking more about this I'm starting to believe more and more > > >> that the privacy-control stuff should be handled by libcamera > > >> and then specifically by the pipeline-handler, with some helper > > >> code to share functionality where possible. > > >> > > >> E.g. on IPU6 equipped Windows laptops there may be some ACPI/WMI > > >> driver which provides a /dev/input/event# SW_CAMERA_LENS_COVER node. > > > > > > Using an event device means that the user would need permissions to > > > access it. Would distributions be able to tell the device apart from > > > other event devices such as mouse/keyboard, where a logged user may not > > > have permission to access all event devices in a multi-seat system ? > > > > input events modaliases contain a lot of info, including what sort > > of events they report, e.g. : > > > > [hans@shalem uvc]$ cat /sys/class/input/input36/modalias > > input:b0003v046Dp405Ee0111-e0,1,2,3,4,11,14,k71,72,73,74,75,77,78,79,7A,7B,7C,7D,7E,7F,80,81,82,83,84,85,86,87,88,89,8A,8B,8C,8E,8F,90,96,98,9B,9C,9E,9F,A1,A3,A4,A5,A6,A7,A8,A9,AB,AC,AD,AE,B0,B1,B2,B3,B4,B5,B6,B7,B8,B9,BA,BB,BC,BD,BE,BF,C0,C1,C2,CC,CE,CF,D0,D1,D2,D4,D8,D9,DB,DF,E0,E1,E4,E5,E6,E7,E8,E9,EA,EB,F0,F1,F4,100,110,111,112,113,114,115,116,117,118,119,11A,11B,11C,11D,11E,11F,161,162,166,16A,16E,172,174,176,177,178,179,17A,17B,17C,17D,17F,180,182,183,185,188,189,18C,18D,18E,18F,190,191,192,193,195,197,198,199,19A,19C,1A0,1A1,1A2,1A3,1A4,1A5,1A6,1A7,1A8,1A9,1AA,1AB,1AC,1AD,1AE,1AF,1B0,1B1,1B7,1BA,240,241,242,243,244,245,246,247,248,249,24A,24B,24C,24D,250,251,260,261,262,263,264,265,r0,1,6,8,B,C,a20,m4,l0,1,2,3,4,sfw > > > > So I believe that we can create a udev rule which matches on input > > devices with SW_CAMERA_LENS_COVER functionality and set a uaccess > > tag on those just like it is done for /dev/video# nodes. > > > > Or we can just use a specific input-device-name (sub) string > > and match on that. > > > > This may require using a separate input_device with just > > the SW_CAMERA_LENS_COVER functionality in some of the laptop > > ACPI / WMI drivers, but that is an acceptable compromise IMHO. > > As long as it's doable I'm OK with it. > > > (we don't want to report privacy sensitive input events on > > these nodes to avoid keylogging). > > > > > Would compositors be able to ignore the device to let libcamera handle > > > it ? > > > > input devices can be opened multiple times and we want the compositor > > to also open it to show camera on/off OSD icons / messages. > > I'm not sure we want that though, as the event should be associated with > a particular camera in messages. It would be better if it still went > through libcamera and pipewire. For OSD we do not necessarily need to know what camera the GPIO is associated with. We just want to give instant feedback about a button on their device. Eg in ChromeOS we just say: "camera off" not "user facing camera off" > > > If opened multiple times all listeners will get the events. > > > > >>>>>> We could include the evdev in the MC graph. That will of course only be > > >>>>>> possible if the kernel knows about that association in the first place. > > >>>>>> At least the 1st category of devices would benefit from this. > > >>>> > > >>>> Yes I was thinking about adding a link to the MC graph for this too. > > >>>> > > >>>> Ricardo I notice that in this v3 series you still create a v4l2-subdev > > >>>> for the GPIO handling and then add an ancillary link for the GPIO subdev > > >>>> to the mc-graph. But I'm not sure how that is helpful. Userspace would > > >>>> still need to do parent matching, but then match the evdev parent to > > >>>> the subdev after getting the subdev from the mc. In that case it might > > >>>> as well look at the physical (USB-interface) parent of the MC/video > > >>>> node and do parent matching on that avoiding the need to go through > > >>>> the MC at all. > > >>>> > > >>>> I think using the MC could still be useful by adding a new type of > > >>>> ancillary link to the MC API which provides a file-path as info to > > >>>> userspace rather then a mc-link and then just directly provide > > >>>> the /dev/input/event# path through this new API? > > > > > > I don't think we need that. MC can model any type of entity and report > > > the device major:minor. That plus ancillary links should give us most of > > > what we need, the only required addition should be a new MC entity > > > function. > > > > Ah interesting yes that should work nicely. > > -- > Regards, > > Laurent Pinchart
On Tue, Nov 26, 2024 at 05:27:57PM +0100, Ricardo Ribalda wrote: > On Mon, 25 Nov 2024 at 22:35, Laurent Pinchart wrote: > > On Mon, Nov 25, 2024 at 03:41:19PM +0100, Hans de Goede wrote: > > > On 25-Nov-24 2:14 PM, Laurent Pinchart wrote: > > > > On Mon, Nov 25, 2024 at 01:01:14PM +0100, Hans de Goede wrote: > > > >> On 18-Nov-24 5:47 PM, Ricardo Ribalda wrote: > > > >>> On Mon, 18 Nov 2024 at 16:43, Hans de Goede wrote: > > > >>>> On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote: > > > >>>>> On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart wrote: > > > > > > <snip> > > > > > > >>>>>> Is there any ACPI- or WMI-provided information that could assist with > > > >>>>>> associating a privacy GPIO with a camera ? > > > >> > > > >> I just realized I did not answer this question from Laurent > > > >> in my previous reply. > > > >> > > > >> No unfortunately there is no ACPI- or WMI-provided information that > > > >> could assist with associating ACPI/WMI camera privacy controls with > > > >> a specific camera. Note that these are typically not exposed as a GPIO, > > > >> but rather as some vendor firmware interface. > > > >> > > > >> Thinking more about this I'm starting to believe more and more > > > >> that the privacy-control stuff should be handled by libcamera > > > >> and then specifically by the pipeline-handler, with some helper > > > >> code to share functionality where possible. > > > >> > > > >> E.g. on IPU6 equipped Windows laptops there may be some ACPI/WMI > > > >> driver which provides a /dev/input/event# SW_CAMERA_LENS_COVER node. > > > > > > > > Using an event device means that the user would need permissions to > > > > access it. Would distributions be able to tell the device apart from > > > > other event devices such as mouse/keyboard, where a logged user may not > > > > have permission to access all event devices in a multi-seat system ? > > > > > > input events modaliases contain a lot of info, including what sort > > > of events they report, e.g. : > > > > > > [hans@shalem uvc]$ cat /sys/class/input/input36/modalias > > > input:b0003v046Dp405Ee0111-e0,1,2,3,4,11,14,k71,72,73,74,75,77,78,79,7A,7B,7C,7D,7E,7F,80,81,82,83,84,85,86,87,88,89,8A,8B,8C,8E,8F,90,96,98,9B,9C,9E,9F,A1,A3,A4,A5,A6,A7,A8,A9,AB,AC,AD,AE,B0,B1,B2,B3,B4,B5,B6,B7,B8,B9,BA,BB,BC,BD,BE,BF,C0,C1,C2,CC,CE,CF,D0,D1,D2,D4,D8,D9,DB,DF,E0,E1,E4,E5,E6,E7,E8,E9,EA,EB,F0,F1,F4,100,110,111,112,113,114,115,116,117,118,119,11A,11B,11C,11D,11E,11F,161,162,166,16A,16E,172,174,176,177,178,179,17A,17B,17C,17D,17F,180,182,183,185,188,189,18C,18D,18E,18F,190,191,192,193,195,197,198,199,19A,19C,1A0,1A1,1A2,1A3,1A4,1A5,1A6,1A7,1A8,1A9,1AA,1AB,1AC,1AD,1AE,1AF,1B0,1B1,1B7,1BA,240,241,242,243,244,245,246,247,248,249,24A,24B,24C,24D,250,251,260,261,262,263,264,265,r0,1,6,8,B,C,a20,m4,l0,1,2,3,4,sfw > > > > > > So I believe that we can create a udev rule which matches on input > > > devices with SW_CAMERA_LENS_COVER functionality and set a uaccess > > > tag on those just like it is done for /dev/video# nodes. > > > > > > Or we can just use a specific input-device-name (sub) string > > > and match on that. > > > > > > This may require using a separate input_device with just > > > the SW_CAMERA_LENS_COVER functionality in some of the laptop > > > ACPI / WMI drivers, but that is an acceptable compromise IMHO. > > > > As long as it's doable I'm OK with it. > > > > > (we don't want to report privacy sensitive input events on > > > these nodes to avoid keylogging). > > > > > > > Would compositors be able to ignore the device to let libcamera handle > > > > it ? > > > > > > input devices can be opened multiple times and we want the compositor > > > to also open it to show camera on/off OSD icons / messages. > > > > I'm not sure we want that though, as the event should be associated with > > a particular camera in messages. It would be better if it still went > > through libcamera and pipewire. > > For OSD we do not necessarily need to know what camera the GPIO is > associated with. > > We just want to give instant feedback about a button on their device. > Eg in ChromeOS we just say: "camera off" not "user facing camera off" That may be true of Chrome OS, but in general, other systems may want to provide more detailed information. I wouldn't model the API and architecture just on Chrome OS. > > > If opened multiple times all listeners will get the events. > > > > > > >>>>>> We could include the evdev in the MC graph. That will of course only be > > > >>>>>> possible if the kernel knows about that association in the first place. > > > >>>>>> At least the 1st category of devices would benefit from this. > > > >>>> > > > >>>> Yes I was thinking about adding a link to the MC graph for this too. > > > >>>> > > > >>>> Ricardo I notice that in this v3 series you still create a v4l2-subdev > > > >>>> for the GPIO handling and then add an ancillary link for the GPIO subdev > > > >>>> to the mc-graph. But I'm not sure how that is helpful. Userspace would > > > >>>> still need to do parent matching, but then match the evdev parent to > > > >>>> the subdev after getting the subdev from the mc. In that case it might > > > >>>> as well look at the physical (USB-interface) parent of the MC/video > > > >>>> node and do parent matching on that avoiding the need to go through > > > >>>> the MC at all. > > > >>>> > > > >>>> I think using the MC could still be useful by adding a new type of > > > >>>> ancillary link to the MC API which provides a file-path as info to > > > >>>> userspace rather then a mc-link and then just directly provide > > > >>>> the /dev/input/event# path through this new API? > > > > > > > > I don't think we need that. MC can model any type of entity and report > > > > the device major:minor. That plus ancillary links should give us most of > > > > what we need, the only required addition should be a new MC entity > > > > function. > > > > > > Ah interesting yes that should work nicely.
On Tue, 26 Nov 2024 at 17:51, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Nov 26, 2024 at 05:27:57PM +0100, Ricardo Ribalda wrote: > > On Mon, 25 Nov 2024 at 22:35, Laurent Pinchart wrote: > > > On Mon, Nov 25, 2024 at 03:41:19PM +0100, Hans de Goede wrote: > > > > On 25-Nov-24 2:14 PM, Laurent Pinchart wrote: > > > > > On Mon, Nov 25, 2024 at 01:01:14PM +0100, Hans de Goede wrote: > > > > >> On 18-Nov-24 5:47 PM, Ricardo Ribalda wrote: > > > > >>> On Mon, 18 Nov 2024 at 16:43, Hans de Goede wrote: > > > > >>>> On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote: > > > > >>>>> On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart wrote: > > > > > > > > <snip> > > > > > > > > >>>>>> Is there any ACPI- or WMI-provided information that could assist with > > > > >>>>>> associating a privacy GPIO with a camera ? > > > > >> > > > > >> I just realized I did not answer this question from Laurent > > > > >> in my previous reply. > > > > >> > > > > >> No unfortunately there is no ACPI- or WMI-provided information that > > > > >> could assist with associating ACPI/WMI camera privacy controls with > > > > >> a specific camera. Note that these are typically not exposed as a GPIO, > > > > >> but rather as some vendor firmware interface. > > > > >> > > > > >> Thinking more about this I'm starting to believe more and more > > > > >> that the privacy-control stuff should be handled by libcamera > > > > >> and then specifically by the pipeline-handler, with some helper > > > > >> code to share functionality where possible. > > > > >> > > > > >> E.g. on IPU6 equipped Windows laptops there may be some ACPI/WMI > > > > >> driver which provides a /dev/input/event# SW_CAMERA_LENS_COVER node. > > > > > > > > > > Using an event device means that the user would need permissions to > > > > > access it. Would distributions be able to tell the device apart from > > > > > other event devices such as mouse/keyboard, where a logged user may not > > > > > have permission to access all event devices in a multi-seat system ? > > > > > > > > input events modaliases contain a lot of info, including what sort > > > > of events they report, e.g. : > > > > > > > > [hans@shalem uvc]$ cat /sys/class/input/input36/modalias > > > > input:b0003v046Dp405Ee0111-e0,1,2,3,4,11,14,k71,72,73,74,75,77,78,79,7A,7B,7C,7D,7E,7F,80,81,82,83,84,85,86,87,88,89,8A,8B,8C,8E,8F,90,96,98,9B,9C,9E,9F,A1,A3,A4,A5,A6,A7,A8,A9,AB,AC,AD,AE,B0,B1,B2,B3,B4,B5,B6,B7,B8,B9,BA,BB,BC,BD,BE,BF,C0,C1,C2,CC,CE,CF,D0,D1,D2,D4,D8,D9,DB,DF,E0,E1,E4,E5,E6,E7,E8,E9,EA,EB,F0,F1,F4,100,110,111,112,113,114,115,116,117,118,119,11A,11B,11C,11D,11E,11F,161,162,166,16A,16E,172,174,176,177,178,179,17A,17B,17C,17D,17F,180,182,183,185,188,189,18C,18D,18E,18F,190,191,192,193,195,197,198,199,19A,19C,1A0,1A1,1A2,1A3,1A4,1A5,1A6,1A7,1A8,1A9,1AA,1AB,1AC,1AD,1AE,1AF,1B0,1B1,1B7,1BA,240,241,242,243,244,245,246,247,248,249,24A,24B,24C,24D,250,251,260,261,262,263,264,265,r0,1,6,8,B,C,a20,m4,l0,1,2,3,4,sfw > > > > > > > > So I believe that we can create a udev rule which matches on input > > > > devices with SW_CAMERA_LENS_COVER functionality and set a uaccess > > > > tag on those just like it is done for /dev/video# nodes. > > > > > > > > Or we can just use a specific input-device-name (sub) string > > > > and match on that. > > > > > > > > This may require using a separate input_device with just > > > > the SW_CAMERA_LENS_COVER functionality in some of the laptop > > > > ACPI / WMI drivers, but that is an acceptable compromise IMHO. > > > > > > As long as it's doable I'm OK with it. > > > > > > > (we don't want to report privacy sensitive input events on > > > > these nodes to avoid keylogging). > > > > > > > > > Would compositors be able to ignore the device to let libcamera handle > > > > > it ? > > > > > > > > input devices can be opened multiple times and we want the compositor > > > > to also open it to show camera on/off OSD icons / messages. > > > > > > I'm not sure we want that though, as the event should be associated with > > > a particular camera in messages. It would be better if it still went > > > through libcamera and pipewire. > > > > For OSD we do not necessarily need to know what camera the GPIO is > > associated with. > > > > We just want to give instant feedback about a button on their device. > > Eg in ChromeOS we just say: "camera off" not "user facing camera off" > > That may be true of Chrome OS, but in general, other systems may want to > provide more detailed information. I wouldn't model the API and > architecture just on Chrome OS. It is not about ChromeOS, it is about the use case. We were talking about 2 usecases: - instant feedback for a button. Actor: OSD / composer - this camera is disabled, please use other camera or enable it: Actor camera app, or camera "service" (read pipewire, libcamera, or the permission handler for snap) There are some examples showing that for "instant feedback" there is no need to link the event to the camera: - there is hardware where this is not possible to establish the link. - ChromeOS does not show the camera name (when it has enough information to do so) - I believe Hans mentioned that Windows does not show the camera name. - (Hans, are you wiring SW_CAMERA_LENS_COVER to the user right now?) Do you know of a system where this info is needed? My problem is that I do not see where libcamera fits for the "instant feedback" usecase: - libcamera will be running as a service and telling the UI that the camera is disabled? how will it communicate with the OS? - the OS will run a "libcamera helper" every second to get the switch status for every camera? - the OS will wait for an input event and run a "libcamera helper" to find the correlation with the camera? I think it is simpler that the OS just waits for an SW_CAMERA_LENS_COVER event and display "camera off". The same way it waits for "caps lock" today In any case: - for uvc, it seems like it is easy to go from evdev to videodev (and the other way around). Check my previous email - udev seems to have a lot of information about the evdev to configure the permissions in a way that cover most (all?) of the usecases/architectures > > > > > If opened multiple times all listeners will get the events. > > > > > > > > >>>>>> We could include the evdev in the MC graph. That will of course only be > > > > >>>>>> possible if the kernel knows about that association in the first place. > > > > >>>>>> At least the 1st category of devices would benefit from this. > > > > >>>> > > > > >>>> Yes I was thinking about adding a link to the MC graph for this too. > > > > >>>> > > > > >>>> Ricardo I notice that in this v3 series you still create a v4l2-subdev > > > > >>>> for the GPIO handling and then add an ancillary link for the GPIO subdev > > > > >>>> to the mc-graph. But I'm not sure how that is helpful. Userspace would > > > > >>>> still need to do parent matching, but then match the evdev parent to > > > > >>>> the subdev after getting the subdev from the mc. In that case it might > > > > >>>> as well look at the physical (USB-interface) parent of the MC/video > > > > >>>> node and do parent matching on that avoiding the need to go through > > > > >>>> the MC at all. > > > > >>>> > > > > >>>> I think using the MC could still be useful by adding a new type of > > > > >>>> ancillary link to the MC API which provides a file-path as info to > > > > >>>> userspace rather then a mc-link and then just directly provide > > > > >>>> the /dev/input/event# path through this new API? > > > > > > > > > > I don't think we need that. MC can model any type of entity and report > > > > > the device major:minor. That plus ancillary links should give us most of > > > > > what we need, the only required addition should be a new MC entity > > > > > function. > > > > > > > > Ah interesting yes that should work nicely. > > -- > Regards, > > Laurent Pinchart
On Tue, Nov 26, 2024 at 06:12:46PM +0100, Ricardo Ribalda wrote: > On Tue, 26 Nov 2024 at 17:51, Laurent Pinchart wrote: > > On Tue, Nov 26, 2024 at 05:27:57PM +0100, Ricardo Ribalda wrote: > > > On Mon, 25 Nov 2024 at 22:35, Laurent Pinchart wrote: > > > > On Mon, Nov 25, 2024 at 03:41:19PM +0100, Hans de Goede wrote: > > > > > On 25-Nov-24 2:14 PM, Laurent Pinchart wrote: > > > > > > On Mon, Nov 25, 2024 at 01:01:14PM +0100, Hans de Goede wrote: > > > > > >> On 18-Nov-24 5:47 PM, Ricardo Ribalda wrote: > > > > > >>> On Mon, 18 Nov 2024 at 16:43, Hans de Goede wrote: > > > > > >>>> On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote: > > > > > >>>>> On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart wrote: > > > > > > > > > > <snip> > > > > > > > > > > >>>>>> Is there any ACPI- or WMI-provided information that could assist with > > > > > >>>>>> associating a privacy GPIO with a camera ? > > > > > >> > > > > > >> I just realized I did not answer this question from Laurent > > > > > >> in my previous reply. > > > > > >> > > > > > >> No unfortunately there is no ACPI- or WMI-provided information that > > > > > >> could assist with associating ACPI/WMI camera privacy controls with > > > > > >> a specific camera. Note that these are typically not exposed as a GPIO, > > > > > >> but rather as some vendor firmware interface. > > > > > >> > > > > > >> Thinking more about this I'm starting to believe more and more > > > > > >> that the privacy-control stuff should be handled by libcamera > > > > > >> and then specifically by the pipeline-handler, with some helper > > > > > >> code to share functionality where possible. > > > > > >> > > > > > >> E.g. on IPU6 equipped Windows laptops there may be some ACPI/WMI > > > > > >> driver which provides a /dev/input/event# SW_CAMERA_LENS_COVER node. > > > > > > > > > > > > Using an event device means that the user would need permissions to > > > > > > access it. Would distributions be able to tell the device apart from > > > > > > other event devices such as mouse/keyboard, where a logged user may not > > > > > > have permission to access all event devices in a multi-seat system ? > > > > > > > > > > input events modaliases contain a lot of info, including what sort > > > > > of events they report, e.g. : > > > > > > > > > > [hans@shalem uvc]$ cat /sys/class/input/input36/modalias > > > > > input:b0003v046Dp405Ee0111-e0,1,2,3,4,11,14,k71,72,73,74,75,77,78,79,7A,7B,7C,7D,7E,7F,80,81,82,83,84,85,86,87,88,89,8A,8B,8C,8E,8F,90,96,98,9B,9C,9E,9F,A1,A3,A4,A5,A6,A7,A8,A9,AB,AC,AD,AE,B0,B1,B2,B3,B4,B5,B6,B7,B8,B9,BA,BB,BC,BD,BE,BF,C0,C1,C2,CC,CE,CF,D0,D1,D2,D4,D8,D9,DB,DF,E0,E1,E4,E5,E6,E7,E8,E9,EA,EB,F0,F1,F4,100,110,111,112,113,114,115,116,117,118,119,11A,11B,11C,11D,11E,11F,161,162,166,16A,16E,172,174,176,177,178,179,17A,17B,17C,17D,17F,180,182,183,185,188,189,18C,18D,18E,18F,190,191,192,193,195,197,198,199,19A,19C,1A0,1A1,1A2,1A3,1A4,1A5,1A6,1A7,1A8,1A9,1AA,1AB,1AC,1AD,1AE,1AF,1B0,1B1,1B7,1BA,240,241,242,243,244,245,246,247,248,249,24A,24B,24C,24D,250,251,260,261,262,263,264,265,r0,1,6,8,B,C,a20,m4,l0,1,2,3,4,sfw > > > > > > > > > > So I believe that we can create a udev rule which matches on input > > > > > devices with SW_CAMERA_LENS_COVER functionality and set a uaccess > > > > > tag on those just like it is done for /dev/video# nodes. > > > > > > > > > > Or we can just use a specific input-device-name (sub) string > > > > > and match on that. > > > > > > > > > > This may require using a separate input_device with just > > > > > the SW_CAMERA_LENS_COVER functionality in some of the laptop > > > > > ACPI / WMI drivers, but that is an acceptable compromise IMHO. > > > > > > > > As long as it's doable I'm OK with it. > > > > > > > > > (we don't want to report privacy sensitive input events on > > > > > these nodes to avoid keylogging). > > > > > > > > > > > Would compositors be able to ignore the device to let libcamera handle > > > > > > it ? > > > > > > > > > > input devices can be opened multiple times and we want the compositor > > > > > to also open it to show camera on/off OSD icons / messages. > > > > > > > > I'm not sure we want that though, as the event should be associated with > > > > a particular camera in messages. It would be better if it still went > > > > through libcamera and pipewire. > > > > > > For OSD we do not necessarily need to know what camera the GPIO is > > > associated with. > > > > > > We just want to give instant feedback about a button on their device. > > > Eg in ChromeOS we just say: "camera off" not "user facing camera off" > > > > That may be true of Chrome OS, but in general, other systems may want to > > provide more detailed information. I wouldn't model the API and > > architecture just on Chrome OS. > > It is not about ChromeOS, it is about the use case. > > We were talking about 2 usecases: > - instant feedback for a button. Actor: OSD / composer > - this camera is disabled, please use other camera or enable it: Actor > camera app, or camera "service" (read pipewire, libcamera, or the > permission handler for snap) > > There are some examples showing that for "instant feedback" there is > no need to link the event to the camera: > - there is hardware where this is not possible to establish the link. > - ChromeOS does not show the camera name (when it has enough > information to do so) > - I believe Hans mentioned that Windows does not show the camera name. > - (Hans, are you wiring SW_CAMERA_LENS_COVER to the user right now?) > Do you know of a system where this info is needed? > > My problem is that I do not see where libcamera fits for the "instant > feedback" usecase: > - libcamera will be running as a service and telling the UI that the > camera is disabled? how will it communicate with the OS? Not libcamera itself, but a camera service on top of it. For typical desktop cases, that would be pipewire. I don't know how it communicates with other actors, that's not my area of expertise, but I would be surprised if it wouldn't be able to. > - the OS will run a "libcamera helper" every second to get the switch > status for every camera? > - the OS will wait for an input event and run a "libcamera helper" to > find the correlation with the camera? > > I think it is simpler that the OS just waits for an > SW_CAMERA_LENS_COVER event and display "camera off". The same way it > waits for "caps lock" today > > In any case: > - for uvc, it seems like it is easy to go from evdev to videodev (and > the other way around). Check my previous email > - udev seems to have a lot of information about the evdev to configure > the permissions in a way that cover most (all?) of the > usecases/architectures > > > > > > If opened multiple times all listeners will get the events. > > > > > > > > > > >>>>>> We could include the evdev in the MC graph. That will of course only be > > > > > >>>>>> possible if the kernel knows about that association in the first place. > > > > > >>>>>> At least the 1st category of devices would benefit from this. > > > > > >>>> > > > > > >>>> Yes I was thinking about adding a link to the MC graph for this too. > > > > > >>>> > > > > > >>>> Ricardo I notice that in this v3 series you still create a v4l2-subdev > > > > > >>>> for the GPIO handling and then add an ancillary link for the GPIO subdev > > > > > >>>> to the mc-graph. But I'm not sure how that is helpful. Userspace would > > > > > >>>> still need to do parent matching, but then match the evdev parent to > > > > > >>>> the subdev after getting the subdev from the mc. In that case it might > > > > > >>>> as well look at the physical (USB-interface) parent of the MC/video > > > > > >>>> node and do parent matching on that avoiding the need to go through > > > > > >>>> the MC at all. > > > > > >>>> > > > > > >>>> I think using the MC could still be useful by adding a new type of > > > > > >>>> ancillary link to the MC API which provides a file-path as info to > > > > > >>>> userspace rather then a mc-link and then just directly provide > > > > > >>>> the /dev/input/event# path through this new API? > > > > > > > > > > > > I don't think we need that. MC can model any type of entity and report > > > > > > the device major:minor. That plus ancillary links should give us most of > > > > > > what we need, the only required addition should be a new MC entity > > > > > > function. > > > > > > > > > > Ah interesting yes that should work nicely.
Some notebooks have a button to disable the camera (not to be mistaken with the mechanical cover). This is a standard GPIO linked to the camera via the ACPI table. 4 years ago we added support for this button in UVC via the Privacy control. This has three issues: - If the camera has its own privacy control, it will be masked. - We need to power-up the camera to read the privacy control gpio. - Other drivers have not followed this approach and have used evdev. We tried to fix the power-up issues implementing "granular power saving" but it has been more complicated than anticipated... This patchset implements the Privacy GPIO as a evdev. The first patch of this set is already in Laurent's tree... but I include it to get some CI coverage. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- Changes in v3: - CodeStyle (Thanks Sakari) - Re-implement as input device - Make the code depend on UVC_INPUT_EVDEV - Link to v2: https://lore.kernel.org/r/20241108-uvc-subdev-v2-0-85d8a051a3d3@chromium.org Changes in v2: - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ - Create uvc_gpio_cleanup and uvc_gpio_deinit - Refactor quirk: do not disable irq - Change define number for MEDIA_ENT_F_GPIO - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org --- Ricardo Ribalda (8): media: uvcvideo: Fix crash during unbind if gpio unit is in use media: uvcvideo: Factor out gpio functions to its own file media: uvcvideo: Re-implement privacy GPIO as an input device Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" media: uvcvideo: Create ancillary link for GPIO subdevice media: v4l2-core: Add new MEDIA_ENT_F_GPIO media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity media: uvcvideo: Introduce UVC_QUIRK_PRIVACY_DURING_STREAM .../userspace-api/media/mediactl/media-types.rst | 4 + drivers/media/usb/uvc/Kconfig | 2 +- drivers/media/usb/uvc/Makefile | 3 + drivers/media/usb/uvc/uvc_ctrl.c | 40 +----- drivers/media/usb/uvc/uvc_driver.c | 112 +--------------- drivers/media/usb/uvc/uvc_entity.c | 21 ++- drivers/media/usb/uvc/uvc_gpio.c | 144 +++++++++++++++++++++ drivers/media/usb/uvc/uvc_status.c | 13 +- drivers/media/usb/uvc/uvc_video.c | 4 + drivers/media/usb/uvc/uvcvideo.h | 31 +++-- drivers/media/v4l2-core/v4l2-async.c | 3 +- include/uapi/linux/media.h | 1 + 12 files changed, 223 insertions(+), 155 deletions(-) --- base-commit: 1b3bb4d69f20be5931abc18a6dbc24ff687fa780 change-id: 20241030-uvc-subdev-89f4467a00b5 Best regards,