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

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

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

We tried to fix the power-up issues implementing "granular power
saving" but it has been more complicated than anticipated....

Last year, we proposed a patchset to implement the privacy gpio as a
subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/

I think it is a pretty clean solution and makes sense to use a
subdevice for something that is a sub device of the camera :).

This is an attempt to continue with that approach.

Tested on gimble:
gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0
Driver Info:
        Driver version   : 6.6.56
        Capabilities     : 0x00000000
Media Driver Info:
        Driver name      : uvcvideo
        Model            : HP 5M Camera: HP 5M Camera
        Serial           : 0001
        Bus info         : usb-0000:00:14.0-6
        Media version    : 6.6.56
        Hardware revision: 0x00009601 (38401)
        Driver version   : 6.6.56
Interface Info:
        ID               : 0x0300001d
        Type             : V4L Sub-Device
Entity Info:
        ID               : 0x00000013 (19)
        Name             : GPIO
        Function         : Unknown sub-device (00020006)

Camera Controls

                        privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only, volatile

gimble-rev3 ~ # media-ctl  -p
Media controller API version 6.6.56

Media device information
------------------------
driver          uvcvideo
model           HP 5M Camera: HP 5M Camera
serial          0001
bus info        usb-0000:00:14.0-6
hw revision     0x9601
driver version  6.6.56

Device topology
- entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link)
            type Node subtype V4L flags 1
            device node name /dev/video0
        pad0: Sink
                <- "Extension 8":1 [ENABLED,IMMUTABLE]

- entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link)
            type Node subtype V4L flags 0
            device node name /dev/video1

- entity 8: Extension 8 (2 pads, 2 links, 0 routes)
            type V4L2 subdev subtype Unknown flags 0
        pad0: Sink
                <- "Extension 4":1 [ENABLED,IMMUTABLE]
        pad1: Source
                -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE]

- entity 11: Extension 4 (2 pads, 2 links, 0 routes)
             type V4L2 subdev subtype Unknown flags 0
        pad0: Sink
                <- "Processing 2":1 [ENABLED,IMMUTABLE]
        pad1: Source
                -> "Extension 8":0 [ENABLED,IMMUTABLE]

- entity 14: Processing 2 (2 pads, 2 links, 0 routes)
             type V4L2 subdev subtype Unknown flags 0
        pad0: Sink
                <- "Camera 1":0 [ENABLED,IMMUTABLE]
        pad1: Source
                -> "Extension 4":0 [ENABLED,IMMUTABLE]

- entity 17: Camera 1 (1 pad, 1 link, 0 routes)
             type V4L2 subdev subtype Sensor flags 0
        pad0: Source
                -> "Processing 2":0 [ENABLED,IMMUTABLE]

- entity 19: GPIO (0 pad, 0 link, 0 routes)
             type V4L2 subdev subtype Decoder flags 0
             device node name /dev/v4l-subdev0

Signed-off-by: Ricardo Ribalda <ribalda@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 (5):
      media: uvcvideo: Factor out gpio functions to its own file
      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

Yunke Cao (1):
      media: uvcvideo: Re-implement privacy GPIO as a separate subdevice

 .../userspace-api/media/mediactl/media-types.rst   |   4 +
 drivers/media/usb/uvc/Makefile                     |   3 +-
 drivers/media/usb/uvc/uvc_ctrl.c                   |  40 +----
 drivers/media/usb/uvc/uvc_driver.c                 | 123 +-------------
 drivers/media/usb/uvc/uvc_entity.c                 |  20 ++-
 drivers/media/usb/uvc/uvc_gpio.c                   | 187 +++++++++++++++++++++
 drivers/media/usb/uvc/uvc_video.c                  |   4 +
 drivers/media/usb/uvc/uvcvideo.h                   |  34 ++--
 drivers/media/v4l2-core/v4l2-async.c               |   3 +-
 include/uapi/linux/media.h                         |   1 +
 10 files changed, 252 insertions(+), 167 deletions(-)
---
base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525
change-id: 20241030-uvc-subdev-89f4467a00b5

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v2 1/6] 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 v2 2/6] media: uvcvideo: Re-implement privacy GPIO as a separate subdevice

[Ricardo Ribalda]

From: Yunke Cao <yunkec@chromium.org>

Reimplement privacy GPIO as a v4l2 subdev with a volatile privacy control.
A v4l2 control event is sent in irq when privacy control value changes.

The behavior matches the original implementation, except that the
control is of a separate subdevice.

V4L2 control kAPI is used for simplicity.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: Yunke Cao <yunkec@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c   |  20 -------
 drivers/media/usb/uvc/uvc_driver.c |   4 +-
 drivers/media/usb/uvc/uvc_entity.c |   6 ++
 drivers/media/usb/uvc/uvc_gpio.c   | 111 ++++++++++++++++++++++---------------
 drivers/media/usb/uvc/uvcvideo.h   |  20 ++++---
 5 files changed, 88 insertions(+), 73 deletions(-)

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..fe5a53fd0e14 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1779,6 +1779,7 @@ static void uvc_delete(struct kref *kref)
 	struct uvc_device *dev = container_of(kref, struct uvc_device, ref);
 	struct list_head *p, *n;
 
+	uvc_gpio_cleanup(dev);
 	uvc_status_cleanup(dev);
 	uvc_ctrl_cleanup_device(dev);
 
@@ -2171,6 +2172,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 +2184,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..af72df96bc57 100644
--- a/drivers/media/usb/uvc/uvc_gpio.c
+++ b/drivers/media/usb/uvc/uvc_gpio.c
@@ -7,57 +7,46 @@
 
 #include <linux/kernel.h>
 #include <linux/gpio/consumer.h>
+#include <media/v4l2-ctrls.h>
 #include "uvcvideo.h"
 
-static void uvc_gpio_event(struct uvc_device *dev)
+static int uvc_gpio_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
-	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);
-}
+	struct uvc_gpio *gpio =
+		container_of(ctrl->handler, struct uvc_gpio, hdl);
+	int ret;
 
-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;
+	ret = gpiod_get_value_cansleep(gpio->gpio_privacy);
+	if (ret < 0)
+		return ret;
 
-	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
+	ctrl->cur.val = ret;
 
 	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 const struct v4l2_ctrl_ops uvc_gpio_ctrl_ops = {
+	.g_volatile_ctrl = uvc_gpio_g_volatile_ctrl,
+};
 
 static irqreturn_t uvc_gpio_irq(int irq, void *data)
 {
-	struct uvc_device *dev = data;
+	struct uvc_gpio *uvc_gpio = data;
+	int new_val;
+
+	new_val = gpiod_get_value_cansleep(uvc_gpio->gpio_privacy);
+	if (new_val < 0)
+		return IRQ_HANDLED;
+
+	v4l2_ctrl_s_ctrl(uvc_gpio->privacy_ctrl, new_val);
 
-	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 +56,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,21 +74,53 @@ 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;
+
+	v4l2_ctrl_handler_init(&unit->gpio.hdl, 1);
+	unit->gpio.privacy_ctrl = v4l2_ctrl_new_std(&unit->gpio.hdl,
+						    &uvc_gpio_ctrl_ops,
+						    V4L2_CID_PRIVACY,
+						    0, 1, 1, init_val);
+	if (!unit->gpio.privacy_ctrl) {
+		ret = unit->gpio.hdl.error;
+		goto cleanup;
+	}
+
+	unit->gpio.privacy_ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE |
+					  V4L2_CTRL_FLAG_READ_ONLY;
+
+	unit->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			      V4L2_SUBDEV_FL_HAS_EVENTS;
+	unit->subdev.ctrl_handler = &unit->gpio.hdl;
+
+	ret = v4l2_device_register_subdev_nodes(&dev->vdev);
+	if (ret)
+		goto cleanup;
+
 	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
 				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
 				   IRQF_TRIGGER_RISING,
-				   "uvc_privacy_gpio", dev);
+				   "uvc_privacy_gpio", &unit->gpio);
+	if (ret)
+		goto cleanup;
 
-	unit->gpio.initialized = !ret;
+	unit->gpio.initialized = true;
 
+	return 0;
+
+cleanup:
+	v4l2_ctrl_handler_free(&unit->gpio.hdl);
 	return ret;
 }
 
@@ -114,5 +129,13 @@ 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);
+	free_irq(dev->gpio_unit->gpio.irq, &dev->gpio_unit->gpio);
+}
+
+void uvc_gpio_cleanup(struct uvc_device *dev)
+{
+	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
+		return;
+
+	v4l2_ctrl_handler_free(&dev->gpio_unit->gpio.hdl);
 }
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 91ed59b54d9a..4a06db329aa9 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>
@@ -171,6 +172,14 @@ struct uvc_control {
 
 #define UVC_ENTITY_FLAG_DEFAULT		(1 << 0)
 
+struct uvc_gpio {
+	int irq;
+	bool initialized;
+	struct v4l2_ctrl_handler hdl;
+	struct v4l2_ctrl *privacy_ctrl;
+	struct gpio_desc *gpio_privacy;
+};
+
 struct uvc_entity {
 	struct list_head list;		/* Entity as part of a UVC device. */
 	struct list_head chain;		/* Entity as part of a video device chain. */
@@ -229,13 +238,7 @@ struct uvc_entity {
 			u8  *bmControlsType;
 		} extension;
 
-		struct {
-			u8  bControlSize;
-			u8  *bmControls;
-			struct gpio_desc *gpio_privacy;
-			int irq;
-			bool initialized;
-		} gpio;
+		struct uvc_gpio gpio;
 	};
 
 	u8 bNrInPins;
@@ -822,7 +825,8 @@ size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
 
 /* gpio */
 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);
+void uvc_gpio_cleanup(struct uvc_device *dev);
 
 #endif

-- 
2.47.0.277.g8800431eea-goog

[PATCH v2 3/6] Revert "media: uvcvideo: Allow entity-defined get_info and get_cur"

[Ricardo Ribalda]

With the privacy gpio now handled as a subdevice, 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 4a06db329aa9..1eeef2c12fbe 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -244,11 +244,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 v2 4/6] media: uvcvideo: Create ancillary link for GPIO subdevice

[Ricardo Ribalda]

Make an ancillary device between the streaming subdevice and the GPIO
subdevice.

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

diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
index c1b69f9eaa56..dad77b96fe16 100644
--- a/drivers/media/usb/uvc/uvc_entity.c
+++ b/drivers/media/usb/uvc/uvc_entity.c
@@ -53,6 +53,16 @@ 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

Re: [PATCH v2 4/6] media: uvcvideo: Create ancillary link for GPIO subdevice

[Sakari Ailus]

Hi Ricardo,

On Fri, Nov 08, 2024 at 08:25:48PM +0000, Ricardo Ribalda wrote:
> Make an ancillary device between the streaming subdevice and the GPIO

s/device/link/

> subdevice.

Either subdev or sub-device. Same above.

> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_entity.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
> index c1b69f9eaa56..dad77b96fe16 100644
> --- a/drivers/media/usb/uvc/uvc_entity.c
> +++ b/drivers/media/usb/uvc/uvc_entity.c
> @@ -53,6 +53,16 @@ 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) {

This seems to be over 80 for no apparent reason.

> +		struct media_link *link;
> +
> +		link = media_create_ancillary_link(sink,
> +					&chain->dev->gpio_unit->subdev.entity);

And following the alignment rules would push this over 80. I have no
problem with that.

> +		if (IS_ERR(link))
> +			return PTR_ERR(link);
> +	}
> +
>  	return 0;
>  }
>  
> 

-- 
Regards,

Sakari Ailus

[PATCH v2 5/6] 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 v2 6/6] 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 +++-
 drivers/media/usb/uvc/uvc_gpio.c   | 46 ++++++++++++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvc_video.c  |  4 ++++
 drivers/media/usb/uvc/uvcvideo.h   |  3 +++
 4 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
index dad77b96fe16..3cb95df1f670 100644
--- a/drivers/media/usb/uvc/uvc_entity.c
+++ b/drivers/media/usb/uvc/uvc_entity.c
@@ -114,6 +114,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:
@@ -121,7 +124,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;
diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
index af72df96bc57..b19ca222c9f2 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 <media/v4l2-ctrls.h>
@@ -16,6 +17,9 @@ static int uvc_gpio_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 		container_of(ctrl->handler, struct uvc_gpio, hdl);
 	int ret;
 
+	if (!gpio->gpio_ready)
+		return -EBUSY;
+
 	ret = gpiod_get_value_cansleep(gpio->gpio_privacy);
 	if (ret < 0)
 		return ret;
@@ -34,6 +38,9 @@ static irqreturn_t uvc_gpio_irq(int irq, void *data)
 	struct uvc_gpio *uvc_gpio = data;
 	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;
@@ -43,6 +50,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;
@@ -64,6 +89,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));
@@ -74,6 +108,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->gpio_unit->gpio);
+}
+
 int uvc_gpio_init(struct uvc_device *dev)
 {
 	struct uvc_entity *unit = dev->gpio_unit;
@@ -97,6 +141,8 @@ int uvc_gpio_init(struct uvc_device *dev)
 		goto cleanup;
 	}
 
+	uvc_gpio_quirk(dev, false);
+
 	unit->gpio.privacy_ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE |
 					  V4L2_CTRL_FLAG_READ_ONLY;
 
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 1eeef2c12fbe..b03152c9651b 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
@@ -175,6 +176,7 @@ struct uvc_control {
 struct uvc_gpio {
 	int irq;
 	bool initialized;
+	bool gpio_ready;
 	struct v4l2_ctrl_handler hdl;
 	struct v4l2_ctrl *privacy_ctrl;
 	struct gpio_desc *gpio_privacy;
@@ -823,5 +825,6 @@ 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_cleanup(struct uvc_device *dev);
+void uvc_gpio_quirk(struct uvc_device *dev, bool stream_on);
 
 #endif

-- 
2.47.0.277.g8800431eea-goog

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

[Mauro Carvalho Chehab]

Em Fri, 08 Nov 2024 20:25:44 +0000
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> 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 two 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.
> 
> We tried to fix the power-up issues implementing "granular power
> saving" but it has been more complicated than anticipated....
> 
> Last year, we proposed a patchset to implement the privacy gpio as a
> subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/
> 
> I think it is a pretty clean solution and makes sense to use a
> subdevice for something that is a sub device of the camera :).
> 
> This is an attempt to continue with that approach.
> 
> Tested on gimble:
> gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0

No matter if internally implemented as a subdevice or not,
UVC is not a MC-centric device[1]. 

It means that UVC can be compiled without media controller support,
and that its functionality shall be visible via /dev/video* nodes.

So, whatever internal implementation it is used, it shall not require
config MEDIA_CONTROLLER and the control shall be visible via
/dev/video*.

Moving privacy control out of /dev/video would mean that this will break 
support for it on existing applications, which is a big nack. Now, it would
be acceptable to have this visible via V4L2 and subdev APIs.

[1] https://www.kernel.org/doc/html/latest/userspace-api/media/glossary.html#term-MC-centric

Regards,
Mauro

> Driver Info:
>         Driver version   : 6.6.56
>         Capabilities     : 0x00000000
> Media Driver Info:
>         Driver name      : uvcvideo
>         Model            : HP 5M Camera: HP 5M Camera
>         Serial           : 0001
>         Bus info         : usb-0000:00:14.0-6
>         Media version    : 6.6.56
>         Hardware revision: 0x00009601 (38401)
>         Driver version   : 6.6.56
> Interface Info:
>         ID               : 0x0300001d
>         Type             : V4L Sub-Device
> Entity Info:
>         ID               : 0x00000013 (19)
>         Name             : GPIO
>         Function         : Unknown sub-device (00020006)
> 
> Camera Controls
> 
>                         privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only, volatile
> 
> gimble-rev3 ~ # media-ctl  -p
> Media controller API version 6.6.56
> 
> Media device information
> ------------------------
> driver          uvcvideo
> model           HP 5M Camera: HP 5M Camera
> serial          0001
> bus info        usb-0000:00:14.0-6
> hw revision     0x9601
> driver version  6.6.56
> 
> Device topology
> - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link)
>             type Node subtype V4L flags 1
>             device node name /dev/video0
>         pad0: Sink
>                 <- "Extension 8":1 [ENABLED,IMMUTABLE]
> 
> - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video1
> 
> - entity 8: Extension 8 (2 pads, 2 links, 0 routes)
>             type V4L2 subdev subtype Unknown flags 0
>         pad0: Sink
>                 <- "Extension 4":1 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE]  
> 
> - entity 11: Extension 4 (2 pads, 2 links, 0 routes)
>              type V4L2 subdev subtype Unknown flags 0
>         pad0: Sink
>                 <- "Processing 2":1 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 -> "Extension 8":0 [ENABLED,IMMUTABLE]  
> 
> - entity 14: Processing 2 (2 pads, 2 links, 0 routes)
>              type V4L2 subdev subtype Unknown flags 0
>         pad0: Sink
>                 <- "Camera 1":0 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 -> "Extension 4":0 [ENABLED,IMMUTABLE]  
> 
> - entity 17: Camera 1 (1 pad, 1 link, 0 routes)
>              type V4L2 subdev subtype Sensor flags 0
>         pad0: Source
>                 -> "Processing 2":0 [ENABLED,IMMUTABLE]  
> 
> - entity 19: GPIO (0 pad, 0 link, 0 routes)
>              type V4L2 subdev subtype Decoder flags 0
>              device node name /dev/v4l-subdev0
> 
> Signed-off-by: Ricardo Ribalda <ribalda@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 (5):
>       media: uvcvideo: Factor out gpio functions to its own file
>       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
> 
> Yunke Cao (1):
>       media: uvcvideo: Re-implement privacy GPIO as a separate subdevice
> 
>  .../userspace-api/media/mediactl/media-types.rst   |   4 +
>  drivers/media/usb/uvc/Makefile                     |   3 +-
>  drivers/media/usb/uvc/uvc_ctrl.c                   |  40 +----
>  drivers/media/usb/uvc/uvc_driver.c                 | 123 +-------------
>  drivers/media/usb/uvc/uvc_entity.c                 |  20 ++-
>  drivers/media/usb/uvc/uvc_gpio.c                   | 187 +++++++++++++++++++++
>  drivers/media/usb/uvc/uvc_video.c                  |   4 +
>  drivers/media/usb/uvc/uvcvideo.h                   |  34 ++--
>  drivers/media/v4l2-core/v4l2-async.c               |   3 +-
>  include/uapi/linux/media.h                         |   1 +
>  10 files changed, 252 insertions(+), 167 deletions(-)
> ---
> base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525
> change-id: 20241030-uvc-subdev-89f4467a00b5
> 
> Best regards,



Thanks,
Mauro

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

[Ricardo Ribalda]

Hi Mauro

On Sat, 9 Nov 2024 at 15:05, Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Fri, 08 Nov 2024 20:25:44 +0000
> Ricardo Ribalda <ribalda@chromium.org> escreveu:
>
> > 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 two 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.
> >
> > We tried to fix the power-up issues implementing "granular power
> > saving" but it has been more complicated than anticipated....
> >
> > Last year, we proposed a patchset to implement the privacy gpio as a
> > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/
> >
> > I think it is a pretty clean solution and makes sense to use a
> > subdevice for something that is a sub device of the camera :).
> >
> > This is an attempt to continue with that approach.
> >
> > Tested on gimble:
> > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0
>
> No matter if internally implemented as a subdevice or not,
> UVC is not a MC-centric device[1].
>
> It means that UVC can be compiled without media controller support,
> and that its functionality shall be visible via /dev/video* nodes.
>
> So, whatever internal implementation it is used, it shall not require
> config MEDIA_CONTROLLER and the control shall be visible via
> /dev/video*.
>
> Moving privacy control out of /dev/video would mean that this will break
> support for it on existing applications, which is a big nack. Now, it would
> be acceptable to have this visible via V4L2 and subdev APIs.

I have googled a bit, and it seems that the only users of this feature
is ChromeOS, so I do not expect any existing applications to be
impacted.

I can keep the old API, but that does not solve the issue when the
camera supports the privacy control and it is also attached to a GPIO.

I do not see a big requirement to depend on the MEDIA_CONTROLLER to
use the privacy GPIO. Remember that this feature is not part of the
camera itself, it is an external GPIO.

What about trying the subdevice approach, and if we break any app,
implement both APIs (legacy and subdevice)?

Please note that If a device has an internal Privacy control it will
still work via /dev/videoX.
>
> [1] https://www.kernel.org/doc/html/latest/userspace-api/media/glossary.html#term-MC-centric
>
> Regards,
> Mauro
>
> > Driver Info:
> >         Driver version   : 6.6.56
> >         Capabilities     : 0x00000000
> > Media Driver Info:
> >         Driver name      : uvcvideo
> >         Model            : HP 5M Camera: HP 5M Camera
> >         Serial           : 0001
> >         Bus info         : usb-0000:00:14.0-6
> >         Media version    : 6.6.56
> >         Hardware revision: 0x00009601 (38401)
> >         Driver version   : 6.6.56
> > Interface Info:
> >         ID               : 0x0300001d
> >         Type             : V4L Sub-Device
> > Entity Info:
> >         ID               : 0x00000013 (19)
> >         Name             : GPIO
> >         Function         : Unknown sub-device (00020006)
> >
> > Camera Controls
> >
> >                         privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only, volatile
> >
> > gimble-rev3 ~ # media-ctl  -p
> > Media controller API version 6.6.56
> >
> > Media device information
> > ------------------------
> > driver          uvcvideo
> > model           HP 5M Camera: HP 5M Camera
> > serial          0001
> > bus info        usb-0000:00:14.0-6
> > hw revision     0x9601
> > driver version  6.6.56
> >
> > Device topology
> > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link)
> >             type Node subtype V4L flags 1
> >             device node name /dev/video0
> >         pad0: Sink
> >                 <- "Extension 8":1 [ENABLED,IMMUTABLE]
> >
> > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link)
> >             type Node subtype V4L flags 0
> >             device node name /dev/video1
> >
> > - entity 8: Extension 8 (2 pads, 2 links, 0 routes)
> >             type V4L2 subdev subtype Unknown flags 0
> >         pad0: Sink
> >                 <- "Extension 4":1 [ENABLED,IMMUTABLE]
> >         pad1: Source
> >                 -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE]
> >
> > - entity 11: Extension 4 (2 pads, 2 links, 0 routes)
> >              type V4L2 subdev subtype Unknown flags 0
> >         pad0: Sink
> >                 <- "Processing 2":1 [ENABLED,IMMUTABLE]
> >         pad1: Source
> >                 -> "Extension 8":0 [ENABLED,IMMUTABLE]
> >
> > - entity 14: Processing 2 (2 pads, 2 links, 0 routes)
> >              type V4L2 subdev subtype Unknown flags 0
> >         pad0: Sink
> >                 <- "Camera 1":0 [ENABLED,IMMUTABLE]
> >         pad1: Source
> >                 -> "Extension 4":0 [ENABLED,IMMUTABLE]
> >
> > - entity 17: Camera 1 (1 pad, 1 link, 0 routes)
> >              type V4L2 subdev subtype Sensor flags 0
> >         pad0: Source
> >                 -> "Processing 2":0 [ENABLED,IMMUTABLE]
> >
> > - entity 19: GPIO (0 pad, 0 link, 0 routes)
> >              type V4L2 subdev subtype Decoder flags 0
> >              device node name /dev/v4l-subdev0
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@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 (5):
> >       media: uvcvideo: Factor out gpio functions to its own file
> >       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
> >
> > Yunke Cao (1):
> >       media: uvcvideo: Re-implement privacy GPIO as a separate subdevice
> >
> >  .../userspace-api/media/mediactl/media-types.rst   |   4 +
> >  drivers/media/usb/uvc/Makefile                     |   3 +-
> >  drivers/media/usb/uvc/uvc_ctrl.c                   |  40 +----
> >  drivers/media/usb/uvc/uvc_driver.c                 | 123 +-------------
> >  drivers/media/usb/uvc/uvc_entity.c                 |  20 ++-
> >  drivers/media/usb/uvc/uvc_gpio.c                   | 187 +++++++++++++++++++++
> >  drivers/media/usb/uvc/uvc_video.c                  |   4 +
> >  drivers/media/usb/uvc/uvcvideo.h                   |  34 ++--
> >  drivers/media/v4l2-core/v4l2-async.c               |   3 +-
> >  include/uapi/linux/media.h                         |   1 +
> >  10 files changed, 252 insertions(+), 167 deletions(-)
> > ---
> > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525
> > change-id: 20241030-uvc-subdev-89f4467a00b5
> >
> > Best regards,
>
>
>
> Thanks,
> Mauro



-- 
Ricardo Ribalda

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

[Hans de Goede]

Hi Ricardo,

FYI / some background: I have been asked to start helping /
co-maintaining UVC with Laurent. I'll send out a patch adding
myself as UVC maintainer soon.

On 8-Nov-24 9:25 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 two 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.
> 
> We tried to fix the power-up issues implementing "granular power
> saving" but it has been more complicated than anticipated....

I have been discussing UVC power-management with Laurent, also
related to power-consumption issues caused by libcamera's pipeline
handler holding open the /dev/video# node as long as the camera
manager object exists.

For now we have fixed this with some relatively small changes to
libcamera's uvcvideo pipeline handler, but that is really meant
as an interim solution and as this privacy control series shows
the power-management issues are real.

Combined with Mauro's remarks about how this is an userspace ABI break (1)
I think we should maybe first take another look at the powermanagement
issues in general rather then moving forward with this series.

My apologies for this, I realize how annoying it can be when you are
working on a patch series to fix a specific issue and a reviewer
moves the goal-posts like this. But I do really think that just fixing
the generic power-management issues would be better and I also think
that this should be feasible / not too hard.

Here is what I have in mind for this:

1. Assume that the results of trying a specific fmt do not change over time.

2. Only allow userspace to request fmts which match one of the enum-fmts ->
   enum-frame-sizes -> enum-frame-rates tripplet results
   (constrain what userspace requests to these)

3. Run the equivalent of tryfmt on all possible combinations (so the usaul
   3 levels nested loop for this) on probe() and cache the results

4. Make try_fmt / set_fmt not poweron the device but instead constrain
   the requested fmt to one from our cached fmts

5. On stream-on do the actual power-on + set-fmt + verify that we get
   what we expect based on the cache, and otherwise return -EIO.

I think that should sort the issue, assuming that 1. above holds true.

One downside is that this stops UVC button presses from working when
not streaming. But userspace will typically only open the /dev/video#
node if it plans to stream anyways so there should not be much of
a difference wrt button press behavior.

This should also make camera enumeration faster for apps, since
most apps / frameworks do the whole 3 levels nested loop for this
on startup, for which atm we go out to the hw, which now instead
will come from the fmts cache and thus will be much much faster,
so this should lead to a noticeable speedup for apps accessing UVC
cameras which would be another nice win.

Downside is that the initial probe will take longer see we do
all the tryfmt-s there now. But I think that taking a bit longer
to probe while the machine is booting should not be an issue.

Regards,

Hans


1) Which is technically correct, but FWIW I agree with you that I think
most userspace consumers will not care




> Last year, we proposed a patchset to implement the privacy gpio as a
> subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/
> 
> I think it is a pretty clean solution and makes sense to use a
> subdevice for something that is a sub device of the camera :).
> 
> This is an attempt to continue with that approach.
> 
> Tested on gimble:
> gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0
> Driver Info:
>         Driver version   : 6.6.56
>         Capabilities     : 0x00000000
> Media Driver Info:
>         Driver name      : uvcvideo
>         Model            : HP 5M Camera: HP 5M Camera
>         Serial           : 0001
>         Bus info         : usb-0000:00:14.0-6
>         Media version    : 6.6.56
>         Hardware revision: 0x00009601 (38401)
>         Driver version   : 6.6.56
> Interface Info:
>         ID               : 0x0300001d
>         Type             : V4L Sub-Device
> Entity Info:
>         ID               : 0x00000013 (19)
>         Name             : GPIO
>         Function         : Unknown sub-device (00020006)
> 
> Camera Controls
> 
>                         privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only, volatile
> 
> gimble-rev3 ~ # media-ctl  -p
> Media controller API version 6.6.56
> 
> Media device information
> ------------------------
> driver          uvcvideo
> model           HP 5M Camera: HP 5M Camera
> serial          0001
> bus info        usb-0000:00:14.0-6
> hw revision     0x9601
> driver version  6.6.56
> 
> Device topology
> - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link)
>             type Node subtype V4L flags 1
>             device node name /dev/video0
>         pad0: Sink
>                 <- "Extension 8":1 [ENABLED,IMMUTABLE]
> 
> - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video1
> 
> - entity 8: Extension 8 (2 pads, 2 links, 0 routes)
>             type V4L2 subdev subtype Unknown flags 0
>         pad0: Sink
>                 <- "Extension 4":1 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE]
> 
> - entity 11: Extension 4 (2 pads, 2 links, 0 routes)
>              type V4L2 subdev subtype Unknown flags 0
>         pad0: Sink
>                 <- "Processing 2":1 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 -> "Extension 8":0 [ENABLED,IMMUTABLE]
> 
> - entity 14: Processing 2 (2 pads, 2 links, 0 routes)
>              type V4L2 subdev subtype Unknown flags 0
>         pad0: Sink
>                 <- "Camera 1":0 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 -> "Extension 4":0 [ENABLED,IMMUTABLE]
> 
> - entity 17: Camera 1 (1 pad, 1 link, 0 routes)
>              type V4L2 subdev subtype Sensor flags 0
>         pad0: Source
>                 -> "Processing 2":0 [ENABLED,IMMUTABLE]
> 
> - entity 19: GPIO (0 pad, 0 link, 0 routes)
>              type V4L2 subdev subtype Decoder flags 0
>              device node name /dev/v4l-subdev0
> 
> Signed-off-by: Ricardo Ribalda <ribalda@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 (5):
>       media: uvcvideo: Factor out gpio functions to its own file
>       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
> 
> Yunke Cao (1):
>       media: uvcvideo: Re-implement privacy GPIO as a separate subdevice
> 
>  .../userspace-api/media/mediactl/media-types.rst   |   4 +
>  drivers/media/usb/uvc/Makefile                     |   3 +-
>  drivers/media/usb/uvc/uvc_ctrl.c                   |  40 +----
>  drivers/media/usb/uvc/uvc_driver.c                 | 123 +-------------
>  drivers/media/usb/uvc/uvc_entity.c                 |  20 ++-
>  drivers/media/usb/uvc/uvc_gpio.c                   | 187 +++++++++++++++++++++
>  drivers/media/usb/uvc/uvc_video.c                  |   4 +
>  drivers/media/usb/uvc/uvcvideo.h                   |  34 ++--
>  drivers/media/v4l2-core/v4l2-async.c               |   3 +-
>  include/uapi/linux/media.h                         |   1 +
>  10 files changed, 252 insertions(+), 167 deletions(-)
> ---
> base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525
> change-id: 20241030-uvc-subdev-89f4467a00b5
> 
> Best regards,

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

[Ricardo Ribalda]

Hi Hans

On Sat, 9 Nov 2024 at 16:37, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Ricardo,
>
> FYI / some background: I have been asked to start helping /
> co-maintaining UVC with Laurent. I'll send out a patch adding
> myself as UVC maintainer soon.

Great! I talked with Laurent yesterday, I hope that we can maintain
the driver the three of us in the near future.

>
> On 8-Nov-24 9:25 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 two 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.
> >
> > We tried to fix the power-up issues implementing "granular power
> > saving" but it has been more complicated than anticipated....
>
> I have been discussing UVC power-management with Laurent, also
> related to power-consumption issues caused by libcamera's pipeline
> handler holding open the /dev/video# node as long as the camera
> manager object exists.
>
> For now we have fixed this with some relatively small changes to
> libcamera's uvcvideo pipeline handler, but that is really meant
> as an interim solution and as this privacy control series shows
> the power-management issues are real.

Indeed, we have tried to fixed it before:
https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/

btw this recently landed patch was to work in this direction :)
https://lore.kernel.org/linux-media/20240926-guenter-mini-v7-1-690441953d4a@chromium.org/

The more people interested in this problem the better.

>
> Combined with Mauro's remarks about how this is an userspace ABI break (1)
> I think we should maybe first take another look at the powermanagement
> issues in general rather then moving forward with this series.
>
> My apologies for this, I realize how annoying it can be when you are
> working on a patch series to fix a specific issue and a reviewer
> moves the goal-posts like this. But I do really think that just fixing
> the generic power-management issues would be better and I also think
> that this should be feasible / not too hard.
>
> Here is what I have in mind for this:
>
> 1. Assume that the results of trying a specific fmt do not change over time.
>
> 2. Only allow userspace to request fmts which match one of the enum-fmts ->
>    enum-frame-sizes -> enum-frame-rates tripplet results
>    (constrain what userspace requests to these)
>
> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
>    3 levels nested loop for this) on probe() and cache the results
>
> 4. Make try_fmt / set_fmt not poweron the device but instead constrain
>    the requested fmt to one from our cached fmts
>
> 5. On stream-on do the actual power-on + set-fmt + verify that we get
>    what we expect based on the cache, and otherwise return -EIO.

Can we start powering up the device during try/set fmt and then
implement the format caching as an improvement?
Laurent mentioned that some cameras missbehave if a lot of controls
are set during probing. I hope that this approach does not trigger
those, and if it does it would be easier to revert if we do the work
in two steps.

>
> I think that should sort the issue, assuming that 1. above holds true.
>
> One downside is that this stops UVC button presses from working when
> not streaming. But userspace will typically only open the /dev/video#
> node if it plans to stream anyways so there should not be much of
> a difference wrt button press behavior.

I do not personally use the button, but it is currently implemented as
a standard HID device. Making it only work during streamon() might be
a bit weird.
I am afraid that if there is a button we should keep the current behaviour.

>
> This should also make camera enumeration faster for apps, since
> most apps / frameworks do the whole 3 levels nested loop for this
> on startup, for which atm we go out to the hw, which now instead
> will come from the fmts cache and thus will be much much faster,
> so this should lead to a noticeable speedup for apps accessing UVC
> cameras which would be another nice win.
>
> Downside is that the initial probe will take longer see we do
> all the tryfmt-s there now. But I think that taking a bit longer
> to probe while the machine is booting should not be an issue.

How do you pretend to handle the controls? Do you plan to power-up the
device during s_ctrl() or set them only during streamon()?
If we power-up the device during s_ctrl we need to take care of the
asynchronous controls (typically pan/tilt/zoom), The device must be
powered until the control finishes, and the device might never reply
control_done if the firmware is not properly implemented.
If we set the controls only during streamon, we will break some
usecases. There are some video conferencing equipment that do homing
during streamoff. That will be a serious API breakage.

This patchset is not only to fix the powersaving issues, but also to
fix the issue when a camera has  a gpio privacy switch and an internal
Privacy control.  4 years ago I did not see any camera with Privacy
control (in 100s of models), now they are common.
Can we have both changes, gpio subdevice and granular power saving?

>
> Regards,
>
> Hans
>
>
> 1) Which is technically correct, but FWIW I agree with you that I think
> most userspace consumers will not care
>
>
>
>
> > Last year, we proposed a patchset to implement the privacy gpio as a
> > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/
> >
> > I think it is a pretty clean solution and makes sense to use a
> > subdevice for something that is a sub device of the camera :).
> >
> > This is an attempt to continue with that approach.
> >
> > Tested on gimble:
> > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0
> > Driver Info:
> >         Driver version   : 6.6.56
> >         Capabilities     : 0x00000000
> > Media Driver Info:
> >         Driver name      : uvcvideo
> >         Model            : HP 5M Camera: HP 5M Camera
> >         Serial           : 0001
> >         Bus info         : usb-0000:00:14.0-6
> >         Media version    : 6.6.56
> >         Hardware revision: 0x00009601 (38401)
> >         Driver version   : 6.6.56
> > Interface Info:
> >         ID               : 0x0300001d
> >         Type             : V4L Sub-Device
> > Entity Info:
> >         ID               : 0x00000013 (19)
> >         Name             : GPIO
> >         Function         : Unknown sub-device (00020006)
> >
> > Camera Controls
> >
> >                         privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only, volatile
> >
> > gimble-rev3 ~ # media-ctl  -p
> > Media controller API version 6.6.56
> >
> > Media device information
> > ------------------------
> > driver          uvcvideo
> > model           HP 5M Camera: HP 5M Camera
> > serial          0001
> > bus info        usb-0000:00:14.0-6
> > hw revision     0x9601
> > driver version  6.6.56
> >
> > Device topology
> > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link)
> >             type Node subtype V4L flags 1
> >             device node name /dev/video0
> >         pad0: Sink
> >                 <- "Extension 8":1 [ENABLED,IMMUTABLE]
> >
> > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link)
> >             type Node subtype V4L flags 0
> >             device node name /dev/video1
> >
> > - entity 8: Extension 8 (2 pads, 2 links, 0 routes)
> >             type V4L2 subdev subtype Unknown flags 0
> >         pad0: Sink
> >                 <- "Extension 4":1 [ENABLED,IMMUTABLE]
> >         pad1: Source
> >                 -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE]
> >
> > - entity 11: Extension 4 (2 pads, 2 links, 0 routes)
> >              type V4L2 subdev subtype Unknown flags 0
> >         pad0: Sink
> >                 <- "Processing 2":1 [ENABLED,IMMUTABLE]
> >         pad1: Source
> >                 -> "Extension 8":0 [ENABLED,IMMUTABLE]
> >
> > - entity 14: Processing 2 (2 pads, 2 links, 0 routes)
> >              type V4L2 subdev subtype Unknown flags 0
> >         pad0: Sink
> >                 <- "Camera 1":0 [ENABLED,IMMUTABLE]
> >         pad1: Source
> >                 -> "Extension 4":0 [ENABLED,IMMUTABLE]
> >
> > - entity 17: Camera 1 (1 pad, 1 link, 0 routes)
> >              type V4L2 subdev subtype Sensor flags 0
> >         pad0: Source
> >                 -> "Processing 2":0 [ENABLED,IMMUTABLE]
> >
> > - entity 19: GPIO (0 pad, 0 link, 0 routes)
> >              type V4L2 subdev subtype Decoder flags 0
> >              device node name /dev/v4l-subdev0
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@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 (5):
> >       media: uvcvideo: Factor out gpio functions to its own file
> >       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
> >
> > Yunke Cao (1):
> >       media: uvcvideo: Re-implement privacy GPIO as a separate subdevice
> >
> >  .../userspace-api/media/mediactl/media-types.rst   |   4 +
> >  drivers/media/usb/uvc/Makefile                     |   3 +-
> >  drivers/media/usb/uvc/uvc_ctrl.c                   |  40 +----
> >  drivers/media/usb/uvc/uvc_driver.c                 | 123 +-------------
> >  drivers/media/usb/uvc/uvc_entity.c                 |  20 ++-
> >  drivers/media/usb/uvc/uvc_gpio.c                   | 187 +++++++++++++++++++++
> >  drivers/media/usb/uvc/uvc_video.c                  |   4 +
> >  drivers/media/usb/uvc/uvcvideo.h                   |  34 ++--
> >  drivers/media/v4l2-core/v4l2-async.c               |   3 +-
> >  include/uapi/linux/media.h                         |   1 +
> >  10 files changed, 252 insertions(+), 167 deletions(-)
> > ---
> > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525
> > change-id: 20241030-uvc-subdev-89f4467a00b5
> >
> > Best regards,
>


-- 
Ricardo Ribalda

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

[Mauro Carvalho Chehab]

Em Sat, 9 Nov 2024 17:29:54 +0100
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> >
> > I think that should sort the issue, assuming that 1. above holds true.
> >
> > One downside is that this stops UVC button presses from working when
> > not streaming. But userspace will typically only open the /dev/video#
> > node if it plans to stream anyways so there should not be much of
> > a difference wrt button press behavior.  
> 
> I do not personally use the button, but it is currently implemented as
> a standard HID device. 

IMO, controlling the privacy via evdev is the best approach then. There's
no need for a RW control neither at subdev or at device level. It could
make sense a Read only to allow apps to read, but still it shall be up to
the Kernel to protect the stream if the button is pressed.

> Making it only work during streamon() might be
> a bit weird.
> I am afraid that if there is a button we should keep the current behaviour.

Privacy matters only when streaming. IMO the Kernel check for it needs to
be done at DQBUF time and at read() calls, as one can enable/disable the
camera while doing videoconf calls. I do that a lot with app "soft" buttons,
and on devices that physically support cutting the video. 

I don't trust myself privacy soft buttons, specially when handled in userspace,
so what I have are webcam covers (and a small stick glued at a laptop camera
that has a too small sensor for a webcam cover). I only remove the cover/stick
when I want to participate on videoconf with video enabled with the builtin
camera.

Regards

Thanks,
Mauro

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

[Hans Verkuil]

On 10/11/2024 11:02, Mauro Carvalho Chehab wrote:
> Em Sat, 9 Nov 2024 17:29:54 +0100
> Ricardo Ribalda <ribalda@chromium.org> escreveu:
> 
>>>
>>> I think that should sort the issue, assuming that 1. above holds true.
>>>
>>> One downside is that this stops UVC button presses from working when
>>> not streaming. But userspace will typically only open the /dev/video#
>>> node if it plans to stream anyways so there should not be much of
>>> a difference wrt button press behavior.  
>>
>> I do not personally use the button, but it is currently implemented as
>> a standard HID device. 
> 
> IMO, controlling the privacy via evdev is the best approach then. There's
> no need for a RW control neither at subdev or at device level. It could
> make sense a Read only to allow apps to read, but still it shall be up to
> the Kernel to protect the stream if the button is pressed.
> 
>> Making it only work during streamon() might be
>> a bit weird.
>> I am afraid that if there is a button we should keep the current behaviour.
> 
> Privacy matters only when streaming. IMO the Kernel check for it needs to
> be done at DQBUF time and at read() calls, as one can enable/disable the
> camera while doing videoconf calls. I do that a lot with app "soft" buttons,
> and on devices that physically support cutting the video. 

We could add a vb2_s_privacy(bool privacy) function to vb2 to tell vb2 if the privacy
mode is on. And if so, take action. E.g. calling QBUF/DQBUF would return a -EACCES error.

That will ensure consistent behavior for all drivers that have a privacy function.

Note that there are two types of privacy GPIO: one that triggers when a physical
cover is moved, blocking the sensor, and one that is a button relying on software
to stop streaming video. In the first case it is informative, but you can keep
streaming.

Regards,

	Hans

> 
> I don't trust myself privacy soft buttons, specially when handled in userspace,
> so what I have are webcam covers (and a small stick glued at a laptop camera
> that has a too small sensor for a webcam cover). I only remove the cover/stick
> when I want to participate on videoconf with video enabled with the builtin
> camera.
> 
> Regards
> 
> Thanks,
> Mauro
> 

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

[Ricardo Ribalda]

On Sun, 10 Nov 2024 at 11:29, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 10/11/2024 11:02, Mauro Carvalho Chehab wrote:
> > Em Sat, 9 Nov 2024 17:29:54 +0100
> > Ricardo Ribalda <ribalda@chromium.org> escreveu:
> >
> >>>
> >>> I think that should sort the issue, assuming that 1. above holds true.
> >>>
> >>> One downside is that this stops UVC button presses from working when
> >>> not streaming. But userspace will typically only open the /dev/video#
> >>> node if it plans to stream anyways so there should not be much of
> >>> a difference wrt button press behavior.
> >>
> >> I do not personally use the button, but it is currently implemented as
> >> a standard HID device.
> >
> > IMO, controlling the privacy via evdev is the best approach then. There's
> > no need for a RW control neither at subdev or at device level. It could
> > make sense a Read only to allow apps to read, but still it shall be up to
> > the Kernel to protect the stream if the button is pressed.
> >
> >> Making it only work during streamon() might be
> >> a bit weird.
> >> I am afraid that if there is a button we should keep the current behaviour.
> >
> > Privacy matters only when streaming. IMO the Kernel check for it needs to
> > be done at DQBUF time and at read() calls, as one can enable/disable the
> > camera while doing videoconf calls. I do that a lot with app "soft" buttons,
> > and on devices that physically support cutting the video.
>
> We could add a vb2_s_privacy(bool privacy) function to vb2 to tell vb2 if the privacy
> mode is on. And if so, take action. E.g. calling QBUF/DQBUF would return a -EACCES error.
>
> That will ensure consistent behavior for all drivers that have a privacy function.

I am not against adding a feature like this, but we still need a way
to notify userspace about a change of the privacy state when the user
presses it.
Controls are great for this.

>
> Note that there are two types of privacy GPIO: one that triggers when a physical
> cover is moved, blocking the sensor, and one that is a button relying on software
> to stop streaming video. In the first case it is informative, but you can keep
> streaming.

I am curious who implements a software privacy switch. I would
definitely use a physical cover in those devices.

Chromebooks only support physical cover and hardware privacy switch. I
have not seen software privacy switches.

>
> Regards,
>
>         Hans
>
> >
> > I don't trust myself privacy soft buttons, specially when handled in userspace,
> > so what I have are webcam covers (and a small stick glued at a laptop camera
> > that has a too small sensor for a webcam cover). I only remove the cover/stick
> > when I want to participate on videoconf with video enabled with the builtin
> > camera.
> >
> > Regards
> >
> > Thanks,
> > Mauro
> >
>


-- 
Ricardo Ribalda

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

[Hans Verkuil]

On 10/11/2024 11:37, Ricardo Ribalda wrote:
> On Sun, 10 Nov 2024 at 11:29, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 10/11/2024 11:02, Mauro Carvalho Chehab wrote:
>>> Em Sat, 9 Nov 2024 17:29:54 +0100
>>> Ricardo Ribalda <ribalda@chromium.org> escreveu:
>>>
>>>>>
>>>>> I think that should sort the issue, assuming that 1. above holds true.
>>>>>
>>>>> One downside is that this stops UVC button presses from working when
>>>>> not streaming. But userspace will typically only open the /dev/video#
>>>>> node if it plans to stream anyways so there should not be much of
>>>>> a difference wrt button press behavior.
>>>>
>>>> I do not personally use the button, but it is currently implemented as
>>>> a standard HID device.
>>>
>>> IMO, controlling the privacy via evdev is the best approach then. There's
>>> no need for a RW control neither at subdev or at device level. It could
>>> make sense a Read only to allow apps to read, but still it shall be up to
>>> the Kernel to protect the stream if the button is pressed.
>>>
>>>> Making it only work during streamon() might be
>>>> a bit weird.
>>>> I am afraid that if there is a button we should keep the current behaviour.
>>>
>>> Privacy matters only when streaming. IMO the Kernel check for it needs to
>>> be done at DQBUF time and at read() calls, as one can enable/disable the
>>> camera while doing videoconf calls. I do that a lot with app "soft" buttons,
>>> and on devices that physically support cutting the video.
>>
>> We could add a vb2_s_privacy(bool privacy) function to vb2 to tell vb2 if the privacy
>> mode is on. And if so, take action. E.g. calling QBUF/DQBUF would return a -EACCES error.
>>
>> That will ensure consistent behavior for all drivers that have a privacy function.
> 
> I am not against adding a feature like this, but we still need a way
> to notify userspace about a change of the privacy state when the user
> presses it.
> Controls are great for this.
> 
>>
>> Note that there are two types of privacy GPIO: one that triggers when a physical
>> cover is moved, blocking the sensor, and one that is a button relying on software
>> to stop streaming video. In the first case it is informative, but you can keep
>> streaming.
> 
> I am curious who implements a software privacy switch. I would
> definitely use a physical cover in those devices.
> 
> Chromebooks only support physical cover and hardware privacy switch. I
> have not seen software privacy switches.

I actually thought this discussion was about software privacy switched.
I haven't seen any, but it wouldn't surprise me if there are webcams that do something
like that.

For proper privacy covers I don't think you need a vb2 implementation, even if you
keep streaming you should just see black video, no need to refuse QBUF/DQBUF.

Regards,

	Hans

> 
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> I don't trust myself privacy soft buttons, specially when handled in userspace,
>>> so what I have are webcam covers (and a small stick glued at a laptop camera
>>> that has a too small sensor for a webcam cover). I only remove the cover/stick
>>> when I want to participate on videoconf with video enabled with the builtin
>>> camera.
>>>
>>> Regards
>>>
>>> Thanks,
>>> Mauro
>>>
>>
> 
> 

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

[Mauro Carvalho Chehab]

Em Sun, 10 Nov 2024 11:37:46 +0100
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> > > Privacy matters only when streaming. IMO the Kernel check for it needs to
> > > be done at DQBUF time and at read() calls, as one can enable/disable the
> > > camera while doing videoconf calls. I do that a lot with app "soft" buttons,
> > > and on devices that physically support cutting the video.  
> >
> > We could add a vb2_s_privacy(bool privacy) function to vb2 to tell vb2 if the privacy
> > mode is on. And if so, take action. E.g. calling QBUF/DQBUF would return a -EACCES error.
> >
> > That will ensure consistent behavior for all drivers that have a privacy function.  
> 
> I am not against adding a feature like this, but we still need a way
> to notify userspace about a change of the privacy state when the user
> presses it.
> Controls are great for this.

It doesn't sound a good idea. See, from users PoV, they want the stream
to start with black frames when privacy is on, and dynamically being
able to enable/disable actual frame output. So, the best is to have the
stream running independently of the privacy being enabled or not.

Thanks,
Mauro

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

[Ricardo Ribalda]

Hi Mauro

On Sun, 10 Nov 2024 at 11:03, Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Sat, 9 Nov 2024 17:29:54 +0100
> Ricardo Ribalda <ribalda@chromium.org> escreveu:
>
> > >
> > > I think that should sort the issue, assuming that 1. above holds true.
> > >
> > > One downside is that this stops UVC button presses from working when
> > > not streaming. But userspace will typically only open the /dev/video#
> > > node if it plans to stream anyways so there should not be much of
> > > a difference wrt button press behavior.
> >
> > I do not personally use the button, but it is currently implemented as
> > a standard HID device.
>
> IMO, controlling the privacy via evdev is the best approach then. There's
> no need for a RW control neither at subdev or at device level. It could
> make sense a Read only to allow apps to read, but still it shall be up to
> the Kernel to protect the stream if the button is pressed.
>
> > Making it only work during streamon() might be
> > a bit weird.
> > I am afraid that if there is a button we should keep the current behaviour.
>
> Privacy matters only when streaming. IMO the Kernel check for it needs to
> be done at DQBUF time and at read() calls, as one can enable/disable the
> camera while doing videoconf calls. I do that a lot with app "soft" buttons,
> and on devices that physically support cutting the video.
>
> I don't trust myself privacy soft buttons, specially when handled in userspace,
> so what I have are webcam covers (and a small stick glued at a laptop camera
> that has a too small sensor for a webcam cover). I only remove the cover/stick
> when I want to participate on videoconf with video enabled with the builtin
> camera.
>
> Regards

I think we are mixing up concepts here.

On one side we have the uvc button. You can see one here
https://www.sellpy.dk/item/2Yk1ZULbki?utm_source=google&utm_medium=cpc&utm_campaign=17610409619&gad_source=1&gclid=Cj0KCQiA0MG5BhD1ARIsAEcZtwR9-09ZtTIVNbVknrZCtCd7ezVM8YFw1yQXfs81FWhofg9eW-iBrsIaAopVEALw_wcB
That button is not represented as a hid device. We do not know how the
user will use this button. They could even use it to start an app when
pressed.

On the other side we have  the privacy gpio. The chassis has a switch
that is connected to the camera and to the SOC. You can see one here:
https://support.hp.com/ie-en/document/ish_3960099-3335046-16 .We link
the camera with a gpio via the acpi table. When the user flips the
button, the camera produces black frames and the SOC gets an IRQ. The
IRQ is used to display a message like "Camera off" and the value of
the GPIO can be checked when an app is running to tell the user:
"Camera not available, flip the privacy button if you want to use it."
Userspace cannot change the value of the gpio. It is read-only,
userspace cannot override the privacy switch. The privacy gpio is
represented with a control in /dev/videoX This patchset wants to move
it to /dev/v4l2-subdevX

To make things more complicated. Recently some cameras are starting to
have their own privacy control without the need of an external gpio.
This is also represented as a control in /dev/videoX.


Now that we have these 3 concepts in place:

Today a uvc camera is powered up when /dev/videoX is open(), not when
it is streaming. This means that if we want to get an event for the
privacy gpio we have to powerup the camera, which results in power
consumption. This can be fixed by moving the control to a subdevice
(technically the gpio is not part of the camera, so it makes sense).

If we only powerup the camera during streamon we will break the uvc
button, and the async controls.



>
> Thanks,
> Mauro



-- 
Ricardo Ribalda

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

[Ricardo Ribalda]

On Sun, 10 Nov 2024 at 11:32, Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Mauro
>
> On Sun, 10 Nov 2024 at 11:03, Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > Em Sat, 9 Nov 2024 17:29:54 +0100
> > Ricardo Ribalda <ribalda@chromium.org> escreveu:
> >
> > > >
> > > > I think that should sort the issue, assuming that 1. above holds true.
> > > >
> > > > One downside is that this stops UVC button presses from working when
> > > > not streaming. But userspace will typically only open the /dev/video#
> > > > node if it plans to stream anyways so there should not be much of
> > > > a difference wrt button press behavior.
> > >
> > > I do not personally use the button, but it is currently implemented as
> > > a standard HID device.
> >
> > IMO, controlling the privacy via evdev is the best approach then. There's
> > no need for a RW control neither at subdev or at device level. It could
> > make sense a Read only to allow apps to read, but still it shall be up to
> > the Kernel to protect the stream if the button is pressed.
> >
> > > Making it only work during streamon() might be
> > > a bit weird.
> > > I am afraid that if there is a button we should keep the current behaviour.
> >
> > Privacy matters only when streaming. IMO the Kernel check for it needs to
> > be done at DQBUF time and at read() calls, as one can enable/disable the
> > camera while doing videoconf calls. I do that a lot with app "soft" buttons,
> > and on devices that physically support cutting the video.
> >
> > I don't trust myself privacy soft buttons, specially when handled in userspace,
> > so what I have are webcam covers (and a small stick glued at a laptop camera
> > that has a too small sensor for a webcam cover). I only remove the cover/stick
> > when I want to participate on videoconf with video enabled with the builtin
> > camera.
> >
> > Regards
>
> I think we are mixing up concepts here.
>
> On one side we have the uvc button. You can see one here
> https://www.sellpy.dk/item/2Yk1ZULbki?utm_source=google&utm_medium=cpc&utm_campaign=17610409619&gad_source=1&gclid=Cj0KCQiA0MG5BhD1ARIsAEcZtwR9-09ZtTIVNbVknrZCtCd7ezVM8YFw1yQXfs81FWhofg9eW-iBrsIaAopVEALw_wcB
> That button is not represented as a hid device. We do not know how the
That button is NOW represented as a hid device. sorry :)
> user will use this button. They could even use it to start an app when
> pressed.
>
> On the other side we have  the privacy gpio. The chassis has a switch
> that is connected to the camera and to the SOC. You can see one here:
> https://support.hp.com/ie-en/document/ish_3960099-3335046-16 .We link
> the camera with a gpio via the acpi table. When the user flips the
> button, the camera produces black frames and the SOC gets an IRQ. The
> IRQ is used to display a message like "Camera off" and the value of
> the GPIO can be checked when an app is running to tell the user:
> "Camera not available, flip the privacy button if you want to use it."
> Userspace cannot change the value of the gpio. It is read-only,
> userspace cannot override the privacy switch. The privacy gpio is
> represented with a control in /dev/videoX This patchset wants to move
> it to /dev/v4l2-subdevX
>
> To make things more complicated. Recently some cameras are starting to
> have their own privacy control without the need of an external gpio.
> This is also represented as a control in /dev/videoX.
>
>
> Now that we have these 3 concepts in place:
>
> Today a uvc camera is powered up when /dev/videoX is open(), not when
> it is streaming. This means that if we want to get an event for the
> privacy gpio we have to powerup the camera, which results in power
> consumption. This can be fixed by moving the control to a subdevice
> (technically the gpio is not part of the camera, so it makes sense).
>
> If we only powerup the camera during streamon we will break the uvc
> button, and the async controls.
>
>
>
> >
> > Thanks,
> > Mauro
>
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda

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

[Mauro Carvalho Chehab]

Em Sun, 10 Nov 2024 11:32:16 +0100
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> Hi Mauro
> 
> On Sun, 10 Nov 2024 at 11:03, Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > Em Sat, 9 Nov 2024 17:29:54 +0100
> > Ricardo Ribalda <ribalda@chromium.org> escreveu:
> >  
> > > >
> > > > I think that should sort the issue, assuming that 1. above holds true.
> > > >
> > > > One downside is that this stops UVC button presses from working when
> > > > not streaming. But userspace will typically only open the /dev/video#
> > > > node if it plans to stream anyways so there should not be much of
> > > > a difference wrt button press behavior.  
> > >
> > > I do not personally use the button, but it is currently implemented as
> > > a standard HID device.  
> >
> > IMO, controlling the privacy via evdev is the best approach then. There's
> > no need for a RW control neither at subdev or at device level. It could
> > make sense a Read only to allow apps to read, but still it shall be up to
> > the Kernel to protect the stream if the button is pressed.
> >  
> > > Making it only work during streamon() might be
> > > a bit weird.
> > > I am afraid that if there is a button we should keep the current behaviour.  
> >
> > Privacy matters only when streaming. IMO the Kernel check for it needs to
> > be done at DQBUF time and at read() calls, as one can enable/disable the
> > camera while doing videoconf calls. I do that a lot with app "soft" buttons,
> > and on devices that physically support cutting the video.
> >
> > I don't trust myself privacy soft buttons, specially when handled in userspace,
> > so what I have are webcam covers (and a small stick glued at a laptop camera
> > that has a too small sensor for a webcam cover). I only remove the cover/stick
> > when I want to participate on videoconf with video enabled with the builtin
> > camera.
> >
> > Regards  
> 
> I think we are mixing up concepts here.
> 
> On one side we have the uvc button. You can see one here
> https://www.sellpy.dk/item/2Yk1ZULbki?utm_source=google&utm_medium=cpc&utm_campaign=17610409619&gad_source=1&gclid=Cj0KCQiA0MG5BhD1ARIsAEcZtwR9-09ZtTIVNbVknrZCtCd7ezVM8YFw1yQXfs81FWhofg9eW-iBrsIaAopVEALw_wcB
> That button is not represented as a hid device. We do not know how the
> user will use this button. They could even use it to start an app when
> pressed.

Old cameras have a <snapshot> button. Maybe that's the case of the device
you're pointing, as it looks some non-uvc Logitech cameras I have myself.

> On the other side we have  the privacy gpio. The chassis has a switch
> that is connected to the camera and to the SOC. You can see one here:
> https://support.hp.com/ie-en/document/ish_3960099-3335046-16 .We link
> the camera with a gpio via the acpi table. When the user flips the
> button, the camera produces black frames and the SOC gets an IRQ. 

OK, so the hardware warrants black frames. Sounds a more secure
implementation.

> The IRQ is used to display a message like "Camera off" and the value of
> the GPIO can be checked when an app is running to tell the user:
> "Camera not available, flip the privacy button if you want to use it."

So, it is not really a privacy gpio/control. It is instead a privacy
notification control.

I would better name it to clearly indicate what it is about.

> Userspace cannot change the value of the gpio. It is read-only,
> userspace cannot override the privacy switch. The privacy gpio is
> represented with a control in /dev/videoX This patchset wants to move
> it to /dev/v4l2-subdevX

Well, if it is really a gpio pin, kernel (and eventually userspace) can force
it to pullup (or pulldown) state, forcing one of the states. If, instead is 
an output-only pin, kernel/userspace can't control it at all.

> To make things more complicated. Recently some cameras are starting to
> have their own privacy control without the need of an external gpio.
> This is also represented as a control in /dev/videoX.

IMO, both privacy notification events shall be reported the same way,
no matter if they use GPIO, an input pin or something else.

> Now that we have these 3 concepts in place:
> 
> Today a uvc camera is powered up when /dev/videoX is open(), not when
> it is streaming.

Ideally, the part of the hardware responsible for streaming shall be
powered on only while streaming. I agree with Hans de Goede: better
have this fixed before the privacy notification patches.

> This means that if we want to get an event for the
> privacy gpio we have to powerup the camera, which results in power
> consumption. This can be fixed by moving the control to a subdevice
> (technically the gpio is not part of the camera, so it makes sense).

Ok, but as you said, not all cameras implement it as a separate gpio.

> If we only powerup the camera during streamon we will break the uvc
> button, and the async controls.

Why? IMO, it shall use regmap in a way that the register settings
will be sent to the device only when the camera control hardware is
powered up. On a complex device, there are likely at least two power
up hardware: the camera control logic and the streaming logic.

Not sure if both are visible via UVC spec, though.

Thanks,
Mauro

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

[Ricardo Ribalda]

On Sun, 10 Nov 2024 at 13:46, Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Sun, 10 Nov 2024 11:32:16 +0100
> Ricardo Ribalda <ribalda@chromium.org> escreveu:
>
> > Hi Mauro
> >
> > On Sun, 10 Nov 2024 at 11:03, Mauro Carvalho Chehab
> > <mchehab+huawei@kernel.org> wrote:
> > >
> > > Em Sat, 9 Nov 2024 17:29:54 +0100
> > > Ricardo Ribalda <ribalda@chromium.org> escreveu:
> > >
> > > > >
> > > > > I think that should sort the issue, assuming that 1. above holds true.
> > > > >
> > > > > One downside is that this stops UVC button presses from working when
> > > > > not streaming. But userspace will typically only open the /dev/video#
> > > > > node if it plans to stream anyways so there should not be much of
> > > > > a difference wrt button press behavior.
> > > >
> > > > I do not personally use the button, but it is currently implemented as
> > > > a standard HID device.
> > >
> > > IMO, controlling the privacy via evdev is the best approach then. There's
> > > no need for a RW control neither at subdev or at device level. It could
> > > make sense a Read only to allow apps to read, but still it shall be up to
> > > the Kernel to protect the stream if the button is pressed.
> > >
> > > > Making it only work during streamon() might be
> > > > a bit weird.
> > > > I am afraid that if there is a button we should keep the current behaviour.
> > >
> > > Privacy matters only when streaming. IMO the Kernel check for it needs to
> > > be done at DQBUF time and at read() calls, as one can enable/disable the
> > > camera while doing videoconf calls. I do that a lot with app "soft" buttons,
> > > and on devices that physically support cutting the video.
> > >
> > > I don't trust myself privacy soft buttons, specially when handled in userspace,
> > > so what I have are webcam covers (and a small stick glued at a laptop camera
> > > that has a too small sensor for a webcam cover). I only remove the cover/stick
> > > when I want to participate on videoconf with video enabled with the builtin
> > > camera.
> > >
> > > Regards
> >
> > I think we are mixing up concepts here.
> >
> > On one side we have the uvc button. You can see one here
> > https://www.sellpy.dk/item/2Yk1ZULbki?utm_source=google&utm_medium=cpc&utm_campaign=17610409619&gad_source=1&gclid=Cj0KCQiA0MG5BhD1ARIsAEcZtwR9-09ZtTIVNbVknrZCtCd7ezVM8YFw1yQXfs81FWhofg9eW-iBrsIaAopVEALw_wcB
> > That button is not represented as a hid device. We do not know how the
> > user will use this button. They could even use it to start an app when
> > pressed.
>
> Old cameras have a <snapshot> button. Maybe that's the case of the device
> you're pointing, as it looks some non-uvc Logitech cameras I have myself.
>
> > On the other side we have  the privacy gpio. The chassis has a switch
> > that is connected to the camera and to the SOC. You can see one here:
> > https://support.hp.com/ie-en/document/ish_3960099-3335046-16 .We link
> > the camera with a gpio via the acpi table. When the user flips the
> > button, the camera produces black frames and the SOC gets an IRQ.
>
> OK, so the hardware warrants black frames. Sounds a more secure
> implementation.
>
> > The IRQ is used to display a message like "Camera off" and the value of
> > the GPIO can be checked when an app is running to tell the user:
> > "Camera not available, flip the privacy button if you want to use it."
>
> So, it is not really a privacy gpio/control. It is instead a privacy
> notification control.
>
> I would better name it to clearly indicate what it is about.
>
> > Userspace cannot change the value of the gpio. It is read-only,
> > userspace cannot override the privacy switch. The privacy gpio is
> > represented with a control in /dev/videoX This patchset wants to move
> > it to /dev/v4l2-subdevX
>
> Well, if it is really a gpio pin, kernel (and eventually userspace) can force
> it to pullup (or pulldown) state, forcing one of the states. If, instead is
> an output-only pin, kernel/userspace can't control it at all.
>
> > To make things more complicated. Recently some cameras are starting to
> > have their own privacy control without the need of an external gpio.
> > This is also represented as a control in /dev/videoX.
>
> IMO, both privacy notification events shall be reported the same way,
> no matter if they use GPIO, an input pin or something else.

How do we handle devices that have internal privacy and GPIO if we do
not use a subdevice?

>
> > Now that we have these 3 concepts in place:
> >
> > Today a uvc camera is powered up when /dev/videoX is open(), not when
> > it is streaming.
>
> Ideally, the part of the hardware responsible for streaming shall be
> powered on only while streaming. I agree with Hans de Goede: better
> have this fixed before the privacy notification patches.
>
> > This means that if we want to get an event for the
> > privacy gpio we have to powerup the camera, which results in power
> > consumption. This can be fixed by moving the control to a subdevice
> > (technically the gpio is not part of the camera, so it makes sense).
>
> Ok, but as you said, not all cameras implement it as a separate gpio.

For the device that are not a separate gpio you need to powerup the
device to read it.


>
> > If we only powerup the camera during streamon we will break the uvc
> > button, and the async controls.
>
> Why? IMO, it shall use regmap in a way that the register settings
> will be sent to the device only when the camera control hardware is
> powered up. On a complex device, there are likely at least two power
> up hardware: the camera control logic and the streaming logic.
>
> Not sure if both are visible via UVC spec, though.

There are controls that need to be accessed without streaming, like
the ones that do homing (calibrate the motors), report occupancy of a
room.

If we modify the driver behaviour to  read/write controls only during
this streaming we will stop supporting current use cases and
definately break applications.

>
> Thanks,
> Mauro



-- 
Ricardo Ribalda

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

[Laurent Pinchart]

On Sat, Nov 09, 2024 at 05:29:54PM +0100, Ricardo Ribalda wrote:
> On Sat, 9 Nov 2024 at 16:37, Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi Ricardo,
> >
> > FYI / some background: I have been asked to start helping /
> > co-maintaining UVC with Laurent. I'll send out a patch adding
> > myself as UVC maintainer soon.
> 
> Great! I talked with Laurent yesterday, I hope that we can maintain
> the driver the three of us in the near future.
> 
> > On 8-Nov-24 9:25 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 two 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.
> > >
> > > We tried to fix the power-up issues implementing "granular power
> > > saving" but it has been more complicated than anticipated....
> >
> > I have been discussing UVC power-management with Laurent, also
> > related to power-consumption issues caused by libcamera's pipeline
> > handler holding open the /dev/video# node as long as the camera
> > manager object exists.
> >
> > For now we have fixed this with some relatively small changes to
> > libcamera's uvcvideo pipeline handler, but that is really meant
> > as an interim solution and as this privacy control series shows
> > the power-management issues are real.
> 
> Indeed, we have tried to fixed it before:
> https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
> 
> btw this recently landed patch was to work in this direction :)
> https://lore.kernel.org/linux-media/20240926-guenter-mini-v7-1-690441953d4a@chromium.org/
> 
> The more people interested in this problem the better.
> 
> > Combined with Mauro's remarks about how this is an userspace ABI break (1)
> > I think we should maybe first take another look at the powermanagement
> > issues in general rather then moving forward with this series.
> >
> > My apologies for this, I realize how annoying it can be when you are
> > working on a patch series to fix a specific issue and a reviewer
> > moves the goal-posts like this. But I do really think that just fixing
> > the generic power-management issues would be better and I also think
> > that this should be feasible / not too hard.
> >
> > Here is what I have in mind for this:
> >
> > 1. Assume that the results of trying a specific fmt do not change over time.
> >
> > 2. Only allow userspace to request fmts which match one of the enum-fmts ->
> >    enum-frame-sizes -> enum-frame-rates tripplet results
> >    (constrain what userspace requests to these)
> >
> > 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
> >    3 levels nested loop for this) on probe() and cache the results
> >
> > 4. Make try_fmt / set_fmt not poweron the device but instead constrain
> >    the requested fmt to one from our cached fmts
> >
> > 5. On stream-on do the actual power-on + set-fmt + verify that we get
> >    what we expect based on the cache, and otherwise return -EIO.
> 
> Can we start powering up the device during try/set fmt and then
> implement the format caching as an improvement?

This sounds worth trying. We'll need to test it on a wide range of
devices though, both internal and external.

> Laurent mentioned that some cameras missbehave if a lot of controls
> are set during probing. I hope that this approach does not trigger
> those, and if it does it would be easier to revert if we do the work
> in two steps.
> 
> > I think that should sort the issue, assuming that 1. above holds true.
> >
> > One downside is that this stops UVC button presses from working when
> > not streaming. But userspace will typically only open the /dev/video#
> > node if it plans to stream anyways so there should not be much of
> > a difference wrt button press behavior.
> 
> I do not personally use the button, but it is currently implemented as
> a standard HID device. Making it only work during streamon() might be
> a bit weird.
> I am afraid that if there is a button we should keep the current behaviour.
> 
> >
> > This should also make camera enumeration faster for apps, since
> > most apps / frameworks do the whole 3 levels nested loop for this
> > on startup, for which atm we go out to the hw, which now instead
> > will come from the fmts cache and thus will be much much faster,
> > so this should lead to a noticeable speedup for apps accessing UVC
> > cameras which would be another nice win.
> >
> > Downside is that the initial probe will take longer see we do
> > all the tryfmt-s there now. But I think that taking a bit longer
> > to probe while the machine is booting should not be an issue.
> 
> How do you pretend to handle the controls? Do you plan to power-up the
> device during s_ctrl() or set them only during streamon()?
> If we power-up the device during s_ctrl we need to take care of the
> asynchronous controls (typically pan/tilt/zoom), The device must be
> powered until the control finishes, and the device might never reply
> control_done if the firmware is not properly implemented.
> If we set the controls only during streamon, we will break some
> usecases. There are some video conferencing equipment that do homing
> during streamoff. That will be a serious API breakage.
> 
> This patchset is not only to fix the powersaving issues, but also to
> fix the issue when a camera has  a gpio privacy switch and an internal
> Privacy control.  4 years ago I did not see any camera with Privacy
> control (in 100s of models), now they are common.
> Can we have both changes, gpio subdevice and granular power saving?
> 
> > Regards,
> >
> > Hans
> >
> > 1) Which is technically correct, but FWIW I agree with you that I think
> > most userspace consumers will not care
> >
> > > Last year, we proposed a patchset to implement the privacy gpio as a
> > > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/
> > >
> > > I think it is a pretty clean solution and makes sense to use a
> > > subdevice for something that is a sub device of the camera :).
> > >
> > > This is an attempt to continue with that approach.
> > >
> > > Tested on gimble:
> > > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0
> > > Driver Info:
> > >         Driver version   : 6.6.56
> > >         Capabilities     : 0x00000000
> > > Media Driver Info:
> > >         Driver name      : uvcvideo
> > >         Model            : HP 5M Camera: HP 5M Camera
> > >         Serial           : 0001
> > >         Bus info         : usb-0000:00:14.0-6
> > >         Media version    : 6.6.56
> > >         Hardware revision: 0x00009601 (38401)
> > >         Driver version   : 6.6.56
> > > Interface Info:
> > >         ID               : 0x0300001d
> > >         Type             : V4L Sub-Device
> > > Entity Info:
> > >         ID               : 0x00000013 (19)
> > >         Name             : GPIO
> > >         Function         : Unknown sub-device (00020006)
> > >
> > > Camera Controls
> > >
> > >                         privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only, volatile
> > >
> > > gimble-rev3 ~ # media-ctl  -p
> > > Media controller API version 6.6.56
> > >
> > > Media device information
> > > ------------------------
> > > driver          uvcvideo
> > > model           HP 5M Camera: HP 5M Camera
> > > serial          0001
> > > bus info        usb-0000:00:14.0-6
> > > hw revision     0x9601
> > > driver version  6.6.56
> > >
> > > Device topology
> > > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link)
> > >             type Node subtype V4L flags 1
> > >             device node name /dev/video0
> > >         pad0: Sink
> > >                 <- "Extension 8":1 [ENABLED,IMMUTABLE]
> > >
> > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link)
> > >             type Node subtype V4L flags 0
> > >             device node name /dev/video1
> > >
> > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes)
> > >             type V4L2 subdev subtype Unknown flags 0
> > >         pad0: Sink
> > >                 <- "Extension 4":1 [ENABLED,IMMUTABLE]
> > >         pad1: Source
> > >                 -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE]
> > >
> > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes)
> > >              type V4L2 subdev subtype Unknown flags 0
> > >         pad0: Sink
> > >                 <- "Processing 2":1 [ENABLED,IMMUTABLE]
> > >         pad1: Source
> > >                 -> "Extension 8":0 [ENABLED,IMMUTABLE]
> > >
> > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes)
> > >              type V4L2 subdev subtype Unknown flags 0
> > >         pad0: Sink
> > >                 <- "Camera 1":0 [ENABLED,IMMUTABLE]
> > >         pad1: Source
> > >                 -> "Extension 4":0 [ENABLED,IMMUTABLE]
> > >
> > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes)
> > >              type V4L2 subdev subtype Sensor flags 0
> > >         pad0: Source
> > >                 -> "Processing 2":0 [ENABLED,IMMUTABLE]
> > >
> > > - entity 19: GPIO (0 pad, 0 link, 0 routes)
> > >              type V4L2 subdev subtype Decoder flags 0
> > >              device node name /dev/v4l-subdev0
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@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 (5):
> > >       media: uvcvideo: Factor out gpio functions to its own file
> > >       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
> > >
> > > Yunke Cao (1):
> > >       media: uvcvideo: Re-implement privacy GPIO as a separate subdevice
> > >
> > >  .../userspace-api/media/mediactl/media-types.rst   |   4 +
> > >  drivers/media/usb/uvc/Makefile                     |   3 +-
> > >  drivers/media/usb/uvc/uvc_ctrl.c                   |  40 +----
> > >  drivers/media/usb/uvc/uvc_driver.c                 | 123 +-------------
> > >  drivers/media/usb/uvc/uvc_entity.c                 |  20 ++-
> > >  drivers/media/usb/uvc/uvc_gpio.c                   | 187 +++++++++++++++++++++
> > >  drivers/media/usb/uvc/uvc_video.c                  |   4 +
> > >  drivers/media/usb/uvc/uvcvideo.h                   |  34 ++--
> > >  drivers/media/v4l2-core/v4l2-async.c               |   3 +-
> > >  include/uapi/linux/media.h                         |   1 +
> > >  10 files changed, 252 insertions(+), 167 deletions(-)
> > > ---
> > > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525
> > > change-id: 20241030-uvc-subdev-89f4467a00b5

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Sat, Nov 09, 2024 at 05:29:54PM +0100, Ricardo Ribalda wrote:
> > On Sat, 9 Nov 2024 at 16:37, Hans de Goede <hdegoede@redhat.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > FYI / some background: I have been asked to start helping /
> > > co-maintaining UVC with Laurent. I'll send out a patch adding
> > > myself as UVC maintainer soon.
> >
> > Great! I talked with Laurent yesterday, I hope that we can maintain
> > the driver the three of us in the near future.
> >
> > > On 8-Nov-24 9:25 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 two 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.
> > > >
> > > > We tried to fix the power-up issues implementing "granular power
> > > > saving" but it has been more complicated than anticipated....
> > >
> > > I have been discussing UVC power-management with Laurent, also
> > > related to power-consumption issues caused by libcamera's pipeline
> > > handler holding open the /dev/video# node as long as the camera
> > > manager object exists.
> > >
> > > For now we have fixed this with some relatively small changes to
> > > libcamera's uvcvideo pipeline handler, but that is really meant
> > > as an interim solution and as this privacy control series shows
> > > the power-management issues are real.
> >
> > Indeed, we have tried to fixed it before:
> > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
> >
> > btw this recently landed patch was to work in this direction :)
> > https://lore.kernel.org/linux-media/20240926-guenter-mini-v7-1-690441953d4a@chromium.org/
> >
> > The more people interested in this problem the better.
> >
> > > Combined with Mauro's remarks about how this is an userspace ABI break (1)
> > > I think we should maybe first take another look at the powermanagement
> > > issues in general rather then moving forward with this series.
> > >
> > > My apologies for this, I realize how annoying it can be when you are
> > > working on a patch series to fix a specific issue and a reviewer
> > > moves the goal-posts like this. But I do really think that just fixing
> > > the generic power-management issues would be better and I also think
> > > that this should be feasible / not too hard.
> > >
> > > Here is what I have in mind for this:
> > >
> > > 1. Assume that the results of trying a specific fmt do not change over time.
> > >
> > > 2. Only allow userspace to request fmts which match one of the enum-fmts ->
> > >    enum-frame-sizes -> enum-frame-rates tripplet results
> > >    (constrain what userspace requests to these)
> > >
> > > 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
> > >    3 levels nested loop for this) on probe() and cache the results
> > >
> > > 4. Make try_fmt / set_fmt not poweron the device but instead constrain
> > >    the requested fmt to one from our cached fmts
> > >
> > > 5. On stream-on do the actual power-on + set-fmt + verify that we get
> > >    what we expect based on the cache, and otherwise return -EIO.
> >
> > Can we start powering up the device during try/set fmt and then
> > implement the format caching as an improvement?
>
> This sounds worth trying. We'll need to test it on a wide range of
> devices though, both internal and external.

We still need a plan for asynchronous controls.

And we have to decide if we stop supporting the uvc button (maybe we
can start by moving USB_VIDEO_CLASS_INPUT_EVDEV to staging and see
what happens?)


>
> > Laurent mentioned that some cameras missbehave if a lot of controls
> > are set during probing. I hope that this approach does not trigger
> > those, and if it does it would be easier to revert if we do the work
> > in two steps.
> >
> > > I think that should sort the issue, assuming that 1. above holds true.
> > >
> > > One downside is that this stops UVC button presses from working when
> > > not streaming. But userspace will typically only open the /dev/video#
> > > node if it plans to stream anyways so there should not be much of
> > > a difference wrt button press behavior.
> >
> > I do not personally use the button, but it is currently implemented as
> > a standard HID device. Making it only work during streamon() might be
> > a bit weird.
> > I am afraid that if there is a button we should keep the current behaviour.
> >
> > >
> > > This should also make camera enumeration faster for apps, since
> > > most apps / frameworks do the whole 3 levels nested loop for this
> > > on startup, for which atm we go out to the hw, which now instead
> > > will come from the fmts cache and thus will be much much faster,
> > > so this should lead to a noticeable speedup for apps accessing UVC
> > > cameras which would be another nice win.
> > >
> > > Downside is that the initial probe will take longer see we do
> > > all the tryfmt-s there now. But I think that taking a bit longer
> > > to probe while the machine is booting should not be an issue.
> >
> > How do you pretend to handle the controls? Do you plan to power-up the
> > device during s_ctrl() or set them only during streamon()?
> > If we power-up the device during s_ctrl we need to take care of the
> > asynchronous controls (typically pan/tilt/zoom), The device must be
> > powered until the control finishes, and the device might never reply
> > control_done if the firmware is not properly implemented.
> > If we set the controls only during streamon, we will break some
> > usecases. There are some video conferencing equipment that do homing
> > during streamoff. That will be a serious API breakage.
> >
> > This patchset is not only to fix the powersaving issues, but also to
> > fix the issue when a camera has  a gpio privacy switch and an internal
> > Privacy control.  4 years ago I did not see any camera with Privacy
> > control (in 100s of models), now they are common.
> > Can we have both changes, gpio subdevice and granular power saving?
> >
> > > Regards,
> > >
> > > Hans
> > >
> > > 1) Which is technically correct, but FWIW I agree with you that I think
> > > most userspace consumers will not care
> > >
> > > > Last year, we proposed a patchset to implement the privacy gpio as a
> > > > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/
> > > >
> > > > I think it is a pretty clean solution and makes sense to use a
> > > > subdevice for something that is a sub device of the camera :).
> > > >
> > > > This is an attempt to continue with that approach.
> > > >
> > > > Tested on gimble:
> > > > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0
> > > > Driver Info:
> > > >         Driver version   : 6.6.56
> > > >         Capabilities     : 0x00000000
> > > > Media Driver Info:
> > > >         Driver name      : uvcvideo
> > > >         Model            : HP 5M Camera: HP 5M Camera
> > > >         Serial           : 0001
> > > >         Bus info         : usb-0000:00:14.0-6
> > > >         Media version    : 6.6.56
> > > >         Hardware revision: 0x00009601 (38401)
> > > >         Driver version   : 6.6.56
> > > > Interface Info:
> > > >         ID               : 0x0300001d
> > > >         Type             : V4L Sub-Device
> > > > Entity Info:
> > > >         ID               : 0x00000013 (19)
> > > >         Name             : GPIO
> > > >         Function         : Unknown sub-device (00020006)
> > > >
> > > > Camera Controls
> > > >
> > > >                         privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only, volatile
> > > >
> > > > gimble-rev3 ~ # media-ctl  -p
> > > > Media controller API version 6.6.56
> > > >
> > > > Media device information
> > > > ------------------------
> > > > driver          uvcvideo
> > > > model           HP 5M Camera: HP 5M Camera
> > > > serial          0001
> > > > bus info        usb-0000:00:14.0-6
> > > > hw revision     0x9601
> > > > driver version  6.6.56
> > > >
> > > > Device topology
> > > > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link)
> > > >             type Node subtype V4L flags 1
> > > >             device node name /dev/video0
> > > >         pad0: Sink
> > > >                 <- "Extension 8":1 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link)
> > > >             type Node subtype V4L flags 0
> > > >             device node name /dev/video1
> > > >
> > > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes)
> > > >             type V4L2 subdev subtype Unknown flags 0
> > > >         pad0: Sink
> > > >                 <- "Extension 4":1 [ENABLED,IMMUTABLE]
> > > >         pad1: Source
> > > >                 -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes)
> > > >              type V4L2 subdev subtype Unknown flags 0
> > > >         pad0: Sink
> > > >                 <- "Processing 2":1 [ENABLED,IMMUTABLE]
> > > >         pad1: Source
> > > >                 -> "Extension 8":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes)
> > > >              type V4L2 subdev subtype Unknown flags 0
> > > >         pad0: Sink
> > > >                 <- "Camera 1":0 [ENABLED,IMMUTABLE]
> > > >         pad1: Source
> > > >                 -> "Extension 4":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes)
> > > >              type V4L2 subdev subtype Sensor flags 0
> > > >         pad0: Source
> > > >                 -> "Processing 2":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 19: GPIO (0 pad, 0 link, 0 routes)
> > > >              type V4L2 subdev subtype Decoder flags 0
> > > >              device node name /dev/v4l-subdev0
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@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 (5):
> > > >       media: uvcvideo: Factor out gpio functions to its own file
> > > >       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
> > > >
> > > > Yunke Cao (1):
> > > >       media: uvcvideo: Re-implement privacy GPIO as a separate subdevice
> > > >
> > > >  .../userspace-api/media/mediactl/media-types.rst   |   4 +
> > > >  drivers/media/usb/uvc/Makefile                     |   3 +-
> > > >  drivers/media/usb/uvc/uvc_ctrl.c                   |  40 +----
> > > >  drivers/media/usb/uvc/uvc_driver.c                 | 123 +-------------
> > > >  drivers/media/usb/uvc/uvc_entity.c                 |  20 ++-
> > > >  drivers/media/usb/uvc/uvc_gpio.c                   | 187 +++++++++++++++++++++
> > > >  drivers/media/usb/uvc/uvc_video.c                  |   4 +
> > > >  drivers/media/usb/uvc/uvcvideo.h                   |  34 ++--
> > > >  drivers/media/v4l2-core/v4l2-async.c               |   3 +-
> > > >  include/uapi/linux/media.h                         |   1 +
> > > >  10 files changed, 252 insertions(+), 167 deletions(-)
> > > > ---
> > > > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525
> > > > change-id: 20241030-uvc-subdev-89f4467a00b5
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

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

[Hans de Goede]

Hi Ricardo,

On 10-Nov-24 5:04 PM, Ricardo Ribalda wrote:
> On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:

<snip>

>>> Can we start powering up the device during try/set fmt and then
>>> implement the format caching as an improvement?
>>
>> This sounds worth trying. We'll need to test it on a wide range of
>> devices though, both internal and external.

Ack, as mentioned in the other mail which I just send I think
this is worth trying.

> We still need a plan for asynchronous controls.

As I mentioned in that other email I think we can do the same there.

So basically delay powering up the camera from /dev/video# open till
the first moment we actually need to communicate to the camera and
track per file-handle if we did a usb_autopm_get_interface() for
that file-handle and if yes, then do the put-interface on file-handle
close.

> And we have to decide if we stop supporting the uvc button (maybe we
> can start by moving USB_VIDEO_CLASS_INPUT_EVDEV to staging and see
> what happens?)

As I mentioned in other threads I do not think that the button
only working changing from:

"only works when /dev/video# is open"

to:

"only works when streaming from /dev/video#"

(or actually only works when some action on the camera which
requires it to be powered-on has been done).

is a big deal, since most apps which open /dev/video# for
a longer time will almost always do so to actually do something
with the camera, at which point the button will work just as
before.

And for apps which only do a short-lived open of /dev/video#
the button does not work with the current code either.

TL;DR: IMHO it is fine if the button only works when streaming.

Regards,

Hans

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

[Laurent Pinchart]

On Mon, Nov 25, 2024 at 01:31:49PM +0100, Hans de Goede wrote:
> Hi Ricardo,
> 
> On 10-Nov-24 5:04 PM, Ricardo Ribalda wrote:
> > On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart wrote:
> 
> <snip>
> 
> >>> Can we start powering up the device during try/set fmt and then
> >>> implement the format caching as an improvement?
> >>
> >> This sounds worth trying. We'll need to test it on a wide range of
> >> devices though, both internal and external.
> 
> Ack, as mentioned in the other mail which I just send I think
> this is worth trying.
> 
> > We still need a plan for asynchronous controls.
> 
> As I mentioned in that other email I think we can do the same there.
> 
> So basically delay powering up the camera from /dev/video# open till
> the first moment we actually need to communicate to the camera and
> track per file-handle if we did a usb_autopm_get_interface() for
> that file-handle and if yes, then do the put-interface on file-handle
> close.
> 
> > And we have to decide if we stop supporting the uvc button (maybe we
> > can start by moving USB_VIDEO_CLASS_INPUT_EVDEV to staging and see
> > what happens?)
> 
> As I mentioned in other threads I do not think that the button
> only working changing from:
> 
> "only works when /dev/video# is open"
> 
> to:
> 
> "only works when streaming from /dev/video#"
> 
> (or actually only works when some action on the camera which
> requires it to be powered-on has been done).
> 
> is a big deal, since most apps which open /dev/video# for
> a longer time will almost always do so to actually do something
> with the camera, at which point the button will work just as
> before.
> 
> And for apps which only do a short-lived open of /dev/video#
> the button does not work with the current code either.
> 
> TL;DR: IMHO it is fine if the button only works when streaming.

I'm fine with that, we can reconsider if people complain. It would be
painful though, as it could mean reverting everything we'll build
related to power management from now on until someone notices the new
behaviour, which could easily take a year. The risk is low, but the
consequences serious.

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi,

On 25-Nov-24 1:56 PM, Laurent Pinchart wrote:
> On Mon, Nov 25, 2024 at 01:31:49PM +0100, Hans de Goede wrote:
>> Hi Ricardo,
>>
>> On 10-Nov-24 5:04 PM, Ricardo Ribalda wrote:
>>> On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart wrote:
>>
>> <snip>
>>
>>>>> Can we start powering up the device during try/set fmt and then
>>>>> implement the format caching as an improvement?
>>>>
>>>> This sounds worth trying. We'll need to test it on a wide range of
>>>> devices though, both internal and external.
>>
>> Ack, as mentioned in the other mail which I just send I think
>> this is worth trying.
>>
>>> We still need a plan for asynchronous controls.
>>
>> As I mentioned in that other email I think we can do the same there.
>>
>> So basically delay powering up the camera from /dev/video# open till
>> the first moment we actually need to communicate to the camera and
>> track per file-handle if we did a usb_autopm_get_interface() for
>> that file-handle and if yes, then do the put-interface on file-handle
>> close.
>>
>>> And we have to decide if we stop supporting the uvc button (maybe we
>>> can start by moving USB_VIDEO_CLASS_INPUT_EVDEV to staging and see
>>> what happens?)
>>
>> As I mentioned in other threads I do not think that the button
>> only working changing from:
>>
>> "only works when /dev/video# is open"
>>
>> to:
>>
>> "only works when streaming from /dev/video#"
>>
>> (or actually only works when some action on the camera which
>> requires it to be powered-on has been done).
>>
>> is a big deal, since most apps which open /dev/video# for
>> a longer time will almost always do so to actually do something
>> with the camera, at which point the button will work just as
>> before.
>>
>> And for apps which only do a short-lived open of /dev/video#
>> the button does not work with the current code either.
>>
>> TL;DR: IMHO it is fine if the button only works when streaming.
> 
> I'm fine with that, we can reconsider if people complain. It would be
> painful though, as it could mean reverting everything we'll build
> related to power management from now on until someone notices the new
> behaviour, which could easily take a year. The risk is low, but the
> consequences serious.

I think that if some users complain we can just add a default off module
option to restore the old behavior for use-cases which somehow depend
on that.

Doing an extra usb_autopm_get_interface() + usb_autopm_put_interface()
at open/close() time if the option is set is easy, and that will just
render out other get() and put() calls into no-ops.

So we always have that route as an escape-hatch.

Regards,

Hans

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

[Ricardo Ribalda]

On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Sat, Nov 09, 2024 at 05:29:54PM +0100, Ricardo Ribalda wrote:
> > On Sat, 9 Nov 2024 at 16:37, Hans de Goede <hdegoede@redhat.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > FYI / some background: I have been asked to start helping /
> > > co-maintaining UVC with Laurent. I'll send out a patch adding
> > > myself as UVC maintainer soon.
> >
> > Great! I talked with Laurent yesterday, I hope that we can maintain
> > the driver the three of us in the near future.
> >
> > > On 8-Nov-24 9:25 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 two 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.
> > > >
> > > > We tried to fix the power-up issues implementing "granular power
> > > > saving" but it has been more complicated than anticipated....
> > >
> > > I have been discussing UVC power-management with Laurent, also
> > > related to power-consumption issues caused by libcamera's pipeline
> > > handler holding open the /dev/video# node as long as the camera
> > > manager object exists.
> > >
> > > For now we have fixed this with some relatively small changes to
> > > libcamera's uvcvideo pipeline handler, but that is really meant
> > > as an interim solution and as this privacy control series shows
> > > the power-management issues are real.
> >
> > Indeed, we have tried to fixed it before:
> > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
> >
> > btw this recently landed patch was to work in this direction :)
> > https://lore.kernel.org/linux-media/20240926-guenter-mini-v7-1-690441953d4a@chromium.org/
> >
> > The more people interested in this problem the better.
> >
> > > Combined with Mauro's remarks about how this is an userspace ABI break (1)
> > > I think we should maybe first take another look at the powermanagement
> > > issues in general rather then moving forward with this series.
> > >
> > > My apologies for this, I realize how annoying it can be when you are
> > > working on a patch series to fix a specific issue and a reviewer
> > > moves the goal-posts like this. But I do really think that just fixing
> > > the generic power-management issues would be better and I also think
> > > that this should be feasible / not too hard.
> > >
> > > Here is what I have in mind for this:
> > >
> > > 1. Assume that the results of trying a specific fmt do not change over time.
> > >
> > > 2. Only allow userspace to request fmts which match one of the enum-fmts ->
> > >    enum-frame-sizes -> enum-frame-rates tripplet results
> > >    (constrain what userspace requests to these)
> > >
> > > 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
> > >    3 levels nested loop for this) on probe() and cache the results
> > >
> > > 4. Make try_fmt / set_fmt not poweron the device but instead constrain
> > >    the requested fmt to one from our cached fmts
> > >
> > > 5. On stream-on do the actual power-on + set-fmt + verify that we get
> > >    what we expect based on the cache, and otherwise return -EIO.
> >
> > Can we start powering up the device during try/set fmt and then
> > implement the format caching as an improvement?
>
> This sounds worth trying. We'll need to test it on a wide range of
> devices though, both internal and external.

For what is worth, we have been running something similar to
https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
in ChromeOS and it has worked fine....

But I am pretty sure that it has issues with async controls :S

>
> > Laurent mentioned that some cameras missbehave if a lot of controls
> > are set during probing. I hope that this approach does not trigger
> > those, and if it does it would be easier to revert if we do the work
> > in two steps.
> >
> > > I think that should sort the issue, assuming that 1. above holds true.
> > >
> > > One downside is that this stops UVC button presses from working when
> > > not streaming. But userspace will typically only open the /dev/video#
> > > node if it plans to stream anyways so there should not be much of
> > > a difference wrt button press behavior.
> >
> > I do not personally use the button, but it is currently implemented as
> > a standard HID device. Making it only work during streamon() might be
> > a bit weird.
> > I am afraid that if there is a button we should keep the current behaviour.
> >
> > >
> > > This should also make camera enumeration faster for apps, since
> > > most apps / frameworks do the whole 3 levels nested loop for this
> > > on startup, for which atm we go out to the hw, which now instead
> > > will come from the fmts cache and thus will be much much faster,
> > > so this should lead to a noticeable speedup for apps accessing UVC
> > > cameras which would be another nice win.
> > >
> > > Downside is that the initial probe will take longer see we do
> > > all the tryfmt-s there now. But I think that taking a bit longer
> > > to probe while the machine is booting should not be an issue.
> >
> > How do you pretend to handle the controls? Do you plan to power-up the
> > device during s_ctrl() or set them only during streamon()?
> > If we power-up the device during s_ctrl we need to take care of the
> > asynchronous controls (typically pan/tilt/zoom), The device must be
> > powered until the control finishes, and the device might never reply
> > control_done if the firmware is not properly implemented.
> > If we set the controls only during streamon, we will break some
> > usecases. There are some video conferencing equipment that do homing
> > during streamoff. That will be a serious API breakage.
> >
> > This patchset is not only to fix the powersaving issues, but also to
> > fix the issue when a camera has  a gpio privacy switch and an internal
> > Privacy control.  4 years ago I did not see any camera with Privacy
> > control (in 100s of models), now they are common.
> > Can we have both changes, gpio subdevice and granular power saving?
> >
> > > Regards,
> > >
> > > Hans
> > >
> > > 1) Which is technically correct, but FWIW I agree with you that I think
> > > most userspace consumers will not care
> > >
> > > > Last year, we proposed a patchset to implement the privacy gpio as a
> > > > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/
> > > >
> > > > I think it is a pretty clean solution and makes sense to use a
> > > > subdevice for something that is a sub device of the camera :).
> > > >
> > > > This is an attempt to continue with that approach.
> > > >
> > > > Tested on gimble:
> > > > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0
> > > > Driver Info:
> > > >         Driver version   : 6.6.56
> > > >         Capabilities     : 0x00000000
> > > > Media Driver Info:
> > > >         Driver name      : uvcvideo
> > > >         Model            : HP 5M Camera: HP 5M Camera
> > > >         Serial           : 0001
> > > >         Bus info         : usb-0000:00:14.0-6
> > > >         Media version    : 6.6.56
> > > >         Hardware revision: 0x00009601 (38401)
> > > >         Driver version   : 6.6.56
> > > > Interface Info:
> > > >         ID               : 0x0300001d
> > > >         Type             : V4L Sub-Device
> > > > Entity Info:
> > > >         ID               : 0x00000013 (19)
> > > >         Name             : GPIO
> > > >         Function         : Unknown sub-device (00020006)
> > > >
> > > > Camera Controls
> > > >
> > > >                         privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only, volatile
> > > >
> > > > gimble-rev3 ~ # media-ctl  -p
> > > > Media controller API version 6.6.56
> > > >
> > > > Media device information
> > > > ------------------------
> > > > driver          uvcvideo
> > > > model           HP 5M Camera: HP 5M Camera
> > > > serial          0001
> > > > bus info        usb-0000:00:14.0-6
> > > > hw revision     0x9601
> > > > driver version  6.6.56
> > > >
> > > > Device topology
> > > > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link)
> > > >             type Node subtype V4L flags 1
> > > >             device node name /dev/video0
> > > >         pad0: Sink
> > > >                 <- "Extension 8":1 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link)
> > > >             type Node subtype V4L flags 0
> > > >             device node name /dev/video1
> > > >
> > > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes)
> > > >             type V4L2 subdev subtype Unknown flags 0
> > > >         pad0: Sink
> > > >                 <- "Extension 4":1 [ENABLED,IMMUTABLE]
> > > >         pad1: Source
> > > >                 -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes)
> > > >              type V4L2 subdev subtype Unknown flags 0
> > > >         pad0: Sink
> > > >                 <- "Processing 2":1 [ENABLED,IMMUTABLE]
> > > >         pad1: Source
> > > >                 -> "Extension 8":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes)
> > > >              type V4L2 subdev subtype Unknown flags 0
> > > >         pad0: Sink
> > > >                 <- "Camera 1":0 [ENABLED,IMMUTABLE]
> > > >         pad1: Source
> > > >                 -> "Extension 4":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes)
> > > >              type V4L2 subdev subtype Sensor flags 0
> > > >         pad0: Source
> > > >                 -> "Processing 2":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 19: GPIO (0 pad, 0 link, 0 routes)
> > > >              type V4L2 subdev subtype Decoder flags 0
> > > >              device node name /dev/v4l-subdev0
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@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 (5):
> > > >       media: uvcvideo: Factor out gpio functions to its own file
> > > >       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
> > > >
> > > > Yunke Cao (1):
> > > >       media: uvcvideo: Re-implement privacy GPIO as a separate subdevice
> > > >
> > > >  .../userspace-api/media/mediactl/media-types.rst   |   4 +
> > > >  drivers/media/usb/uvc/Makefile                     |   3 +-
> > > >  drivers/media/usb/uvc/uvc_ctrl.c                   |  40 +----
> > > >  drivers/media/usb/uvc/uvc_driver.c                 | 123 +-------------
> > > >  drivers/media/usb/uvc/uvc_entity.c                 |  20 ++-
> > > >  drivers/media/usb/uvc/uvc_gpio.c                   | 187 +++++++++++++++++++++
> > > >  drivers/media/usb/uvc/uvc_video.c                  |   4 +
> > > >  drivers/media/usb/uvc/uvcvideo.h                   |  34 ++--
> > > >  drivers/media/v4l2-core/v4l2-async.c               |   3 +-
> > > >  include/uapi/linux/media.h                         |   1 +
> > > >  10 files changed, 252 insertions(+), 167 deletions(-)
> > > > ---
> > > > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525
> > > > change-id: 20241030-uvc-subdev-89f4467a00b5
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

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

[Hans de Goede]

Hi Ricardo,

On 10-Nov-24 5:07 PM, Ricardo Ribalda wrote:
> On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:

<snip>

>>>> Here is what I have in mind for this:
>>>>
>>>> 1. Assume that the results of trying a specific fmt do not change over time.
>>>>
>>>> 2. Only allow userspace to request fmts which match one of the enum-fmts ->
>>>>    enum-frame-sizes -> enum-frame-rates tripplet results
>>>>    (constrain what userspace requests to these)
>>>>
>>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
>>>>    3 levels nested loop for this) on probe() and cache the results
>>>>
>>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain
>>>>    the requested fmt to one from our cached fmts
>>>>
>>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get
>>>>    what we expect based on the cache, and otherwise return -EIO.
>>>
>>> Can we start powering up the device during try/set fmt and then
>>> implement the format caching as an improvement?
>>
>> This sounds worth trying. We'll need to test it on a wide range of
>> devices though, both internal and external.
> 
> For what is worth, we have been running something similar to
> https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
> in ChromeOS and it has worked fine....
> 
> But I am pretty sure that it has issues with async controls :S

Interesting that is actually a lot more aggressive (as in doing a
usb_autopm_put_interface() often) then what I expected when you said:

"Can we start powering up the device during try/set fmt"

As I mentioned in my other emails what I think would worth nicely
is delay the initial usb_autopm_get_interface() till we need it
and then just leave the camera on till /dev/video# gets closed.

That idea is based on dividing apps in 2 groups:

1. Apps just temporarily opening /dev/video# nodes for discovery,
where we ideally would not power-up the camera.

2. Apps (could even be the same app) opening /dev/video# for
a longer time because it actually want to use the camera
at the moment of opening and which close the /dev/video# node
when done with the camera.

Your code seems to also covers a 3th group of apps:

3. Apps opening /dev/video# for a long time, while not using
it all the time.

Although it would be nice to also cover those, IMHO those are
not well behaved apps and I'm not sure if we need to cover those.

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

Regards,

Hans

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

[Laurent Pinchart]

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

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

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

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi,

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

Keeping devices open has advantages and disadvantages. E.g. keeping
/dev/input/event# nodes open will also typically lead to e.g.
touchscreens staying powered on.

Generally speaking it is not unheard of to expect userspace to
behave in a certain way for things like this for power-consumption
reasons.

I guess the question is how far do we want to go inside the uvc
driver to avoid userspace needing to close the /dev/video# nodes
when unused.

Ricardo's patch from here:

https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/

goes all the way and if I understand Ricardo correctly this is
already in use in ChromeOS ?

So we could also go with this. Maybe put it behind a Kconfig option
for a while ?

AFAICT the only thing which needs to be figured out there is async
controls.

I think we can simply set a long autosuspend delay on devices with
async controls to deal with that ?

I have a Logitech QuickCam Orbit (non AF) UVC camera which has
pan + tilt controls and AFAICT these work fine with v4l2-ctl
which immediately closes the /dev/video# node after the set-ctrl
command. But I'm not sure if I have tested this without the camera
streaming at the time. Anyways that is at least one camera I can test.

Regards,

Hans

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

[Ricardo Ribalda]

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

It is in use on just some devices. I try to not diverge from upstream if I can.

>
> So we could also go with this. Maybe put it behind a Kconfig option
> for a while ?

I can try to work on a new version of the patch, including support for
async controls the way I described in my previous email.
Let me know what you think.

>
> AFAICT the only thing which needs to be figured out there is async
> controls.
>
> I think we can simply set a long autosuspend delay on devices with
> async controls to deal with that ?
>
> I have a Logitech QuickCam Orbit (non AF) UVC camera which has
> pan + tilt controls and AFAICT these work fine with v4l2-ctl
> which immediately closes the /dev/video# node after the set-ctrl
> command. But I'm not sure if I have tested this without the camera
> streaming at the time. Anyways that is at least one camera I can test.
>
> Regards,
>
> Hans
>
>


-- 
Ricardo Ribalda

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

[Hans de Goede]

Hi Ricardo,

On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Sat, 9 Nov 2024 at 16:37, Hans de Goede <hdegoede@redhat.com> wrote:

<snip>

Note only replying to the button remark here, to try and disentangle
that from the general power-management discussions.

>> One downside is that this stops UVC button presses from working when
>> not streaming. But userspace will typically only open the /dev/video#
>> node if it plans to stream anyways so there should not be much of
>> a difference wrt button press behavior.
> 
> I do not personally use the button, but it is currently implemented as
> a standard HID device. Making it only work during streamon() might be
> a bit weird.
> I am afraid that if there is a button we should keep the current behaviour.

There are 2 sort of "snapshot" buttons on UVC cameras

1. Snapshot buttons handled through the UVC protocol / USB interface.
These require the UVC interface to be powered on and the status interrupt
URB to be submitted (uvc_status_start() called).

These will only work if the /dev/video# node is open, otherwise the UVC
interface is powered down and the status interrupt URB is not submitted.

IOW most of the time these already do not work, since most of the time
userspace will not have /dev/video# open (otherwise we would have
the power-consumption issues this patch-series tries to fix everywhere).

IMHO not having these working only when /dev/video# is open and instead
only having them working when streaming is a not a big deal since usually
userspace will only open /dev/video# to stream anyways (except for udev
probing, but that is very short lived and does not help with the button).

2. Snapshot buttons which use a separate standard USB HID interface

Since these use a separate USB interface, using the usb-hid driver.
These do always work and these are handled completely independent
of the UVC driver so it does not matter what we do in the UVC driver.

I hope this helps clarify the button situation.

Regards,

Hans

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

[Hans de Goede]

Hi Ricardo,

On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote:

<snip>

>> I have been discussing UVC power-management with Laurent, also
>> related to power-consumption issues caused by libcamera's pipeline
>> handler holding open the /dev/video# node as long as the camera
>> manager object exists.

<snip>

>> Here is what I have in mind for this:
>>
>> 1. Assume that the results of trying a specific fmt do not change over time.
>>
>> 2. Only allow userspace to request fmts which match one of the enum-fmts ->
>>    enum-frame-sizes -> enum-frame-rates tripplet results
>>    (constrain what userspace requests to these)
>>
>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
>>    3 levels nested loop for this) on probe() and cache the results
>>
>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain
>>    the requested fmt to one from our cached fmts
>>
>> 5. On stream-on do the actual power-on + set-fmt + verify that we get
>>    what we expect based on the cache, and otherwise return -EIO.
> 
> Can we start powering up the device during try/set fmt and then
> implement the format caching as an improvement?

Yes, actually looking at how complex this is when e.g. also taking
controls into account I think that taking small steps is a good idea.

I have lately mostly been working on sensor drivers where delaying
applying format settings + all controls to stream-on is normal.

So that is the mental model I'm applying to uvc here, but that might
not be entirely applicable.

> Laurent mentioned that some cameras missbehave if a lot of controls
> are set during probing. I hope that this approach does not trigger
> those, and if it does it would be easier to revert if we do the work
> in two steps.

Ack, taking small steps sounds like a good plan.

<snip>

>> This should also make camera enumeration faster for apps, since
>> most apps / frameworks do the whole 3 levels nested loop for this
>> on startup, for which atm we go out to the hw, which now instead
>> will come from the fmts cache and thus will be much much faster,
>> so this should lead to a noticeable speedup for apps accessing UVC
>> cameras which would be another nice win.
>>
>> Downside is that the initial probe will take longer see we do
>> all the tryfmt-s there now. But I think that taking a bit longer
>> to probe while the machine is booting should not be an issue.
> 
> How do you pretend to handle the controls? Do you plan to power-up the
> device during s_ctrl() or set them only during streamon()?
> If we power-up the device during s_ctrl we need to take care of the
> asynchronous controls (typically pan/tilt/zoom), The device must be
> powered until the control finishes, and the device might never reply
> control_done if the firmware is not properly implemented.
> If we set the controls only during streamon, we will break some
> usecases. There are some video conferencing equipment that do homing
> during streamoff. That will be a serious API breakage.

How to handle controls is a good idea.

Based on my sensor experience my initial idea was to just cache them
all. Basically make set_ctrl succeed but do not actually do anyhing
when the camera is not already powered on and then on stream-on call
__v4l2_ctrl_handler_setup() to get all current values applied.

But as you indicate that will likely not work well with async controls,
although we already have this issue when using v4l2-ctl from the cmdline
on such a control and that seems to work fine. Just because we allow
the USB connection to sleep, does not mean that the camera cannot finish
doing applying the async control.

But I can see how some cameras might not like this and having 2 different
paths for different controls also is undesirable.

Combine that with what Laurent said about devices not liking it when
you set too much controls in a short time and I do think we need to
immediately apply ctrls.

I see 2 ways of doing that:

1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
and then on set_ctrl do a pm_runtime_get_sync() +
pm_runtime_put_autosuspend() giving the camera 1 second to finish
applying the async ctrl (which might not be enough for e.g homing) +
also avoid doing suspend + resume all the time if multiple ctrls are send

2. Instead of immediately powering on the camera on /dev/video# open
track per fh if the camera has been powered on and then on the first
set-ctrl, or the first other IOCTL like try/set-fmt which requires
the camera to be powered on power it up and then keep it on until
the fh is closed, since apps hitting these paths are likely to do
more stuff which requires the camera to be powered on.

This should avoid apps (like udev rules) just doing a simple get-cap
query of powering on the camera, while at the same time preserving
the current behavior for apps which want to actually do something
with the camera, so the chance of regressions should be small.

I guess the time between power-up and sending the first request to
the camera will change slightly. But most apps which actually want
to do stuff with the camera will likely already do so immediately
after opening /dev/video# so the timing change should be negligible.

I guess 2. is pretty similar to your proposal to delay power-on
to the first set/try-fmt, which I assume also already does
something similar wrt controls ?

I think that 2. can work nicely and that would be nice to have,
even though it does not fix the privacy-control + power-mgmt issue.

Regards,

Hans

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

[Laurent Pinchart]

On Mon, Nov 25, 2024 at 01:25:41PM +0100, Hans de Goede wrote:
> Hi Ricardo,
> 
> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote:
> 
> <snip>
> 
> >> I have been discussing UVC power-management with Laurent, also
> >> related to power-consumption issues caused by libcamera's pipeline
> >> handler holding open the /dev/video# node as long as the camera
> >> manager object exists.
> 
> <snip>
> 
> >> Here is what I have in mind for this:
> >>
> >> 1. Assume that the results of trying a specific fmt do not change over time.
> >>
> >> 2. Only allow userspace to request fmts which match one of the enum-fmts ->
> >>    enum-frame-sizes -> enum-frame-rates tripplet results
> >>    (constrain what userspace requests to these)
> >>
> >> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
> >>    3 levels nested loop for this) on probe() and cache the results
> >>
> >> 4. Make try_fmt / set_fmt not poweron the device but instead constrain
> >>    the requested fmt to one from our cached fmts
> >>
> >> 5. On stream-on do the actual power-on + set-fmt + verify that we get
> >>    what we expect based on the cache, and otherwise return -EIO.
> > 
> > Can we start powering up the device during try/set fmt and then
> > implement the format caching as an improvement?
> 
> Yes, actually looking at how complex this is when e.g. also taking
> controls into account I think that taking small steps is a good idea.
> 
> I have lately mostly been working on sensor drivers where delaying
> applying format settings + all controls to stream-on is normal.
> 
> So that is the mental model I'm applying to uvc here, but that might
> not be entirely applicable.
> 
> > Laurent mentioned that some cameras missbehave if a lot of controls
> > are set during probing. I hope that this approach does not trigger
> > those, and if it does it would be easier to revert if we do the work
> > in two steps.
> 
> Ack, taking small steps sounds like a good plan.
> 
> <snip>
> 
> >> This should also make camera enumeration faster for apps, since
> >> most apps / frameworks do the whole 3 levels nested loop for this
> >> on startup, for which atm we go out to the hw, which now instead
> >> will come from the fmts cache and thus will be much much faster,
> >> so this should lead to a noticeable speedup for apps accessing UVC
> >> cameras which would be another nice win.
> >>
> >> Downside is that the initial probe will take longer see we do
> >> all the tryfmt-s there now. But I think that taking a bit longer
> >> to probe while the machine is booting should not be an issue.
> > 
> > How do you pretend to handle the controls? Do you plan to power-up the
> > device during s_ctrl() or set them only during streamon()?
> > If we power-up the device during s_ctrl we need to take care of the
> > asynchronous controls (typically pan/tilt/zoom), The device must be
> > powered until the control finishes, and the device might never reply
> > control_done if the firmware is not properly implemented.

Talking about powering up is a bit misleading here. What this is about
is resuming the USB device. The device stays powered when it is
suspended, and the value of controls is retained unless the USB device
gets reset. The .resume() handler does not restore controls, the
.reset_resume() handler does.

Some UVC cameras have the USB_QUIRK_RESET_RESUME quirk set, but the
majority doesn't. Annoyingly there's a blanket entry for all Logitech
UVC devices in drivers/usb/core/quirks.c, which has led the uvcvideo
driver to grow a UVC_QUIRK_NO_RESET_RESUME quirk.

> > If we set the controls only during streamon, we will break some
> > usecases. There are some video conferencing equipment that do homing
> > during streamoff. That will be a serious API breakage.
> 
> How to handle controls is a good idea.
> 
> Based on my sensor experience my initial idea was to just cache them
> all. Basically make set_ctrl succeed but do not actually do anyhing
> when the camera is not already powered on and then on stream-on call
> __v4l2_ctrl_handler_setup() to get all current values applied.
> 
> But as you indicate that will likely not work well with async controls,

It's not all asynchronous controls, only the ones that have an impact on
the device that can be observed without streaming. Mechanical pan/tilt
is the main use case. We could handle those controls in a special way if
needed.

> although we already have this issue when using v4l2-ctl from the cmdline
> on such a control and that seems to work fine. Just because we allow
> the USB connection to sleep, does not mean that the camera cannot finish
> doing applying the async control.
> 
> But I can see how some cameras might not like this and having 2 different
> paths for different controls also is undesirable.
> 
> Combine that with what Laurent said about devices not liking it when
> you set too much controls in a short time and I do think we need to
> immediately apply ctrls.
> 
> I see 2 ways of doing that:
> 
> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
> and then on set_ctrl do a pm_runtime_get_sync() +
> pm_runtime_put_autosuspend() giving the camera 1 second to finish
> applying the async ctrl (which might not be enough for e.g homing) +
> also avoid doing suspend + resume all the time if multiple ctrls are send
> 
> 2. Instead of immediately powering on the camera on /dev/video# open
> track per fh if the camera has been powered on and then on the first
> set-ctrl, or the first other IOCTL like try/set-fmt which requires
> the camera to be powered on power it up and then keep it on until
> the fh is closed, since apps hitting these paths are likely to do
> more stuff which requires the camera to be powered on.

A mode of operation where a userspace action causes a state change and
the only way to change back to the previous state is to close the device
often leads to problems. I'd rather not do this unless we have to
completely rule out all other options.

> This should avoid apps (like udev rules) just doing a simple get-cap
> query of powering on the camera, while at the same time preserving
> the current behavior for apps which want to actually do something
> with the camera, so the chance of regressions should be small.
> 
> I guess the time between power-up and sending the first request to
> the camera will change slightly. But most apps which actually want
> to do stuff with the camera will likely already do so immediately
> after opening /dev/video# so the timing change should be negligible.
> 
> I guess 2. is pretty similar to your proposal to delay power-on
> to the first set/try-fmt, which I assume also already does
> something similar wrt controls ?
> 
> I think that 2. can work nicely and that would be nice to have,
> even though it does not fix the privacy-control + power-mgmt issue.

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Laurent,

On 25-Nov-24 1:49 PM, Laurent Pinchart wrote:
> On Mon, Nov 25, 2024 at 01:25:41PM +0100, Hans de Goede wrote:

<snip>

>> I see 2 ways of doing that:
>>
>> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
>> and then on set_ctrl do a pm_runtime_get_sync() +
>> pm_runtime_put_autosuspend() giving the camera 1 second to finish
>> applying the async ctrl (which might not be enough for e.g homing) +
>> also avoid doing suspend + resume all the time if multiple ctrls are send
>>
>> 2. Instead of immediately powering on the camera on /dev/video# open
>> track per fh if the camera has been powered on and then on the first
>> set-ctrl, or the first other IOCTL like try/set-fmt which requires
>> the camera to be powered on power it up and then keep it on until
>> the fh is closed, since apps hitting these paths are likely to do
>> more stuff which requires the camera to be powered on.
> 
> A mode of operation where a userspace action causes a state change and
> the only way to change back to the previous state is to close the device
> often leads to problems. I'd rather not do this unless we have to
> completely rule out all other options.

But we already have that today. We already do the usb_autopm_get_interface()
as soon as /dev/video# gets opened and the only way to undo it is to close
/dev/video#.

What I'm suggesting is to no longer do the usb_autopm_get_interface()
on all opens, but only on some.

Where "some" are the ones where we come to the conclusion that we actually
need to power-up the USB-bus / interface because we want to talk to
the device.

IOW delay the usb_autopm_get_interface() until the first action which
actually requires it.

This should be a very minimal change from the pov of USB interactions
with the actual device, so a small change of regressions while at
least not powering on the device during udev discovery.

I guess one could argue that the cases where this is a win are so
small that this is not worth it.

Regards,

Hans

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

[Laurent Pinchart]

On Mon, Nov 25, 2024 at 02:29:51PM +0100, Hans de Goede wrote:
> On 25-Nov-24 1:49 PM, Laurent Pinchart wrote:
> > On Mon, Nov 25, 2024 at 01:25:41PM +0100, Hans de Goede wrote:
> 
> <snip>
> 
> >> I see 2 ways of doing that:
> >>
> >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
> >> and then on set_ctrl do a pm_runtime_get_sync() +
> >> pm_runtime_put_autosuspend() giving the camera 1 second to finish
> >> applying the async ctrl (which might not be enough for e.g homing) +
> >> also avoid doing suspend + resume all the time if multiple ctrls are send
> >>
> >> 2. Instead of immediately powering on the camera on /dev/video# open
> >> track per fh if the camera has been powered on and then on the first
> >> set-ctrl, or the first other IOCTL like try/set-fmt which requires
> >> the camera to be powered on power it up and then keep it on until
> >> the fh is closed, since apps hitting these paths are likely to do
> >> more stuff which requires the camera to be powered on.
> > 
> > A mode of operation where a userspace action causes a state change and
> > the only way to change back to the previous state is to close the device
> > often leads to problems. I'd rather not do this unless we have to
> > completely rule out all other options.
> 
> But we already have that today. We already do the usb_autopm_get_interface()
> as soon as /dev/video# gets opened and the only way to undo it is to close
> /dev/video#.

Yes, but close() is the counterpart of open(). Breaking the symmetry is
what bothers me, it's not nice from an application point of view. It
wouldn't force instance have solved the issue of keeping the device
powered when used through libcamera (or anything else that keeps the
device node open after using the camera, or just after querying some of
its capabilities through TRY_FMT).

> What I'm suggesting is to no longer do the usb_autopm_get_interface()
> on all opens, but only on some.
> 
> Where "some" are the ones where we come to the conclusion that we actually
> need to power-up the USB-bus / interface because we want to talk to
> the device.
> 
> IOW delay the usb_autopm_get_interface() until the first action which
> actually requires it.
> 
> This should be a very minimal change from the pov of USB interactions
> with the actual device, so a small change of regressions while at
> least not powering on the device during udev discovery.
> 
> I guess one could argue that the cases where this is a win are so
> small that this is not worth it.

Is there a significant enough gain through this approach, and are the
other approaches impractical ? If so we can consider this.

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

On Mon, 25 Nov 2024 at 13:25, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Ricardo,
>
> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote:
>
> <snip>
>
> >> I have been discussing UVC power-management with Laurent, also
> >> related to power-consumption issues caused by libcamera's pipeline
> >> handler holding open the /dev/video# node as long as the camera
> >> manager object exists.
>
> <snip>
>
> >> Here is what I have in mind for this:
> >>
> >> 1. Assume that the results of trying a specific fmt do not change over time.
> >>
> >> 2. Only allow userspace to request fmts which match one of the enum-fmts ->
> >>    enum-frame-sizes -> enum-frame-rates tripplet results
> >>    (constrain what userspace requests to these)
> >>
> >> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
> >>    3 levels nested loop for this) on probe() and cache the results
> >>
> >> 4. Make try_fmt / set_fmt not poweron the device but instead constrain
> >>    the requested fmt to one from our cached fmts
> >>
> >> 5. On stream-on do the actual power-on + set-fmt + verify that we get
> >>    what we expect based on the cache, and otherwise return -EIO.
> >
> > Can we start powering up the device during try/set fmt and then
> > implement the format caching as an improvement?
>
> Yes, actually looking at how complex this is when e.g. also taking
> controls into account I think that taking small steps is a good idea.
>
> I have lately mostly been working on sensor drivers where delaying
> applying format settings + all controls to stream-on is normal.
>
> So that is the mental model I'm applying to uvc here, but that might
> not be entirely applicable.
>
> > Laurent mentioned that some cameras missbehave if a lot of controls
> > are set during probing. I hope that this approach does not trigger
> > those, and if it does it would be easier to revert if we do the work
> > in two steps.
>
> Ack, taking small steps sounds like a good plan.
>
> <snip>
>
> >> This should also make camera enumeration faster for apps, since
> >> most apps / frameworks do the whole 3 levels nested loop for this
> >> on startup, for which atm we go out to the hw, which now instead
> >> will come from the fmts cache and thus will be much much faster,
> >> so this should lead to a noticeable speedup for apps accessing UVC
> >> cameras which would be another nice win.
> >>
> >> Downside is that the initial probe will take longer see we do
> >> all the tryfmt-s there now. But I think that taking a bit longer
> >> to probe while the machine is booting should not be an issue.
> >
> > How do you pretend to handle the controls? Do you plan to power-up the
> > device during s_ctrl() or set them only during streamon()?
> > If we power-up the device during s_ctrl we need to take care of the
> > asynchronous controls (typically pan/tilt/zoom), The device must be
> > powered until the control finishes, and the device might never reply
> > control_done if the firmware is not properly implemented.
> > If we set the controls only during streamon, we will break some
> > usecases. There are some video conferencing equipment that do homing
> > during streamoff. That will be a serious API breakage.
>
> How to handle controls is a good idea.
>
> Based on my sensor experience my initial idea was to just cache them
> all. Basically make set_ctrl succeed but do not actually do anyhing
> when the camera is not already powered on and then on stream-on call
> __v4l2_ctrl_handler_setup() to get all current values applied.
>
> But as you indicate that will likely not work well with async controls,
> although we already have this issue when using v4l2-ctl from the cmdline
> on such a control and that seems to work fine.

-----
> Just because we allow
> the USB connection to sleep, does not mean that the camera cannot finish
> doing applying the async control.
>
Not sure what you mean with this sentence. Could you explain it
differently? Sorry

> But I can see how some cameras might not like this and having 2 different
> paths for different controls also is undesirable.
>
> Combine that with what Laurent said about devices not liking it when
> you set too much controls in a short time and I do think we need to
> immediately apply ctrls.
>
> I see 2 ways of doing that:
>
> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
> and then on set_ctrl do a pm_runtime_get_sync() +
> pm_runtime_put_autosuspend() giving the camera 1 second to finish
> applying the async ctrl (which might not be enough for e.g homing) +
> also avoid doing suspend + resume all the time if multiple ctrls are send

What about 1.5:

during s_ctrl():
usb_autopm_get_interface()
if the control is UVC_CTRL_FLAG_ASYNCHRONOUS.
       usb_autopm_get_interface()
set the actual control in the hardware
usb_autopm_put_interface()

during uvc_ctrl_status_event():
   usb_autopm_put_interface()

during close():
   send all the missing usb_autopm_put_interface()


This way:
- we do not have an artificial delay that might not work for all the use cases
- cameras with noncompliant async controls will have the same PM
behaviour as now  (will be powered on until close() )

We do the same with the rest of the actions that require hardware access, like:
https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/

This way:
- Apps that do not need to access the hardware, do not wake it up, and
we do not break usecases.

Next steps will be:
 - cache the formats
 - move the actual set_ctrl to streamon... but if we can do that I
would argue than we can move completely to the control framework.



>
> 2. Instead of immediately powering on the camera on /dev/video# open
> track per fh if the camera has been powered on and then on the first
> set-ctrl, or the first other IOCTL like try/set-fmt which requires
> the camera to be powered on power it up and then keep it on until
> the fh is closed, since apps hitting these paths are likely to do
> more stuff which requires the camera to be powered on.
>
> This should avoid apps (like udev rules) just doing a simple get-cap
> query of powering on the camera, while at the same time preserving
> the current behavior for apps which want to actually do something
> with the camera, so the chance of regressions should be small.
>
> I guess the time between power-up and sending the first request to
> the camera will change slightly. But most apps which actually want
> to do stuff with the camera will likely already do so immediately
> after opening /dev/video# so the timing change should be negligible.
>
> I guess 2. is pretty similar to your proposal to delay power-on
> to the first set/try-fmt, which I assume also already does
> something similar wrt controls ?
>
> I think that 2. can work nicely and that would be nice to have,
> even though it does not fix the privacy-control + power-mgmt issue.
>
> Regards,
>
> Hans
>
>
>


-- 
Ricardo Ribalda

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

[Hans de Goede]

Hi Ricardo,

On 25-Nov-24 2:39 PM, Ricardo Ribalda wrote:
> On Mon, 25 Nov 2024 at 13:25, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Ricardo,
>>
>> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote:
>>
>> <snip>
>>
>>>> I have been discussing UVC power-management with Laurent, also
>>>> related to power-consumption issues caused by libcamera's pipeline
>>>> handler holding open the /dev/video# node as long as the camera
>>>> manager object exists.
>>
>> <snip>
>>
>>>> Here is what I have in mind for this:
>>>>
>>>> 1. Assume that the results of trying a specific fmt do not change over time.
>>>>
>>>> 2. Only allow userspace to request fmts which match one of the enum-fmts ->
>>>>    enum-frame-sizes -> enum-frame-rates tripplet results
>>>>    (constrain what userspace requests to these)
>>>>
>>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
>>>>    3 levels nested loop for this) on probe() and cache the results
>>>>
>>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain
>>>>    the requested fmt to one from our cached fmts
>>>>
>>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get
>>>>    what we expect based on the cache, and otherwise return -EIO.
>>>
>>> Can we start powering up the device during try/set fmt and then
>>> implement the format caching as an improvement?
>>
>> Yes, actually looking at how complex this is when e.g. also taking
>> controls into account I think that taking small steps is a good idea.
>>
>> I have lately mostly been working on sensor drivers where delaying
>> applying format settings + all controls to stream-on is normal.
>>
>> So that is the mental model I'm applying to uvc here, but that might
>> not be entirely applicable.
>>
>>> Laurent mentioned that some cameras missbehave if a lot of controls
>>> are set during probing. I hope that this approach does not trigger
>>> those, and if it does it would be easier to revert if we do the work
>>> in two steps.
>>
>> Ack, taking small steps sounds like a good plan.
>>
>> <snip>
>>
>>>> This should also make camera enumeration faster for apps, since
>>>> most apps / frameworks do the whole 3 levels nested loop for this
>>>> on startup, for which atm we go out to the hw, which now instead
>>>> will come from the fmts cache and thus will be much much faster,
>>>> so this should lead to a noticeable speedup for apps accessing UVC
>>>> cameras which would be another nice win.
>>>>
>>>> Downside is that the initial probe will take longer see we do
>>>> all the tryfmt-s there now. But I think that taking a bit longer
>>>> to probe while the machine is booting should not be an issue.
>>>
>>> How do you pretend to handle the controls? Do you plan to power-up the
>>> device during s_ctrl() or set them only during streamon()?
>>> If we power-up the device during s_ctrl we need to take care of the
>>> asynchronous controls (typically pan/tilt/zoom), The device must be
>>> powered until the control finishes, and the device might never reply
>>> control_done if the firmware is not properly implemented.
>>> If we set the controls only during streamon, we will break some
>>> usecases. There are some video conferencing equipment that do homing
>>> during streamoff. That will be a serious API breakage.
>>
>> How to handle controls is a good idea.
>>
>> Based on my sensor experience my initial idea was to just cache them
>> all. Basically make set_ctrl succeed but do not actually do anyhing
>> when the camera is not already powered on and then on stream-on call
>> __v4l2_ctrl_handler_setup() to get all current values applied.
>>
>> But as you indicate that will likely not work well with async controls,
>> although we already have this issue when using v4l2-ctl from the cmdline
>> on such a control and that seems to work fine.
> 
> -----
>> Just because we allow
>> the USB connection to sleep, does not mean that the camera cannot finish
>> doing applying the async control.
>>
> Not sure what you mean with this sentence. Could you explain it
> differently? Sorry
> 
>> But I can see how some cameras might not like this and having 2 different
>> paths for different controls also is undesirable.
>>
>> Combine that with what Laurent said about devices not liking it when
>> you set too much controls in a short time and I do think we need to
>> immediately apply ctrls.
>>
>> I see 2 ways of doing that:
>>
>> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
>> and then on set_ctrl do a pm_runtime_get_sync() +
>> pm_runtime_put_autosuspend() giving the camera 1 second to finish
>> applying the async ctrl (which might not be enough for e.g homing) +
>> also avoid doing suspend + resume all the time if multiple ctrls are send
> 
> What about 1.5:
> 
> during s_ctrl():
> usb_autopm_get_interface()
> if the control is UVC_CTRL_FLAG_ASYNCHRONOUS.
>        usb_autopm_get_interface()
> set the actual control in the hardware
> usb_autopm_put_interface()
> 
> during uvc_ctrl_status_event():
>    usb_autopm_put_interface()

How do we match this to the usb_autopm_get_interface()
call ? At a minimum we would need some counter to
track pending (not acked through status interrupt urb)
async control requests and only do the put() if that
counter >= 1 (and then decrease the counter).

We don't want to do unbalanced puts here in case of
buggy cameras sending unexpected / too many
ctrl status events.

> during close():
>    send all the missing usb_autopm_put_interface()

Except for my one remark this is an interesting
proposal.

Maybe also do a dev_warn() if there are missing
usb_autopm_put_interface() calls pending on close() ?

> This way:
> - we do not have an artificial delay that might not work for all the use cases
> - cameras with noncompliant async controls will have the same PM
> behaviour as now  (will be powered on until close() )
> 
> We do the same with the rest of the actions that require hardware access, like:
> https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
> 
> This way:
> - Apps that do not need to access the hardware, do not wake it up, and
> we do not break usecases.
> 
> Next steps will be:
>  - cache the formats
>  - move the actual set_ctrl to streamon... but if we can do that I
> would argue than we can move completely to the control framework.

Right I had forgotten that the UVC driver does not use the control
framework. I think moving to that would be a prerequisite for moving
the set_ctrl to stream_on.

Regards,

Hans

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

[Ricardo Ribalda]

Hi

On Mon, 25 Nov 2024 at 15:02, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Ricardo,
>
> On 25-Nov-24 2:39 PM, Ricardo Ribalda wrote:
> > On Mon, 25 Nov 2024 at 13:25, Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi Ricardo,
> >>
> >> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote:
> >>
> >> <snip>
> >>
> >>>> I have been discussing UVC power-management with Laurent, also
> >>>> related to power-consumption issues caused by libcamera's pipeline
> >>>> handler holding open the /dev/video# node as long as the camera
> >>>> manager object exists.
> >>
> >> <snip>
> >>
> >>>> Here is what I have in mind for this:
> >>>>
> >>>> 1. Assume that the results of trying a specific fmt do not change over time.
> >>>>
> >>>> 2. Only allow userspace to request fmts which match one of the enum-fmts ->
> >>>>    enum-frame-sizes -> enum-frame-rates tripplet results
> >>>>    (constrain what userspace requests to these)
> >>>>
> >>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
> >>>>    3 levels nested loop for this) on probe() and cache the results
> >>>>
> >>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain
> >>>>    the requested fmt to one from our cached fmts
> >>>>
> >>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get
> >>>>    what we expect based on the cache, and otherwise return -EIO.
> >>>
> >>> Can we start powering up the device during try/set fmt and then
> >>> implement the format caching as an improvement?
> >>
> >> Yes, actually looking at how complex this is when e.g. also taking
> >> controls into account I think that taking small steps is a good idea.
> >>
> >> I have lately mostly been working on sensor drivers where delaying
> >> applying format settings + all controls to stream-on is normal.
> >>
> >> So that is the mental model I'm applying to uvc here, but that might
> >> not be entirely applicable.
> >>
> >>> Laurent mentioned that some cameras missbehave if a lot of controls
> >>> are set during probing. I hope that this approach does not trigger
> >>> those, and if it does it would be easier to revert if we do the work
> >>> in two steps.
> >>
> >> Ack, taking small steps sounds like a good plan.
> >>
> >> <snip>
> >>
> >>>> This should also make camera enumeration faster for apps, since
> >>>> most apps / frameworks do the whole 3 levels nested loop for this
> >>>> on startup, for which atm we go out to the hw, which now instead
> >>>> will come from the fmts cache and thus will be much much faster,
> >>>> so this should lead to a noticeable speedup for apps accessing UVC
> >>>> cameras which would be another nice win.
> >>>>
> >>>> Downside is that the initial probe will take longer see we do
> >>>> all the tryfmt-s there now. But I think that taking a bit longer
> >>>> to probe while the machine is booting should not be an issue.
> >>>
> >>> How do you pretend to handle the controls? Do you plan to power-up the
> >>> device during s_ctrl() or set them only during streamon()?
> >>> If we power-up the device during s_ctrl we need to take care of the
> >>> asynchronous controls (typically pan/tilt/zoom), The device must be
> >>> powered until the control finishes, and the device might never reply
> >>> control_done if the firmware is not properly implemented.
> >>> If we set the controls only during streamon, we will break some
> >>> usecases. There are some video conferencing equipment that do homing
> >>> during streamoff. That will be a serious API breakage.
> >>
> >> How to handle controls is a good idea.
> >>
> >> Based on my sensor experience my initial idea was to just cache them
> >> all. Basically make set_ctrl succeed but do not actually do anyhing
> >> when the camera is not already powered on and then on stream-on call
> >> __v4l2_ctrl_handler_setup() to get all current values applied.
> >>
> >> But as you indicate that will likely not work well with async controls,
> >> although we already have this issue when using v4l2-ctl from the cmdline
> >> on such a control and that seems to work fine.
> >
> > -----
> >> Just because we allow
> >> the USB connection to sleep, does not mean that the camera cannot finish
> >> doing applying the async control.
> >>
> > Not sure what you mean with this sentence. Could you explain it
> > differently? Sorry
> >
> >> But I can see how some cameras might not like this and having 2 different
> >> paths for different controls also is undesirable.
> >>
> >> Combine that with what Laurent said about devices not liking it when
> >> you set too much controls in a short time and I do think we need to
> >> immediately apply ctrls.
> >>
> >> I see 2 ways of doing that:
> >>
> >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
> >> and then on set_ctrl do a pm_runtime_get_sync() +
> >> pm_runtime_put_autosuspend() giving the camera 1 second to finish
> >> applying the async ctrl (which might not be enough for e.g homing) +
> >> also avoid doing suspend + resume all the time if multiple ctrls are send
> >
> > What about 1.5:
> >
> > during s_ctrl():
> > usb_autopm_get_interface()
> > if the control is UVC_CTRL_FLAG_ASYNCHRONOUS.
> >        usb_autopm_get_interface()
> > set the actual control in the hardware
> > usb_autopm_put_interface()
> >
> > during uvc_ctrl_status_event():
> >    usb_autopm_put_interface()
>
> How do we match this to the usb_autopm_get_interface()
> call ? At a minimum we would need some counter to
> track pending (not acked through status interrupt urb)
> async control requests and only do the put() if that
> counter >= 1 (and then decrease the counter).
>
> We don't want to do unbalanced puts here in case of
> buggy cameras sending unexpected / too many
> ctrl status events.
>
> > during close():
> >    send all the missing usb_autopm_put_interface()
>
> Except for my one remark this is an interesting
> proposal.

I have just upload a patchset implementing this. I tried
v4l2-compliance and using the camera app.

I think it looks promissing

Shall we move the discussion there?

https://lore.kernel.org/linux-media/20241126-uvc-granpower-ng-v1-0-6312bf26549c@chromium.org/T/#t


>
> Maybe also do a dev_warn() if there are missing
> usb_autopm_put_interface() calls pending on close() ?
>
> > This way:
> > - we do not have an artificial delay that might not work for all the use cases
> > - cameras with noncompliant async controls will have the same PM
> > behaviour as now  (will be powered on until close() )
> >
> > We do the same with the rest of the actions that require hardware access, like:
> > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
> >
> > This way:
> > - Apps that do not need to access the hardware, do not wake it up, and
> > we do not break usecases.
> >
> > Next steps will be:
> >  - cache the formats
> >  - move the actual set_ctrl to streamon... but if we can do that I
> > would argue than we can move completely to the control framework.
>
> Right I had forgotten that the UVC driver does not use the control
> framework. I think moving to that would be a prerequisite for moving
> the set_ctrl to stream_on.
>
> Regards,
>
> Hans
>


--
Ricardo Ribalda

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

[Laurent Pinchart]

On Tue, Nov 26, 2024 at 05:22:20PM +0100, Ricardo Ribalda wrote:
> On Mon, 25 Nov 2024 at 15:02, Hans de Goede wrote:
> > On 25-Nov-24 2:39 PM, Ricardo Ribalda wrote:
> > > On Mon, 25 Nov 2024 at 13:25, Hans de Goede wrote:
> > >> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote:
> > >>
> > >> <snip>
> > >>
> > >>>> I have been discussing UVC power-management with Laurent, also
> > >>>> related to power-consumption issues caused by libcamera's pipeline
> > >>>> handler holding open the /dev/video# node as long as the camera
> > >>>> manager object exists.
> > >>
> > >> <snip>
> > >>
> > >>>> Here is what I have in mind for this:
> > >>>>
> > >>>> 1. Assume that the results of trying a specific fmt do not change over time.
> > >>>>
> > >>>> 2. Only allow userspace to request fmts which match one of the enum-fmts ->
> > >>>>    enum-frame-sizes -> enum-frame-rates tripplet results
> > >>>>    (constrain what userspace requests to these)
> > >>>>
> > >>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
> > >>>>    3 levels nested loop for this) on probe() and cache the results
> > >>>>
> > >>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain
> > >>>>    the requested fmt to one from our cached fmts
> > >>>>
> > >>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get
> > >>>>    what we expect based on the cache, and otherwise return -EIO.
> > >>>
> > >>> Can we start powering up the device during try/set fmt and then
> > >>> implement the format caching as an improvement?
> > >>
> > >> Yes, actually looking at how complex this is when e.g. also taking
> > >> controls into account I think that taking small steps is a good idea.
> > >>
> > >> I have lately mostly been working on sensor drivers where delaying
> > >> applying format settings + all controls to stream-on is normal.
> > >>
> > >> So that is the mental model I'm applying to uvc here, but that might
> > >> not be entirely applicable.
> > >>
> > >>> Laurent mentioned that some cameras missbehave if a lot of controls
> > >>> are set during probing. I hope that this approach does not trigger
> > >>> those, and if it does it would be easier to revert if we do the work
> > >>> in two steps.
> > >>
> > >> Ack, taking small steps sounds like a good plan.
> > >>
> > >> <snip>
> > >>
> > >>>> This should also make camera enumeration faster for apps, since
> > >>>> most apps / frameworks do the whole 3 levels nested loop for this
> > >>>> on startup, for which atm we go out to the hw, which now instead
> > >>>> will come from the fmts cache and thus will be much much faster,
> > >>>> so this should lead to a noticeable speedup for apps accessing UVC
> > >>>> cameras which would be another nice win.
> > >>>>
> > >>>> Downside is that the initial probe will take longer see we do
> > >>>> all the tryfmt-s there now. But I think that taking a bit longer
> > >>>> to probe while the machine is booting should not be an issue.
> > >>>
> > >>> How do you pretend to handle the controls? Do you plan to power-up the
> > >>> device during s_ctrl() or set them only during streamon()?
> > >>> If we power-up the device during s_ctrl we need to take care of the
> > >>> asynchronous controls (typically pan/tilt/zoom), The device must be
> > >>> powered until the control finishes, and the device might never reply
> > >>> control_done if the firmware is not properly implemented.
> > >>> If we set the controls only during streamon, we will break some
> > >>> usecases. There are some video conferencing equipment that do homing
> > >>> during streamoff. That will be a serious API breakage.
> > >>
> > >> How to handle controls is a good idea.
> > >>
> > >> Based on my sensor experience my initial idea was to just cache them
> > >> all. Basically make set_ctrl succeed but do not actually do anyhing
> > >> when the camera is not already powered on and then on stream-on call
> > >> __v4l2_ctrl_handler_setup() to get all current values applied.
> > >>
> > >> But as you indicate that will likely not work well with async controls,
> > >> although we already have this issue when using v4l2-ctl from the cmdline
> > >> on such a control and that seems to work fine.
> > >
> > > -----
> > >> Just because we allow
> > >> the USB connection to sleep, does not mean that the camera cannot finish
> > >> doing applying the async control.
> > >>
> > > Not sure what you mean with this sentence. Could you explain it
> > > differently? Sorry
> > >
> > >> But I can see how some cameras might not like this and having 2 different
> > >> paths for different controls also is undesirable.
> > >>
> > >> Combine that with what Laurent said about devices not liking it when
> > >> you set too much controls in a short time and I do think we need to
> > >> immediately apply ctrls.
> > >>
> > >> I see 2 ways of doing that:
> > >>
> > >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
> > >> and then on set_ctrl do a pm_runtime_get_sync() +
> > >> pm_runtime_put_autosuspend() giving the camera 1 second to finish
> > >> applying the async ctrl (which might not be enough for e.g homing) +
> > >> also avoid doing suspend + resume all the time if multiple ctrls are send
> > >
> > > What about 1.5:
> > >
> > > during s_ctrl():
> > > usb_autopm_get_interface()
> > > if the control is UVC_CTRL_FLAG_ASYNCHRONOUS.
> > >        usb_autopm_get_interface()
> > > set the actual control in the hardware
> > > usb_autopm_put_interface()
> > >
> > > during uvc_ctrl_status_event():
> > >    usb_autopm_put_interface()
> >
> > How do we match this to the usb_autopm_get_interface()
> > call ? At a minimum we would need some counter to
> > track pending (not acked through status interrupt urb)
> > async control requests and only do the put() if that
> > counter >= 1 (and then decrease the counter).
> >
> > We don't want to do unbalanced puts here in case of
> > buggy cameras sending unexpected / too many
> > ctrl status events.

We would need a counter indeed, which is a big red flag of bad
engineering. It will be fragile at best.

> > > during close():
> > >    send all the missing usb_autopm_put_interface()
> >
> > Except for my one remark this is an interesting
> > proposal.
> 
> I have just upload a patchset implementing this. I tried
> v4l2-compliance and using the camera app.
> 
> I think it looks promissing
> 
> Shall we move the discussion there?
> 
> https://lore.kernel.org/linux-media/20241126-uvc-granpower-ng-v1-0-6312bf26549c@chromium.org/T/#t

You're sending too many patch series too quickly, even before we can
come to an agreement on any item being discussed. Experimenting is
helpful, but if we keep moving the discussion from one series to the
next, that won't work. Let's keep it here, and focus on one problem at a
time, or the end result will be slower merging of the patches.

> > Maybe also do a dev_warn() if there are missing
> > usb_autopm_put_interface() calls pending on close() ?
> >
> > > This way:
> > > - we do not have an artificial delay that might not work for all the use cases
> > > - cameras with noncompliant async controls will have the same PM
> > > behaviour as now  (will be powered on until close() )
> > >
> > > We do the same with the rest of the actions that require hardware access, like:
> > > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
> > >
> > > This way:
> > > - Apps that do not need to access the hardware, do not wake it up, and
> > > we do not break usecases.
> > >
> > > Next steps will be:
> > >  - cache the formats
> > >  - move the actual set_ctrl to streamon... but if we can do that I
> > > would argue than we can move completely to the control framework.
> >
> > Right I had forgotten that the UVC driver does not use the control
> > framework. I think moving to that would be a prerequisite for moving
> > the set_ctrl to stream_on.

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

On Tue, 26 Nov 2024 at 18:18, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Nov 26, 2024 at 05:22:20PM +0100, Ricardo Ribalda wrote:
> > On Mon, 25 Nov 2024 at 15:02, Hans de Goede wrote:
> > > On 25-Nov-24 2:39 PM, Ricardo Ribalda wrote:
> > > > On Mon, 25 Nov 2024 at 13:25, Hans de Goede wrote:
> > > >> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote:
> > > >>
> > > >> <snip>
> > > >>
> > > >>>> I have been discussing UVC power-management with Laurent, also
> > > >>>> related to power-consumption issues caused by libcamera's pipeline
> > > >>>> handler holding open the /dev/video# node as long as the camera
> > > >>>> manager object exists.
> > > >>
> > > >> <snip>
> > > >>
> > > >>>> Here is what I have in mind for this:
> > > >>>>
> > > >>>> 1. Assume that the results of trying a specific fmt do not change over time.
> > > >>>>
> > > >>>> 2. Only allow userspace to request fmts which match one of the enum-fmts ->
> > > >>>>    enum-frame-sizes -> enum-frame-rates tripplet results
> > > >>>>    (constrain what userspace requests to these)
> > > >>>>
> > > >>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
> > > >>>>    3 levels nested loop for this) on probe() and cache the results
> > > >>>>
> > > >>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain
> > > >>>>    the requested fmt to one from our cached fmts
> > > >>>>
> > > >>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get
> > > >>>>    what we expect based on the cache, and otherwise return -EIO.
> > > >>>
> > > >>> Can we start powering up the device during try/set fmt and then
> > > >>> implement the format caching as an improvement?
> > > >>
> > > >> Yes, actually looking at how complex this is when e.g. also taking
> > > >> controls into account I think that taking small steps is a good idea.
> > > >>
> > > >> I have lately mostly been working on sensor drivers where delaying
> > > >> applying format settings + all controls to stream-on is normal.
> > > >>
> > > >> So that is the mental model I'm applying to uvc here, but that might
> > > >> not be entirely applicable.
> > > >>
> > > >>> Laurent mentioned that some cameras missbehave if a lot of controls
> > > >>> are set during probing. I hope that this approach does not trigger
> > > >>> those, and if it does it would be easier to revert if we do the work
> > > >>> in two steps.
> > > >>
> > > >> Ack, taking small steps sounds like a good plan.
> > > >>
> > > >> <snip>
> > > >>
> > > >>>> This should also make camera enumeration faster for apps, since
> > > >>>> most apps / frameworks do the whole 3 levels nested loop for this
> > > >>>> on startup, for which atm we go out to the hw, which now instead
> > > >>>> will come from the fmts cache and thus will be much much faster,
> > > >>>> so this should lead to a noticeable speedup for apps accessing UVC
> > > >>>> cameras which would be another nice win.
> > > >>>>
> > > >>>> Downside is that the initial probe will take longer see we do
> > > >>>> all the tryfmt-s there now. But I think that taking a bit longer
> > > >>>> to probe while the machine is booting should not be an issue.
> > > >>>
> > > >>> How do you pretend to handle the controls? Do you plan to power-up the
> > > >>> device during s_ctrl() or set them only during streamon()?
> > > >>> If we power-up the device during s_ctrl we need to take care of the
> > > >>> asynchronous controls (typically pan/tilt/zoom), The device must be
> > > >>> powered until the control finishes, and the device might never reply
> > > >>> control_done if the firmware is not properly implemented.
> > > >>> If we set the controls only during streamon, we will break some
> > > >>> usecases. There are some video conferencing equipment that do homing
> > > >>> during streamoff. That will be a serious API breakage.
> > > >>
> > > >> How to handle controls is a good idea.
> > > >>
> > > >> Based on my sensor experience my initial idea was to just cache them
> > > >> all. Basically make set_ctrl succeed but do not actually do anyhing
> > > >> when the camera is not already powered on and then on stream-on call
> > > >> __v4l2_ctrl_handler_setup() to get all current values applied.
> > > >>
> > > >> But as you indicate that will likely not work well with async controls,
> > > >> although we already have this issue when using v4l2-ctl from the cmdline
> > > >> on such a control and that seems to work fine.
> > > >
> > > > -----
> > > >> Just because we allow
> > > >> the USB connection to sleep, does not mean that the camera cannot finish
> > > >> doing applying the async control.
> > > >>
> > > > Not sure what you mean with this sentence. Could you explain it
> > > > differently? Sorry
> > > >
> > > >> But I can see how some cameras might not like this and having 2 different
> > > >> paths for different controls also is undesirable.
> > > >>
> > > >> Combine that with what Laurent said about devices not liking it when
> > > >> you set too much controls in a short time and I do think we need to
> > > >> immediately apply ctrls.
> > > >>
> > > >> I see 2 ways of doing that:
> > > >>
> > > >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
> > > >> and then on set_ctrl do a pm_runtime_get_sync() +
> > > >> pm_runtime_put_autosuspend() giving the camera 1 second to finish
> > > >> applying the async ctrl (which might not be enough for e.g homing) +
> > > >> also avoid doing suspend + resume all the time if multiple ctrls are send
> > > >
> > > > What about 1.5:
> > > >
> > > > during s_ctrl():
> > > > usb_autopm_get_interface()
> > > > if the control is UVC_CTRL_FLAG_ASYNCHRONOUS.
> > > >        usb_autopm_get_interface()
> > > > set the actual control in the hardware
> > > > usb_autopm_put_interface()
> > > >
> > > > during uvc_ctrl_status_event():
> > > >    usb_autopm_put_interface()
> > >
> > > How do we match this to the usb_autopm_get_interface()
> > > call ? At a minimum we would need some counter to
> > > track pending (not acked through status interrupt urb)
> > > async control requests and only do the put() if that
> > > counter >= 1 (and then decrease the counter).
> > >
> > > We don't want to do unbalanced puts here in case of
> > > buggy cameras sending unexpected / too many
> > > ctrl status events.
>
> We would need a counter indeed, which is a big red flag of bad
> engineering. It will be fragile at best.
>
> > > > during close():
> > > >    send all the missing usb_autopm_put_interface()
> > >
> > > Except for my one remark this is an interesting
> > > proposal.
> >
> > I have just upload a patchset implementing this. I tried
> > v4l2-compliance and using the camera app.
> >
> > I think it looks promissing
> >
> > Shall we move the discussion there?
> >
> > https://lore.kernel.org/linux-media/20241126-uvc-granpower-ng-v1-0-6312bf26549c@chromium.org/T/#t
>
> You're sending too many patch series too quickly, even before we can
> come to an agreement on any item being discussed. Experimenting is
> helpful, but if we keep moving the discussion from one series to the
> next, that won't work. Let's keep it here, and focus on one problem at a
> time, or the end result will be slower merging of the patches.

I'd argue that it is better to discuss power management in a series called
"uvcvideo: Implement Granular Power Saving"
than in another called
"media: uvcvideo: Implement the Privacy GPIO as a subdevice"

Those two problems are orthogonal.

I also believe that Hans agreed that that approach was worth exploring....

>
> > > Maybe also do a dev_warn() if there are missing
> > > usb_autopm_put_interface() calls pending on close() ?
> > >
> > > > This way:
> > > > - we do not have an artificial delay that might not work for all the use cases
> > > > - cameras with noncompliant async controls will have the same PM
> > > > behaviour as now  (will be powered on until close() )
> > > >
> > > > We do the same with the rest of the actions that require hardware access, like:
> > > > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
> > > >
> > > > This way:
> > > > - Apps that do not need to access the hardware, do not wake it up, and
> > > > we do not break usecases.
> > > >
> > > > Next steps will be:
> > > >  - cache the formats
> > > >  - move the actual set_ctrl to streamon... but if we can do that I
> > > > would argue than we can move completely to the control framework.
> > >
> > > Right I had forgotten that the UVC driver does not use the control
> > > framework. I think moving to that would be a prerequisite for moving
> > > the set_ctrl to stream_on.
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

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

[Laurent Pinchart]

On Sat, Nov 09, 2024 at 04:37:10PM +0100, Hans de Goede wrote:
> Hi Ricardo,
> 
> FYI / some background: I have been asked to start helping /
> co-maintaining UVC with Laurent. I'll send out a patch adding
> myself as UVC maintainer soon.
> 
> On 8-Nov-24 9:25 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 two 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.
> > 
> > We tried to fix the power-up issues implementing "granular power
> > saving" but it has been more complicated than anticipated....
> 
> I have been discussing UVC power-management with Laurent, also
> related to power-consumption issues caused by libcamera's pipeline
> handler holding open the /dev/video# node as long as the camera
> manager object exists.
> 
> For now we have fixed this with some relatively small changes to
> libcamera's uvcvideo pipeline handler, but that is really meant
> as an interim solution and as this privacy control series shows
> the power-management issues are real.
> 
> Combined with Mauro's remarks about how this is an userspace ABI break (1)
> I think we should maybe first take another look at the powermanagement
> issues in general rather then moving forward with this series.
> 
> My apologies for this, I realize how annoying it can be when you are
> working on a patch series to fix a specific issue and a reviewer
> moves the goal-posts like this. But I do really think that just fixing
> the generic power-management issues would be better and I also think
> that this should be feasible / not too hard.
> 
> Here is what I have in mind for this:
> 
> 1. Assume that the results of trying a specific fmt do not change over time.
> 
> 2. Only allow userspace to request fmts which match one of the enum-fmts ->
>    enum-frame-sizes -> enum-frame-rates tripplet results
>    (constrain what userspace requests to these)
> 
> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
>    3 levels nested loop for this) on probe() and cache the results

This is possibly problematic. If I recall correctly (it was a very long
time ago), the gstreamer v4l2src element used to do this, and it
resulted in very large delays (> 10s with some devices), and possibly in
some devices crashing as well.

> 4. Make try_fmt / set_fmt not poweron the device but instead constrain
>    the requested fmt to one from our cached fmts
> 
> 5. On stream-on do the actual power-on + set-fmt + verify that we get
>    what we expect based on the cache, and otherwise return -EIO.
> 
> I think that should sort the issue, assuming that 1. above holds true.
> 
> One downside is that this stops UVC button presses from working when
> not streaming. But userspace will typically only open the /dev/video#
> node if it plans to stream anyways so there should not be much of
> a difference wrt button press behavior.
> 
> This should also make camera enumeration faster for apps, since
> most apps / frameworks do the whole 3 levels nested loop for this
> on startup, for which atm we go out to the hw, which now instead
> will come from the fmts cache and thus will be much much faster,
> so this should lead to a noticeable speedup for apps accessing UVC
> cameras which would be another nice win.
> 
> Downside is that the initial probe will take longer see we do
> all the tryfmt-s there now. But I think that taking a bit longer
> to probe while the machine is booting should not be an issue.

It depends on how much "a bit" is. I don't think a >10s delay would be
acceptable.

> 1) Which is technically correct, but FWIW I agree with you that I think
> most userspace consumers will not care
> 
> > Last year, we proposed a patchset to implement the privacy gpio as a
> > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/
> > 
> > I think it is a pretty clean solution and makes sense to use a
> > subdevice for something that is a sub device of the camera :).
> > 
> > This is an attempt to continue with that approach.
> > 
> > Tested on gimble:
> > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0
> > Driver Info:
> >         Driver version   : 6.6.56
> >         Capabilities     : 0x00000000
> > Media Driver Info:
> >         Driver name      : uvcvideo
> >         Model            : HP 5M Camera: HP 5M Camera
> >         Serial           : 0001
> >         Bus info         : usb-0000:00:14.0-6
> >         Media version    : 6.6.56
> >         Hardware revision: 0x00009601 (38401)
> >         Driver version   : 6.6.56
> > Interface Info:
> >         ID               : 0x0300001d
> >         Type             : V4L Sub-Device
> > Entity Info:
> >         ID               : 0x00000013 (19)
> >         Name             : GPIO
> >         Function         : Unknown sub-device (00020006)
> > 
> > Camera Controls
> > 
> >                         privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only, volatile
> > 
> > gimble-rev3 ~ # media-ctl  -p
> > Media controller API version 6.6.56
> > 
> > Media device information
> > ------------------------
> > driver          uvcvideo
> > model           HP 5M Camera: HP 5M Camera
> > serial          0001
> > bus info        usb-0000:00:14.0-6
> > hw revision     0x9601
> > driver version  6.6.56
> > 
> > Device topology
> > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link)
> >             type Node subtype V4L flags 1
> >             device node name /dev/video0
> >         pad0: Sink
> >                 <- "Extension 8":1 [ENABLED,IMMUTABLE]
> > 
> > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link)
> >             type Node subtype V4L flags 0
> >             device node name /dev/video1
> > 
> > - entity 8: Extension 8 (2 pads, 2 links, 0 routes)
> >             type V4L2 subdev subtype Unknown flags 0
> >         pad0: Sink
> >                 <- "Extension 4":1 [ENABLED,IMMUTABLE]
> >         pad1: Source
> >                 -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 11: Extension 4 (2 pads, 2 links, 0 routes)
> >              type V4L2 subdev subtype Unknown flags 0
> >         pad0: Sink
> >                 <- "Processing 2":1 [ENABLED,IMMUTABLE]
> >         pad1: Source
> >                 -> "Extension 8":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 14: Processing 2 (2 pads, 2 links, 0 routes)
> >              type V4L2 subdev subtype Unknown flags 0
> >         pad0: Sink
> >                 <- "Camera 1":0 [ENABLED,IMMUTABLE]
> >         pad1: Source
> >                 -> "Extension 4":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 17: Camera 1 (1 pad, 1 link, 0 routes)
> >              type V4L2 subdev subtype Sensor flags 0
> >         pad0: Source
> >                 -> "Processing 2":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 19: GPIO (0 pad, 0 link, 0 routes)
> >              type V4L2 subdev subtype Decoder flags 0
> >              device node name /dev/v4l-subdev0
> > 
> > Signed-off-by: Ricardo Ribalda <ribalda@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 (5):
> >       media: uvcvideo: Factor out gpio functions to its own file
> >       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
> > 
> > Yunke Cao (1):
> >       media: uvcvideo: Re-implement privacy GPIO as a separate subdevice
> > 
> >  .../userspace-api/media/mediactl/media-types.rst   |   4 +
> >  drivers/media/usb/uvc/Makefile                     |   3 +-
> >  drivers/media/usb/uvc/uvc_ctrl.c                   |  40 +----
> >  drivers/media/usb/uvc/uvc_driver.c                 | 123 +-------------
> >  drivers/media/usb/uvc/uvc_entity.c                 |  20 ++-
> >  drivers/media/usb/uvc/uvc_gpio.c                   | 187 +++++++++++++++++++++
> >  drivers/media/usb/uvc/uvc_video.c                  |   4 +
> >  drivers/media/usb/uvc/uvcvideo.h                   |  34 ++--
> >  drivers/media/v4l2-core/v4l2-async.c               |   3 +-
> >  include/uapi/linux/media.h                         |   1 +
> >  10 files changed, 252 insertions(+), 167 deletions(-)
> > ---
> > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525
> > change-id: 20241030-uvc-subdev-89f4467a00b5

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Ricardo, Et al.,

On 8-Nov-24 9:25 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 two 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.

Thinking more about this I think we need to start with looking at the userspace
API for privacy controls, define how we want that to look and then go from
there.

The reason I'm writing this is because due to my work in drivers/platform/x86
(pdx86) on EC / ACPI / WMI drivers for non chromebooks I am aware of at least
4 different methods camera on/off (aka privacy) toggles are being reported
to userspace at the moment. Adding a v4l2-ctrl on a subdev instead of directly
on /dev/video# would be adding a 5th method which seems highly undesirable.

Instead I would like to first focus on fixing these userspace API
inconsistencies agreeing on a single API we want to use everywhere
going forward. We don't need to fix all drivers at once, but IMHO we
should agree on what the API should look like and document that and
any future drivers implementing camera privacy control related code
then must use the new API.

Lets start with the 3 APIs I'm currently aware of:

1. uvcvideo driver exporting V4L2_CID_PRIVACY on /dev/video#
uvcvideo seems to be the only user of this CID (i)

2. pdx86 drivers exporting an input evdev with EV_SW,
SW_CAMERA_LENS_COVER. This is somewhat of a special case
for some Dell laptops with an electro-mechanical shutter
operated by the EC. But this is not also used by
hp-wmi.c where it does not necessarily indicate the
status of a mechanical cover, but also possibly simply
disconnecting the camera from the USB bus.

3. pdx86 drivers exporting an input evdev with EV_KEY,
KEY_CAMERA_ACCESS_ENABLE, KEY_CAMERA_ACCESS_DISABLE
These KEY codes are based on offical the HUTRR72 HID/HUT
extension and as such may also be send by USB/I2C/BT HID
devices.

The only user outside of hid-input.c is the recently added
drivers/platform/x86/lenovo-wmi-camera.c driver and I'm
wondering if that should not use SW_CAMERA_LENS_COVER
instead. I'll ask the driver author about how this 

4. pdx86 drivers exporting an input evdev with EV_KEY,
KEY_CAMERA. Note this 4th method lacks information on if
the camera was enabled or disabled. In many cases this
is send to indicate that the EC has either dropped
a UVC camera of the bus, or added it to the bus.
Ideally we would have some helper checking for internal
UVC camera presence and turn this into 2 or 3.

TL;DR: it a mess.

Circling back to this patch-set, note how 3 of the 4
currently in use variants today use in input evdev.

I think that using an input evdev (shared with the
snapshot button if present) will give us a nice out for
the power-management issue with the V4L2_CID_PRIVACY,
while at the same time giving a nice opportunity to
standardize on a single userspace API.

My proposal would be to standardize on SW_CAMERA_LENS_COVER
I realize that the GPIO does not always indicate a lens
cover, but the resulting black frames are the same result
as if there were a lens cover and looking at:

https://support.hp.com/ie-en/document/ish_3960099-3335046-16

and then the second picture when expanding "Locate and use
the webcam privacy switch" that does look like it may be
an actual cover which reports back its state through a GPIO.

The reason why I'm not in favor of using
KEY_CAMERA_ACCESS_ENABLE + KEY_CAMERA_ACCESS_DISABLE is that
looking at the HUTRR72 it talks about:
"Enables programmatic access to camera device"
which suggests that it is a request to the OS / desktop-
environment to block camera access at the software level,
rather then reporting back that a hw-level block is in place.

And since these may be used by any HID device we are not of
control in how these will be used.

Ricardo, what do you think of instead of using a v4l-subdev,
using an input evdev (shared with the existing one) reporting
SW_CAMERA_LENS_COVER ?  The v4l-subdev approach will need
userspace changes anyways and if we are going to make userspace
changes we might as well use the best API available.

One downside of going the evdev route is that it is a bit
harder for userspace to map the evdev to a camera:

1. For the various WMI interfaces this already is impossible,
and just to show a notification it is not necessary (using
an external cam will make things weird though).

2. For UVC cameras mapping the evdev to the /dev/video#
node can still be done by looking if they share a parent
USB interface. This is e.g. already done in apps like
xawtv looking at the PCI parent to pair up /dev/video#
for video capture with the ALSA interface exposed for
sound by bttv cards.

3. We can maybe do something at the media-controller
level to help userspace linking a camera to its evdev node.
This would also be helpful for the existing WMI interfaces.

Regards,

Hans



i) With the exception of drivers/media/pci/intel/ivsc/mei_csi.c
which has a V4L2_CID_PRIVACY control which always reads 0, so
I guess we can / should probably drop that.



p.s.

I do plan to also get back to you on the actually powermanagement
discussion. But only so many hours in a day, so it will probably
be a couple of days.

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

[Hans de Goede]

p.s.

On 11-Nov-24 1:59 PM, Hans de Goede wrote:
> Hi Ricardo, Et al.,
> 
> On 8-Nov-24 9:25 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 two 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.
> 
> Thinking more about this I think we need to start with looking at the userspace
> API for privacy controls, define how we want that to look and then go from
> there.
> 
> The reason I'm writing this is because due to my work in drivers/platform/x86
> (pdx86) on EC / ACPI / WMI drivers for non chromebooks I am aware of at least
> 4 different methods camera on/off (aka privacy) toggles are being reported
> to userspace at the moment. Adding a v4l2-ctrl on a subdev instead of directly
> on /dev/video# would be adding a 5th method which seems highly undesirable.
> 
> Instead I would like to first focus on fixing these userspace API
> inconsistencies agreeing on a single API we want to use everywhere
> going forward. We don't need to fix all drivers at once, but IMHO we
> should agree on what the API should look like and document that and
> any future drivers implementing camera privacy control related code
> then must use the new API.
> 
> Lets start with the 3 APIs I'm currently aware of:
> 
> 1. uvcvideo driver exporting V4L2_CID_PRIVACY on /dev/video#
> uvcvideo seems to be the only user of this CID (i)
> 
> 2. pdx86 drivers exporting an input evdev with EV_SW,
> SW_CAMERA_LENS_COVER. This is somewhat of a special case
> for some Dell laptops with an electro-mechanical shutter
> operated by the EC. But this is not also used by
> hp-wmi.c where it does not necessarily indicate the

s/not/now/

> status of a mechanical cover, but also possibly simply
> disconnecting the camera from the USB bus.
> 
> 3. pdx86 drivers exporting an input evdev with EV_KEY,
> KEY_CAMERA_ACCESS_ENABLE, KEY_CAMERA_ACCESS_DISABLE
> These KEY codes are based on offical the HUTRR72 HID/HUT
> extension and as such may also be send by USB/I2C/BT HID
> devices.
> 
> The only user outside of hid-input.c is the recently added
> drivers/platform/x86/lenovo-wmi-camera.c driver and I'm
> wondering if that should not use SW_CAMERA_LENS_COVER
> instead. I'll ask the driver author about how this 

I have send an email out about possibly switching this to
SW_CAMERA_LENS_COVER :

https://lore.kernel.org/platform-driver-x86/5666914c-e8c2-481d-8fdf-aff82865c228@redhat.com/

Regards,

Hans



> 4. pdx86 drivers exporting an input evdev with EV_KEY,
> KEY_CAMERA. Note this 4th method lacks information on if
> the camera was enabled or disabled. In many cases this
> is send to indicate that the EC has either dropped
> a UVC camera of the bus, or added it to the bus.
> Ideally we would have some helper checking for internal
> UVC camera presence and turn this into 2 or 3.
> 
> TL;DR: it a mess.
> 
> Circling back to this patch-set, note how 3 of the 4
> currently in use variants today use in input evdev.
> 
> I think that using an input evdev (shared with the
> snapshot button if present) will give us a nice out for
> the power-management issue with the V4L2_CID_PRIVACY,
> while at the same time giving a nice opportunity to
> standardize on a single userspace API.
> 
> My proposal would be to standardize on SW_CAMERA_LENS_COVER
> I realize that the GPIO does not always indicate a lens
> cover, but the resulting black frames are the same result
> as if there were a lens cover and looking at:
> 
> https://support.hp.com/ie-en/document/ish_3960099-3335046-16
> 
> and then the second picture when expanding "Locate and use
> the webcam privacy switch" that does look like it may be
> an actual cover which reports back its state through a GPIO.
> 
> The reason why I'm not in favor of using
> KEY_CAMERA_ACCESS_ENABLE + KEY_CAMERA_ACCESS_DISABLE is that
> looking at the HUTRR72 it talks about:
> "Enables programmatic access to camera device"
> which suggests that it is a request to the OS / desktop-
> environment to block camera access at the software level,
> rather then reporting back that a hw-level block is in place.
> 
> And since these may be used by any HID device we are not of
> control in how these will be used.
> 
> Ricardo, what do you think of instead of using a v4l-subdev,
> using an input evdev (shared with the existing one) reporting
> SW_CAMERA_LENS_COVER ?  The v4l-subdev approach will need
> userspace changes anyways and if we are going to make userspace
> changes we might as well use the best API available.
> 
> One downside of going the evdev route is that it is a bit
> harder for userspace to map the evdev to a camera:
> 
> 1. For the various WMI interfaces this already is impossible,
> and just to show a notification it is not necessary (using
> an external cam will make things weird though).
> 
> 2. For UVC cameras mapping the evdev to the /dev/video#
> node can still be done by looking if they share a parent
> USB interface. This is e.g. already done in apps like
> xawtv looking at the PCI parent to pair up /dev/video#
> for video capture with the ALSA interface exposed for
> sound by bttv cards.
> 
> 3. We can maybe do something at the media-controller
> level to help userspace linking a camera to its evdev node.
> This would also be helpful for the existing WMI interfaces.
> 
> Regards,
> 
> Hans
> 
> 
> 
> i) With the exception of drivers/media/pci/intel/ivsc/mei_csi.c
> which has a V4L2_CID_PRIVACY control which always reads 0, so
> I guess we can / should probably drop that.
> 
> 
> 
> p.s.
> 
> I do plan to also get back to you on the actually powermanagement
> discussion. But only so many hours in a day, so it will probably
> be a couple of days.
> 
> 

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

[Ricardo Ribalda]

Hi Hans

On Mon, 11 Nov 2024 at 13:59, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Ricardo, Et al.,
>
> On 8-Nov-24 9:25 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 two 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.
>
> Thinking more about this I think we need to start with looking at the userspace
> API for privacy controls, define how we want that to look and then go from
> there.
>
> The reason I'm writing this is because due to my work in drivers/platform/x86
> (pdx86) on EC / ACPI / WMI drivers for non chromebooks I am aware of at least
> 4 different methods camera on/off (aka privacy) toggles are being reported
> to userspace at the moment. Adding a v4l2-ctrl on a subdev instead of directly
> on /dev/video# would be adding a 5th method which seems highly undesirable.
>
> Instead I would like to first focus on fixing these userspace API
> inconsistencies agreeing on a single API we want to use everywhere
> going forward. We don't need to fix all drivers at once, but IMHO we
> should agree on what the API should look like and document that and
> any future drivers implementing camera privacy control related code
> then must use the new API.
>
> Lets start with the 3 APIs I'm currently aware of:
>
> 1. uvcvideo driver exporting V4L2_CID_PRIVACY on /dev/video#
> uvcvideo seems to be the only user of this CID (i)
>
> 2. pdx86 drivers exporting an input evdev with EV_SW,
> SW_CAMERA_LENS_COVER. This is somewhat of a special case
> for some Dell laptops with an electro-mechanical shutter
> operated by the EC. But this is not also used by
> hp-wmi.c where it does not necessarily indicate the
> status of a mechanical cover, but also possibly simply
> disconnecting the camera from the USB bus.
>
> 3. pdx86 drivers exporting an input evdev with EV_KEY,
> KEY_CAMERA_ACCESS_ENABLE, KEY_CAMERA_ACCESS_DISABLE
> These KEY codes are based on offical the HUTRR72 HID/HUT
> extension and as such may also be send by USB/I2C/BT HID
> devices.
>
> The only user outside of hid-input.c is the recently added
> drivers/platform/x86/lenovo-wmi-camera.c driver and I'm
> wondering if that should not use SW_CAMERA_LENS_COVER
> instead. I'll ask the driver author about how this
>
> 4. pdx86 drivers exporting an input evdev with EV_KEY,
> KEY_CAMERA. Note this 4th method lacks information on if
> the camera was enabled or disabled. In many cases this
> is send to indicate that the EC has either dropped
> a UVC camera of the bus, or added it to the bus.
> Ideally we would have some helper checking for internal
> UVC camera presence and turn this into 2 or 3.
>
> TL;DR: it a mess.
>
> Circling back to this patch-set, note how 3 of the 4
> currently in use variants today use in input evdev.
>
> I think that using an input evdev (shared with the
> snapshot button if present) will give us a nice out for
> the power-management issue with the V4L2_CID_PRIVACY,
> while at the same time giving a nice opportunity to
> standardize on a single userspace API.
>
> My proposal would be to standardize on SW_CAMERA_LENS_COVER
> I realize that the GPIO does not always indicate a lens
> cover, but the resulting black frames are the same result
> as if there were a lens cover and looking at:
>
> https://support.hp.com/ie-en/document/ish_3960099-3335046-16
>
> and then the second picture when expanding "Locate and use
> the webcam privacy switch" that does look like it may be
> an actual cover which reports back its state through a GPIO.
>
> The reason why I'm not in favor of using
> KEY_CAMERA_ACCESS_ENABLE + KEY_CAMERA_ACCESS_DISABLE is that
> looking at the HUTRR72 it talks about:
> "Enables programmatic access to camera device"
> which suggests that it is a request to the OS / desktop-
> environment to block camera access at the software level,
> rather then reporting back that a hw-level block is in place.
>
> And since these may be used by any HID device we are not of
> control in how these will be used.
>
> Ricardo, what do you think of instead of using a v4l-subdev,
> using an input evdev (shared with the existing one) reporting
> SW_CAMERA_LENS_COVER ?  The v4l-subdev approach will need
> userspace changes anyways and if we are going to make userspace
> changes we might as well use the best API available.

I just sent a patchset using SW_CAMERA_LENS_COVER

I guess the internal uvc privacy (UVC_CT_PRIVACY_CONTROL) shall NOT be
converted to evdev:
- If we do so, we cannot differentiate external gpio and internal, for
devices that have both
- There is no warranty that we will get a uvc_event when the control
changes, so we would have to constantly poll the device

Regards!



>
> One downside of going the evdev route is that it is a bit
> harder for userspace to map the evdev to a camera:
>
> 1. For the various WMI interfaces this already is impossible,
> and just to show a notification it is not necessary (using
> an external cam will make things weird though).
>
> 2. For UVC cameras mapping the evdev to the /dev/video#
> node can still be done by looking if they share a parent
> USB interface. This is e.g. already done in apps like
> xawtv looking at the PCI parent to pair up /dev/video#
> for video capture with the ALSA interface exposed for
> sound by bttv cards.
>
> 3. We can maybe do something at the media-controller
> level to help userspace linking a camera to its evdev node.
> This would also be helpful for the existing WMI interfaces.
>
> Regards,
>
> Hans
>
>
>
> i) With the exception of drivers/media/pci/intel/ivsc/mei_csi.c
> which has a V4L2_CID_PRIVACY control which always reads 0, so
> I guess we can / should probably drop that.
>
>
>
> p.s.
>
> I do plan to also get back to you on the actually powermanagement
> discussion. But only so many hours in a day, so it will probably
> be a couple of days.
>
>


--
Ricardo Ribalda

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

[Hans de Goede]

Hi Ricardo,

On 12-Nov-24 6:31 PM, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Mon, 11 Nov 2024 at 13:59, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Ricardo, Et al.,
>>
>> On 8-Nov-24 9:25 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 two 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.
>>
>> Thinking more about this I think we need to start with looking at the userspace
>> API for privacy controls, define how we want that to look and then go from
>> there.
>>
>> The reason I'm writing this is because due to my work in drivers/platform/x86
>> (pdx86) on EC / ACPI / WMI drivers for non chromebooks I am aware of at least
>> 4 different methods camera on/off (aka privacy) toggles are being reported
>> to userspace at the moment. Adding a v4l2-ctrl on a subdev instead of directly
>> on /dev/video# would be adding a 5th method which seems highly undesirable.
>>
>> Instead I would like to first focus on fixing these userspace API
>> inconsistencies agreeing on a single API we want to use everywhere
>> going forward. We don't need to fix all drivers at once, but IMHO we
>> should agree on what the API should look like and document that and
>> any future drivers implementing camera privacy control related code
>> then must use the new API.
>>
>> Lets start with the 3 APIs I'm currently aware of:
>>
>> 1. uvcvideo driver exporting V4L2_CID_PRIVACY on /dev/video#
>> uvcvideo seems to be the only user of this CID (i)
>>
>> 2. pdx86 drivers exporting an input evdev with EV_SW,
>> SW_CAMERA_LENS_COVER. This is somewhat of a special case
>> for some Dell laptops with an electro-mechanical shutter
>> operated by the EC. But this is not also used by
>> hp-wmi.c where it does not necessarily indicate the
>> status of a mechanical cover, but also possibly simply
>> disconnecting the camera from the USB bus.
>>
>> 3. pdx86 drivers exporting an input evdev with EV_KEY,
>> KEY_CAMERA_ACCESS_ENABLE, KEY_CAMERA_ACCESS_DISABLE
>> These KEY codes are based on offical the HUTRR72 HID/HUT
>> extension and as such may also be send by USB/I2C/BT HID
>> devices.
>>
>> The only user outside of hid-input.c is the recently added
>> drivers/platform/x86/lenovo-wmi-camera.c driver and I'm
>> wondering if that should not use SW_CAMERA_LENS_COVER
>> instead. I'll ask the driver author about how this
>>
>> 4. pdx86 drivers exporting an input evdev with EV_KEY,
>> KEY_CAMERA. Note this 4th method lacks information on if
>> the camera was enabled or disabled. In many cases this
>> is send to indicate that the EC has either dropped
>> a UVC camera of the bus, or added it to the bus.
>> Ideally we would have some helper checking for internal
>> UVC camera presence and turn this into 2 or 3.
>>
>> TL;DR: it a mess.
>>
>> Circling back to this patch-set, note how 3 of the 4
>> currently in use variants today use in input evdev.
>>
>> I think that using an input evdev (shared with the
>> snapshot button if present) will give us a nice out for
>> the power-management issue with the V4L2_CID_PRIVACY,
>> while at the same time giving a nice opportunity to
>> standardize on a single userspace API.
>>
>> My proposal would be to standardize on SW_CAMERA_LENS_COVER
>> I realize that the GPIO does not always indicate a lens
>> cover, but the resulting black frames are the same result
>> as if there were a lens cover and looking at:
>>
>> https://support.hp.com/ie-en/document/ish_3960099-3335046-16
>>
>> and then the second picture when expanding "Locate and use
>> the webcam privacy switch" that does look like it may be
>> an actual cover which reports back its state through a GPIO.
>>
>> The reason why I'm not in favor of using
>> KEY_CAMERA_ACCESS_ENABLE + KEY_CAMERA_ACCESS_DISABLE is that
>> looking at the HUTRR72 it talks about:
>> "Enables programmatic access to camera device"
>> which suggests that it is a request to the OS / desktop-
>> environment to block camera access at the software level,
>> rather then reporting back that a hw-level block is in place.
>>
>> And since these may be used by any HID device we are not of
>> control in how these will be used.
>>
>> Ricardo, what do you think of instead of using a v4l-subdev,
>> using an input evdev (shared with the existing one) reporting
>> SW_CAMERA_LENS_COVER ?  The v4l-subdev approach will need
>> userspace changes anyways and if we are going to make userspace
>> changes we might as well use the best API available.
> 
> I just sent a patchset using SW_CAMERA_LENS_COVER

I'm glad that you like my proposal and thank you for immediately
implementing it and sending out a v3.

I was expecting us to first have a bit more discussion about
what the userspace API should look like and what we should do
wrt keeping / deprecating V4L2_CID_PRIVACY.

But I'm glad that you like the evdev SW_CAMERA_LENS_COVER idea,
at least I assume you like it since you went for it for v3 :)

I'll reply to your v3 cover-letter to discuss what we should do
wrt keeping / deprecating V4L2_CID_PRIVACY.

IMHO it would be good to hold of on sending out a v4 until we
have hashed out how we want this all to look userspace API wise,
otherwise you'll just spend a lot of time doing revisions
pursuing a moving target.

> I guess the internal uvc privacy (UVC_CT_PRIVACY_CONTROL) shall NOT be
> converted to evdev:
> - If we do so, we cannot differentiate external gpio and internal, for
> devices that have both
> - There is no warranty that we will get a uvc_event when the control
> changes, so we would have to constantly poll the device

These are good questions, lets also discuss this in a thread
with the v3 cover-letter as start to keep all discussion in one place.

Regards,

Hans