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

16884 diff mbox series

From: Jacopo Mondi <jacopo@jmondi.org>

Collect subdev capabilities at open() time.

Model the V4L2SubdevCapabilties as V4L2Capability from the video
device class.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@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(-)

Quoting Laurent Pinchart via libcamera-devel (2022-08-01 01:05:37)
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Collect subdev capabilities at open() time.
> 
> Model the V4L2SubdevCapabilties as V4L2Capability from the video
> device class.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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?

Otherwise, I like this.


Reviewed-by: Kieran Bingham <kieran.bingham@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
>

On Mon, Aug 01, 2022 at 03:05:37AM +0300, Laurent Pinchart via libcamera-devel wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Collect subdev capabilities at open() time.
> 
> Model the V4L2SubdevCapabilties as V4L2Capability from the video
> device class.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@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;
> +	}
> +
> +	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
>

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@jmondi.org>
> > 
> > Collect subdev capabilities at open() time.
> > 
> > Model the V4L2SubdevCapabilties as V4L2Capability from the video
> > device class.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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@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