| Message ID | 20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org |
|---|---|
| Headers | show |
| Series | meida: uvcvideo: reimplement privacy gpio as a separate subdevice | expand |
Hi Yunke Thank you very much for the patchset :) On Wed, 11 Jan 2023 at 09:52, Yunke Cao <yunkec@chromium.org> wrote: > > privacy_gpio in uvc were added as V4L2_CID_PRIVACY in uvc video node in > https://lore.kernel.org/all/20201223133528.55014-1-ribalda@chromium.org/ > > Userspace applications often require to constantly poll privacy control. > Currently, polling privacy control requires keeping the video node open, > which prevents the camera from autosuspending. > > This patchset adds a separate v4l2 subdevice. Userspace access the gpio > via V4L2_CID_PRIVACY in the new subdevice. Applications can poll the > privacy control status without opening the video node and activate the > camera. > > The non-gpio V4L2_CID_PRIVACY in uvc is not affected. Since this is a RFC, lets focus on the idea and not on the code itself. - I am missing a reference to the subdevice from the media device. How will a user figure out that /dev/v4l-subdev0 is the privacy gpio of /dev/media0 and not /dev/media1?. Thake a look to the "ancillary links" - We have already exposed the control as part of the main video device, that means that we need to keep that API. The control on /dev/v4l-subdev0 should "mirror" the control on /dev/video0 - There is no need to v4l2_ctrl_fill_event(), if you modify the control with a set controll function, the media controller should take care of everything @Sakari Ailus @Hans Verkuil : Assuming a correct implementation, how would you feel about exposing a privacy gpio as a subdevice? Thanks!!! > > Suggested-by: Ricardo Ribalda <ribalda@chromium.org> > Signed-off-by: Yunke Cao <yunkec@chromium.org> > --- > Yunke Cao (3): > media: v4l2-ctrls: Expose v4l2_ctrl_fill_event() > media: uvcvideo: remove entity privacy control in the uvc video node > media: uvcvideo: reimplement privacy GPIO as a separate subdevice > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ------- > drivers/media/usb/uvc/uvc_driver.c | 44 ++---------------- > drivers/media/usb/uvc/uvc_entity.c | 76 +++++++++++++++++++++++++++++++ > drivers/media/usb/uvc/uvcvideo.h | 19 +++++--- > drivers/media/v4l2-core/v4l2-ctrls-core.c | 9 ++-- > include/media/v4l2-ctrls.h | 12 +++++ > 6 files changed, 111 insertions(+), 66 deletions(-) > --- > base-commit: 7dd4b804e08041ff56c88bdd8da742d14b17ed25 > change-id: 20230111-uvc_privacy_subdev-1e7a167e86eb > > Best regards, > -- > Yunke Cao <yunkec@chromium.org>
On Tue, 14 Feb 2023 at 06:46, Yunke Cao <yunkec@chromium.org> wrote: > > Hi! > > On Fri, Jan 13, 2023 at 5:26 PM Ricardo Ribalda <ribalda@chromium.org> wrote: > > > > Hi Yunke > > > > Thank you very much for the patchset :) > > > > On Wed, 11 Jan 2023 at 09:52, Yunke Cao <yunkec@chromium.org> wrote: > > > > > > privacy_gpio in uvc were added as V4L2_CID_PRIVACY in uvc video node in > > > https://lore.kernel.org/all/20201223133528.55014-1-ribalda@chromium.org/ > > > > > > Userspace applications often require to constantly poll privacy control. > > > Currently, polling privacy control requires keeping the video node open, > > > which prevents the camera from autosuspending. > > > > > > This patchset adds a separate v4l2 subdevice. Userspace access the gpio > > > via V4L2_CID_PRIVACY in the new subdevice. Applications can poll the > > > privacy control status without opening the video node and activate the > > > camera. > > > > > > The non-gpio V4L2_CID_PRIVACY in uvc is not affected. > > > > Since this is a RFC, lets focus on the idea and not on the code itself. > > > > - I am missing a reference to the subdevice from the media device. How > > will a user figure out that /dev/v4l-subdev0 is the privacy gpio of > > /dev/media0 and not /dev/media1?. Thake a look to the "ancillary > > links" > > - We have already exposed the control as part of the main video > > device, that means that we need to keep that API. The control on > > /dev/v4l-subdev0 should "mirror" the control on /dev/video0 > > - There is no need to v4l2_ctrl_fill_event(), if you modify the > > control with a set controll function, the media controller should take > > care of everything > > Thanks! I will fix these in the next version if we decide to proceed. > > > > > @Sakari Ailus @Hans Verkuil : Assuming a correct implementation, how > > would you feel about exposing a privacy gpio as a subdevice? > > > > Sakari, Hans, do you think this idea makes sense? Friendly ping > > Best, > Yunke > > > > > Thanks!!! > > > > > > > > > > Suggested-by: Ricardo Ribalda <ribalda@chromium.org> > > > Signed-off-by: Yunke Cao <yunkec@chromium.org> > > > --- > > > Yunke Cao (3): > > > media: v4l2-ctrls: Expose v4l2_ctrl_fill_event() > > > media: uvcvideo: remove entity privacy control in the uvc video node > > > media: uvcvideo: reimplement privacy GPIO as a separate subdevice > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ------- > > > drivers/media/usb/uvc/uvc_driver.c | 44 ++---------------- > > > drivers/media/usb/uvc/uvc_entity.c | 76 +++++++++++++++++++++++++++++++ > > > drivers/media/usb/uvc/uvcvideo.h | 19 +++++--- > > > drivers/media/v4l2-core/v4l2-ctrls-core.c | 9 ++-- > > > include/media/v4l2-ctrls.h | 12 +++++ > > > 6 files changed, 111 insertions(+), 66 deletions(-) > > > --- > > > base-commit: 7dd4b804e08041ff56c88bdd8da742d14b17ed25 > > > change-id: 20230111-uvc_privacy_subdev-1e7a167e86eb > > > > > > Best regards, > > > -- > > > Yunke Cao <yunkec@chromium.org> > > > > > > > > -- > > Ricardo Ribalda
Hi Sakari Could you take a look at this RFC? Would be great to have your opinion from a subdevice point of view. Thanks! On Wed, 11 Jan 2023 at 09:52, Yunke Cao <yunkec@chromium.org> wrote: > > privacy_gpio in uvc were added as V4L2_CID_PRIVACY in uvc video node in > https://lore.kernel.org/all/20201223133528.55014-1-ribalda@chromium.org/ > > Userspace applications often require to constantly poll privacy control. > Currently, polling privacy control requires keeping the video node open, > which prevents the camera from autosuspending. > > This patchset adds a separate v4l2 subdevice. Userspace access the gpio > via V4L2_CID_PRIVACY in the new subdevice. Applications can poll the > privacy control status without opening the video node and activate the > camera. > > The non-gpio V4L2_CID_PRIVACY in uvc is not affected. > > Suggested-by: Ricardo Ribalda <ribalda@chromium.org> > Signed-off-by: Yunke Cao <yunkec@chromium.org> > --- > Yunke Cao (3): > media: v4l2-ctrls: Expose v4l2_ctrl_fill_event() > media: uvcvideo: remove entity privacy control in the uvc video node > media: uvcvideo: reimplement privacy GPIO as a separate subdevice > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ------- > drivers/media/usb/uvc/uvc_driver.c | 44 ++---------------- > drivers/media/usb/uvc/uvc_entity.c | 76 +++++++++++++++++++++++++++++++ > drivers/media/usb/uvc/uvcvideo.h | 19 +++++--- > drivers/media/v4l2-core/v4l2-ctrls-core.c | 9 ++-- > include/media/v4l2-ctrls.h | 12 +++++ > 6 files changed, 111 insertions(+), 66 deletions(-) > --- > base-commit: 7dd4b804e08041ff56c88bdd8da742d14b17ed25 > change-id: 20230111-uvc_privacy_subdev-1e7a167e86eb > > Best regards, > -- > Yunke Cao <yunkec@chromium.org>
Hi Yunke, A comment below: On 11/01/2023 09:52, Yunke Cao wrote: > 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: Yunke Cao <yunkec@chromium.org> > --- > drivers/media/usb/uvc/uvc_driver.c | 44 +++------------------- > drivers/media/usb/uvc/uvc_entity.c | 76 ++++++++++++++++++++++++++++++++++++++ > drivers/media/usb/uvc/uvcvideo.h | 19 +++++++--- > 3 files changed, 94 insertions(+), 45 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index e4bcb5011360..25848f4dbce0 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1209,43 +1209,6 @@ static int uvc_parse_control(struct uvc_device *dev) > * 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; > @@ -1279,8 +1242,6 @@ static int uvc_gpio_parse(struct uvc_device *dev) > 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); > @@ -2202,6 +2163,11 @@ static int uvc_probe(struct usb_interface *intf, > if (media_device_register(&dev->mdev) < 0) > goto error; > #endif > + > + /* Expose all subdev's nodes*/ > + if (v4l2_device_register_subdev_nodes(&dev->vdev) < 0) > + goto error; > + > /* Save our data pointer in the interface data. */ > usb_set_intfdata(intf, dev); > > diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c > index 7c4d2f93d351..c8e41b42ffd8 100644 > --- a/drivers/media/usb/uvc/uvc_entity.c > +++ b/drivers/media/usb/uvc/uvc_entity.c > @@ -56,17 +56,90 @@ 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) > { > + if (UVC_ENTITY_TYPE(entity) == UVC_EXT_GPIO_UNIT) > + v4l2_ctrl_handler_free(&entity->gpio.hdl); > + > if (UVC_ENTITY_TYPE(entity) != UVC_TT_STREAMING) > media_entity_cleanup(&entity->subdev.entity); > else if (entity->vdev != NULL) > media_entity_cleanup(&entity->vdev->entity); > } > > +static int uvc_gpio_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct uvc_gpio *gpio = > + container_of(ctrl->handler, struct uvc_gpio, hdl); > + > + ctrl->cur.val = gpiod_get_value_cansleep(gpio->gpio_privacy); > + > + return 0; > +} > + > +static const struct v4l2_ctrl_ops uvc_gpio_ctrl_ops = { > + .g_volatile_ctrl = uvc_gpio_g_volatile_ctrl, > +}; > + > +void uvc_gpio_event(struct uvc_device *dev) > +{ > + struct uvc_entity *unit = dev->gpio_unit; > + struct v4l2_ctrl *ctrl; > + struct v4l2_event ev; > + s32 old_val; > + int ret; > + > + if (!unit) > + return; > + > + ctrl = unit->gpio.privacy_ctrl; > + old_val = ctrl->cur.val; > + > + v4l2_ctrl_lock(ctrl); > + > + ret = uvc_gpio_g_volatile_ctrl(ctrl); > + if (ret < 0 || old_val == ctrl->cur.val) { > + v4l2_ctrl_unlock(ctrl); > + return; > + } > + > + v4l2_ctrl_fill_event(&ev, ctrl, V4L2_EVENT_CTRL_CH_VALUE); > + v4l2_ctrl_unlock(ctrl); > + > + v4l2_event_queue(unit->subdev.devnode, &ev); > +} > + > +static void uvc_gpio_init_ctrl(struct uvc_entity *entity) > +{ > + struct v4l2_ctrl *ctrl; > + struct v4l2_ctrl_handler *hdl = &entity->gpio.hdl; > + > + entity->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | > + V4L2_SUBDEV_FL_HAS_EVENTS; > + > + v4l2_ctrl_handler_init(hdl, 1); > + entity->subdev.ctrl_handler = hdl; > + ctrl = v4l2_ctrl_new_std(hdl, &uvc_gpio_ctrl_ops, V4L2_CID_PRIVACY, > + 0, 1, 1, 0); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE | > + V4L2_CTRL_FLAG_READ_ONLY; This is not a volatile control. You know exactly what the value is and when it changes since a gpio interrupt is issued. So this is just a regular read-only control, and when uvc_gpio_event is called you can just call v4l2_ctrl_s_ctrl with the new value from gpiod_get_value_cansleep(). No need to expose v4l2_ctrl_fill_event either since the call above will take care of the control event. Regards, Hans > + > + /* Read GPIO value to initialize the control. */ > + uvc_gpio_g_volatile_ctrl(ctrl); > + > + entity->gpio.privacy_ctrl = ctrl; > +} > + > static int uvc_mc_init_entity(struct uvc_video_chain *chain, > struct uvc_entity *entity) > { > @@ -113,6 +186,9 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain, > > entity->subdev.entity.function = function; > > + if (UVC_ENTITY_TYPE(entity) == UVC_EXT_GPIO_UNIT) > + uvc_gpio_init_ctrl(entity); > + > ret = media_entity_pads_init(&entity->subdev.entity, > entity->num_pads, entity->pads); > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index df93db259312..19ca2896c398 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> > @@ -163,6 +164,15 @@ struct uvc_control { > > #define UVC_ENTITY_FLAG_DEFAULT (1 << 0) > > +struct uvc_gpio { > + u8 bControlSize; > + u8 *bmControls; > + struct gpio_desc *gpio_privacy; > + int irq; > + struct v4l2_ctrl_handler hdl; > + struct v4l2_ctrl *privacy_ctrl; > +}; > + > 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. */ > @@ -221,12 +231,7 @@ struct uvc_entity { > u8 *bmControlsType; > } extension; > > - struct { > - u8 bControlSize; > - u8 *bmControls; > - struct gpio_desc *gpio_privacy; > - int irq; > - } gpio; > + struct uvc_gpio gpio; > }; > > u8 bNrInPins; > @@ -766,6 +771,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > struct uvc_xu_control_query *xqry); > > +void uvc_gpio_event(struct uvc_device *dev); > + > /* Utility functions */ > struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, > u8 epaddr); >
privacy_gpio in uvc were added as V4L2_CID_PRIVACY in uvc video node in https://lore.kernel.org/all/20201223133528.55014-1-ribalda@chromium.org/ Userspace applications often require to constantly poll privacy control. Currently, polling privacy control requires keeping the video node open, which prevents the camera from autosuspending. This patchset adds a separate v4l2 subdevice. Userspace access the gpio via V4L2_CID_PRIVACY in the new subdevice. Applications can poll the privacy control status without opening the video node and activate the camera. The non-gpio V4L2_CID_PRIVACY in uvc is not affected. Suggested-by: Ricardo Ribalda <ribalda@chromium.org> Signed-off-by: Yunke Cao <yunkec@chromium.org> --- Yunke Cao (3): media: v4l2-ctrls: Expose v4l2_ctrl_fill_event() media: uvcvideo: remove entity privacy control in the uvc video node media: uvcvideo: reimplement privacy GPIO as a separate subdevice drivers/media/usb/uvc/uvc_ctrl.c | 17 ------- drivers/media/usb/uvc/uvc_driver.c | 44 ++---------------- drivers/media/usb/uvc/uvc_entity.c | 76 +++++++++++++++++++++++++++++++ drivers/media/usb/uvc/uvcvideo.h | 19 +++++--- drivers/media/v4l2-core/v4l2-ctrls-core.c | 9 ++-- include/media/v4l2-ctrls.h | 12 +++++ 6 files changed, 111 insertions(+), 66 deletions(-) --- base-commit: 7dd4b804e08041ff56c88bdd8da742d14b17ed25 change-id: 20230111-uvc_privacy_subdev-1e7a167e86eb Best regards,