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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart