[v6,1/5] libcamera: v4l2_device: add frame start event helpers

22926 diff mbox series

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.

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

Hi Stanislaw,

Thank you for the patch. 

On Wed, Mar 05, 2025 at 02:52:52PM +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.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # v3
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> # v5
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  include/libcamera/internal/v4l2_device.h |  1 +
>  src/libcamera/v4l2_device.cpp            | 18 ++++++++++++++++++
>  2 files changed, 19 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..c3e9cd4a 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -449,6 +449,24 @@ 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);

Maybe I'm missing something. To me it appears that I must not call this
function while frame start events are enabled, otherwise these events
would then also be unsubscribed as the subscription is per fd - or am I
wrong here? In the current use-cases that does no harm because
supportFrameStart() is always called before enabling frame start events
and never after. But it is an unexpected restriction on the class
interface. Should we mention that in the docs? Caching the result
internally and also filling it from within setFrameStartEnabled() might
be an overkill?

Best regards,
Stefan

> +	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
>

Quoting Stefan Klug (2025-03-31 16:12:58)
> Hi Stanislaw,
> 
> Thank you for the patch. 
> 
> On Wed, Mar 05, 2025 at 02:52:52PM +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.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # v3
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> # v5
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > ---
> >  include/libcamera/internal/v4l2_device.h |  1 +
> >  src/libcamera/v4l2_device.cpp            | 18 ++++++++++++++++++
> >  2 files changed, 19 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..c3e9cd4a 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -449,6 +449,24 @@ 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);
> 
> Maybe I'm missing something. To me it appears that I must not call this
> function while frame start events are enabled, otherwise these events
> would then also be unsubscribed as the subscription is per fd - or am I
> wrong here? In the current use-cases that does no harm because
> supportFrameStart() is always called before enabling frame start events
> and never after. But it is an unexpected restriction on the class
> interface. Should we mention that in the docs? Caching the result
> internally and also filling it from within setFrameStartEnabled() might
> be an overkill?

For what it's worth, I like this approach keeping the 'test' distinctly
separated.

I can see other events I might need to subscribe to in the future - and
I think I'd batch them so they all get checked at startup.

Definitely for sure - we can only 'test' if the event is available
before we subscribe, but with the current limitations, I think this is
the right thing to do.

--
Kieran

> 
> Best regards,
> Stefan
> 
> > +     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
> >

On Mon, Mar 31, 2025 at 05:53:21PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2025-03-31 16:12:58)
> > Hi Stanislaw,
> > 
> > Thank you for the patch. 
> > 
> > On Wed, Mar 05, 2025 at 02:52:52PM +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.
> > > 
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # v3
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> # v5
> > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > > ---
> > >  include/libcamera/internal/v4l2_device.h |  1 +
> > >  src/libcamera/v4l2_device.cpp            | 18 ++++++++++++++++++
> > >  2 files changed, 19 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..c3e9cd4a 100644
> > > --- a/src/libcamera/v4l2_device.cpp
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -449,6 +449,24 @@ 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);
> > 
> > Maybe I'm missing something. To me it appears that I must not call this
> > function while frame start events are enabled, otherwise these events
> > would then also be unsubscribed as the subscription is per fd - or am I
> > wrong here? In the current use-cases that does no harm because
> > supportFrameStart() is always called before enabling frame start events
> > and never after. But it is an unexpected restriction on the class
> > interface. Should we mention that in the docs? Caching the result
> > internally and also filling it from within setFrameStartEnabled() might
> > be an overkill?
> 
> For what it's worth, I like this approach keeping the 'test' distinctly
> separated.
> 
> I can see other events I might need to subscribe to in the future - and
> I think I'd batch them so they all get checked at startup.
> 
> Definitely for sure - we can only 'test' if the event is available
> before we subscribe, but with the current limitations, I think this is
> the right thing to do.

Should we try to add an enum event API extension to V4L2 in parallel to
this ?

> > > +     return true;
> > > +}
> > > +
> > >  /**
> > >   * \brief Enable or disable frame start event notification
> > >   * \param[in] enable True to enable frame start events, false to disable them

On Mon, Mar 31, 2025 at 05:12:58PM +0200, Stefan Klug wrote:
> Hi Stanislaw,
> 
> Thank you for the patch. 
> 
> On Wed, Mar 05, 2025 at 02:52:52PM +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.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # v3
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> # v5
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > ---
> >  include/libcamera/internal/v4l2_device.h |  1 +
> >  src/libcamera/v4l2_device.cpp            | 18 ++++++++++++++++++
> >  2 files changed, 19 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..c3e9cd4a 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -449,6 +449,24 @@ 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
> > + */
> > +

Stray empty line.

> > +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);
> 
> Maybe I'm missing something. To me it appears that I must not call this
> function while frame start events are enabled, otherwise these events
> would then also be unsubscribed as the subscription is per fd - or am I
> wrong here? In the current use-cases that does no harm because
> supportFrameStart() is always called before enabling frame start events
> and never after. But it is an unexpected restriction on the class
> interface. Should we mention that in the docs? Caching the result
> internally and also filling it from within setFrameStartEnabled() might
> be an overkill?

Let's at least expand the documentation a bit, as that's easy:

/**
 * \brief Check if frame start event is supported
 *
 * Due to limitations in the kernel API, this function may disable the frame
 * start event as a side effect. It should only be called during initialization,
 * before enabling the frame start event with setFrameStartEnabled().
 *
 * \return True if frame start event is supported, false otherwise
 */

This will improve the user experience a lot, they will now be able to
get a weird behaviour because they didn't read the documentation,
instead of getting the same behaviour because it was undocumented :-)

> > +	return true;
> > +}
> > +
> >  /**
> >   * \brief Enable or disable frame start event notification
> >   * \param[in] enable True to enable frame start events, false to disable them