[PATCH v3 1/2] libcamera: v4l2_device: add frame start event helpers

[Laurent Pinchart]

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 at ideasonboard.com>
> 
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at 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

-- 
Regards,

Laurent Pinchart