[libcamera-devel] [PATCH 07/13] libcamera: v4l2_subdevice: Collect subdev capabilities

[Laurent Pinchart]

Hi Kieran,

On Wed, Aug 03, 2022 at 12:58:29PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-08-01 01:05:37)
> > From: Jacopo Mondi <jacopo at jmondi.org>
> > 
> > Collect subdev capabilities at open() time.
> > 
> > Model the V4L2SubdevCapabilties as V4L2Capability from the video
> > device class.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > Changes on top of Jacopo's initial work:
> > 
> > - Rename V4L2SubdevCapabilities to V4L2SubdeviceCapability and make it
> >   final, to match the V4L2VideoDevice class
> > - Add V4L2Subdevice::caps()
> > ---
> >  include/libcamera/internal/v4l2_subdevice.h | 13 ++++++
> >  src/libcamera/v4l2_subdevice.cpp            | 50 ++++++++++++++++++++-
> >  2 files changed, 62 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> > index 2db392d5e37a..a1d3144c6a7f 100644
> > --- a/include/libcamera/internal/v4l2_subdevice.h
> > +++ b/include/libcamera/internal/v4l2_subdevice.h
> > @@ -29,6 +29,17 @@ namespace libcamera {
> >  
> >  class MediaDevice;
> >  
> > +struct V4L2SubdeviceCapability final : v4l2_subdev_capability {
> > +       bool isReadOnly() const
> > +       {
> > +               return capabilities & V4L2_SUBDEV_CAP_RO_SUBDEV;
> > +       }
> > +       bool hasStreams() const
> > +       {
> > +               return capabilities & V4L2_SUBDEV_CAP_MPLEXED;
> > +       }
> > +};
> > +
> >  struct V4L2SubdeviceFormat {
> >         uint32_t mbus_code;
> >         Size size;
> > @@ -70,6 +81,7 @@ public:
> >                       Whence whence = ActiveFormat);
> >  
> >         const std::string &model();
> > +       const V4L2SubdeviceCapability &caps() const { return caps_; }
> >  
> >         static std::unique_ptr<V4L2Subdevice>
> >         fromEntityName(const MediaDevice *media, const std::string &entity);
> > @@ -87,6 +99,7 @@ private:
> >         const MediaEntity *entity_;
> >  
> >         std::string model_;
> > +       struct V4L2SubdeviceCapability caps_;
> >  };
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 37960b76a044..a1672b2365f2 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -133,6 +133,30 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> >  
> >  } /* namespace */
> >  
> > +/**
> > + * \struct V4L2SubdeviceCapability
> > + * \brief struct v4l2_subdev_capability object wrapper and helpers
> > + *
> > + * The V4L2SubdeviceCapability structure manages the information returned by the
> > + * VIDIOC_SUBDEV_QUERYCAP ioctl.
> > + */
> > +
> > +/**
> > + * \fn V4L2SubdeviceCapability::isReadOnly()
> > + * \brief Retrieve if a subdevice is registered as read-only
> > + *
> > + * A V4L2 subdevice is registered as read-only if V4L2_SUBDEV_CAP_RO_SUBDEV
> > + * is listed as part of its capabilities.
> > + *
> > + * \return True if the subdevice is registered as read-only, false otherwise
> > + */
> > +
> > +/**
> > + * \fn V4L2SubdeviceCapability::hasStreams()
> > + * \brief Retrieve if a subdevice supports the V4L2 streams API
> > + * \return True if the subdevice supports the streams API, false otherwise
> > + */
> > +
> >  /**
> >   * \struct V4L2SubdeviceFormat
> >   * \brief The V4L2 sub-device image format and sizes
> > @@ -284,7 +308,25 @@ V4L2Subdevice::~V4L2Subdevice()
> >   */
> >  int V4L2Subdevice::open()
> >  {
> > -       return V4L2Device::open(O_RDWR);
> > +       int ret = V4L2Device::open(O_RDWR);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /*
> > +        * Try to query the subdev capabilities. The VIDIOC_SUBDEV_QUERYCAP API
> > +        * was introduced in kernel v5.8, ENOTTY errors must be ignored to
> > +        * support older kernels.
> > +        */
> > +       caps_ = {};
> > +       ret = ioctl(VIDIOC_SUBDEV_QUERYCAP, &caps_);
> > +       if (ret < 0 && errno != ENOTTY) {
> > +               ret = -errno;
> > +               LOG(V4L2, Error)
> > +                       << "Unable to query capabilities: " << strerror(-ret);
> > +               return ret;
> > +       }
> 
> I wonder if we should report ENOTTY errors through a Debug level print,
> but perhaps we could do that generically in our ioctl call (in a
> separate patch) to report that an attempt was made on an unsupported
> call?

I've thought about it, but even as a debug message, I fear it may be too
verbose if it ends up being triggered by ioctls we call constantly. I
suppose that may be more of a risk on video nodes than on subdevs, so
maybe that could be a middle ground ? I'm tempted to delay it until we
find a need during debugging though.

> Otherwise, I like this.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > +
> > +       return 0;
> >  }
> >  
> >  /**
> > @@ -527,6 +569,12 @@ const std::string &V4L2Subdevice::model()
> >         return model_;
> >  }
> >  
> > +/**
> > + * \fn V4L2Subdevice::caps()
> > + * \brief Retrieve the subdevice V4L2 capabilities
> > + * \return The subdevice V4L2 capabilities
> > + */
> > +
> >  /**
> >   * \brief Create a new video subdevice instance from \a entity in media device
> >   * \a media

-- 
Regards,

Laurent Pinchart