| Message ID | 20260925-v4l2-events-v1-4-b2ec212eb5d0@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
2026. 09. 25. 17:16 keltezéssel, Daniel Scally írta: > The current implementation of V4L2Device::setFrameStartEnabled hard > codes V4L2_EVENT_FRAME_SYNC as it passes the subscription to the > kernel. Re-work the function such that the subcription details are subscription > parametised. This will allow consumers to suscribe to any supported subscribe > events. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > include/libcamera/internal/v4l2_device.h | 5 ++-- > src/libcamera/pipeline/ipu3/cio2.cpp | 6 ++-- > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 6 ++-- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++-- > .../pipeline/rpi/common/pipeline_base.cpp | 6 ++-- > src/libcamera/pipeline/simple/simple.cpp | 6 ++-- > src/libcamera/v4l2_device.cpp | 32 ++++++++++++++++------ > 7 files changed, 47 insertions(+), 22 deletions(-) > > [...] > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index f231bbb17d8528aca274388b8dcaccc0dc339ad3..b93f3fff7280be1f0357341f217931cd290ba97f 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -58,8 +58,7 @@ LOG_DEFINE_CATEGORY(V4L2) > * at open() time, and the \a logTag to prefix log messages with. > */ > V4L2Device::V4L2Device(const std::string &deviceNode) > - : deviceNode_(deviceNode), fdEventNotifier_(nullptr), > - frameStartEnabled_(false) > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr) > { > } > > @@ -499,21 +498,32 @@ bool V4L2Device::supportsEvents(V4L2EventSubscription &sub) > } > > /** > - * \brief Enable or disable frame start event notification > + * \brief Enable or event notifications > + * \param[in] sub Details of the event to subscribe to > * \param[in] enable True to enable frame start events, false to disable them > * > - * This function enables or disables generation of frame start events. Once > - * enabled, the events are signalled through the eventReady signal. > + * This function enables or disables generation of events for a particular > + * subscriptions. Once enabled, the events are signalled through the eventReady > + * signal. > * > * \return 0 on success, a negative error code otherwise > */ > -int V4L2Device::setFrameStartEnabled(bool enable) > +int V4L2Device::setEventsEnabled(V4L2EventSubscription &sub, bool enable) Since the type is so small, I'd pass it by value: V4L2EventSubscription sub but at least via a const reference in any case; and then you can do e.g. data->frontendDevice()->setEventsEnabled(V4L2Event::Type::FrameSync, true); which is much more convenient than creating a separate object. Same with `supportsEvents()`. > { > - if (frameStartEnabled_ == enable) > + if (sub.type() >= V4L2Event::Type::NumberOfEventTypes) This is effectively already diagnosed by `V4L2Event::typeToV4L2()`, so I think it could be omitted. > + return -EINVAL; > + > + if (subscribedEvents_.find(sub) != subscribedEvents_.end()) subscribedEvents_.contains(sub) is an option since C++20. > return 0; > > struct v4l2_event_subscription event{}; > - event.type = V4L2_EVENT_FRAME_SYNC; > + > + auto v4l2EventType = V4L2Event::typeToV4L2(sub.type()); > + if (!v4l2EventType) > + return -EINVAL; > + > + event.type = *v4l2EventType; > + event.id = sub.id(); > > unsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT > : VIDIOC_UNSUBSCRIBE_EVENT; > @@ -522,7 +532,11 @@ int V4L2Device::setFrameStartEnabled(bool enable) > return ret; > > fdEventNotifier_->setEnabled(enable); > - frameStartEnabled_ = enable; > + > + if (enable) > + subscribedEvents_.insert(sub); > + else > + subscribedEvents_.erase(sub); > > return ret; > } >
Hi Barnabas On 02/10/2026 16:06, Barnabás Pőcze wrote: > 2026. 09. 25. 17:16 keltezéssel, Daniel Scally írta: >> The current implementation of V4L2Device::setFrameStartEnabled hard >> codes V4L2_EVENT_FRAME_SYNC as it passes the subscription to the >> kernel. Re-work the function such that the subcription details are > > subscription > > >> parametised. This will allow consumers to suscribe to any supported > > subscribe > > >> events. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> include/libcamera/internal/v4l2_device.h | 5 ++-- >> src/libcamera/pipeline/ipu3/cio2.cpp | 6 ++-- >> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 6 ++-- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++-- >> .../pipeline/rpi/common/pipeline_base.cpp | 6 ++-- >> src/libcamera/pipeline/simple/simple.cpp | 6 ++-- >> src/libcamera/v4l2_device.cpp | 32 ++++++++++++++++------ >> 7 files changed, 47 insertions(+), 22 deletions(-) >> >> [...] >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp >> index f231bbb17d8528aca274388b8dcaccc0dc339ad3..b93f3fff7280be1f0357341f217931cd290ba97f 100644 >> --- a/src/libcamera/v4l2_device.cpp >> +++ b/src/libcamera/v4l2_device.cpp >> @@ -58,8 +58,7 @@ LOG_DEFINE_CATEGORY(V4L2) >> * at open() time, and the \a logTag to prefix log messages with. >> */ >> V4L2Device::V4L2Device(const std::string &deviceNode) >> - : deviceNode_(deviceNode), fdEventNotifier_(nullptr), >> - frameStartEnabled_(false) >> + : deviceNode_(deviceNode), fdEventNotifier_(nullptr) >> { >> } >> @@ -499,21 +498,32 @@ bool V4L2Device::supportsEvents(V4L2EventSubscription &sub) >> } >> /** >> - * \brief Enable or disable frame start event notification >> + * \brief Enable or event notifications >> + * \param[in] sub Details of the event to subscribe to >> * \param[in] enable True to enable frame start events, false to disable them >> * >> - * This function enables or disables generation of frame start events. Once >> - * enabled, the events are signalled through the eventReady signal. >> + * This function enables or disables generation of events for a particular >> + * subscriptions. Once enabled, the events are signalled through the eventReady >> + * signal. >> * >> * \return 0 on success, a negative error code otherwise >> */ >> -int V4L2Device::setFrameStartEnabled(bool enable) >> +int V4L2Device::setEventsEnabled(V4L2EventSubscription &sub, bool enable) > > Since the type is so small, I'd pass it by value: > > V4L2EventSubscription sub > > but at least via a const reference in any case; and then you can do e.g. > > data->frontendDevice()->setEventsEnabled(V4L2Event::Type::FrameSync, true); > > which is much more convenient than creating a separate object. > Same with `supportsEvents()`. This needs the v4l2_event_subscription (or some representation of it), as there are V4L2 event types that need both the type and id fields (control events for example) for the subscription to be specific enough. Or possibly instead of the v4l2_event_subscription, a type parameter and an id parameter with a default of 0...but even then we'd need to extend later if we wanted to set non-zero flags for the sub.> > >> { >> - if (frameStartEnabled_ == enable) >> + if (sub.type() >= V4L2Event::Type::NumberOfEventTypes) > > This is effectively already diagnosed by `V4L2Event::typeToV4L2()`, > so I think it could be omitted. > > >> + return -EINVAL; >> + >> + if (subscribedEvents_.find(sub) != subscribedEvents_.end()) > > subscribedEvents_.contains(sub) > > is an option since C++20. ack! Thanks Dan > > >> return 0; >> struct v4l2_event_subscription event{}; >> - event.type = V4L2_EVENT_FRAME_SYNC; >> + >> + auto v4l2EventType = V4L2Event::typeToV4L2(sub.type()); >> + if (!v4l2EventType) >> + return -EINVAL; >> + >> + event.type = *v4l2EventType; >> + event.id = sub.id(); >> unsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT >> : VIDIOC_UNSUBSCRIBE_EVENT; >> @@ -522,7 +532,11 @@ int V4L2Device::setFrameStartEnabled(bool enable) >> return ret; >> fdEventNotifier_->setEnabled(enable); >> - frameStartEnabled_ = enable; >> + >> + if (enable) >> + subscribedEvents_.insert(sub); >> + else >> + subscribedEvents_.erase(sub); >> return ret; >> } >> >
2026. 10. 05. 16:51 keltezéssel, Dan Scally írta: > Hi Barnabas > > On 02/10/2026 16:06, Barnabás Pőcze wrote: >> 2026. 09. 25. 17:16 keltezéssel, Daniel Scally írta: >>> The current implementation of V4L2Device::setFrameStartEnabled hard >>> codes V4L2_EVENT_FRAME_SYNC as it passes the subscription to the >>> kernel. Re-work the function such that the subcription details are >> >> subscription >> >> >>> parametised. This will allow consumers to suscribe to any supported >> >> subscribe >> >> >>> events. >>> >>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>> --- >>> include/libcamera/internal/v4l2_device.h | 5 ++-- >>> src/libcamera/pipeline/ipu3/cio2.cpp | 6 ++-- >>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 6 ++-- >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++-- >>> .../pipeline/rpi/common/pipeline_base.cpp | 6 ++-- >>> src/libcamera/pipeline/simple/simple.cpp | 6 ++-- >>> src/libcamera/v4l2_device.cpp | 32 ++++++++++++++++------ >>> 7 files changed, 47 insertions(+), 22 deletions(-) >>> >>> [...] >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp >>> index f231bbb17d8528aca274388b8dcaccc0dc339ad3..b93f3fff7280be1f0357341f217931cd290ba97f 100644 >>> --- a/src/libcamera/v4l2_device.cpp >>> +++ b/src/libcamera/v4l2_device.cpp >>> @@ -58,8 +58,7 @@ LOG_DEFINE_CATEGORY(V4L2) >>> * at open() time, and the \a logTag to prefix log messages with. >>> */ >>> V4L2Device::V4L2Device(const std::string &deviceNode) >>> - : deviceNode_(deviceNode), fdEventNotifier_(nullptr), >>> - frameStartEnabled_(false) >>> + : deviceNode_(deviceNode), fdEventNotifier_(nullptr) >>> { >>> } >>> @@ -499,21 +498,32 @@ bool V4L2Device::supportsEvents(V4L2EventSubscription &sub) >>> } >>> /** >>> - * \brief Enable or disable frame start event notification >>> + * \brief Enable or event notifications >>> + * \param[in] sub Details of the event to subscribe to >>> * \param[in] enable True to enable frame start events, false to disable them >>> * >>> - * This function enables or disables generation of frame start events. Once >>> - * enabled, the events are signalled through the eventReady signal. >>> + * This function enables or disables generation of events for a particular >>> + * subscriptions. Once enabled, the events are signalled through the eventReady >>> + * signal. >>> * >>> * \return 0 on success, a negative error code otherwise >>> */ >>> -int V4L2Device::setFrameStartEnabled(bool enable) >>> +int V4L2Device::setEventsEnabled(V4L2EventSubscription &sub, bool enable) >> >> Since the type is so small, I'd pass it by value: >> >> V4L2EventSubscription sub >> >> but at least via a const reference in any case; and then you can do e.g. >> >> data->frontendDevice()->setEventsEnabled(V4L2Event::Type::FrameSync, true); >> >> which is much more convenient than creating a separate object. >> Same with `supportsEvents()`. > > This needs the v4l2_event_subscription (or some representation of it), as there are V4L2 event types that need both the type and id fields (control events for example) for the subscription to be specific enough. Or possibly instead of the v4l2_event_subscription, a type parameter and an id parameter with a default of 0...but even then we'd need to extend later if we wanted to set non-zero flags for the sub.> But there is already that type, no? `V4L2EventSubscription`? I'm just saying if you pass the already existing type by value, then even something like data->frontendDevice()->setEventsEnabled(V4L2Event::Type::FrameSync, true); will "just" work. And it is equivalent to data->frontendDevice()->setEventsEnabled({ /*type*/ V4L2Event::Type::FrameSync, /*id*/ 0 }, true); because you made the constructor have `= 0` for the `id` parameter. >> >>> { >>> - if (frameStartEnabled_ == enable) >>> + if (sub.type() >= V4L2Event::Type::NumberOfEventTypes) >> >> This is effectively already diagnosed by `V4L2Event::typeToV4L2()`, >> so I think it could be omitted. >> >> >>> + return -EINVAL; >>> + >>> + if (subscribedEvents_.find(sub) != subscribedEvents_.end()) >> >> subscribedEvents_.contains(sub) >> >> is an option since C++20. > > ack! > > Thanks > Dan > >> >> >>> return 0; >>> struct v4l2_event_subscription event{}; >>> - event.type = V4L2_EVENT_FRAME_SYNC; >>> + >>> + auto v4l2EventType = V4L2Event::typeToV4L2(sub.type()); >>> + if (!v4l2EventType) >>> + return -EINVAL; >>> + >>> + event.type = *v4l2EventType; >>> + event.id = sub.id(); >>> unsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT >>> : VIDIOC_UNSUBSCRIBE_EVENT; >>> @@ -522,7 +532,11 @@ int V4L2Device::setFrameStartEnabled(bool enable) >>> return ret; >>> fdEventNotifier_->setEnabled(enable); >>> - frameStartEnabled_ = enable; >>> + >>> + if (enable) >>> + subscribedEvents_.insert(sub); >>> + else >>> + subscribedEvents_.erase(sub); >>> return ret; >>> } >>> >> >
Hi Barnabas On 05/10/2026 15:55, Barnabás Pőcze wrote: > 2026. 10. 05. 16:51 keltezéssel, Dan Scally írta: >> Hi Barnabas >> >> On 02/10/2026 16:06, Barnabás Pőcze wrote: >>> 2026. 09. 25. 17:16 keltezéssel, Daniel Scally írta: >>>> The current implementation of V4L2Device::setFrameStartEnabled hard >>>> codes V4L2_EVENT_FRAME_SYNC as it passes the subscription to the >>>> kernel. Re-work the function such that the subcription details are >>> >>> subscription >>> >>> >>>> parametised. This will allow consumers to suscribe to any supported >>> >>> subscribe >>> >>> >>>> events. >>>> >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>>> --- >>>> include/libcamera/internal/v4l2_device.h | 5 ++-- >>>> src/libcamera/pipeline/ipu3/cio2.cpp | 6 ++-- >>>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 6 ++-- >>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++-- >>>> .../pipeline/rpi/common/pipeline_base.cpp | 6 ++-- >>>> src/libcamera/pipeline/simple/simple.cpp | 6 ++-- >>>> src/libcamera/v4l2_device.cpp | 32 ++++++++++++++++------ >>>> 7 files changed, 47 insertions(+), 22 deletions(-) >>>> >>>> [...] >>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp >>>> index f231bbb17d8528aca274388b8dcaccc0dc339ad3..b93f3fff7280be1f0357341f217931cd290ba97f 100644 >>>> --- a/src/libcamera/v4l2_device.cpp >>>> +++ b/src/libcamera/v4l2_device.cpp >>>> @@ -58,8 +58,7 @@ LOG_DEFINE_CATEGORY(V4L2) >>>> * at open() time, and the \a logTag to prefix log messages with. >>>> */ >>>> V4L2Device::V4L2Device(const std::string &deviceNode) >>>> - : deviceNode_(deviceNode), fdEventNotifier_(nullptr), >>>> - frameStartEnabled_(false) >>>> + : deviceNode_(deviceNode), fdEventNotifier_(nullptr) >>>> { >>>> } >>>> @@ -499,21 +498,32 @@ bool V4L2Device::supportsEvents(V4L2EventSubscription &sub) >>>> } >>>> /** >>>> - * \brief Enable or disable frame start event notification >>>> + * \brief Enable or event notifications >>>> + * \param[in] sub Details of the event to subscribe to >>>> * \param[in] enable True to enable frame start events, false to disable them >>>> * >>>> - * This function enables or disables generation of frame start events. Once >>>> - * enabled, the events are signalled through the eventReady signal. >>>> + * This function enables or disables generation of events for a particular >>>> + * subscriptions. Once enabled, the events are signalled through the eventReady >>>> + * signal. >>>> * >>>> * \return 0 on success, a negative error code otherwise >>>> */ >>>> -int V4L2Device::setFrameStartEnabled(bool enable) >>>> +int V4L2Device::setEventsEnabled(V4L2EventSubscription &sub, bool enable) >>> >>> Since the type is so small, I'd pass it by value: >>> >>> V4L2EventSubscription sub >>> >>> but at least via a const reference in any case; and then you can do e.g. >>> >>> data->frontendDevice()->setEventsEnabled(V4L2Event::Type::FrameSync, true); >>> >>> which is much more convenient than creating a separate object. >>> Same with `supportsEvents()`. >> >> This needs the v4l2_event_subscription (or some representation of it), as there are V4L2 event >> types that need both the type and id fields (control events for example) for the subscription to >> be specific enough. Or possibly instead of the v4l2_event_subscription, a type parameter and an id >> parameter with a default of 0...but even then we'd need to extend later if we wanted to set non- >> zero flags for the sub.> > > But there is already that type, no? `V4L2EventSubscription`? I'm just saying if you > pass the already existing type by value, then even something like > > data->frontendDevice()->setEventsEnabled(V4L2Event::Type::FrameSync, true); > > will "just" work. And it is equivalent to > > data->frontendDevice()->setEventsEnabled({ /*type*/ V4L2Event::Type::FrameSync, /*id*/ 0 }, true); > > because you made the constructor have `= 0` for the `id` parameter. Ooh. I see...ok let me try that. I dropped V4L2EventSubscription (indeed the entire first patch of this set) but I assume the same applies to struct v4l2_event_subscription Thanks Dna > > >>> >>>> { >>>> - if (frameStartEnabled_ == enable) >>>> + if (sub.type() >= V4L2Event::Type::NumberOfEventTypes) >>> >>> This is effectively already diagnosed by `V4L2Event::typeToV4L2()`, >>> so I think it could be omitted. >>> >>> >>>> + return -EINVAL; >>>> + >>>> + if (subscribedEvents_.find(sub) != subscribedEvents_.end()) >>> >>> subscribedEvents_.contains(sub) >>> >>> is an option since C++20. >> >> ack! >> >> Thanks >> Dan >> >>> >>> >>>> return 0; >>>> struct v4l2_event_subscription event{}; >>>> - event.type = V4L2_EVENT_FRAME_SYNC; >>>> + >>>> + auto v4l2EventType = V4L2Event::typeToV4L2(sub.type()); >>>> + if (!v4l2EventType) >>>> + return -EINVAL; >>>> + >>>> + event.type = *v4l2EventType; >>>> + event.id = sub.id(); >>>> unsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT >>>> : VIDIOC_UNSUBSCRIBE_EVENT; >>>> @@ -522,7 +532,11 @@ int V4L2Device::setFrameStartEnabled(bool enable) >>>> return ret; >>>> fdEventNotifier_->setEnabled(enable); >>>> - frameStartEnabled_ = enable; >>>> + >>>> + if (enable) >>>> + subscribedEvents_.insert(sub); >>>> + else >>>> + subscribedEvents_.erase(sub); >>>> return ret; >>>> } >>>> >>> >> >
2026. 10. 05. 17:00 keltezéssel, Dan Scally írta: > Hi Barnabas > > On 05/10/2026 15:55, Barnabás Pőcze wrote: >> 2026. 10. 05. 16:51 keltezéssel, Dan Scally írta: >>> Hi Barnabas >>> >>> On 02/10/2026 16:06, Barnabás Pőcze wrote: >>>> 2026. 09. 25. 17:16 keltezéssel, Daniel Scally írta: >>>>> The current implementation of V4L2Device::setFrameStartEnabled hard >>>>> codes V4L2_EVENT_FRAME_SYNC as it passes the subscription to the >>>>> kernel. Re-work the function such that the subcription details are >>>> >>>> subscription >>>> >>>> >>>>> parametised. This will allow consumers to suscribe to any supported >>>> >>>> subscribe >>>> >>>> >>>>> events. >>>>> >>>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>>>> --- >>>>> include/libcamera/internal/v4l2_device.h | 5 ++-- >>>>> src/libcamera/pipeline/ipu3/cio2.cpp | 6 ++-- >>>>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 6 ++-- >>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++-- >>>>> .../pipeline/rpi/common/pipeline_base.cpp | 6 ++-- >>>>> src/libcamera/pipeline/simple/simple.cpp | 6 ++-- >>>>> src/libcamera/v4l2_device.cpp | 32 ++++++++++++++++------ >>>>> 7 files changed, 47 insertions(+), 22 deletions(-) >>>>> >>>>> [...] >>>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp >>>>> index f231bbb17d8528aca274388b8dcaccc0dc339ad3..b93f3fff7280be1f0357341f217931cd290ba97f 100644 >>>>> --- a/src/libcamera/v4l2_device.cpp >>>>> +++ b/src/libcamera/v4l2_device.cpp >>>>> @@ -58,8 +58,7 @@ LOG_DEFINE_CATEGORY(V4L2) >>>>> * at open() time, and the \a logTag to prefix log messages with. >>>>> */ >>>>> V4L2Device::V4L2Device(const std::string &deviceNode) >>>>> - : deviceNode_(deviceNode), fdEventNotifier_(nullptr), >>>>> - frameStartEnabled_(false) >>>>> + : deviceNode_(deviceNode), fdEventNotifier_(nullptr) >>>>> { >>>>> } >>>>> @@ -499,21 +498,32 @@ bool V4L2Device::supportsEvents(V4L2EventSubscription &sub) >>>>> } >>>>> /** >>>>> - * \brief Enable or disable frame start event notification >>>>> + * \brief Enable or event notifications >>>>> + * \param[in] sub Details of the event to subscribe to >>>>> * \param[in] enable True to enable frame start events, false to disable them >>>>> * >>>>> - * This function enables or disables generation of frame start events. Once >>>>> - * enabled, the events are signalled through the eventReady signal. >>>>> + * This function enables or disables generation of events for a particular >>>>> + * subscriptions. Once enabled, the events are signalled through the eventReady >>>>> + * signal. >>>>> * >>>>> * \return 0 on success, a negative error code otherwise >>>>> */ >>>>> -int V4L2Device::setFrameStartEnabled(bool enable) >>>>> +int V4L2Device::setEventsEnabled(V4L2EventSubscription &sub, bool enable) >>>> >>>> Since the type is so small, I'd pass it by value: >>>> >>>> V4L2EventSubscription sub >>>> >>>> but at least via a const reference in any case; and then you can do e.g. >>>> >>>> data->frontendDevice()->setEventsEnabled(V4L2Event::Type::FrameSync, true); >>>> >>>> which is much more convenient than creating a separate object. >>>> Same with `supportsEvents()`. >>> >>> This needs the v4l2_event_subscription (or some representation of it), as there are V4L2 event types that need both the type and id fields (control events for example) for the subscription to be specific enough. Or possibly instead of the v4l2_event_subscription, a type parameter and an id parameter with a default of 0...but even then we'd need to extend later if we wanted to set non- zero flags for the sub.> >> >> But there is already that type, no? `V4L2EventSubscription`? I'm just saying if you >> pass the already existing type by value, then even something like >> >> data->frontendDevice()->setEventsEnabled(V4L2Event::Type::FrameSync, true); >> >> will "just" work. And it is equivalent to >> >> data->frontendDevice()->setEventsEnabled({ /*type*/ V4L2Event::Type::FrameSync, /*id*/ 0 }, true); >> >> because you made the constructor have `= 0` for the `id` parameter. > > Ooh. I see...ok let me try that. I dropped V4L2EventSubscription (indeed the entire first patch of this set) but I assume the same applies to struct v4l2_event_subscription Ahh, then nevermind, I didn't realize you dropped the `V4L2EventSubscription` type. The above won't work with `v4l2_event_subscription` sadly. > > Thanks > Dna > >> >> >>>> >>>>> { >>>>> - if (frameStartEnabled_ == enable) >>>>> + if (sub.type() >= V4L2Event::Type::NumberOfEventTypes) >>>> >>>> This is effectively already diagnosed by `V4L2Event::typeToV4L2()`, >>>> so I think it could be omitted. >>>> >>>> >>>>> + return -EINVAL; >>>>> + >>>>> + if (subscribedEvents_.find(sub) != subscribedEvents_.end()) >>>> >>>> subscribedEvents_.contains(sub) >>>> >>>> is an option since C++20. >>> >>> ack! >>> >>> Thanks >>> Dan >>> >>>> >>>> >>>>> return 0; >>>>> struct v4l2_event_subscription event{}; >>>>> - event.type = V4L2_EVENT_FRAME_SYNC; >>>>> + >>>>> + auto v4l2EventType = V4L2Event::typeToV4L2(sub.type()); >>>>> + if (!v4l2EventType) >>>>> + return -EINVAL; >>>>> + >>>>> + event.type = *v4l2EventType; >>>>> + event.id = sub.id(); >>>>> unsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT >>>>> : VIDIOC_UNSUBSCRIBE_EVENT; >>>>> @@ -522,7 +532,11 @@ int V4L2Device::setFrameStartEnabled(bool enable) >>>>> return ret; >>>>> fdEventNotifier_->setEnabled(enable); >>>>> - frameStartEnabled_ = enable; >>>>> + >>>>> + if (enable) >>>>> + subscribedEvents_.insert(sub); >>>>> + else >>>>> + subscribedEvents_.erase(sub); >>>>> return ret; >>>>> } >>>>> >>>> >>> >> >
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index 0c3bbcf2df39f581ceffb9afa6b008368b302156..384323be6db1d46249200b4b4375ac4c85695502 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -10,6 +10,7 @@ #include <map> #include <memory> #include <optional> +#include <set> #include <span> #include <stdint.h> #include <vector> @@ -48,7 +49,7 @@ public: std::string devicePath() const; bool supportsEvents(V4L2EventSubscription &sub); - int setFrameStartEnabled(bool enable); + int setEventsEnabled(V4L2EventSubscription &sub, bool enable); Signal<std::shared_ptr<V4L2Event>> eventReady; void updateControlInfo(); @@ -91,7 +92,7 @@ private: UniqueFD fd_; std::unique_ptr<EventNotifier> fdEventNotifier_; - bool frameStartEnabled_; + std::set<V4L2EventSubscription> subscribedEvents_; }; } /* namespace libcamera */ diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index 7481b2686df070e94fc5f698141db3d3b97896ea..c5b51c222e8544fad6b6a0112dccae35ce3622f0 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -356,7 +356,8 @@ int CIO2Device::start() return ret; } - ret = csi2_->setFrameStartEnabled(true); + V4L2EventSubscription sub(V4L2Event::Type::FrameSync); + ret = csi2_->setEventsEnabled(sub, true); if (ret) { stop(); return ret; @@ -369,7 +370,8 @@ int CIO2Device::stop() { int ret; - csi2_->setFrameStartEnabled(false); + V4L2EventSubscription sub(V4L2Event::Type::FrameSync); + csi2_->setEventsEnabled(sub, false); ret = output_->streamOff(); diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp index 627882f194447912cbed7d97e20bd8baec0e982a..ad5ed3d55958fb496d54da32d8bb3d5d9ec6a758 100644 --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp @@ -1393,7 +1393,8 @@ int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const Control return ret; } - ret = isp_->setFrameStartEnabled(true); + V4L2EventSubscription sub(V4L2Event::Type::FrameSync); + ret = isp_->setEventsEnabled(sub, true); if (ret) LOG(MaliC55, Error) << "Failed to enable frame start events"; @@ -1404,7 +1405,8 @@ void PipelineHandlerMaliC55::stopDevice(Camera *camera) { MaliC55CameraData *data = cameraData(camera); - isp_->setFrameStartEnabled(false); + V4L2EventSubscription sub(V4L2Event::Type::FrameSync); + isp_->setEventsEnabled(sub, false); if (auto *mem = std::get_if<MaliC55CameraData::Memory>(&data->input_)) { ivc_->streamOff(); diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 53ec0007cc95c5ab2da0a5d8a86b5a3330d55f99..9b2ca055aa10f0b7ef5625ab06ec5b4b2ea38f66 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -1305,7 +1305,8 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL return ret; } - isp_->setFrameStartEnabled(true); + V4L2EventSubscription sub(V4L2Event::Type::FrameSync); + isp_->setEventsEnabled(sub, true); activeCamera_ = camera; @@ -1318,7 +1319,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera) RkISP1CameraData *data = cameraData(camera); int ret; - isp_->setFrameStartEnabled(false); + V4L2EventSubscription sub(V4L2Event::Type::FrameSync); + isp_->setEventsEnabled(sub, false); data->ipa_->stop(); @@ -1654,7 +1656,7 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer) * Record the sensor's timestamp in the request metadata. * * \todo The sensor timestamp should be better estimated by connecting - * to the V4L2Device::frameStart signal. + * to the V4L2Device::FrameSync signal. */ request->_d()->metadata().set(controls::SensorTimestamp, metadata.timestamp); diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index c6b51e8c335635788cdc99b7a33e37df7de8a1cb..8c5945cab31755025fad8fa849dc2935876252ca 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp @@ -703,7 +703,8 @@ int PipelineHandlerBase::start(Camera *camera, const ControlList *controls) data->state_ = CameraData::State::Idle; /* Enable SOF event generation. */ - data->frontendDevice()->setFrameStartEnabled(true); + V4L2EventSubscription sub(V4L2Event::Type::FrameSync); + data->frontendDevice()->setEventsEnabled(sub, true); data->platformStart(); @@ -732,7 +733,8 @@ void PipelineHandlerBase::stopDevice(Camera *camera) } /* Disable SOF event generation. */ - data->frontendDevice()->setFrameStartEnabled(false); + V4L2EventSubscription sub(V4L2Event::Type::FrameSync); + data->frontendDevice()->setEventsEnabled(sub, false); data->clearIncompleteRequests(); diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index af3caf74ff56d4e6cc45ad098a93e9fde45449c2..b6c2aa9592bdd27b517274e4d2bd99a59040b42b 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -1679,7 +1679,8 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL data->delayedCtrls_->reset(); if (frameStartEmitter) { - ret = frameStartEmitter->setFrameStartEnabled(true); + V4L2EventSubscription sub(V4L2Event::Type::FrameSync); + ret = frameStartEmitter->setEventsEnabled(sub, true); if (ret) { stop(camera); return ret; @@ -1723,7 +1724,8 @@ void SimplePipelineHandler::stopDevice(Camera *camera) V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_; if (frameStartEmitter) { - frameStartEmitter->setFrameStartEnabled(false); + V4L2EventSubscription sub(V4L2Event::Type::FrameSync); + frameStartEmitter->setEventsEnabled(sub, false); frameStartEmitter->eventReady.connect(data, &SimpleCameraData::handleEvent); } diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index f231bbb17d8528aca274388b8dcaccc0dc339ad3..b93f3fff7280be1f0357341f217931cd290ba97f 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -58,8 +58,7 @@ LOG_DEFINE_CATEGORY(V4L2) * at open() time, and the \a logTag to prefix log messages with. */ V4L2Device::V4L2Device(const std::string &deviceNode) - : deviceNode_(deviceNode), fdEventNotifier_(nullptr), - frameStartEnabled_(false) + : deviceNode_(deviceNode), fdEventNotifier_(nullptr) { } @@ -499,21 +498,32 @@ bool V4L2Device::supportsEvents(V4L2EventSubscription &sub) } /** - * \brief Enable or disable frame start event notification + * \brief Enable or event notifications + * \param[in] sub Details of the event to subscribe to * \param[in] enable True to enable frame start events, false to disable them * - * This function enables or disables generation of frame start events. Once - * enabled, the events are signalled through the eventReady signal. + * This function enables or disables generation of events for a particular + * subscriptions. Once enabled, the events are signalled through the eventReady + * signal. * * \return 0 on success, a negative error code otherwise */ -int V4L2Device::setFrameStartEnabled(bool enable) +int V4L2Device::setEventsEnabled(V4L2EventSubscription &sub, bool enable) { - if (frameStartEnabled_ == enable) + if (sub.type() >= V4L2Event::Type::NumberOfEventTypes) + return -EINVAL; + + if (subscribedEvents_.find(sub) != subscribedEvents_.end()) return 0; struct v4l2_event_subscription event{}; - event.type = V4L2_EVENT_FRAME_SYNC; + + auto v4l2EventType = V4L2Event::typeToV4L2(sub.type()); + if (!v4l2EventType) + return -EINVAL; + + event.type = *v4l2EventType; + event.id = sub.id(); unsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT : VIDIOC_UNSUBSCRIBE_EVENT; @@ -522,7 +532,11 @@ int V4L2Device::setFrameStartEnabled(bool enable) return ret; fdEventNotifier_->setEnabled(enable); - frameStartEnabled_ = enable; + + if (enable) + subscribedEvents_.insert(sub); + else + subscribedEvents_.erase(sub); return ret; }
The current implementation of V4L2Device::setFrameStartEnabled hard codes V4L2_EVENT_FRAME_SYNC as it passes the subscription to the kernel. Re-work the function such that the subcription details are parametised. This will allow consumers to suscribe to any supported events. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- include/libcamera/internal/v4l2_device.h | 5 ++-- src/libcamera/pipeline/ipu3/cio2.cpp | 6 ++-- src/libcamera/pipeline/mali-c55/mali-c55.cpp | 6 ++-- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++-- .../pipeline/rpi/common/pipeline_base.cpp | 6 ++-- src/libcamera/pipeline/simple/simple.cpp | 6 ++-- src/libcamera/v4l2_device.cpp | 32 ++++++++++++++++------ 7 files changed, 47 insertions(+), 22 deletions(-)