[v5,1/5] libcamera: v4l2_device: add frame start event helpers
diff mbox series

Message ID 20250225164116.414301-2-stanislaw.gruszka@linux.intel.com
State Superseded
Headers show
Series
  • libcamera: start frame events changes
Related show

Commit Message

Stanislaw Gruszka Feb. 25, 2025, 4:41 p.m. UTC 
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.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # v3
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 include/libcamera/internal/v4l2_device.h |  1 +
 src/libcamera/v4l2_device.cpp            | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

Comments

Kieran Bingham March 1, 2025, 11:17 p.m. UTC | #1 
Quoting Stanislaw Gruszka (2025-02-25 16:41:12)
> 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.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # v3
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  include/libcamera/internal/v4l2_device.h |  1 +
>  src/libcamera/v4l2_device.cpp            | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index affe52c2..a647c96a 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -45,6 +45,7 @@ public:
>         const std::string &deviceNode() const { return deviceNode_; }
>         std::string devicePath() const;
>  
> +       bool supportsFrameStartEvent();
>         int setFrameStartEnabled(bool enable);
>         Signal<uint32_t> frameStart;
>  
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 2f65a43a..23c7d1ef 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -449,6 +449,25 @@ 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 {
> +       };

Curiously my local clang-format is trying to make this one line, but the
CI doesn't report anything here.

So I'll leave it as it is ;-)

Aha, and I've already got a tag on here so ... to the next one.

--
Kieran

> +       event.type = V4L2_EVENT_FRAME_SYNC;
> +
> +       int ret = ioctl(VIDIOC_SUBSCRIBE_EVENT, &event);
> +       if (ret)
> +               return false;
> +
> +       ioctl(VIDIOC_UNSUBSCRIBE_EVENT, &event);
> +       return true;
> +}
> +
>  /**
>   * \brief Enable or disable frame start event notification
>   * \param[in] enable True to enable frame start events, false to disable them
> -- 
> 2.43.0
>
Laurent Pinchart March 2, 2025, 12:38 a.m. UTC | #2 
Hi Stan,

Thank you for the patch.

On Tue, Feb 25, 2025 at 05:41:12PM +0100, Stanislaw Gruszka wrote:
> 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.
> 

That paragraph should be dropped, that second helper has been removed.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # v3
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  include/libcamera/internal/v4l2_device.h |  1 +
>  src/libcamera/v4l2_device.cpp            | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index affe52c2..a647c96a 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -45,6 +45,7 @@ public:
>  	const std::string &deviceNode() const { return deviceNode_; }
>  	std::string devicePath() const;
>  
> +	bool supportsFrameStartEvent();
>  	int setFrameStartEnabled(bool enable);
>  	Signal<uint32_t> frameStart;
>  
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 2f65a43a..23c7d1ef 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -449,6 +449,25 @@ 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)
> +		return false;
> +
> +	ioctl(VIDIOC_UNSUBSCRIBE_EVENT, &event);
> +	return true;
> +}
> +
>  /**
>   * \brief Enable or disable frame start event notification
>   * \param[in] enable True to enable frame start events, false to disable them
Stanislaw Gruszka March 3, 2025, 10:26 a.m. UTC | #3 
Hi Kieran,

On Sat, Mar 01, 2025 at 11:17:32PM +0000, Kieran Bingham wrote:
> Quoting Stanislaw Gruszka (2025-02-25 16:41:12)
> >  
> > +/**
> > + * \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 {
> > +       };
> 
> Curiously my local clang-format is trying to make this one line, but the
> CI doesn't report anything here.
> 
> So I'll leave it as it is ;-)
> 
> Aha, and I've already got a tag on here so ... to the next one.

Does it mean you have a clang-format-ignore file ?

Regards
Stanislaw
Barnabás Pőcze March 3, 2025, 11:01 a.m. UTC | #4 
Hi


2025. 03. 02. 0:17 keltezéssel, Kieran Bingham írta:
> Quoting Stanislaw Gruszka (2025-02-25 16:41:12)
>> 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.
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # v3
>> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
>> ---
>>   include/libcamera/internal/v4l2_device.h |  1 +
>>   src/libcamera/v4l2_device.cpp            | 19 +++++++++++++++++++
>>   2 files changed, 20 insertions(+)
>>
>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
>> index affe52c2..a647c96a 100644
>> --- a/include/libcamera/internal/v4l2_device.h
>> +++ b/include/libcamera/internal/v4l2_device.h
>> @@ -45,6 +45,7 @@ public:
>>          const std::string &deviceNode() const { return deviceNode_; }
>>          std::string devicePath() const;
>>
>> +       bool supportsFrameStartEvent();
>>          int setFrameStartEnabled(bool enable);
>>          Signal<uint32_t> frameStart;
>>
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 2f65a43a..23c7d1ef 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -449,6 +449,25 @@ 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 {
>> +       };
> 
> Curiously my local clang-format is trying to make this one line, but the
> CI doesn't report anything here.
> 
> So I'll leave it as it is ;-)
> 
> Aha, and I've already got a tag on here so ... to the next one.

Mine also does not like it (clang-format version 19.1.7):

$ ./utils/checkstyle.py HEAD~5..
----------------------------------------------------------------------------------------------
e7ff7c085a79342e933ae378e710e98a52f1799d libcamera: v4l2_device: add frame start event helpers
----------------------------------------------------------------------------------------------
--- src/libcamera/v4l2_device.cpp
+++ src/libcamera/v4l2_device.cpp
@@ -456,8 +456,7 @@
  
  bool V4L2Device::supportsFrameStartEvent()
  {
-	struct v4l2_event_subscription event {
-	};
+	struct v4l2_event_subscription event{};
  	event.type = V4L2_EVENT_FRAME_SYNC;
  
  	int ret = ioctl(VIDIOC_SUBSCRIBE_EVENT, &event);
---
1 potential issue detected, please review

---------------------------------------------------------------------------------------------------
2f377f31a4b7f9993c765208ece1c20430def83f pipeline: simple: Use proper device for frame start events
---------------------------------------------------------------------------------------------------
No issue detected

----------------------------------------------------------------------------------------------------
80571431a1d6d3cd9b2540d6f85e247b442a5036 pipeline: simple: Connect and disconnect start frame events
----------------------------------------------------------------------------------------------------
No issue detected

--------------------------------------------------------------------------------------
56169675ca7acee83648c1fa04278da3f446e0c6 pipeline: simple: Do not apply controls twice
--------------------------------------------------------------------------------------
No issue detected

----------------------------------------------------------------------------------------------------
005bd327d8ab71172ee7b8c61dac826e78af520c pipeline: simple: Create DelayedControls instance once only
----------------------------------------------------------------------------------------------------
No issue detected


Regards,
Barnabás Pőcze

> 
> --
> Kieran
> 
>> +       event.type = V4L2_EVENT_FRAME_SYNC;
>> +
>> +       int ret = ioctl(VIDIOC_SUBSCRIBE_EVENT, &event);
>> +       if (ret)
>> +               return false;
>> +
>> +       ioctl(VIDIOC_UNSUBSCRIBE_EVENT, &event);
>> +       return true;
>> +}
>> +
>>   /**
>>    * \brief Enable or disable frame start event notification
>>    * \param[in] enable True to enable frame start events, false to disable them
>> --
>> 2.43.0
>>
Kieran Bingham March 3, 2025, 12:48 p.m. UTC | #5 
Quoting Stanislaw Gruszka (2025-03-03 10:26:38)
> Hi Kieran,
> 
> On Sat, Mar 01, 2025 at 11:17:32PM +0000, Kieran Bingham wrote:
> > Quoting Stanislaw Gruszka (2025-02-25 16:41:12)
> > >  
> > > +/**
> > > + * \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 {
> > > +       };
> > 
> > Curiously my local clang-format is trying to make this one line, but the
> > CI doesn't report anything here.
> > 
> > So I'll leave it as it is ;-)
> > 
> > Aha, and I've already got a tag on here so ... to the next one.
> 
> Does it mean you have a clang-format-ignore file ?

I'm not sure I understand the question. But no - I don't have an
'ignore' file on my laptop. Or do you mean one in my head that I can
ignore things ;-)

--
Kieran

> 
> Regards
> Stanislaw
>
Stanislaw Gruszka March 3, 2025, 2:08 p.m. UTC | #6 
On Mon, Mar 03, 2025 at 12:48:28PM +0000, Kieran Bingham wrote:
> Quoting Stanislaw Gruszka (2025-03-03 10:26:38)
> > Hi Kieran,
> > 
> > On Sat, Mar 01, 2025 at 11:17:32PM +0000, Kieran Bingham wrote:
> > > Quoting Stanislaw Gruszka (2025-02-25 16:41:12)
> > > >  
> > > > +/**
> > > > + * \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 {
> > > > +       };
> > > 
> > > Curiously my local clang-format is trying to make this one line, but the
> > > CI doesn't report anything here.
> > > 
> > > So I'll leave it as it is ;-)
> > > 
> > > Aha, and I've already got a tag on here so ... to the next one.
> > 
> > Does it mean you have a clang-format-ignore file ?
> 
> I'm not sure I understand the question. But no - I don't have an
> 'ignore' file on my laptop. Or do you mean one in my head that I can
> ignore things ;-)

:-) I misunderstood your previous comment.

I thought you wrote about clang-format tag that makes auto-formatter
ignore particular file or lines, but you meant Reviewed-by tag :-)

Anyway, this is clang-format version problem as pointed by Barnabás
and Milan. I'll update the patch with:

      struct v4l2_event_subscription event {};

since newer clang-format formats this way.

Regards
Stanislaw

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
index affe52c2..a647c96a 100644
--- a/include/libcamera/internal/v4l2_device.h
+++ b/include/libcamera/internal/v4l2_device.h
@@ -45,6 +45,7 @@  public:
 	const std::string &deviceNode() const { return deviceNode_; }
 	std::string devicePath() const;
 
+	bool supportsFrameStartEvent();
 	int setFrameStartEnabled(bool enable);
 	Signal<uint32_t> frameStart;
 
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 2f65a43a..23c7d1ef 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -449,6 +449,25 @@  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)
+		return false;
+
+	ioctl(VIDIOC_UNSUBSCRIBE_EVENT, &event);
+	return true;
+}
+
 /**
  * \brief Enable or disable frame start event notification
  * \param[in] enable True to enable frame start events, false to disable them