| Message ID | 20250218091951.13017-2-stanislaw.gruszka@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stanislaw Gruszka (2025-02-18 09:19:50) > Add helper to check if frame start event are supported by subdevice. > Since kernel does not have interface to query supported events > use subscribe interface. > > Additionally add helper for checking if the event enabled. > Will be needed later to proper code flow if enabling the event > fail for some reason. > I've started contemplating tackling this too - with a 'V4L2Event' wrapper ... but that's overkill while we have only a single event to manage, so I think this is fine for now. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > --- > include/libcamera/internal/v4l2_device.h | 2 ++ > src/libcamera/v4l2_device.cpp | 24 ++++++++++++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > index affe52c2..0d807209 100644 > --- a/include/libcamera/internal/v4l2_device.h > +++ b/include/libcamera/internal/v4l2_device.h > @@ -45,6 +45,8 @@ public: > const std::string &deviceNode() const { return deviceNode_; } > std::string devicePath() const; > > + bool supportsFrameStartEvent(); > + bool frameStartEnabled() { return frameStartEnabled_; } > int setFrameStartEnabled(bool enable); > Signal<uint32_t> frameStart; > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 2f65a43a..219c82f6 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -449,6 +449,30 @@ std::string V4L2Device::devicePath() const > return path; > } > > +/** > + * \brief Check if frame start event is supported > + * \return True if frame start event is supported, false otherwise > + */ > + > +bool V4L2Device::supportsFrameStartEvent() > +{ > + struct v4l2_event_subscription event { > + }; > + event.type = V4L2_EVENT_FRAME_SYNC; > + > + int ret = ioctl(VIDIOC_SUBSCRIBE_EVENT, &event); > + if (ret == 0) > + ioctl(VIDIOC_UNSUBSCRIBE_EVENT, &event); > + > + return ret == 0; > +} > + > +/** > + * \fn V4L2Device::frameStartEnabled() > + * \brief Check if frame start event is enabled > + * \return True if frame start event is enabled, false otherwise > + */ > + > /** > * \brief Enable or disable frame start event notification > * \param[in] enable True to enable frame start events, false to disable them > -- > 2.43.0 >
Hello, On Tue, Feb 18, 2025 at 11:56:10AM +0000, Kieran Bingham wrote: > Quoting Stanislaw Gruszka (2025-02-18 09:19:50) > > Add helper to check if frame start event are supported by subdevice. > > Since kernel does not have interface to query supported events > > use subscribe interface. > > > > Additionally add helper for checking if the event enabled. > > Will be needed later to proper code flow if enabling the event > > fail for some reason. > > I've started contemplating tackling this too - with a 'V4L2Event' > wrapper ... but that's overkill while we have only a single event to > manage, so I think this is fine for now. We don't need a V4L2Event type, I think we can use an uint32_t to make the function more generic, it should be an easy change. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > > --- > > include/libcamera/internal/v4l2_device.h | 2 ++ > > src/libcamera/v4l2_device.cpp | 24 ++++++++++++++++++++++++ > > 2 files changed, 26 insertions(+) > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > index affe52c2..0d807209 100644 > > --- a/include/libcamera/internal/v4l2_device.h > > +++ b/include/libcamera/internal/v4l2_device.h > > @@ -45,6 +45,8 @@ public: > > const std::string &deviceNode() const { return deviceNode_; } > > std::string devicePath() const; > > > > + bool supportsFrameStartEvent(); > > + bool frameStartEnabled() { return frameStartEnabled_; } The function should be const. > > int setFrameStartEnabled(bool enable); > > Signal<uint32_t> frameStart; > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 2f65a43a..219c82f6 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -449,6 +449,30 @@ std::string V4L2Device::devicePath() const > > return path; > > } > > > > +/** > > + * \brief Check if frame start event is supported > > + * \return True if frame start event is supported, false otherwise > > + */ > > + > > +bool V4L2Device::supportsFrameStartEvent() > > +{ > > + struct v4l2_event_subscription event { > > + }; > > + event.type = V4L2_EVENT_FRAME_SYNC; > > + > > + int ret = ioctl(VIDIOC_SUBSCRIBE_EVENT, &event); > > + if (ret == 0) > > + ioctl(VIDIOC_UNSUBSCRIBE_EVENT, &event); > > + > > + return ret == 0; I think the following would be a bit more readable. int ret = ioctl(VIDIOC_SUBSCRIBE_EVENT, &event); if (ret) return false; ioctl(VIDIOC_UNSUBSCRIBE_EVENT, &event); return true; > > +} > > + > > +/** > > + * \fn V4L2Device::frameStartEnabled() > > + * \brief Check if frame start event is enabled > > + * \return True if frame start event is enabled, false otherwise > > + */ > > + > > /** > > * \brief Enable or disable frame start event notification > > * \param[in] enable True to enable frame start events, false to disable them
Hi Laurent, On Wed, Feb 19, 2025 at 02:35:07PM +0200, Laurent Pinchart wrote: > > > + bool frameStartEnabled() { return frameStartEnabled_; } > > The function should be const. Right. > > > +bool V4L2Device::supportsFrameStartEvent() > > > +{ > > > + struct v4l2_event_subscription event { > > > + }; > > > + event.type = V4L2_EVENT_FRAME_SYNC; > > > + > > > + int ret = ioctl(VIDIOC_SUBSCRIBE_EVENT, &event); > > > + if (ret == 0) > > > + ioctl(VIDIOC_UNSUBSCRIBE_EVENT, &event); > > > + > > > + return ret == 0; > > I think the following would be a bit more readable. > > int ret = ioctl(VIDIOC_SUBSCRIBE_EVENT, &event); > if (ret) > return false; > > ioctl(VIDIOC_UNSUBSCRIBE_EVENT, &event); > return true; Fine, will recode this way in next version of the patches. Regards Stanislaw
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index affe52c2..0d807209 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -45,6 +45,8 @@ public: const std::string &deviceNode() const { return deviceNode_; } std::string devicePath() const; + bool supportsFrameStartEvent(); + bool frameStartEnabled() { return frameStartEnabled_; } int setFrameStartEnabled(bool enable); Signal<uint32_t> frameStart; diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 2f65a43a..219c82f6 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -449,6 +449,30 @@ std::string V4L2Device::devicePath() const return path; } +/** + * \brief Check if frame start event is supported + * \return True if frame start event is supported, false otherwise + */ + +bool V4L2Device::supportsFrameStartEvent() +{ + struct v4l2_event_subscription event { + }; + event.type = V4L2_EVENT_FRAME_SYNC; + + int ret = ioctl(VIDIOC_SUBSCRIBE_EVENT, &event); + if (ret == 0) + ioctl(VIDIOC_UNSUBSCRIBE_EVENT, &event); + + return ret == 0; +} + +/** + * \fn V4L2Device::frameStartEnabled() + * \brief Check if frame start event is enabled + * \return True if frame start event is enabled, false otherwise + */ + /** * \brief Enable or disable frame start event notification * \param[in] enable True to enable frame start events, false to disable them
Add helper to check if frame start event are supported by subdevice. Since kernel does not have interface to query supported events use subscribe interface. Additionally add helper for checking if the event enabled. Will be needed later to proper code flow if enabling the event fail for some reason. Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> --- include/libcamera/internal/v4l2_device.h | 2 ++ src/libcamera/v4l2_device.cpp | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+)