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

[Laurent Pinchart]

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 at ideasonboard.com> # v3
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com> # v5
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at 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

-- 
Regards,

Laurent Pinchart