| Message ID | 20241020163534.1720297-1-tomm.merciai@gmail.com |
|---|---|
| State | Accepted |
| Commit | e7724e23196ab1b4bc843aa60e917967d95697e4 |
| Headers | show |
| Series | [v2] media: v4l2-subdev: Refactor events | expand |
Hi Laurent, Sakari, Sorry for the delay. Back on this topic. On Mon, Oct 21, 2024 at 10:30:34AM +0300, Laurent Pinchart wrote: > On Mon, Oct 21, 2024 at 08:35:53AM +0200, Tommaso Merciai wrote: > > Hi Laurent, > > Thanks for your review. > > > > On Sun, Oct 20, 2024 at 07:43:54PM +0300, Laurent Pinchart wrote: > > > Hi Tommaso, > > > > > > Thank you for the patch. > > > > > > On Sun, Oct 20, 2024 at 06:35:32PM +0200, Tommaso Merciai wrote: > > > > Controls can be exposed to userspace via a v4l-subdevX device, and > > > > userspace has to be able to subscribe to control events so that it is > > > > notified when the control changes value. > > > > If a control handler is set for the subdev then set the HAS_EVENTS > > > > flag automatically into v4l2_subdev_init_finalize() and use > > > > v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe() > > > > as default if subdev don't have .(un)subscribe control operations. > > > > > > I would add here > > > > > > This simplifies subdev drivers by avoiding the need to set the > > > V4L2_SUBDEV_FL_HAS_EVENTS flag and plug the event handlers, and ensures > > > consistency of the API exposed to userspace. > > > > > > And you can also add > > > > > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Oks, Thanks again. > > > > > Now, can we simplify sensor drivers to drop the event handlers and the > > > flag ? :-) > > > > Yep, plan is add all to support v4l2_subdev_init_finalize() > > Removing: > > > > .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > > > if are used. And ofc V4L2_SUBDEV_FL_HAS_EVENTS. > > What I meant is looking at the I2C sensor drivers that currently > > - call v4l2_subdev_init_finalize() > - set V4L2_SUBDEV_FL_HAS_EVENTS > - set the .subscribe_event() and .unsubscribe_event() handlers > > and dropping the flag and handlers from them. Is that what you plan to > work on ? It's ok for you per/driver patch or you prefer a big single patch? Meanwhile I've prepared something here: https://gitlab.freedesktop.org/linux-media/users/tmerciai/-/compare/next...v6.12.0-rc1-nxp?from_project_id=22111 Let me know if you prefer (un)squashed version. Thanks in advance. :) Thanks & Regards, Tommaso > > > Meanwhile I think I will send v3 with your > > suggestions. :) > > > > > > --- > > > > Changes since v1: > > > > - Aligned event subscription with unsubscription as suggested by LPinchart, > > > > SAilus > > > > > > > > drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++-- > > > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > > index 3a4ba08810d2..fad8fa1f63e8 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > > @@ -691,10 +691,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > > > > return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK); > > > > > > > > case VIDIOC_SUBSCRIBE_EVENT: > > > > - return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg); > > > > + if (v4l2_subdev_has_op(sd, core, subscribe_event)) > > > > + return v4l2_subdev_call(sd, core, subscribe_event, > > > > + vfh, arg); > > > > + > > > > + if ((sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) && > > > > + vfh->ctrl_handler) > > > > + return v4l2_ctrl_subdev_subscribe_event(sd, vfh, arg); > > > > + > > > > + return -ENOIOCTLCMD; > > > > > > > > case VIDIOC_UNSUBSCRIBE_EVENT: > > > > - return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg); > > > > + if (v4l2_subdev_has_op(sd, core, unsubscribe_event)) > > > > + return v4l2_subdev_call(sd, core, unsubscribe_event, > > > > + vfh, arg); > > > > + > > > > + if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) > > > > + return v4l2_event_subdev_unsubscribe(sd, vfh, arg); > > > > + > > > > + return -ENOIOCTLCMD; > > > > > > > > #ifdef CONFIG_VIDEO_ADV_DEBUG > > > > case VIDIOC_DBG_G_REGISTER: > > > > @@ -1641,6 +1656,9 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name, > > > > } > > > > } > > > > > > > > + if (sd->ctrl_handler) > > > > + sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS; > > > > + > > > > state = __v4l2_subdev_state_alloc(sd, name, key); > > > > if (IS_ERR(state)) > > > > return PTR_ERR(state); > > -- > Regards, > > Laurent Pinchart
Hi Tommaso, On Mon, Oct 28, 2024 at 06:32:32PM +0100, Tommaso Merciai wrote: > On Mon, Oct 21, 2024 at 10:30:34AM +0300, Laurent Pinchart wrote: > > On Mon, Oct 21, 2024 at 08:35:53AM +0200, Tommaso Merciai wrote: > > > On Sun, Oct 20, 2024 at 07:43:54PM +0300, Laurent Pinchart wrote: > > > > On Sun, Oct 20, 2024 at 06:35:32PM +0200, Tommaso Merciai wrote: > > > > > Controls can be exposed to userspace via a v4l-subdevX device, and > > > > > userspace has to be able to subscribe to control events so that it is > > > > > notified when the control changes value. > > > > > If a control handler is set for the subdev then set the HAS_EVENTS > > > > > flag automatically into v4l2_subdev_init_finalize() and use > > > > > v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe() > > > > > as default if subdev don't have .(un)subscribe control operations. > > > > > > > > I would add here > > > > > > > > This simplifies subdev drivers by avoiding the need to set the > > > > V4L2_SUBDEV_FL_HAS_EVENTS flag and plug the event handlers, and ensures > > > > consistency of the API exposed to userspace. > > > > > > > > And you can also add > > > > > > > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > Oks, Thanks again. > > > > > > > Now, can we simplify sensor drivers to drop the event handlers and the > > > > flag ? :-) > > > > > > Yep, plan is add all to support v4l2_subdev_init_finalize() > > > Removing: > > > > > > .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > > .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > > > > > if are used. And ofc V4L2_SUBDEV_FL_HAS_EVENTS. > > > > What I meant is looking at the I2C sensor drivers that currently > > > > - call v4l2_subdev_init_finalize() > > - set V4L2_SUBDEV_FL_HAS_EVENTS > > - set the .subscribe_event() and .unsubscribe_event() handlers > > > > and dropping the flag and handlers from them. Is that what you plan to > > work on ? > > It's ok for you per/driver patch or you prefer a big single patch? I'm fine either way. Maybe one large patch to address all the drivers where the flag and handlers are simply dropped, and then one patch per driver where changes are larger (such as adding calls to v4l2_subdev_init_finalize()) ? > Meanwhile I've prepared something here: > > https://gitlab.freedesktop.org/linux-media/users/tmerciai/-/compare/next...v6.12.0-rc1-nxp?from_project_id=22111 > > Let me know if you prefer (un)squashed version. > Thanks in advance. :) > > Thanks & Regards, > Tommaso > > > > > > Meanwhile I think I will send v3 with your > > > suggestions. :) > > > > > > > > --- > > > > > Changes since v1: > > > > > - Aligned event subscription with unsubscription as suggested by LPinchart, > > > > > SAilus > > > > > > > > > > drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++-- > > > > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > index 3a4ba08810d2..fad8fa1f63e8 100644 > > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > @@ -691,10 +691,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > > > > > return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK); > > > > > > > > > > case VIDIOC_SUBSCRIBE_EVENT: > > > > > - return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg); > > > > > + if (v4l2_subdev_has_op(sd, core, subscribe_event)) > > > > > + return v4l2_subdev_call(sd, core, subscribe_event, > > > > > + vfh, arg); > > > > > + > > > > > + if ((sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) && > > > > > + vfh->ctrl_handler) > > > > > + return v4l2_ctrl_subdev_subscribe_event(sd, vfh, arg); > > > > > + > > > > > + return -ENOIOCTLCMD; > > > > > > > > > > case VIDIOC_UNSUBSCRIBE_EVENT: > > > > > - return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg); > > > > > + if (v4l2_subdev_has_op(sd, core, unsubscribe_event)) > > > > > + return v4l2_subdev_call(sd, core, unsubscribe_event, > > > > > + vfh, arg); > > > > > + > > > > > + if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) > > > > > + return v4l2_event_subdev_unsubscribe(sd, vfh, arg); > > > > > + > > > > > + return -ENOIOCTLCMD; > > > > > > > > > > #ifdef CONFIG_VIDEO_ADV_DEBUG > > > > > case VIDIOC_DBG_G_REGISTER: > > > > > @@ -1641,6 +1656,9 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name, > > > > > } > > > > > } > > > > > > > > > > + if (sd->ctrl_handler) > > > > > + sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS; > > > > > + > > > > > state = __v4l2_subdev_state_alloc(sd, name, key); > > > > > if (IS_ERR(state)) > > > > > return PTR_ERR(state);
On Mon, Oct 28, 2024 at 08:36:57PM +0200, Laurent Pinchart wrote: > > > What I meant is looking at the I2C sensor drivers that currently > > > > > > - call v4l2_subdev_init_finalize() > > > - set V4L2_SUBDEV_FL_HAS_EVENTS > > > - set the .subscribe_event() and .unsubscribe_event() handlers > > > > > > and dropping the flag and handlers from them. Is that what you plan to > > > work on ? > > > > It's ok for you per/driver patch or you prefer a big single patch? > > I'm fine either way. Maybe one large patch to address all the drivers > where the flag and handlers are simply dropped, and then one patch per > driver where changes are larger (such as adding calls to > v4l2_subdev_init_finalize()) ? Sounds good to me.
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 3a4ba08810d2..fad8fa1f63e8 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -691,10 +691,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK); case VIDIOC_SUBSCRIBE_EVENT: - return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg); + if (v4l2_subdev_has_op(sd, core, subscribe_event)) + return v4l2_subdev_call(sd, core, subscribe_event, + vfh, arg); + + if ((sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) && + vfh->ctrl_handler) + return v4l2_ctrl_subdev_subscribe_event(sd, vfh, arg); + + return -ENOIOCTLCMD; case VIDIOC_UNSUBSCRIBE_EVENT: - return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg); + if (v4l2_subdev_has_op(sd, core, unsubscribe_event)) + return v4l2_subdev_call(sd, core, unsubscribe_event, + vfh, arg); + + if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) + return v4l2_event_subdev_unsubscribe(sd, vfh, arg); + + return -ENOIOCTLCMD; #ifdef CONFIG_VIDEO_ADV_DEBUG case VIDIOC_DBG_G_REGISTER: @@ -1641,6 +1656,9 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name, } } + if (sd->ctrl_handler) + sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS; + state = __v4l2_subdev_state_alloc(sd, name, key); if (IS_ERR(state)) return PTR_ERR(state);
Controls can be exposed to userspace via a v4l-subdevX device, and userspace has to be able to subscribe to control events so that it is notified when the control changes value. If a control handler is set for the subdev then set the HAS_EVENTS flag automatically into v4l2_subdev_init_finalize() and use v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe() as default if subdev don't have .(un)subscribe control operations. Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> --- Changes since v1: - Aligned event subscription with unsubscription as suggested by LPinchart, SAilus drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)