[PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Ricardo Ribalda]

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,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v3 1/8] media: uvcvideo: Fix crash during unbind if gpio unit is in use

[Ricardo Ribalda]

We used the wrong device for the device managed functions. We used the
usb device, when we should be using the interface device.

If we unbind the driver from the usb interface, the cleanup functions
are never called. In our case, the IRQ is never disabled.

If an IRQ is triggered, it will try to access memory sections that are
already free, causing an OOPS.

We cannot use the function devm_request_threaded_irq here. The devm_*
clean functions may be called after the main structure is released by
uvc_delete.

Luckily this bug has small impact, as it is only affected by devices
with gpio units and the user has to unbind the device, a disconnect will
not trigger this error.

Cc: stable@vger.kernel.org
Fixes: 2886477ff987 ("media: uvcvideo: Implement UVC_EXT_GPIO_UNIT")
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 28 +++++++++++++++++++++-------
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index a96f6ca0889f..cd13bf01265d 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1295,14 +1295,14 @@ static int uvc_gpio_parse(struct uvc_device *dev)
 	struct gpio_desc *gpio_privacy;
 	int irq;
 
-	gpio_privacy = devm_gpiod_get_optional(&dev->udev->dev, "privacy",
+	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
 					       GPIOD_IN);
 	if (IS_ERR_OR_NULL(gpio_privacy))
 		return PTR_ERR_OR_ZERO(gpio_privacy);
 
 	irq = gpiod_to_irq(gpio_privacy);
 	if (irq < 0)
-		return dev_err_probe(&dev->udev->dev, irq,
+		return dev_err_probe(&dev->intf->dev, irq,
 				     "No IRQ for privacy GPIO\n");
 
 	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
@@ -1329,15 +1329,27 @@ static int uvc_gpio_parse(struct uvc_device *dev)
 static int uvc_gpio_init_irq(struct uvc_device *dev)
 {
 	struct uvc_entity *unit = dev->gpio_unit;
+	int ret;
 
 	if (!unit || unit->gpio.irq < 0)
 		return 0;
 
-	return devm_request_threaded_irq(&dev->udev->dev, unit->gpio.irq, NULL,
-					 uvc_gpio_irq,
-					 IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
-					 IRQF_TRIGGER_RISING,
-					 "uvc_privacy_gpio", dev);
+	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
+				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
+				   IRQF_TRIGGER_RISING,
+				   "uvc_privacy_gpio", dev);
+
+	unit->gpio.initialized = !ret;
+
+	return ret;
+}
+
+static void uvc_gpio_deinit(struct uvc_device *dev)
+{
+	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
+		return;
+
+	free_irq(dev->gpio_unit->gpio.irq, dev);
 }
 
 /* ------------------------------------------------------------------------
@@ -1934,6 +1946,8 @@ static void uvc_unregister_video(struct uvc_device *dev)
 {
 	struct uvc_streaming *stream;
 
+	uvc_gpio_deinit(dev);
+
 	list_for_each_entry(stream, &dev->streams, list) {
 		/* Nothing to do here, continue. */
 		if (!video_is_registered(&stream->vdev))
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 07f9921d83f2..965a789ed03e 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -234,6 +234,7 @@ struct uvc_entity {
 			u8  *bmControls;
 			struct gpio_desc *gpio_privacy;
 			int irq;
+			bool initialized;
 		} gpio;
 	};
 

-- 
2.47.0.277.g8800431eea-goog

[PATCH v3 2/8] media: uvcvideo: Factor out gpio functions to its own file

[Ricardo Ribalda]

This is just a refactor patch, no new functionality is added.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/Makefile     |   3 +-
 drivers/media/usb/uvc/uvc_driver.c | 119 +------------------------------------
 drivers/media/usb/uvc/uvc_gpio.c   | 118 ++++++++++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |   8 +++
 4 files changed, 131 insertions(+), 117 deletions(-)

diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
index 4f9eee4f81ab..85514b6e538f 100644
--- a/drivers/media/usb/uvc/Makefile
+++ b/drivers/media/usb/uvc/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
-		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
+		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
+		  uvc_gpio.o
 ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
 uvcvideo-objs  += uvc_entity.o
 endif
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index cd13bf01265d..5b48768a4f7f 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -8,7 +8,6 @@
 
 #include <linux/atomic.h>
 #include <linux/bits.h>
-#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
@@ -775,9 +774,9 @@ static const u8 uvc_media_transport_input_guid[16] =
 	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
 static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
 
-static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
-					       u16 id, unsigned int num_pads,
-					       unsigned int extra_size)
+struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
+					u16 id, unsigned int num_pads,
+					unsigned int extra_size)
 {
 	struct uvc_entity *entity;
 	unsigned int num_inputs;
@@ -1240,118 +1239,6 @@ static int uvc_parse_control(struct uvc_device *dev)
 	return 0;
 }
 
-/* -----------------------------------------------------------------------------
- * Privacy GPIO
- */
-
-static void uvc_gpio_event(struct uvc_device *dev)
-{
-	struct uvc_entity *unit = dev->gpio_unit;
-	struct uvc_video_chain *chain;
-	u8 new_val;
-
-	if (!unit)
-		return;
-
-	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
-
-	/* GPIO entities are always on the first chain. */
-	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
-	uvc_ctrl_status_event(chain, unit->controls, &new_val);
-}
-
-static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
-			    u8 cs, void *data, u16 size)
-{
-	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
-		return -EINVAL;
-
-	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
-
-	return 0;
-}
-
-static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
-			     u8 cs, u8 *caps)
-{
-	if (cs != UVC_CT_PRIVACY_CONTROL)
-		return -EINVAL;
-
-	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
-	return 0;
-}
-
-static irqreturn_t uvc_gpio_irq(int irq, void *data)
-{
-	struct uvc_device *dev = data;
-
-	uvc_gpio_event(dev);
-	return IRQ_HANDLED;
-}
-
-static int uvc_gpio_parse(struct uvc_device *dev)
-{
-	struct uvc_entity *unit;
-	struct gpio_desc *gpio_privacy;
-	int irq;
-
-	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
-					       GPIOD_IN);
-	if (IS_ERR_OR_NULL(gpio_privacy))
-		return PTR_ERR_OR_ZERO(gpio_privacy);
-
-	irq = gpiod_to_irq(gpio_privacy);
-	if (irq < 0)
-		return dev_err_probe(&dev->intf->dev, irq,
-				     "No IRQ for privacy GPIO\n");
-
-	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
-				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
-	if (IS_ERR(unit))
-		return PTR_ERR(unit);
-
-	unit->gpio.gpio_privacy = gpio_privacy;
-	unit->gpio.irq = irq;
-	unit->gpio.bControlSize = 1;
-	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
-	unit->gpio.bmControls[0] = 1;
-	unit->get_cur = uvc_gpio_get_cur;
-	unit->get_info = uvc_gpio_get_info;
-	strscpy(unit->name, "GPIO", sizeof(unit->name));
-
-	list_add_tail(&unit->list, &dev->entities);
-
-	dev->gpio_unit = unit;
-
-	return 0;
-}
-
-static int uvc_gpio_init_irq(struct uvc_device *dev)
-{
-	struct uvc_entity *unit = dev->gpio_unit;
-	int ret;
-
-	if (!unit || unit->gpio.irq < 0)
-		return 0;
-
-	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
-				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
-				   IRQF_TRIGGER_RISING,
-				   "uvc_privacy_gpio", dev);
-
-	unit->gpio.initialized = !ret;
-
-	return ret;
-}
-
-static void uvc_gpio_deinit(struct uvc_device *dev)
-{
-	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
-		return;
-
-	free_irq(dev->gpio_unit->gpio.irq, dev);
-}
-
 /* ------------------------------------------------------------------------
  * UVC device scan
  */
diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
new file mode 100644
index 000000000000..453739acbe8f
--- /dev/null
+++ b/drivers/media/usb/uvc/uvc_gpio.c
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *      uvc_gpio.c  --  USB Video Class driver
+ *
+ *      Copyright 2024 Google LLC
+ */
+
+#include <linux/kernel.h>
+#include <linux/gpio/consumer.h>
+#include "uvcvideo.h"
+
+static void uvc_gpio_event(struct uvc_device *dev)
+{
+	struct uvc_entity *unit = dev->gpio_unit;
+	struct uvc_video_chain *chain;
+	u8 new_val;
+
+	if (!unit)
+		return;
+
+	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
+
+	/* GPIO entities are always on the first chain. */
+	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
+	uvc_ctrl_status_event(chain, unit->controls, &new_val);
+}
+
+static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
+			    u8 cs, void *data, u16 size)
+{
+	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
+		return -EINVAL;
+
+	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
+
+	return 0;
+}
+
+static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
+			     u8 cs, u8 *caps)
+{
+	if (cs != UVC_CT_PRIVACY_CONTROL)
+		return -EINVAL;
+
+	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
+	return 0;
+}
+
+static irqreturn_t uvc_gpio_irq(int irq, void *data)
+{
+	struct uvc_device *dev = data;
+
+	uvc_gpio_event(dev);
+	return IRQ_HANDLED;
+}
+
+int uvc_gpio_parse(struct uvc_device *dev)
+{
+	struct uvc_entity *unit;
+	struct gpio_desc *gpio_privacy;
+	int irq;
+
+	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
+					       GPIOD_IN);
+	if (IS_ERR_OR_NULL(gpio_privacy))
+		return PTR_ERR_OR_ZERO(gpio_privacy);
+
+	irq = gpiod_to_irq(gpio_privacy);
+	if (irq < 0)
+		return dev_err_probe(&dev->udev->dev, irq,
+				     "No IRQ for privacy GPIO\n");
+
+	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
+				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
+	if (IS_ERR(unit))
+		return PTR_ERR(unit);
+
+	unit->gpio.gpio_privacy = gpio_privacy;
+	unit->gpio.irq = irq;
+	unit->gpio.bControlSize = 1;
+	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
+	unit->gpio.bmControls[0] = 1;
+	unit->get_cur = uvc_gpio_get_cur;
+	unit->get_info = uvc_gpio_get_info;
+	strscpy(unit->name, "GPIO", sizeof(unit->name));
+
+	list_add_tail(&unit->list, &dev->entities);
+
+	dev->gpio_unit = unit;
+
+	return 0;
+}
+
+int uvc_gpio_init_irq(struct uvc_device *dev)
+{
+	struct uvc_entity *unit = dev->gpio_unit;
+	int ret;
+
+	if (!unit || unit->gpio.irq < 0)
+		return 0;
+
+	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
+				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
+				   IRQF_TRIGGER_RISING,
+				   "uvc_privacy_gpio", dev);
+
+	unit->gpio.initialized = !ret;
+
+	return ret;
+}
+
+void uvc_gpio_deinit(struct uvc_device *dev)
+{
+	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
+		return;
+
+	free_irq(dev->gpio_unit->gpio.irq, dev);
+}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 965a789ed03e..91ed59b54d9a 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -673,6 +673,9 @@ do {									\
 extern struct uvc_driver uvc_driver;
 
 struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
+struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
+					u16 id, unsigned int num_pads,
+					unsigned int extra_size);
 
 /* Video buffers queue management. */
 int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
@@ -817,4 +820,9 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
 size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
 			    size_t size);
 
+/* gpio */
+int uvc_gpio_parse(struct uvc_device *dev);
+int uvc_gpio_init_irq(struct uvc_device *dev);
+void uvc_gpio_deinit(struct uvc_device *dev);
+
 #endif

-- 
2.47.0.277.g8800431eea-goog

[PATCH v3 3/8] media: uvcvideo: Re-implement privacy GPIO as an input device

[Ricardo Ribalda]

Reimplement privacy GPIO as an input device.

This is an attempt to unify how we notify userspace about a camera that
is covered.

Replace the previous V4L2_CID_PRIVACY control with evdev
SW_CAMERA_LENS_COVER.

This has some main benefits:
- It unifies behaviour with other drivers.
- It allows reading the privacy events without powering up the camera.
- It allows reading the privacy gpio and the internal gpio control (if
  present).

Although this introduces an ABI change, we have only seen ChromeOS using
this feature.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/Kconfig      |  2 +-
 drivers/media/usb/uvc/Makefile     |  6 ++-
 drivers/media/usb/uvc/uvc_ctrl.c   | 20 ----------
 drivers/media/usb/uvc/uvc_driver.c |  3 +-
 drivers/media/usb/uvc/uvc_entity.c |  6 +++
 drivers/media/usb/uvc/uvc_gpio.c   | 76 +++++++++++++-------------------------
 drivers/media/usb/uvc/uvc_status.c | 13 +++++--
 drivers/media/usb/uvc/uvcvideo.h   | 15 +++++---
 8 files changed, 59 insertions(+), 82 deletions(-)

diff --git a/drivers/media/usb/uvc/Kconfig b/drivers/media/usb/uvc/Kconfig
index 579532272fd6..cdbba7fd5bee 100644
--- a/drivers/media/usb/uvc/Kconfig
+++ b/drivers/media/usb/uvc/Kconfig
@@ -17,6 +17,6 @@ config USB_VIDEO_CLASS_INPUT_EVDEV
 	depends on USB_VIDEO_CLASS=INPUT || INPUT=y
 	help
 	  This option makes USB Video Class devices register an input device
-	  to report button events.
+	  to report button events and privacy GPIO.
 
 	  If you are in doubt, say Y.
diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
index 85514b6e538f..b36b124da7a8 100644
--- a/drivers/media/usb/uvc/Makefile
+++ b/drivers/media/usb/uvc/Makefile
@@ -1,8 +1,10 @@
 # SPDX-License-Identifier: GPL-2.0
 uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
-		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
-		  uvc_gpio.o
+		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
 ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
 uvcvideo-objs  += uvc_entity.o
 endif
+ifeq ($(CONFIG_USB_VIDEO_CLASS_INPUT_EVDEV),y)
+uvcvideo-objs  += uvc_gpio.o
+endif
 obj-$(CONFIG_USB_VIDEO_CLASS) += uvcvideo.o
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 4fe26e82e3d1..b98f4778d8aa 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -350,14 +350,6 @@ static const struct uvc_control_info uvc_ctrls[] = {
 				| UVC_CTRL_FLAG_RESTORE
 				| UVC_CTRL_FLAG_AUTO_UPDATE,
 	},
-	{
-		.entity		= UVC_GUID_EXT_GPIO_CONTROLLER,
-		.selector	= UVC_CT_PRIVACY_CONTROL,
-		.index		= 0,
-		.size		= 1,
-		.flags		= UVC_CTRL_FLAG_GET_CUR
-				| UVC_CTRL_FLAG_AUTO_UPDATE,
-	},
 };
 
 static const u32 uvc_control_classes[] = {
@@ -827,15 +819,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 		.v4l2_type	= V4L2_CTRL_TYPE_BOOLEAN,
 		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
 	},
-	{
-		.id		= V4L2_CID_PRIVACY,
-		.entity		= UVC_GUID_EXT_GPIO_CONTROLLER,
-		.selector	= UVC_CT_PRIVACY_CONTROL,
-		.size		= 1,
-		.offset		= 0,
-		.v4l2_type	= V4L2_CTRL_TYPE_BOOLEAN,
-		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
-	},
 	{
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
@@ -2718,9 +2701,6 @@ static int uvc_ctrl_init_chain(struct uvc_video_chain *chain)
 		} else if (UVC_ENTITY_TYPE(entity) == UVC_ITT_CAMERA) {
 			bmControls = entity->camera.bmControls;
 			bControlSize = entity->camera.bControlSize;
-		} else if (UVC_ENTITY_TYPE(entity) == UVC_EXT_GPIO_UNIT) {
-			bmControls = entity->gpio.bmControls;
-			bControlSize = entity->gpio.bControlSize;
 		}
 
 		/* Remove bogus/blacklisted controls */
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 5b48768a4f7f..d6c12be49b5c 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2171,6 +2171,7 @@ static int uvc_probe(struct usb_interface *intf,
 	if (media_device_register(&dev->mdev) < 0)
 		goto error;
 #endif
+
 	/* Save our data pointer in the interface data. */
 	usb_set_intfdata(intf, dev);
 
@@ -2182,7 +2183,7 @@ static int uvc_probe(struct usb_interface *intf,
 			 ret);
 	}
 
-	ret = uvc_gpio_init_irq(dev);
+	ret = uvc_gpio_init(dev);
 	if (ret < 0) {
 		dev_err(&dev->udev->dev,
 			"Unable to request privacy GPIO IRQ (%d)\n", ret);
diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
index cc68dd24eb42..c1b69f9eaa56 100644
--- a/drivers/media/usb/uvc/uvc_entity.c
+++ b/drivers/media/usb/uvc/uvc_entity.c
@@ -56,7 +56,13 @@ static int uvc_mc_create_links(struct uvc_video_chain *chain,
 	return 0;
 }
 
+static const struct v4l2_subdev_core_ops uvc_subdev_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
 static const struct v4l2_subdev_ops uvc_subdev_ops = {
+	.core = &uvc_subdev_core_ops,
 };
 
 void uvc_mc_cleanup_entity(struct uvc_entity *entity)
diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
index 453739acbe8f..80096022ad08 100644
--- a/drivers/media/usb/uvc/uvc_gpio.c
+++ b/drivers/media/usb/uvc/uvc_gpio.c
@@ -7,57 +7,29 @@
 
 #include <linux/kernel.h>
 #include <linux/gpio/consumer.h>
+#include <linux/input.h>
 #include "uvcvideo.h"
 
-static void uvc_gpio_event(struct uvc_device *dev)
-{
-	struct uvc_entity *unit = dev->gpio_unit;
-	struct uvc_video_chain *chain;
-	u8 new_val;
-
-	if (!unit)
-		return;
-
-	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
-
-	/* GPIO entities are always on the first chain. */
-	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
-	uvc_ctrl_status_event(chain, unit->controls, &new_val);
-}
-
-static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
-			    u8 cs, void *data, u16 size)
-{
-	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
-		return -EINVAL;
-
-	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
-
-	return 0;
-}
-
-static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
-			     u8 cs, u8 *caps)
-{
-	if (cs != UVC_CT_PRIVACY_CONTROL)
-		return -EINVAL;
-
-	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
-	return 0;
-}
-
 static irqreturn_t uvc_gpio_irq(int irq, void *data)
 {
 	struct uvc_device *dev = data;
+	struct uvc_gpio *uvc_gpio = &dev->gpio_unit->gpio;
+	int new_val;
+
+	new_val = gpiod_get_value_cansleep(uvc_gpio->gpio_privacy);
+	if (new_val < 0)
+		return IRQ_HANDLED;
+
+	input_report_switch(dev->input, SW_CAMERA_LENS_COVER, new_val);
+	input_sync(dev->input);
 
-	uvc_gpio_event(dev);
 	return IRQ_HANDLED;
 }
 
 int uvc_gpio_parse(struct uvc_device *dev)
 {
-	struct uvc_entity *unit;
 	struct gpio_desc *gpio_privacy;
+	struct uvc_entity *unit;
 	int irq;
 
 	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
@@ -67,23 +39,17 @@ int uvc_gpio_parse(struct uvc_device *dev)
 
 	irq = gpiod_to_irq(gpio_privacy);
 	if (irq < 0)
-		return dev_err_probe(&dev->udev->dev, irq,
+		return dev_err_probe(&dev->intf->dev, irq,
 				     "No IRQ for privacy GPIO\n");
 
 	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
-				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
+				    UVC_EXT_GPIO_UNIT_ID, 0, 0);
 	if (IS_ERR(unit))
 		return PTR_ERR(unit);
 
 	unit->gpio.gpio_privacy = gpio_privacy;
 	unit->gpio.irq = irq;
-	unit->gpio.bControlSize = 1;
-	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
-	unit->gpio.bmControls[0] = 1;
-	unit->get_cur = uvc_gpio_get_cur;
-	unit->get_info = uvc_gpio_get_info;
 	strscpy(unit->name, "GPIO", sizeof(unit->name));
-
 	list_add_tail(&unit->list, &dev->entities);
 
 	dev->gpio_unit = unit;
@@ -91,22 +57,32 @@ int uvc_gpio_parse(struct uvc_device *dev)
 	return 0;
 }
 
-int uvc_gpio_init_irq(struct uvc_device *dev)
+int uvc_gpio_init(struct uvc_device *dev)
 {
 	struct uvc_entity *unit = dev->gpio_unit;
+	int init_val;
 	int ret;
 
 	if (!unit || unit->gpio.irq < 0)
 		return 0;
 
+	init_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
+	if (init_val < 0)
+		return init_val;
+
 	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
 				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
 				   IRQF_TRIGGER_RISING,
 				   "uvc_privacy_gpio", dev);
+	if (ret)
+		return ret;
+
+	input_report_switch(dev->input, SW_CAMERA_LENS_COVER, init_val);
+	input_sync(dev->input);
 
-	unit->gpio.initialized = !ret;
+	unit->gpio.initialized = true;
 
-	return ret;
+	return 0;
 }
 
 void uvc_gpio_deinit(struct uvc_device *dev)
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 06c867510c8f..319f472213f6 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -44,7 +44,7 @@ static int uvc_input_init(struct uvc_device *dev)
 	struct input_dev *input;
 	int ret;
 
-	if (!uvc_input_has_button(dev))
+	if (!uvc_input_has_button(dev) && !dev->gpio_unit)
 		return 0;
 
 	input = input_allocate_device();
@@ -59,8 +59,15 @@ static int uvc_input_init(struct uvc_device *dev)
 	usb_to_input_id(dev->udev, &input->id);
 	input->dev.parent = &dev->intf->dev;
 
-	__set_bit(EV_KEY, input->evbit);
-	__set_bit(KEY_CAMERA, input->keybit);
+	if (uvc_input_has_button(dev)) {
+		__set_bit(EV_KEY, input->evbit);
+		__set_bit(KEY_CAMERA, input->keybit);
+	}
+
+	if (dev->gpio_unit) {
+		__set_bit(EV_SW, input->evbit);
+		__set_bit(SW_CAMERA_LENS_COVER, input->swbit);
+	}
 
 	if ((ret = input_register_device(input)) < 0)
 		goto error;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 91ed59b54d9a..06c4d514d02c 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -15,6 +15,7 @@
 #include <linux/videodev2.h>
 #include <linux/workqueue.h>
 #include <media/media-device.h>
+#include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fh.h>
@@ -229,12 +230,10 @@ struct uvc_entity {
 			u8  *bmControlsType;
 		} extension;
 
-		struct {
-			u8  bControlSize;
-			u8  *bmControls;
-			struct gpio_desc *gpio_privacy;
+		struct uvc_gpio {
 			int irq;
 			bool initialized;
+			struct gpio_desc *gpio_privacy;
 		} gpio;
 	};
 
@@ -821,8 +820,14 @@ size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
 			    size_t size);
 
 /* gpio */
+#ifdef CONFIG_USB_VIDEO_CLASS_INPUT_EVDEV
 int uvc_gpio_parse(struct uvc_device *dev);
-int uvc_gpio_init_irq(struct uvc_device *dev);
+int uvc_gpio_init(struct uvc_device *dev);
 void uvc_gpio_deinit(struct uvc_device *dev);
+#else
+static inline int uvc_gpio_parse(struct uvc_device *dev) {return 0; }
+static inline int uvc_gpio_init(struct uvc_device *dev) {return 0; }
+static inline void uvc_gpio_deinit(struct uvc_device *dev) {};
+#endif
 
 #endif

-- 
2.47.0.277.g8800431eea-goog

[PATCH v3 4/8] Revert "media: uvcvideo: Allow entity-defined get_info and get_cur"

[Ricardo Ribalda]

With the privacy gpio now handled as a evdev, there is no more need
for this.

This reverts commit 65900c581d014499f0f8ceabfc02c652e9a88771.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 20 +++++---------------
 drivers/media/usb/uvc/uvcvideo.h |  5 -----
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index b98f4778d8aa..90bc2132d8d5 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1087,15 +1087,9 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
 		return 0;
 	}
 
-	if (ctrl->entity->get_cur)
-		ret = ctrl->entity->get_cur(chain->dev, ctrl->entity,
-					    ctrl->info.selector, data,
-					    ctrl->info.size);
-	else
-		ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
-				     ctrl->entity->id, chain->dev->intfnum,
-				     ctrl->info.selector, data,
-				     ctrl->info.size);
+	ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
+			     chain->dev->intfnum, ctrl->info.selector, data,
+			     ctrl->info.size);
 
 	if (ret < 0)
 		return ret;
@@ -2055,12 +2049,8 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
 	if (data == NULL)
 		return -ENOMEM;
 
-	if (ctrl->entity->get_info)
-		ret = ctrl->entity->get_info(dev, ctrl->entity,
-					     ctrl->info.selector, data);
-	else
-		ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id,
-				     dev->intfnum, info->selector, data, 1);
+	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
+			     info->selector, data, 1);
 
 	if (!ret) {
 		info->flags &= ~(UVC_CTRL_FLAG_GET_CUR |
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 06c4d514d02c..6002f1c43b69 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -240,11 +240,6 @@ struct uvc_entity {
 	u8 bNrInPins;
 	u8 *baSourceID;
 
-	int (*get_info)(struct uvc_device *dev, struct uvc_entity *entity,
-			u8 cs, u8 *caps);
-	int (*get_cur)(struct uvc_device *dev, struct uvc_entity *entity,
-		       u8 cs, void *data, u16 size);
-
 	unsigned int ncontrols;
 	struct uvc_control *controls;
 };

-- 
2.47.0.277.g8800431eea-goog

[PATCH v3 5/8] media: uvcvideo: Create ancillary link for GPIO subdevice

[Ricardo Ribalda]

Make an ancillary lik between the streaming subdev and the GPIO subdev.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_entity.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
index c1b69f9eaa56..7b8fb85d8c03 100644
--- a/drivers/media/usb/uvc/uvc_entity.c
+++ b/drivers/media/usb/uvc/uvc_entity.c
@@ -53,6 +53,17 @@ static int uvc_mc_create_links(struct uvc_video_chain *chain,
 			return ret;
 	}
 
+	/* Create ancillary link for the GPIO. */
+	if (chain->dev->gpio_unit &&
+	    UVC_ENTITY_TYPE(entity) == UVC_ITT_CAMERA) {
+		struct media_link *link;
+
+		link = media_create_ancillary_link(sink,
+						   &chain->dev->gpio_unit->subdev.entity);
+		if (IS_ERR(link))
+			return PTR_ERR(link);
+	}
+
 	return 0;
 }
 

-- 
2.47.0.277.g8800431eea-goog

[PATCH v3 6/8] media: v4l2-core: Add new MEDIA_ENT_F_GPIO

[Ricardo Ribalda]

Add a new media entity type to define a GPIO entity. This can be used to
represent the privacy switch GPIO associated to a sensor.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 Documentation/userspace-api/media/mediactl/media-types.rst | 4 ++++
 drivers/media/v4l2-core/v4l2-async.c                       | 3 ++-
 include/uapi/linux/media.h                                 | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst
index 6332e8395263..7ede3954a96a 100644
--- a/Documentation/userspace-api/media/mediactl/media-types.rst
+++ b/Documentation/userspace-api/media/mediactl/media-types.rst
@@ -24,6 +24,7 @@ Types and flags used to represent the media graph elements
 .. _MEDIA-ENT-F-CAM-SENSOR:
 .. _MEDIA-ENT-F-FLASH:
 .. _MEDIA-ENT-F-LENS:
+.. _MEDIA-ENT-F-GPIO:
 .. _MEDIA-ENT-F-ATV-DECODER:
 .. _MEDIA-ENT-F-TUNER:
 .. _MEDIA-ENT-F-IF-VID-DECODER:
@@ -100,6 +101,9 @@ Types and flags used to represent the media graph elements
     *  -  ``MEDIA_ENT_F_LENS``
        -  Lens controller entity.
 
+    *  -  ``MEDIA_ENT_F_GPIO``
+       -  GPIO controller entity.
+
     *  -  ``MEDIA_ENT_F_ATV_DECODER``
        -  Analog video decoder, the basic function of the video decoder is
 	  to accept analogue video from a wide variety of sources such as
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index ee884a8221fb..8a902fc897d1 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -320,7 +320,8 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
 	struct media_link *link;
 
 	if (sd->entity.function != MEDIA_ENT_F_LENS &&
-	    sd->entity.function != MEDIA_ENT_F_FLASH)
+	    sd->entity.function != MEDIA_ENT_F_FLASH &&
+	    sd->entity.function != MEDIA_ENT_F_GPIO)
 		return 0;
 
 	if (!n->sd) {
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 1c80b1d6bbaf..d3d045e52d78 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -84,6 +84,7 @@ struct media_device_info {
 #define MEDIA_ENT_F_CAM_SENSOR			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 1)
 #define MEDIA_ENT_F_FLASH			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 2)
 #define MEDIA_ENT_F_LENS			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 3)
+#define MEDIA_ENT_F_GPIO			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 6)
 
 /*
  * Digital TV, analog TV, radio and/or software defined radio tuner functions.

-- 
2.47.0.277.g8800431eea-goog

[PATCH v3 7/8] media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity

[Ricardo Ribalda]

Right now we are setting the entity type to unknown for the privacy GPIO
entity. Which results in the following error in dmesg.
uvcvideo 3-6:1.0: Entity type for entity GPIO was not initialized!

Use the newly created type to fix this.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_entity.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
index 7b8fb85d8c03..c9f806bc03c4 100644
--- a/drivers/media/usb/uvc/uvc_entity.c
+++ b/drivers/media/usb/uvc/uvc_entity.c
@@ -115,6 +115,9 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain,
 		case UVC_ITT_CAMERA:
 			function = MEDIA_ENT_F_CAM_SENSOR;
 			break;
+		case UVC_EXT_GPIO_UNIT:
+			function = MEDIA_ENT_F_GPIO;
+			break;
 		case UVC_TT_VENDOR_SPECIFIC:
 		case UVC_ITT_VENDOR_SPECIFIC:
 		case UVC_ITT_MEDIA_TRANSPORT_INPUT:
@@ -122,7 +125,6 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain,
 		case UVC_OTT_DISPLAY:
 		case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:
 		case UVC_EXTERNAL_VENDOR_SPECIFIC:
-		case UVC_EXT_GPIO_UNIT:
 		default:
 			function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
 			break;

-- 
2.47.0.277.g8800431eea-goog

[PATCH v3 8/8] media: uvcvideo: Introduce UVC_QUIRK_PRIVACY_DURING_STREAM

[Ricardo Ribalda]

Some devices power the GPIO pull-up with the same power-supply as the
camera. Avoid reading the GPIO if the device is not streaming.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_gpio.c  | 58 ++++++++++++++++++++++++++++++++++++---
 drivers/media/usb/uvc/uvc_video.c |  4 +++
 drivers/media/usb/uvc/uvcvideo.h  |  4 +++
 3 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
index 80096022ad08..8cab27c9d90d 100644
--- a/drivers/media/usb/uvc/uvc_gpio.c
+++ b/drivers/media/usb/uvc/uvc_gpio.c
@@ -5,6 +5,7 @@
  *      Copyright 2024 Google LLC
  */
 
+#include <linux/dmi.h>
 #include <linux/kernel.h>
 #include <linux/gpio/consumer.h>
 #include <linux/input.h>
@@ -16,6 +17,9 @@ static irqreturn_t uvc_gpio_irq(int irq, void *data)
 	struct uvc_gpio *uvc_gpio = &dev->gpio_unit->gpio;
 	int new_val;
 
+	if (!uvc_gpio->gpio_ready)
+		return IRQ_HANDLED;
+
 	new_val = gpiod_get_value_cansleep(uvc_gpio->gpio_privacy);
 	if (new_val < 0)
 		return IRQ_HANDLED;
@@ -26,6 +30,24 @@ static irqreturn_t uvc_gpio_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static const struct dmi_system_id privacy_valid_during_streamon[] = {
+	{
+		.ident = "HP Elite c1030 Chromebook",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Jinlon"),
+		},
+	},
+	{
+		.ident = "HP Pro c640 Chromebook",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Dratini"),
+		},
+	},
+	{ } /* terminate list */
+};
+
 int uvc_gpio_parse(struct uvc_device *dev)
 {
 	struct gpio_desc *gpio_privacy;
@@ -47,6 +69,15 @@ int uvc_gpio_parse(struct uvc_device *dev)
 	if (IS_ERR(unit))
 		return PTR_ERR(unit);
 
+	/*
+	 * Note: This quirk will not match external UVC cameras,
+	 * as they will not have the corresponding ACPI GPIO entity.
+	 */
+	if (dmi_check_system(privacy_valid_during_streamon))
+		dev->quirks |= UVC_QUIRK_PRIVACY_DURING_STREAM;
+	else
+		unit->gpio.gpio_ready = true;
+
 	unit->gpio.gpio_privacy = gpio_privacy;
 	unit->gpio.irq = irq;
 	strscpy(unit->name, "GPIO", sizeof(unit->name));
@@ -57,6 +88,16 @@ int uvc_gpio_parse(struct uvc_device *dev)
 	return 0;
 }
 
+void uvc_gpio_quirk(struct uvc_device *dev, bool stream_on)
+{
+	if (!dev->gpio_unit || !(dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM))
+		return;
+
+	dev->gpio_unit->gpio.gpio_ready = stream_on;
+	if (stream_on)
+		uvc_gpio_irq(0, dev);
+}
+
 int uvc_gpio_init(struct uvc_device *dev)
 {
 	struct uvc_entity *unit = dev->gpio_unit;
@@ -66,10 +107,6 @@ int uvc_gpio_init(struct uvc_device *dev)
 	if (!unit || unit->gpio.irq < 0)
 		return 0;
 
-	init_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
-	if (init_val < 0)
-		return init_val;
-
 	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
 				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
 				   IRQF_TRIGGER_RISING,
@@ -77,6 +114,19 @@ int uvc_gpio_init(struct uvc_device *dev)
 	if (ret)
 		return ret;
 
+	if ((dev->quirks & UVC_QUIRK_PRIVACY_DURING_STREAM)) {
+		uvc_gpio_quirk(dev, false);
+		init_val = false;
+	} else {
+		unit->gpio.gpio_ready = true;
+
+		init_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
+		if (init_val < 0) {
+			free_irq(dev->gpio_unit->gpio.irq, dev);
+			return init_val;
+		}
+	}
+
 	input_report_switch(dev->input, SW_CAMERA_LENS_COVER, init_val);
 	input_sync(dev->input);
 
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index cd9c29532fb0..0d542176ccde 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -2296,6 +2296,8 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
 	if (ret < 0)
 		goto error_video;
 
+	uvc_gpio_quirk(stream->dev, true);
+
 	return 0;
 
 error_video:
@@ -2308,6 +2310,8 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
 
 void uvc_video_stop_streaming(struct uvc_streaming *stream)
 {
+	uvc_gpio_quirk(stream->dev, false);
+
 	uvc_video_stop_transfer(stream, 1);
 
 	if (stream->intf->num_altsetting > 1) {
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 6002f1c43b69..1a784fb76ed7 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -77,6 +77,7 @@
 #define UVC_QUIRK_NO_RESET_RESUME	0x00004000
 #define UVC_QUIRK_DISABLE_AUTOSUSPEND	0x00008000
 #define UVC_QUIRK_INVALID_DEVICE_SOF	0x00010000
+#define UVC_QUIRK_PRIVACY_DURING_STREAM	0x00020000
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001
@@ -233,6 +234,7 @@ struct uvc_entity {
 		struct uvc_gpio {
 			int irq;
 			bool initialized;
+			bool gpio_ready;
 			struct gpio_desc *gpio_privacy;
 		} gpio;
 	};
@@ -819,10 +821,12 @@ size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
 int uvc_gpio_parse(struct uvc_device *dev);
 int uvc_gpio_init(struct uvc_device *dev);
 void uvc_gpio_deinit(struct uvc_device *dev);
+void uvc_gpio_quirk(struct uvc_device *dev, bool stream_on);
 #else
 static inline int uvc_gpio_parse(struct uvc_device *dev) {return 0; }
 static inline int uvc_gpio_init(struct uvc_device *dev) {return 0; }
 static inline void uvc_gpio_deinit(struct uvc_device *dev) {};
+static inline void uvc_gpio_quirk(struct uvc_device *dev, bool stream_on) {};
 #endif
 
 #endif

-- 
2.47.0.277.g8800431eea-goog

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Hans de Goede]

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

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\x0f. 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,

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Ricardo Ribalda]

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


-- 
Ricardo Ribalda

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Ricardo Ribalda]

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



-- 
Ricardo Ribalda

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Hans de Goede]

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

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

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Ricardo Ribalda]

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

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Hans de Goede]

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

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Hans de Goede]

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

Re: [PATCH v3 2/8] media: uvcvideo: Factor out gpio functions to its own file

[Hans de Goede]

Hi Ricardo,

On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote:
> This is just a refactor patch, no new functionality is added.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

I guess this patch may need to change depending on if we want
to keep the GPIO handling as a UVC entity or not.

Laurent what is your take on this, should this stay as
a struct uvc_entity; or should the gpio_desc and input_device
be stored directly inside struct uvc_device as is done for
the snapshot-button input_device?


Also de we want a separate input_device for this or do
we re-use the snapshot button one ?

Since my plan is to open-up the permission on the device with
the SW_CAMERA_LENS_COVER to be equal to the /dev/video#
permissions sharing has the downside of allowing keylogging
of the snapshot button.

Either way (one or 2 input-devices) I don't have a strong
preference.

Regards,

Hans




> ---
>  drivers/media/usb/uvc/Makefile     |   3 +-
>  drivers/media/usb/uvc/uvc_driver.c | 119 +------------------------------------
>  drivers/media/usb/uvc/uvc_gpio.c   | 118 ++++++++++++++++++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   |   8 +++
>  4 files changed, 131 insertions(+), 117 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> index 4f9eee4f81ab..85514b6e538f 100644
> --- a/drivers/media/usb/uvc/Makefile
> +++ b/drivers/media/usb/uvc/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
> -		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
> +		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
> +		  uvc_gpio.o
>  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
>  uvcvideo-objs  += uvc_entity.o
>  endif
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index cd13bf01265d..5b48768a4f7f 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -8,7 +8,6 @@
>  
>  #include <linux/atomic.h>
>  #include <linux/bits.h>
> -#include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -775,9 +774,9 @@ static const u8 uvc_media_transport_input_guid[16] =
>  	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
>  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
>  
> -static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> -					       u16 id, unsigned int num_pads,
> -					       unsigned int extra_size)
> +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> +					u16 id, unsigned int num_pads,
> +					unsigned int extra_size)
>  {
>  	struct uvc_entity *entity;
>  	unsigned int num_inputs;
> @@ -1240,118 +1239,6 @@ static int uvc_parse_control(struct uvc_device *dev)
>  	return 0;
>  }
>  
> -/* -----------------------------------------------------------------------------
> - * Privacy GPIO
> - */
> -
> -static void uvc_gpio_event(struct uvc_device *dev)
> -{
> -	struct uvc_entity *unit = dev->gpio_unit;
> -	struct uvc_video_chain *chain;
> -	u8 new_val;
> -
> -	if (!unit)
> -		return;
> -
> -	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> -
> -	/* GPIO entities are always on the first chain. */
> -	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> -	uvc_ctrl_status_event(chain, unit->controls, &new_val);
> -}
> -
> -static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> -			    u8 cs, void *data, u16 size)
> -{
> -	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> -		return -EINVAL;
> -
> -	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> -
> -	return 0;
> -}
> -
> -static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> -			     u8 cs, u8 *caps)
> -{
> -	if (cs != UVC_CT_PRIVACY_CONTROL)
> -		return -EINVAL;
> -
> -	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> -	return 0;
> -}
> -
> -static irqreturn_t uvc_gpio_irq(int irq, void *data)
> -{
> -	struct uvc_device *dev = data;
> -
> -	uvc_gpio_event(dev);
> -	return IRQ_HANDLED;
> -}
> -
> -static int uvc_gpio_parse(struct uvc_device *dev)
> -{
> -	struct uvc_entity *unit;
> -	struct gpio_desc *gpio_privacy;
> -	int irq;
> -
> -	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> -					       GPIOD_IN);
> -	if (IS_ERR_OR_NULL(gpio_privacy))
> -		return PTR_ERR_OR_ZERO(gpio_privacy);
> -
> -	irq = gpiod_to_irq(gpio_privacy);
> -	if (irq < 0)
> -		return dev_err_probe(&dev->intf->dev, irq,
> -				     "No IRQ for privacy GPIO\n");
> -
> -	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> -				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
> -	if (IS_ERR(unit))
> -		return PTR_ERR(unit);
> -
> -	unit->gpio.gpio_privacy = gpio_privacy;
> -	unit->gpio.irq = irq;
> -	unit->gpio.bControlSize = 1;
> -	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> -	unit->gpio.bmControls[0] = 1;
> -	unit->get_cur = uvc_gpio_get_cur;
> -	unit->get_info = uvc_gpio_get_info;
> -	strscpy(unit->name, "GPIO", sizeof(unit->name));
> -
> -	list_add_tail(&unit->list, &dev->entities);
> -
> -	dev->gpio_unit = unit;
> -
> -	return 0;
> -}
> -
> -static int uvc_gpio_init_irq(struct uvc_device *dev)
> -{
> -	struct uvc_entity *unit = dev->gpio_unit;
> -	int ret;
> -
> -	if (!unit || unit->gpio.irq < 0)
> -		return 0;
> -
> -	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> -				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> -				   IRQF_TRIGGER_RISING,
> -				   "uvc_privacy_gpio", dev);
> -
> -	unit->gpio.initialized = !ret;
> -
> -	return ret;
> -}
> -
> -static void uvc_gpio_deinit(struct uvc_device *dev)
> -{
> -	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> -		return;
> -
> -	free_irq(dev->gpio_unit->gpio.irq, dev);
> -}
> -
>  /* ------------------------------------------------------------------------
>   * UVC device scan
>   */
> diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
> new file mode 100644
> index 000000000000..453739acbe8f
> --- /dev/null
> +++ b/drivers/media/usb/uvc/uvc_gpio.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *      uvc_gpio.c  --  USB Video Class driver
> + *
> + *      Copyright 2024 Google LLC
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/gpio/consumer.h>
> +#include "uvcvideo.h"
> +
> +static void uvc_gpio_event(struct uvc_device *dev)
> +{
> +	struct uvc_entity *unit = dev->gpio_unit;
> +	struct uvc_video_chain *chain;
> +	u8 new_val;
> +
> +	if (!unit)
> +		return;
> +
> +	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> +
> +	/* GPIO entities are always on the first chain. */
> +	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> +	uvc_ctrl_status_event(chain, unit->controls, &new_val);
> +}
> +
> +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> +			    u8 cs, void *data, u16 size)
> +{
> +	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> +		return -EINVAL;
> +
> +	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> +
> +	return 0;
> +}
> +
> +static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> +			     u8 cs, u8 *caps)
> +{
> +	if (cs != UVC_CT_PRIVACY_CONTROL)
> +		return -EINVAL;
> +
> +	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> +	return 0;
> +}
> +
> +static irqreturn_t uvc_gpio_irq(int irq, void *data)
> +{
> +	struct uvc_device *dev = data;
> +
> +	uvc_gpio_event(dev);
> +	return IRQ_HANDLED;
> +}
> +
> +int uvc_gpio_parse(struct uvc_device *dev)
> +{
> +	struct uvc_entity *unit;
> +	struct gpio_desc *gpio_privacy;
> +	int irq;
> +
> +	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> +					       GPIOD_IN);
> +	if (IS_ERR_OR_NULL(gpio_privacy))
> +		return PTR_ERR_OR_ZERO(gpio_privacy);
> +
> +	irq = gpiod_to_irq(gpio_privacy);
> +	if (irq < 0)
> +		return dev_err_probe(&dev->udev->dev, irq,
> +				     "No IRQ for privacy GPIO\n");
> +
> +	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> +				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
> +	if (IS_ERR(unit))
> +		return PTR_ERR(unit);
> +
> +	unit->gpio.gpio_privacy = gpio_privacy;
> +	unit->gpio.irq = irq;
> +	unit->gpio.bControlSize = 1;
> +	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> +	unit->gpio.bmControls[0] = 1;
> +	unit->get_cur = uvc_gpio_get_cur;
> +	unit->get_info = uvc_gpio_get_info;
> +	strscpy(unit->name, "GPIO", sizeof(unit->name));
> +
> +	list_add_tail(&unit->list, &dev->entities);
> +
> +	dev->gpio_unit = unit;
> +
> +	return 0;
> +}
> +
> +int uvc_gpio_init_irq(struct uvc_device *dev)
> +{
> +	struct uvc_entity *unit = dev->gpio_unit;
> +	int ret;
> +
> +	if (!unit || unit->gpio.irq < 0)
> +		return 0;
> +
> +	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> +				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> +				   IRQF_TRIGGER_RISING,
> +				   "uvc_privacy_gpio", dev);
> +
> +	unit->gpio.initialized = !ret;
> +
> +	return ret;
> +}
> +
> +void uvc_gpio_deinit(struct uvc_device *dev)
> +{
> +	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> +		return;
> +
> +	free_irq(dev->gpio_unit->gpio.irq, dev);
> +}
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 965a789ed03e..91ed59b54d9a 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -673,6 +673,9 @@ do {									\
>  extern struct uvc_driver uvc_driver;
>  
>  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> +					u16 id, unsigned int num_pads,
> +					unsigned int extra_size);
>  
>  /* Video buffers queue management. */
>  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
> @@ -817,4 +820,9 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
>  size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
>  			    size_t size);
>  
> +/* gpio */
> +int uvc_gpio_parse(struct uvc_device *dev);
> +int uvc_gpio_init_irq(struct uvc_device *dev);
> +void uvc_gpio_deinit(struct uvc_device *dev);
> +
>  #endif
> 

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Ricardo Ribalda]

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


-- 
Ricardo Ribalda

Re: [PATCH v3 2/8] media: uvcvideo: Factor out gpio functions to its own file

[Ricardo Ribalda]

On Mon, 25 Nov 2024 at 15:45, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Ricardo,
>
> On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote:
> > This is just a refactor patch, no new functionality is added.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>
> I guess this patch may need to change depending on if we want
> to keep the GPIO handling as a UVC entity or not.

I have a v4 that removes the virtual uvc entity. Still with that
approach IMHO this patch makes sense.

I was planning to send it today, I am testing it right now.

>
> Laurent what is your take on this, should this stay as
> a struct uvc_entity; or should the gpio_desc and input_device
> be stored directly inside struct uvc_device as is done for
> the snapshot-button input_device?
>
>
> Also de we want a separate input_device for this or do
> we re-use the snapshot button one ?
>
> Since my plan is to open-up the permission on the device with
> the SW_CAMERA_LENS_COVER to be equal to the /dev/video#
> permissions sharing has the downside of allowing keylogging
> of the snapshot button.

A downside of having 2 devices is that userspace will have to either
figure out what evdev they want to use or listen to both....

I do not have a strong preference.


>
> Either way (one or 2 input-devices) I don't have a strong
> preference.
>
> Regards,
>
> Hans
>
>
>
>
> > ---
> >  drivers/media/usb/uvc/Makefile     |   3 +-
> >  drivers/media/usb/uvc/uvc_driver.c | 119 +------------------------------------
> >  drivers/media/usb/uvc/uvc_gpio.c   | 118 ++++++++++++++++++++++++++++++++++++
> >  drivers/media/usb/uvc/uvcvideo.h   |   8 +++
> >  4 files changed, 131 insertions(+), 117 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> > index 4f9eee4f81ab..85514b6e538f 100644
> > --- a/drivers/media/usb/uvc/Makefile
> > +++ b/drivers/media/usb/uvc/Makefile
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
> > -               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
> > +               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
> > +               uvc_gpio.o
> >  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
> >  uvcvideo-objs  += uvc_entity.o
> >  endif
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index cd13bf01265d..5b48768a4f7f 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -8,7 +8,6 @@
> >
> >  #include <linux/atomic.h>
> >  #include <linux/bits.h>
> > -#include <linux/gpio/consumer.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> >  #include <linux/module.h>
> > @@ -775,9 +774,9 @@ static const u8 uvc_media_transport_input_guid[16] =
> >       UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> >  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> >
> > -static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> > -                                            u16 id, unsigned int num_pads,
> > -                                            unsigned int extra_size)
> > +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> > +                                     u16 id, unsigned int num_pads,
> > +                                     unsigned int extra_size)
> >  {
> >       struct uvc_entity *entity;
> >       unsigned int num_inputs;
> > @@ -1240,118 +1239,6 @@ static int uvc_parse_control(struct uvc_device *dev)
> >       return 0;
> >  }
> >
> > -/* -----------------------------------------------------------------------------
> > - * Privacy GPIO
> > - */
> > -
> > -static void uvc_gpio_event(struct uvc_device *dev)
> > -{
> > -     struct uvc_entity *unit = dev->gpio_unit;
> > -     struct uvc_video_chain *chain;
> > -     u8 new_val;
> > -
> > -     if (!unit)
> > -             return;
> > -
> > -     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > -
> > -     /* GPIO entities are always on the first chain. */
> > -     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > -     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > -}
> > -
> > -static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > -                         u8 cs, void *data, u16 size)
> > -{
> > -     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > -             return -EINVAL;
> > -
> > -     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > -
> > -     return 0;
> > -}
> > -
> > -static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > -                          u8 cs, u8 *caps)
> > -{
> > -     if (cs != UVC_CT_PRIVACY_CONTROL)
> > -             return -EINVAL;
> > -
> > -     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > -     return 0;
> > -}
> > -
> > -static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > -{
> > -     struct uvc_device *dev = data;
> > -
> > -     uvc_gpio_event(dev);
> > -     return IRQ_HANDLED;
> > -}
> > -
> > -static int uvc_gpio_parse(struct uvc_device *dev)
> > -{
> > -     struct uvc_entity *unit;
> > -     struct gpio_desc *gpio_privacy;
> > -     int irq;
> > -
> > -     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > -                                            GPIOD_IN);
> > -     if (IS_ERR_OR_NULL(gpio_privacy))
> > -             return PTR_ERR_OR_ZERO(gpio_privacy);
> > -
> > -     irq = gpiod_to_irq(gpio_privacy);
> > -     if (irq < 0)
> > -             return dev_err_probe(&dev->intf->dev, irq,
> > -                                  "No IRQ for privacy GPIO\n");
> > -
> > -     unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> > -                                 UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > -     if (IS_ERR(unit))
> > -             return PTR_ERR(unit);
> > -
> > -     unit->gpio.gpio_privacy = gpio_privacy;
> > -     unit->gpio.irq = irq;
> > -     unit->gpio.bControlSize = 1;
> > -     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > -     unit->gpio.bmControls[0] = 1;
> > -     unit->get_cur = uvc_gpio_get_cur;
> > -     unit->get_info = uvc_gpio_get_info;
> > -     strscpy(unit->name, "GPIO", sizeof(unit->name));
> > -
> > -     list_add_tail(&unit->list, &dev->entities);
> > -
> > -     dev->gpio_unit = unit;
> > -
> > -     return 0;
> > -}
> > -
> > -static int uvc_gpio_init_irq(struct uvc_device *dev)
> > -{
> > -     struct uvc_entity *unit = dev->gpio_unit;
> > -     int ret;
> > -
> > -     if (!unit || unit->gpio.irq < 0)
> > -             return 0;
> > -
> > -     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > -                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > -                                IRQF_TRIGGER_RISING,
> > -                                "uvc_privacy_gpio", dev);
> > -
> > -     unit->gpio.initialized = !ret;
> > -
> > -     return ret;
> > -}
> > -
> > -static void uvc_gpio_deinit(struct uvc_device *dev)
> > -{
> > -     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > -             return;
> > -
> > -     free_irq(dev->gpio_unit->gpio.irq, dev);
> > -}
> > -
> >  /* ------------------------------------------------------------------------
> >   * UVC device scan
> >   */
> > diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
> > new file mode 100644
> > index 000000000000..453739acbe8f
> > --- /dev/null
> > +++ b/drivers/media/usb/uvc/uvc_gpio.c
> > @@ -0,0 +1,118 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *      uvc_gpio.c  --  USB Video Class driver
> > + *
> > + *      Copyright 2024 Google LLC
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/gpio/consumer.h>
> > +#include "uvcvideo.h"
> > +
> > +static void uvc_gpio_event(struct uvc_device *dev)
> > +{
> > +     struct uvc_entity *unit = dev->gpio_unit;
> > +     struct uvc_video_chain *chain;
> > +     u8 new_val;
> > +
> > +     if (!unit)
> > +             return;
> > +
> > +     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > +
> > +     /* GPIO entities are always on the first chain. */
> > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > +     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > +}
> > +
> > +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > +                         u8 cs, void *data, u16 size)
> > +{
> > +     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > +             return -EINVAL;
> > +
> > +     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > +
> > +     return 0;
> > +}
> > +
> > +static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > +                          u8 cs, u8 *caps)
> > +{
> > +     if (cs != UVC_CT_PRIVACY_CONTROL)
> > +             return -EINVAL;
> > +
> > +     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > +     return 0;
> > +}
> > +
> > +static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > +{
> > +     struct uvc_device *dev = data;
> > +
> > +     uvc_gpio_event(dev);
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +int uvc_gpio_parse(struct uvc_device *dev)
> > +{
> > +     struct uvc_entity *unit;
> > +     struct gpio_desc *gpio_privacy;
> > +     int irq;
> > +
> > +     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > +                                            GPIOD_IN);
> > +     if (IS_ERR_OR_NULL(gpio_privacy))
> > +             return PTR_ERR_OR_ZERO(gpio_privacy);
> > +
> > +     irq = gpiod_to_irq(gpio_privacy);
> > +     if (irq < 0)
> > +             return dev_err_probe(&dev->udev->dev, irq,
> > +                                  "No IRQ for privacy GPIO\n");
> > +
> > +     unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> > +                                 UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > +     if (IS_ERR(unit))
> > +             return PTR_ERR(unit);
> > +
> > +     unit->gpio.gpio_privacy = gpio_privacy;
> > +     unit->gpio.irq = irq;
> > +     unit->gpio.bControlSize = 1;
> > +     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > +     unit->gpio.bmControls[0] = 1;
> > +     unit->get_cur = uvc_gpio_get_cur;
> > +     unit->get_info = uvc_gpio_get_info;
> > +     strscpy(unit->name, "GPIO", sizeof(unit->name));
> > +
> > +     list_add_tail(&unit->list, &dev->entities);
> > +
> > +     dev->gpio_unit = unit;
> > +
> > +     return 0;
> > +}
> > +
> > +int uvc_gpio_init_irq(struct uvc_device *dev)
> > +{
> > +     struct uvc_entity *unit = dev->gpio_unit;
> > +     int ret;
> > +
> > +     if (!unit || unit->gpio.irq < 0)
> > +             return 0;
> > +
> > +     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > +                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > +                                IRQF_TRIGGER_RISING,
> > +                                "uvc_privacy_gpio", dev);
> > +
> > +     unit->gpio.initialized = !ret;
> > +
> > +     return ret;
> > +}
> > +
> > +void uvc_gpio_deinit(struct uvc_device *dev)
> > +{
> > +     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > +             return;
> > +
> > +     free_irq(dev->gpio_unit->gpio.irq, dev);
> > +}
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 965a789ed03e..91ed59b54d9a 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -673,6 +673,9 @@ do {                                                                      \
> >  extern struct uvc_driver uvc_driver;
> >
> >  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> > +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> > +                                     u16 id, unsigned int num_pads,
> > +                                     unsigned int extra_size);
> >
> >  /* Video buffers queue management. */
> >  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
> > @@ -817,4 +820,9 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
> >  size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
> >                           size_t size);
> >
> > +/* gpio */
> > +int uvc_gpio_parse(struct uvc_device *dev);
> > +int uvc_gpio_init_irq(struct uvc_device *dev);
> > +void uvc_gpio_deinit(struct uvc_device *dev);
> > +
> >  #endif
> >
>


-- 
Ricardo Ribalda

Re: [PATCH v3 2/8] media: uvcvideo: Factor out gpio functions to its own file

[Hans de Goede]

Hi,

On 25-Nov-24 4:10 PM, Ricardo Ribalda wrote:
> On Mon, 25 Nov 2024 at 15:45, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Ricardo,
>>
>> On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote:
>>> This is just a refactor patch, no new functionality is added.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>
>> I guess this patch may need to change depending on if we want
>> to keep the GPIO handling as a UVC entity or not.
> 
> I have a v4 that removes the virtual uvc entity. Still with that
> approach IMHO this patch makes sense.

Right I was not suggesting dropping it, but atm it is moving
the uvc entity creation around. I was wondering if that
moving around before removing it still makes sense ?

Note I have no preference either way if first moving it
and then dropping it is cleaner, or just easier because
of the history of this patch-set then IMHO either way
is fine.

> I was planning to send it today, I am testing it right now.
> 
>>
>> Laurent what is your take on this, should this stay as
>> a struct uvc_entity; or should the gpio_desc and input_device
>> be stored directly inside struct uvc_device as is done for
>> the snapshot-button input_device?
>>
>>
>> Also de we want a separate input_device for this or do
>> we re-use the snapshot button one ?
>>
>> Since my plan is to open-up the permission on the device with
>> the SW_CAMERA_LENS_COVER to be equal to the /dev/video#
>> permissions sharing has the downside of allowing keylogging
>> of the snapshot button.
> 
> A downside of having 2 devices is that userspace will have to either
> figure out what evdev they want to use or listen to both....

Right, so both would be picked up by the compositor and the
snapshot button is just another multimedia-key then, while
the compositor can use SW_CAMERA_LENS_COVER for its OSD.

The other would be of interest to libcamera.

I'm think we may need a naming convention for the evdev
with SW_CAMERA_LENS_COVER something like "* camera privacy"
or whatever then we can have a udev rule matching on that for
uaccess + libcamera can do a strstr on the name for
"camera privacy" and ignore other evdevs.

Regards,

Hans

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/8] media: uvcvideo: Factor out gpio functions to its own file

[Laurent Pinchart]

On Mon, Nov 25, 2024 at 03:45:38PM +0100, Hans de Goede wrote:
> Hi Ricardo,
> 
> On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote:
> > This is just a refactor patch, no new functionality is added.
> > 
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> I guess this patch may need to change depending on if we want
> to keep the GPIO handling as a UVC entity or not.
> 
> Laurent what is your take on this, should this stay as
> a struct uvc_entity; or should the gpio_desc and input_device
> be stored directly inside struct uvc_device as is done for
> the snapshot-button input_device?

If we stop exposing it as a subdev I'd rather not create a uvc_entity.

> Also de we want a separate input_device for this or do
> we re-use the snapshot button one ?
> 
> Since my plan is to open-up the permission on the device with
> the SW_CAMERA_LENS_COVER to be equal to the /dev/video#
> permissions sharing has the downside of allowing keylogging
> of the snapshot button.

Given that the only standardized usage of the button is to inform
applications they should capture a still image, I don't think that's
much of a problem. Devices can also report that their button is a
generic purpose button, but I would still expect its function to be
camera-related.

Can anyone think of an attack vector ?

> Either way (one or 2 input-devices) I don't have a strong
> preference.
> 
> > ---
> >  drivers/media/usb/uvc/Makefile     |   3 +-
> >  drivers/media/usb/uvc/uvc_driver.c | 119 +------------------------------------
> >  drivers/media/usb/uvc/uvc_gpio.c   | 118 ++++++++++++++++++++++++++++++++++++
> >  drivers/media/usb/uvc/uvcvideo.h   |   8 +++
> >  4 files changed, 131 insertions(+), 117 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> > index 4f9eee4f81ab..85514b6e538f 100644
> > --- a/drivers/media/usb/uvc/Makefile
> > +++ b/drivers/media/usb/uvc/Makefile
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
> > -		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
> > +		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
> > +		  uvc_gpio.o
> >  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
> >  uvcvideo-objs  += uvc_entity.o
> >  endif
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index cd13bf01265d..5b48768a4f7f 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -8,7 +8,6 @@
> >  
> >  #include <linux/atomic.h>
> >  #include <linux/bits.h>
> > -#include <linux/gpio/consumer.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> >  #include <linux/module.h>
> > @@ -775,9 +774,9 @@ static const u8 uvc_media_transport_input_guid[16] =
> >  	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> >  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> >  
> > -static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> > -					       u16 id, unsigned int num_pads,
> > -					       unsigned int extra_size)
> > +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> > +					u16 id, unsigned int num_pads,
> > +					unsigned int extra_size)
> >  {
> >  	struct uvc_entity *entity;
> >  	unsigned int num_inputs;
> > @@ -1240,118 +1239,6 @@ static int uvc_parse_control(struct uvc_device *dev)
> >  	return 0;
> >  }
> >  
> > -/* -----------------------------------------------------------------------------
> > - * Privacy GPIO
> > - */
> > -
> > -static void uvc_gpio_event(struct uvc_device *dev)
> > -{
> > -	struct uvc_entity *unit = dev->gpio_unit;
> > -	struct uvc_video_chain *chain;
> > -	u8 new_val;
> > -
> > -	if (!unit)
> > -		return;
> > -
> > -	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > -
> > -	/* GPIO entities are always on the first chain. */
> > -	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > -	uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > -}
> > -
> > -static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > -			    u8 cs, void *data, u16 size)
> > -{
> > -	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > -		return -EINVAL;
> > -
> > -	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > -
> > -	return 0;
> > -}
> > -
> > -static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > -			     u8 cs, u8 *caps)
> > -{
> > -	if (cs != UVC_CT_PRIVACY_CONTROL)
> > -		return -EINVAL;
> > -
> > -	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > -	return 0;
> > -}
> > -
> > -static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > -{
> > -	struct uvc_device *dev = data;
> > -
> > -	uvc_gpio_event(dev);
> > -	return IRQ_HANDLED;
> > -}
> > -
> > -static int uvc_gpio_parse(struct uvc_device *dev)
> > -{
> > -	struct uvc_entity *unit;
> > -	struct gpio_desc *gpio_privacy;
> > -	int irq;
> > -
> > -	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > -					       GPIOD_IN);
> > -	if (IS_ERR_OR_NULL(gpio_privacy))
> > -		return PTR_ERR_OR_ZERO(gpio_privacy);
> > -
> > -	irq = gpiod_to_irq(gpio_privacy);
> > -	if (irq < 0)
> > -		return dev_err_probe(&dev->intf->dev, irq,
> > -				     "No IRQ for privacy GPIO\n");
> > -
> > -	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> > -				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > -	if (IS_ERR(unit))
> > -		return PTR_ERR(unit);
> > -
> > -	unit->gpio.gpio_privacy = gpio_privacy;
> > -	unit->gpio.irq = irq;
> > -	unit->gpio.bControlSize = 1;
> > -	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > -	unit->gpio.bmControls[0] = 1;
> > -	unit->get_cur = uvc_gpio_get_cur;
> > -	unit->get_info = uvc_gpio_get_info;
> > -	strscpy(unit->name, "GPIO", sizeof(unit->name));
> > -
> > -	list_add_tail(&unit->list, &dev->entities);
> > -
> > -	dev->gpio_unit = unit;
> > -
> > -	return 0;
> > -}
> > -
> > -static int uvc_gpio_init_irq(struct uvc_device *dev)
> > -{
> > -	struct uvc_entity *unit = dev->gpio_unit;
> > -	int ret;
> > -
> > -	if (!unit || unit->gpio.irq < 0)
> > -		return 0;
> > -
> > -	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > -				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > -				   IRQF_TRIGGER_RISING,
> > -				   "uvc_privacy_gpio", dev);
> > -
> > -	unit->gpio.initialized = !ret;
> > -
> > -	return ret;
> > -}
> > -
> > -static void uvc_gpio_deinit(struct uvc_device *dev)
> > -{
> > -	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > -		return;
> > -
> > -	free_irq(dev->gpio_unit->gpio.irq, dev);
> > -}
> > -
> >  /* ------------------------------------------------------------------------
> >   * UVC device scan
> >   */
> > diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
> > new file mode 100644
> > index 000000000000..453739acbe8f
> > --- /dev/null
> > +++ b/drivers/media/usb/uvc/uvc_gpio.c
> > @@ -0,0 +1,118 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *      uvc_gpio.c  --  USB Video Class driver
> > + *
> > + *      Copyright 2024 Google LLC
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/gpio/consumer.h>
> > +#include "uvcvideo.h"
> > +
> > +static void uvc_gpio_event(struct uvc_device *dev)
> > +{
> > +	struct uvc_entity *unit = dev->gpio_unit;
> > +	struct uvc_video_chain *chain;
> > +	u8 new_val;
> > +
> > +	if (!unit)
> > +		return;
> > +
> > +	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > +
> > +	/* GPIO entities are always on the first chain. */
> > +	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > +	uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > +}
> > +
> > +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > +			    u8 cs, void *data, u16 size)
> > +{
> > +	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > +		return -EINVAL;
> > +
> > +	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > +
> > +	return 0;
> > +}
> > +
> > +static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > +			     u8 cs, u8 *caps)
> > +{
> > +	if (cs != UVC_CT_PRIVACY_CONTROL)
> > +		return -EINVAL;
> > +
> > +	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > +{
> > +	struct uvc_device *dev = data;
> > +
> > +	uvc_gpio_event(dev);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +int uvc_gpio_parse(struct uvc_device *dev)
> > +{
> > +	struct uvc_entity *unit;
> > +	struct gpio_desc *gpio_privacy;
> > +	int irq;
> > +
> > +	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > +					       GPIOD_IN);
> > +	if (IS_ERR_OR_NULL(gpio_privacy))
> > +		return PTR_ERR_OR_ZERO(gpio_privacy);
> > +
> > +	irq = gpiod_to_irq(gpio_privacy);
> > +	if (irq < 0)
> > +		return dev_err_probe(&dev->udev->dev, irq,
> > +				     "No IRQ for privacy GPIO\n");
> > +
> > +	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> > +				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > +	if (IS_ERR(unit))
> > +		return PTR_ERR(unit);
> > +
> > +	unit->gpio.gpio_privacy = gpio_privacy;
> > +	unit->gpio.irq = irq;
> > +	unit->gpio.bControlSize = 1;
> > +	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > +	unit->gpio.bmControls[0] = 1;
> > +	unit->get_cur = uvc_gpio_get_cur;
> > +	unit->get_info = uvc_gpio_get_info;
> > +	strscpy(unit->name, "GPIO", sizeof(unit->name));
> > +
> > +	list_add_tail(&unit->list, &dev->entities);
> > +
> > +	dev->gpio_unit = unit;
> > +
> > +	return 0;
> > +}
> > +
> > +int uvc_gpio_init_irq(struct uvc_device *dev)
> > +{
> > +	struct uvc_entity *unit = dev->gpio_unit;
> > +	int ret;
> > +
> > +	if (!unit || unit->gpio.irq < 0)
> > +		return 0;
> > +
> > +	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > +				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > +				   IRQF_TRIGGER_RISING,
> > +				   "uvc_privacy_gpio", dev);
> > +
> > +	unit->gpio.initialized = !ret;
> > +
> > +	return ret;
> > +}
> > +
> > +void uvc_gpio_deinit(struct uvc_device *dev)
> > +{
> > +	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > +		return;
> > +
> > +	free_irq(dev->gpio_unit->gpio.irq, dev);
> > +}
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 965a789ed03e..91ed59b54d9a 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -673,6 +673,9 @@ do {									\
> >  extern struct uvc_driver uvc_driver;
> >  
> >  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> > +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> > +					u16 id, unsigned int num_pads,
> > +					unsigned int extra_size);
> >  
> >  /* Video buffers queue management. */
> >  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
> > @@ -817,4 +820,9 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
> >  size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
> >  			    size_t size);
> >  
> > +/* gpio */
> > +int uvc_gpio_parse(struct uvc_device *dev);
> > +int uvc_gpio_init_irq(struct uvc_device *dev);
> > +void uvc_gpio_deinit(struct uvc_device *dev);
> > +
> >  #endif
> > 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Ricardo Ribalda]

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



-- 
Ricardo Ribalda

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Ricardo Ribalda]

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



-- 
Ricardo Ribalda

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

[Hans de Goede]

Hi,

On 26-Nov-24 6:12 PM, Ricardo Ribalda wrote:
> 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?

I would like to see this wired up in GNOME but I'm not aware of
anyone actively working on this.

I expect that for GNOME a simple OSD with a camera icon with / without
a cross through it will suffice and I expect such a simple implementation
to directly talk to libinput at the compositor level.

If GNOME does ever wants to show a label on the OSD with a description
of which camera it applies to, like it currently does for volume up/down/
mute keys which affect the current default sound output), then I would
expect it to talk to pipewire to get the events instead of directly
through libinput.

Either scenario can be supported with the SW_CAMERA_LENS_COVER userspace
API, so IMHO this is an implementation detail which can be left up to
whomever implements this for GNOME.

FWIW if I were to implement this myself I would go for the simple solution
of not showing a camera description like ChromeOS is currently doing.

Regards,

Hans