[7/9] libcamera: v4l2_subdevice: Add stream support to get/set functions

19558 diff mbox series

Extend the V4L2Subdevice API with stream support for the functions that
get and set formats and selection rectangles. Add a Stream structure to
identify a subdev pad and stream, and use it to extend the V4L2Subdevice
functions that get and set formats and selection rectangles with stream
support.

To preserve the existing pad-based API, implement overloaded functions
that wrap the new stream-based API. This allows callers that are not
stream-aware to use a simpler pad-based API, instead of having to
explicitly set the stream number to 0 in all API calls.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/v4l2_subdevice.h |  41 +++-
 src/libcamera/v4l2_subdevice.cpp            | 232 ++++++++++++++------
 2 files changed, 202 insertions(+), 71 deletions(-)

Hi Laurent

On Tue, Feb 27, 2024 at 04:09:51PM +0200, Laurent Pinchart wrote:
> Extend the V4L2Subdevice API with stream support for the functions that
> get and set formats and selection rectangles. Add a Stream structure to
> identify a subdev pad and stream, and use it to extend the V4L2Subdevice
> functions that get and set formats and selection rectangles with stream
> support.
>
> To preserve the existing pad-based API, implement overloaded functions
> that wrap the new stream-based API. This allows callers that are not
> stream-aware to use a simpler pad-based API, instead of having to
> explicitly set the stream number to 0 in all API calls.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_subdevice.h |  41 +++-
>  src/libcamera/v4l2_subdevice.cpp            | 232 ++++++++++++++------
>  2 files changed, 202 insertions(+), 71 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 1115cfa55594..65003394a984 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -80,6 +80,11 @@ public:
>  		ActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,
>  	};
>
> +	struct Stream {
> +		unsigned int pad;
> +		unsigned int stream;
> +	};
> +
>  	class Routing : public std::vector<struct v4l2_subdev_route>
>  	{
>  	public:
> @@ -93,17 +98,39 @@ public:
>
>  	const MediaEntity *entity() const { return entity_; }
>
> -	int getSelection(unsigned int pad, unsigned int target,
> +	int getSelection(const Stream &stream, unsigned int target,
>  			 Rectangle *rect);
> -	int setSelection(unsigned int pad, unsigned int target,
> +	int getSelection(unsigned int pad, unsigned int target, Rectangle *rect)
> +	{
> +		return getSelection({ pad, 0 }, target, rect);
> +	}
> +	int setSelection(const Stream &stream, unsigned int target,
>  			 Rectangle *rect);
> +	int setSelection(unsigned int pad, unsigned int target, Rectangle *rect)
> +	{
> +		return setSelection({ pad, 0 }, target, rect);
> +	}
>
> -	Formats formats(unsigned int pad);
> +	Formats formats(const Stream &stream);
> +	Formats formats(unsigned int pad)
> +	{
> +		return formats({ pad, 0 });
> +	}
>
> +	int getFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> +		      Whence whence = ActiveFormat);
>  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> +		      Whence whence = ActiveFormat)
> +	{
> +		return getFormat({ pad, 0 }, format, whence);
> +	}
> +	int setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
>  		      Whence whence = ActiveFormat);
>  	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> -		      Whence whence = ActiveFormat);
> +		      Whence whence = ActiveFormat)
> +	{
> +		return setFormat({ pad, 0 }, format, whence);
> +	}
>
>  	int getRouting(Routing *routing, Whence whence = ActiveFormat);
>  	int setRouting(Routing *routing, Whence whence = ActiveFormat);
> @@ -123,8 +150,8 @@ private:
>  	std::optional<ColorSpace>
>  	toColorSpace(const v4l2_mbus_framefmt &format) const;
>
> -	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> -	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> +	std::vector<unsigned int> enumPadCodes(const Stream &stream);
> +	std::vector<SizeRange> enumPadSizes(const Stream &stream,
>  					    unsigned int code);
>
>  	const MediaEntity *entity_;
> @@ -133,4 +160,6 @@ private:
>  	struct V4L2SubdeviceCapability caps_;
>  };
>
> +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 19ddecf714c6..efe8b0f793e9 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -814,6 +814,35 @@ std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f)
>   * \brief The format operation applies to TRY formats
>   */
>
> +/**
> + * \class V4L2Subdevice::Stream
> + * \brief V4L2 subdevice stream
> + *
> + * This class identifies a subdev stream, by bundling the pad number with the
> + * stream number. It is used in all stream-aware functions of the V4L2Subdevice
> + * class to identify the stream the functions operate on.
> + *
> + * \var V4L2Subdevice::Stream::pad
> + * \brief The 0-indexed pad number
> + *
> + * \var V4L2Subdevice::Stream::stream
> + * \brief The stream number
> + */
> +
> +/**
> + * \brief Insert a text representation of a V4L2Subdevice::Stream into an
> + *	output stream

What is the alignment rule when we break \brief ? I see different ones
being used in this patch

> + * \param[in] out The output stream
> + * \param[in] stream The V4L2Subdevice::Stream
> + * \return The output stream \a out
> + */
> +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)
> +{
> +	out << stream.pad << "/" << stream.stream;
> +
> +	return out;
> +}
> +
>  /**
>   * \class V4L2Subdevice::Routing
>   * \brief V4L2 subdevice routing table
> @@ -879,7 +908,10 @@ int V4L2Subdevice::open()
>  		return ret;
>  	}
>
> -	/* If the subdev supports streams, enable the streams API. */
> +	/*
> +	 * If the subdev supports streams, enable the streams API, and retrieve
> +	 * and cache the routing table.

Does it happen in this patch ?

> +	 */
>  	if (caps_.hasStreams()) {
>  		struct v4l2_subdev_client_capability clientCaps{};
>  		clientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> @@ -905,7 +937,7 @@ int V4L2Subdevice::open()
>
>  /**
>   * \brief Get selection rectangle \a rect for \a target
> - * \param[in] pad The 0-indexed pad number the rectangle is retrieved from
> + * \param[in] stream The stream the rectangle is retrieved from
>   * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
>   * \param[out] rect The retrieved selection rectangle
>   *
> @@ -913,13 +945,14 @@ int V4L2Subdevice::open()
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> +int V4L2Subdevice::getSelection(const Stream &stream, unsigned int target,
>  				Rectangle *rect)
>  {
>  	struct v4l2_subdev_selection sel = {};
>
>  	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -	sel.pad = pad;
> +	sel.pad = stream.pad;
> +	sel.stream = stream.stream;
>  	sel.target = target;
>  	sel.flags = 0;
>
> @@ -927,7 +960,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
>  	if (ret < 0) {
>  		LOG(V4L2, Error)
>  			<< "Unable to get rectangle " << target << " on pad "
> -			<< pad << ": " << strerror(-ret);
> +			<< stream.pad << "/" << stream.stream << ": "

Can't you use operator<<(Stream) ?

> +			<< strerror(-ret);
>  		return ret;
>  	}
>
> @@ -939,9 +973,20 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
>  	return 0;
>  }
>
> +/**
> + * \fn V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> + *	Rectangle *rect)
> + * \brief Get selection rectangle \a rect for \a target
> + * \param[in] pad The 0-indexed pad number the rectangle is retrieved from
> + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> + * \param[out] rect The retrieved selection rectangle
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
>  /**
>   * \brief Set selection rectangle \a rect for \a target
> - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> + * \param[in] stream The stream the rectangle is to be applied to
>   * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
>   * \param[inout] rect The selection rectangle to be applied
>   *
> @@ -949,13 +994,14 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> +int V4L2Subdevice::setSelection(const Stream &stream, unsigned int target,
>  				Rectangle *rect)
>  {
>  	struct v4l2_subdev_selection sel = {};
>
>  	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -	sel.pad = pad;
> +	sel.pad = stream.pad;
> +	sel.stream = stream.stream;
>  	sel.target = target;
>  	sel.flags = 0;
>
> @@ -968,7 +1014,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
>  	if (ret < 0) {
>  		LOG(V4L2, Error)
>  			<< "Unable to set rectangle " << target << " on pad "
> -			<< pad << ": " << strerror(-ret);
> +			<< stream.pad << "/" << stream.stream << ": "

ditto

> +			<< strerror(-ret);
>  		return ret;
>  	}
>
> @@ -979,26 +1026,40 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
>
>  	return 0;
>  }
> +
>  /**
> - * \brief Enumerate all media bus codes and frame sizes on a \a pad
> - * \param[in] pad The 0-indexed pad number to enumerate formats on
> + * \fn V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> + *	Rectangle *rect)
> + * \brief Set selection rectangle \a rect for \a target
> + * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> + * \param[inout] rect The selection rectangle to be applied
> + *
> + * \todo Define a V4L2SelectionTarget enum for the selection target
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \brief Enumerate all media bus codes and frame sizes on a \a stream
> + * \param[in] stream The stream to enumerate formats for
>   *
>   * Enumerate all media bus codes and frame sizes supported by the subdevice on
> - * a \a pad.
> + * a \a stream.
>   *
>   * \return A list of the supported device formats
>   */
> -V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> +V4L2Subdevice::Formats V4L2Subdevice::formats(const Stream &stream)
>  {
>  	Formats formats;
>
> -	if (pad >= entity_->pads().size()) {
> -		LOG(V4L2, Error) << "Invalid pad: " << pad;
> +	if (stream.pad >= entity_->pads().size()) {
> +		LOG(V4L2, Error) << "Invalid pad: " << stream.pad;
>  		return {};
>  	}
>
> -	for (unsigned int code : enumPadCodes(pad)) {
> -		std::vector<SizeRange> sizes = enumPadSizes(pad, code);
> +	for (unsigned int code : enumPadCodes(stream)) {
> +		std::vector<SizeRange> sizes = enumPadSizes(stream, code);
>  		if (sizes.empty())
>  			return {};
>
> @@ -1006,7 +1067,7 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
>  		if (!inserted.second) {
>  			LOG(V4L2, Error)
>  				<< "Could not add sizes for media bus code "
> -				<< code << " on pad " << pad;
> +				<< code << " on pad " << stream.pad;
>  			return {};
>  		}
>  	}
> @@ -1014,6 +1075,17 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
>  	return formats;
>  }
>
> +/**
> + * \fn V4L2Subdevice::formats(unsigned int pad)
> + * \brief Enumerate all media bus codes and frame sizes on a \a pad
> + * \param[in] pad The 0-indexed pad number to enumerate formats on
> + *
> + * Enumerate all media bus codes and frame sizes supported by the subdevice on
> + * a \a pad
> + *
> + * \return A list of the supported device formats
> + */
> +
>  std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const
>  {
>  	/*
> @@ -1045,25 +1117,26 @@ std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &
>  }
>
>  /**
> - * \brief Retrieve the image format set on one of the V4L2 subdevice pads
> - * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> + * \brief Retrieve the image format set on one of the V4L2 subdevice streams
> + * \param[in] stream The stream the format is to be retrieved from
>   * \param[out] format The image bus format
>   * \param[in] whence The format to get, \ref V4L2Subdevice::ActiveFormat
>   * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> +int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format,
>  			     Whence whence)
>  {
>  	struct v4l2_subdev_format subdevFmt = {};
>  	subdevFmt.which = whence;
> -	subdevFmt.pad = pad;
> +	subdevFmt.pad = stream.pad;
> +	subdevFmt.stream = stream.stream;
>
>  	int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
>  	if (ret) {
>  		LOG(V4L2, Error)
> -			<< "Unable to get format on pad " << pad
> -			<< ": " << strerror(-ret);
> +			<< "Unable to get format on pad " << stream.pad << "/"
> +			<< stream.stream << ": " << strerror(-ret);
>  		return ret;
>  	}
>
> @@ -1076,6 +1149,66 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  }
>
>  /**
> + * \fn V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> + *	Whence whence)
> + * \brief Retrieve the image format set on one of the V4L2 subdevice pads
> + * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> + * \param[out] format The image bus format
> + * \param[in] whence The format to get, \ref V4L2Subdevice::ActiveFormat
> + * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \brief Set an image format on one of the V4L2 subdevice pads
> + * \param[in] stream The stream the format is to be applied to
> + * \param[inout] format The image bus format to apply to the stream
> + * \param[in] whence The format to set, \ref V4L2Subdevice::ActiveFormat
> + * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> + *
> + * Apply the requested image format to the desired stream and return the
> + * actually applied format parameters, as getFormat() would do.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> +			     Whence whence)
> +{
> +	struct v4l2_subdev_format subdevFmt = {};
> +	subdevFmt.which = whence;
> +	subdevFmt.pad = stream.pad;
> +	subdevFmt.stream = stream.stream;
> +	subdevFmt.format.width = format->size.width;
> +	subdevFmt.format.height = format->size.height;
> +	subdevFmt.format.code = format->code;
> +	subdevFmt.format.field = V4L2_FIELD_NONE;
> +	if (format->colorSpace) {
> +		fromColorSpace(format->colorSpace, subdevFmt.format);
> +
> +		/* The CSC flag is only applicable to source pads. */
> +		if (entity_->pads()[stream.pad]->flags() & MEDIA_PAD_FL_SOURCE)
> +			subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> +	}
> +
> +	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> +	if (ret) {
> +		LOG(V4L2, Error)
> +			<< "Unable to set format on pad " << stream.pad << "/"
> +			<< stream.stream << ": " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	format->size.width = subdevFmt.format.width;
> +	format->size.height = subdevFmt.format.height;
> +	format->code = subdevFmt.format.code;
> +	format->colorSpace = toColorSpace(subdevFmt.format);
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> + *	Whence whence)
>   * \brief Set an image format on one of the V4L2 subdevice pads
>   * \param[in] pad The 0-indexed pad number the format is to be applied to
>   * \param[inout] format The image bus format to apply to the subdevice's pad
> @@ -1087,39 +1220,6 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> -			     Whence whence)
> -{
> -	struct v4l2_subdev_format subdevFmt = {};
> -	subdevFmt.which = whence;
> -	subdevFmt.pad = pad;
> -	subdevFmt.format.width = format->size.width;
> -	subdevFmt.format.height = format->size.height;
> -	subdevFmt.format.code = format->code;
> -	subdevFmt.format.field = V4L2_FIELD_NONE;
> -	if (format->colorSpace) {
> -		fromColorSpace(format->colorSpace, subdevFmt.format);
> -
> -		/* The CSC flag is only applicable to source pads. */
> -		if (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)
> -			subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> -	}
> -
> -	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> -	if (ret) {
> -		LOG(V4L2, Error)
> -			<< "Unable to set format on pad " << pad
> -			<< ": " << strerror(-ret);
> -		return ret;
> -	}
> -
> -	format->size.width = subdevFmt.format.width;
> -	format->size.height = subdevFmt.format.height;
> -	format->code = subdevFmt.format.code;
> -	format->colorSpace = toColorSpace(subdevFmt.format);
> -
> -	return 0;
> -}
>
>  /**
>   * \brief Retrieve the subdevice's internal routing table
> @@ -1282,14 +1382,15 @@ std::string V4L2Subdevice::logPrefix() const
>  	return "'" + entity_->name() + "'";
>  }
>
> -std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> +std::vector<unsigned int> V4L2Subdevice::enumPadCodes(const Stream &stream)
>  {
>  	std::vector<unsigned int> codes;
>  	int ret;
>
>  	for (unsigned int index = 0; ; index++) {
>  		struct v4l2_subdev_mbus_code_enum mbusEnum = {};
> -		mbusEnum.pad = pad;
> +		mbusEnum.pad = stream.pad;
> +		mbusEnum.stream = stream.stream;
>  		mbusEnum.index = index;
>  		mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>
> @@ -1302,15 +1403,15 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
>
>  	if (ret < 0 && ret != -EINVAL) {
>  		LOG(V4L2, Error)
> -			<< "Unable to enumerate formats on pad " << pad
> -			<< ": " << strerror(-ret);
> +			<< "Unable to enumerate formats on pad " << stream.pad
> +			<< "/" << stream.stream << ": " << strerror(-ret);
>  		return {};
>  	}
>
>  	return codes;
>  }
>
> -std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> +std::vector<SizeRange> V4L2Subdevice::enumPadSizes(const Stream &stream,
>  						   unsigned int code)
>  {
>  	std::vector<SizeRange> sizes;
> @@ -1319,7 +1420,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
>  	for (unsigned int index = 0;; index++) {
>  		struct v4l2_subdev_frame_size_enum sizeEnum = {};
>  		sizeEnum.index = index;
> -		sizeEnum.pad = pad;
> +		sizeEnum.pad = stream.pad;
> +		sizeEnum.stream = stream.stream;
>  		sizeEnum.code = code;
>  		sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>
> @@ -1333,8 +1435,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
>
>  	if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
>  		LOG(V4L2, Error)
> -			<< "Unable to enumerate sizes on pad " << pad
> -			<< ": " << strerror(-ret);
> +			<< "Unable to enumerate sizes on pad " << stream.pad
> +			<< "/" << stream.stream << ": " << strerror(-ret);
>  		return {};

The only question I have is if we should allow the V4L2Subdevice API
to expose a pad-only interface or we should bite the bullet and move
it to use Stream. The reason is that, a open() time

	/* If the subdev supports streams, enable the streams API. */
	if (caps_.hasStreams()) {
		struct v4l2_subdev_client_capability clientCaps{};
		clientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;

		ret = ioctl(VIDIOC_SUBDEV_S_CLIENT_CAP, &clientCaps);

And allowing users to use the pad-based API would make stream == 0
implicitly. It's a lot of work though, not sure it's worth it ?

>  	}
>
> --
> Regards,
>
> Laurent Pinchart
>

Hi Jacopo,

On Tue, Feb 27, 2024 at 05:48:35PM +0100, Jacopo Mondi wrote:
> On Tue, Feb 27, 2024 at 04:09:51PM +0200, Laurent Pinchart wrote:
> > Extend the V4L2Subdevice API with stream support for the functions that
> > get and set formats and selection rectangles. Add a Stream structure to
> > identify a subdev pad and stream, and use it to extend the V4L2Subdevice
> > functions that get and set formats and selection rectangles with stream
> > support.
> >
> > To preserve the existing pad-based API, implement overloaded functions
> > that wrap the new stream-based API. This allows callers that are not
> > stream-aware to use a simpler pad-based API, instead of having to
> > explicitly set the stream number to 0 in all API calls.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/internal/v4l2_subdevice.h |  41 +++-
> >  src/libcamera/v4l2_subdevice.cpp            | 232 ++++++++++++++------
> >  2 files changed, 202 insertions(+), 71 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> > index 1115cfa55594..65003394a984 100644
> > --- a/include/libcamera/internal/v4l2_subdevice.h
> > +++ b/include/libcamera/internal/v4l2_subdevice.h
> > @@ -80,6 +80,11 @@ public:
> >  		ActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,
> >  	};
> >
> > +	struct Stream {
> > +		unsigned int pad;
> > +		unsigned int stream;
> > +	};
> > +
> >  	class Routing : public std::vector<struct v4l2_subdev_route>
> >  	{
> >  	public:
> > @@ -93,17 +98,39 @@ public:
> >
> >  	const MediaEntity *entity() const { return entity_; }
> >
> > -	int getSelection(unsigned int pad, unsigned int target,
> > +	int getSelection(const Stream &stream, unsigned int target,
> >  			 Rectangle *rect);
> > -	int setSelection(unsigned int pad, unsigned int target,
> > +	int getSelection(unsigned int pad, unsigned int target, Rectangle *rect)
> > +	{
> > +		return getSelection({ pad, 0 }, target, rect);
> > +	}
> > +	int setSelection(const Stream &stream, unsigned int target,
> >  			 Rectangle *rect);
> > +	int setSelection(unsigned int pad, unsigned int target, Rectangle *rect)
> > +	{
> > +		return setSelection({ pad, 0 }, target, rect);
> > +	}
> >
> > -	Formats formats(unsigned int pad);
> > +	Formats formats(const Stream &stream);
> > +	Formats formats(unsigned int pad)
> > +	{
> > +		return formats({ pad, 0 });
> > +	}
> >
> > +	int getFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> > +		      Whence whence = ActiveFormat);
> >  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > +		      Whence whence = ActiveFormat)
> > +	{
> > +		return getFormat({ pad, 0 }, format, whence);
> > +	}
> > +	int setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> >  		      Whence whence = ActiveFormat);
> >  	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > -		      Whence whence = ActiveFormat);
> > +		      Whence whence = ActiveFormat)
> > +	{
> > +		return setFormat({ pad, 0 }, format, whence);
> > +	}
> >
> >  	int getRouting(Routing *routing, Whence whence = ActiveFormat);
> >  	int setRouting(Routing *routing, Whence whence = ActiveFormat);
> > @@ -123,8 +150,8 @@ private:
> >  	std::optional<ColorSpace>
> >  	toColorSpace(const v4l2_mbus_framefmt &format) const;
> >
> > -	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> > -	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > +	std::vector<unsigned int> enumPadCodes(const Stream &stream);
> > +	std::vector<SizeRange> enumPadSizes(const Stream &stream,
> >  					    unsigned int code);
> >
> >  	const MediaEntity *entity_;
> > @@ -133,4 +160,6 @@ private:
> >  	struct V4L2SubdeviceCapability caps_;
> >  };
> >
> > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);
> > +
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 19ddecf714c6..efe8b0f793e9 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -814,6 +814,35 @@ std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f)
> >   * \brief The format operation applies to TRY formats
> >   */
> >
> > +/**
> > + * \class V4L2Subdevice::Stream
> > + * \brief V4L2 subdevice stream
> > + *
> > + * This class identifies a subdev stream, by bundling the pad number with the
> > + * stream number. It is used in all stream-aware functions of the V4L2Subdevice
> > + * class to identify the stream the functions operate on.
> > + *
> > + * \var V4L2Subdevice::Stream::pad
> > + * \brief The 0-indexed pad number
> > + *
> > + * \var V4L2Subdevice::Stream::stream
> > + * \brief The stream number
> > + */
> > +
> > +/**
> > + * \brief Insert a text representation of a V4L2Subdevice::Stream into an
> > + *	output stream
> 
> What is the alignment rule when we break \brief ? I see different ones
> being used in this patch

Good question :-) Let's decide. Does anybody have a preference between

 * \brief Insert a text representation of a V4L2Subdevice::Stream into an
 *	output stream

(single tab after the '*', no space)

and

 * \brief Insert a text representation of a V4L2Subdevice::Stream into an
 * output stream

(single space after the '*')

?

> > + * \param[in] out The output stream
> > + * \param[in] stream The V4L2Subdevice::Stream
> > + * \return The output stream \a out
> > + */
> > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)
> > +{
> > +	out << stream.pad << "/" << stream.stream;
> > +
> > +	return out;
> > +}
> > +
> >  /**
> >   * \class V4L2Subdevice::Routing
> >   * \brief V4L2 subdevice routing table
> > @@ -879,7 +908,10 @@ int V4L2Subdevice::open()
> >  		return ret;
> >  	}
> >
> > -	/* If the subdev supports streams, enable the streams API. */
> > +	/*
> > +	 * If the subdev supports streams, enable the streams API, and retrieve
> > +	 * and cache the routing table.
> 
> Does it happen in this patch ?

Indeed not. I was planning to do so and then didn't. I'll drop the
comment change.

> > +	 */
> >  	if (caps_.hasStreams()) {
> >  		struct v4l2_subdev_client_capability clientCaps{};
> >  		clientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> > @@ -905,7 +937,7 @@ int V4L2Subdevice::open()
> >
> >  /**
> >   * \brief Get selection rectangle \a rect for \a target
> > - * \param[in] pad The 0-indexed pad number the rectangle is retrieved from
> > + * \param[in] stream The stream the rectangle is retrieved from
> >   * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> >   * \param[out] rect The retrieved selection rectangle
> >   *
> > @@ -913,13 +945,14 @@ int V4L2Subdevice::open()
> >   *
> >   * \return 0 on success or a negative error code otherwise
> >   */
> > -int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > +int V4L2Subdevice::getSelection(const Stream &stream, unsigned int target,
> >  				Rectangle *rect)
> >  {
> >  	struct v4l2_subdev_selection sel = {};
> >
> >  	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > -	sel.pad = pad;
> > +	sel.pad = stream.pad;
> > +	sel.stream = stream.stream;
> >  	sel.target = target;
> >  	sel.flags = 0;
> >
> > @@ -927,7 +960,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> >  	if (ret < 0) {
> >  		LOG(V4L2, Error)
> >  			<< "Unable to get rectangle " << target << " on pad "
> > -			<< pad << ": " << strerror(-ret);
> > +			<< stream.pad << "/" << stream.stream << ": "
> 
> Can't you use operator<<(Stream) ?

I'll fix it, and below too.

> > +			<< strerror(-ret);
> >  		return ret;
> >  	}
> >
> > @@ -939,9 +973,20 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> >  	return 0;
> >  }
> >
> > +/**
> > + * \fn V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > + *	Rectangle *rect)
> > + * \brief Get selection rectangle \a rect for \a target
> > + * \param[in] pad The 0-indexed pad number the rectangle is retrieved from
> > + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > + * \param[out] rect The retrieved selection rectangle
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +
> >  /**
> >   * \brief Set selection rectangle \a rect for \a target
> > - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > + * \param[in] stream The stream the rectangle is to be applied to
> >   * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> >   * \param[inout] rect The selection rectangle to be applied
> >   *
> > @@ -949,13 +994,14 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> >   *
> >   * \return 0 on success or a negative error code otherwise
> >   */
> > -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > +int V4L2Subdevice::setSelection(const Stream &stream, unsigned int target,
> >  				Rectangle *rect)
> >  {
> >  	struct v4l2_subdev_selection sel = {};
> >
> >  	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > -	sel.pad = pad;
> > +	sel.pad = stream.pad;
> > +	sel.stream = stream.stream;
> >  	sel.target = target;
> >  	sel.flags = 0;
> >
> > @@ -968,7 +1014,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> >  	if (ret < 0) {
> >  		LOG(V4L2, Error)
> >  			<< "Unable to set rectangle " << target << " on pad "
> > -			<< pad << ": " << strerror(-ret);
> > +			<< stream.pad << "/" << stream.stream << ": "
> 
> ditto
> 
> > +			<< strerror(-ret);
> >  		return ret;
> >  	}
> >
> > @@ -979,26 +1026,40 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> >
> >  	return 0;
> >  }
> > +
> >  /**
> > - * \brief Enumerate all media bus codes and frame sizes on a \a pad
> > - * \param[in] pad The 0-indexed pad number to enumerate formats on
> > + * \fn V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > + *	Rectangle *rect)
> > + * \brief Set selection rectangle \a rect for \a target
> > + * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > + * \param[inout] rect The selection rectangle to be applied
> > + *
> > + * \todo Define a V4L2SelectionTarget enum for the selection target
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +
> > +/**
> > + * \brief Enumerate all media bus codes and frame sizes on a \a stream
> > + * \param[in] stream The stream to enumerate formats for
> >   *
> >   * Enumerate all media bus codes and frame sizes supported by the subdevice on
> > - * a \a pad.
> > + * a \a stream.
> >   *
> >   * \return A list of the supported device formats
> >   */
> > -V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> > +V4L2Subdevice::Formats V4L2Subdevice::formats(const Stream &stream)
> >  {
> >  	Formats formats;
> >
> > -	if (pad >= entity_->pads().size()) {
> > -		LOG(V4L2, Error) << "Invalid pad: " << pad;
> > +	if (stream.pad >= entity_->pads().size()) {
> > +		LOG(V4L2, Error) << "Invalid pad: " << stream.pad;
> >  		return {};
> >  	}
> >
> > -	for (unsigned int code : enumPadCodes(pad)) {
> > -		std::vector<SizeRange> sizes = enumPadSizes(pad, code);
> > +	for (unsigned int code : enumPadCodes(stream)) {
> > +		std::vector<SizeRange> sizes = enumPadSizes(stream, code);
> >  		if (sizes.empty())
> >  			return {};
> >
> > @@ -1006,7 +1067,7 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> >  		if (!inserted.second) {
> >  			LOG(V4L2, Error)
> >  				<< "Could not add sizes for media bus code "
> > -				<< code << " on pad " << pad;
> > +				<< code << " on pad " << stream.pad;
> >  			return {};
> >  		}
> >  	}
> > @@ -1014,6 +1075,17 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> >  	return formats;
> >  }
> >
> > +/**
> > + * \fn V4L2Subdevice::formats(unsigned int pad)
> > + * \brief Enumerate all media bus codes and frame sizes on a \a pad
> > + * \param[in] pad The 0-indexed pad number to enumerate formats on
> > + *
> > + * Enumerate all media bus codes and frame sizes supported by the subdevice on
> > + * a \a pad
> > + *
> > + * \return A list of the supported device formats
> > + */
> > +
> >  std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const
> >  {
> >  	/*
> > @@ -1045,25 +1117,26 @@ std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &
> >  }
> >
> >  /**
> > - * \brief Retrieve the image format set on one of the V4L2 subdevice pads
> > - * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> > + * \brief Retrieve the image format set on one of the V4L2 subdevice streams
> > + * \param[in] stream The stream the format is to be retrieved from
> >   * \param[out] format The image bus format
> >   * \param[in] whence The format to get, \ref V4L2Subdevice::ActiveFormat
> >   * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> >   * \return 0 on success or a negative error code otherwise
> >   */
> > -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > +int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> >  			     Whence whence)
> >  {
> >  	struct v4l2_subdev_format subdevFmt = {};
> >  	subdevFmt.which = whence;
> > -	subdevFmt.pad = pad;
> > +	subdevFmt.pad = stream.pad;
> > +	subdevFmt.stream = stream.stream;
> >
> >  	int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> >  	if (ret) {
> >  		LOG(V4L2, Error)
> > -			<< "Unable to get format on pad " << pad
> > -			<< ": " << strerror(-ret);
> > +			<< "Unable to get format on pad " << stream.pad << "/"
> > +			<< stream.stream << ": " << strerror(-ret);
> >  		return ret;
> >  	}
> >
> > @@ -1076,6 +1149,66 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> >  }
> >
> >  /**
> > + * \fn V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > + *	Whence whence)
> > + * \brief Retrieve the image format set on one of the V4L2 subdevice pads
> > + * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> > + * \param[out] format The image bus format
> > + * \param[in] whence The format to get, \ref V4L2Subdevice::ActiveFormat
> > + * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +
> > +/**
> > + * \brief Set an image format on one of the V4L2 subdevice pads
> > + * \param[in] stream The stream the format is to be applied to
> > + * \param[inout] format The image bus format to apply to the stream
> > + * \param[in] whence The format to set, \ref V4L2Subdevice::ActiveFormat
> > + * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> > + *
> > + * Apply the requested image format to the desired stream and return the
> > + * actually applied format parameters, as getFormat() would do.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> > +			     Whence whence)
> > +{
> > +	struct v4l2_subdev_format subdevFmt = {};
> > +	subdevFmt.which = whence;
> > +	subdevFmt.pad = stream.pad;
> > +	subdevFmt.stream = stream.stream;
> > +	subdevFmt.format.width = format->size.width;
> > +	subdevFmt.format.height = format->size.height;
> > +	subdevFmt.format.code = format->code;
> > +	subdevFmt.format.field = V4L2_FIELD_NONE;
> > +	if (format->colorSpace) {
> > +		fromColorSpace(format->colorSpace, subdevFmt.format);
> > +
> > +		/* The CSC flag is only applicable to source pads. */
> > +		if (entity_->pads()[stream.pad]->flags() & MEDIA_PAD_FL_SOURCE)
> > +			subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> > +	}
> > +
> > +	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > +	if (ret) {
> > +		LOG(V4L2, Error)
> > +			<< "Unable to set format on pad " << stream.pad << "/"
> > +			<< stream.stream << ": " << strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	format->size.width = subdevFmt.format.width;
> > +	format->size.height = subdevFmt.format.height;
> > +	format->code = subdevFmt.format.code;
> > +	format->colorSpace = toColorSpace(subdevFmt.format);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \fn V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > + *	Whence whence)
> >   * \brief Set an image format on one of the V4L2 subdevice pads
> >   * \param[in] pad The 0-indexed pad number the format is to be applied to
> >   * \param[inout] format The image bus format to apply to the subdevice's pad
> > @@ -1087,39 +1220,6 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> >   *
> >   * \return 0 on success or a negative error code otherwise
> >   */
> > -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > -			     Whence whence)
> > -{
> > -	struct v4l2_subdev_format subdevFmt = {};
> > -	subdevFmt.which = whence;
> > -	subdevFmt.pad = pad;
> > -	subdevFmt.format.width = format->size.width;
> > -	subdevFmt.format.height = format->size.height;
> > -	subdevFmt.format.code = format->code;
> > -	subdevFmt.format.field = V4L2_FIELD_NONE;
> > -	if (format->colorSpace) {
> > -		fromColorSpace(format->colorSpace, subdevFmt.format);
> > -
> > -		/* The CSC flag is only applicable to source pads. */
> > -		if (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)
> > -			subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> > -	}
> > -
> > -	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > -	if (ret) {
> > -		LOG(V4L2, Error)
> > -			<< "Unable to set format on pad " << pad
> > -			<< ": " << strerror(-ret);
> > -		return ret;
> > -	}
> > -
> > -	format->size.width = subdevFmt.format.width;
> > -	format->size.height = subdevFmt.format.height;
> > -	format->code = subdevFmt.format.code;
> > -	format->colorSpace = toColorSpace(subdevFmt.format);
> > -
> > -	return 0;
> > -}
> >
> >  /**
> >   * \brief Retrieve the subdevice's internal routing table
> > @@ -1282,14 +1382,15 @@ std::string V4L2Subdevice::logPrefix() const
> >  	return "'" + entity_->name() + "'";
> >  }
> >
> > -std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > +std::vector<unsigned int> V4L2Subdevice::enumPadCodes(const Stream &stream)
> >  {
> >  	std::vector<unsigned int> codes;
> >  	int ret;
> >
> >  	for (unsigned int index = 0; ; index++) {
> >  		struct v4l2_subdev_mbus_code_enum mbusEnum = {};
> > -		mbusEnum.pad = pad;
> > +		mbusEnum.pad = stream.pad;
> > +		mbusEnum.stream = stream.stream;
> >  		mbusEnum.index = index;
> >  		mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >
> > @@ -1302,15 +1403,15 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> >
> >  	if (ret < 0 && ret != -EINVAL) {
> >  		LOG(V4L2, Error)
> > -			<< "Unable to enumerate formats on pad " << pad
> > -			<< ": " << strerror(-ret);
> > +			<< "Unable to enumerate formats on pad " << stream.pad
> > +			<< "/" << stream.stream << ": " << strerror(-ret);
> >  		return {};
> >  	}
> >
> >  	return codes;
> >  }
> >
> > -std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > +std::vector<SizeRange> V4L2Subdevice::enumPadSizes(const Stream &stream,
> >  						   unsigned int code)
> >  {
> >  	std::vector<SizeRange> sizes;
> > @@ -1319,7 +1420,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> >  	for (unsigned int index = 0;; index++) {
> >  		struct v4l2_subdev_frame_size_enum sizeEnum = {};
> >  		sizeEnum.index = index;
> > -		sizeEnum.pad = pad;
> > +		sizeEnum.pad = stream.pad;
> > +		sizeEnum.stream = stream.stream;
> >  		sizeEnum.code = code;
> >  		sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >
> > @@ -1333,8 +1435,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> >
> >  	if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> >  		LOG(V4L2, Error)
> > -			<< "Unable to enumerate sizes on pad " << pad
> > -			<< ": " << strerror(-ret);
> > +			<< "Unable to enumerate sizes on pad " << stream.pad
> > +			<< "/" << stream.stream << ": " << strerror(-ret);
> >  		return {};
> 
> The only question I have is if we should allow the V4L2Subdevice API
> to expose a pad-only interface or we should bite the bullet and move
> it to use Stream. The reason is that, a open() time
> 
> 	/* If the subdev supports streams, enable the streams API. */
> 	if (caps_.hasStreams()) {
> 		struct v4l2_subdev_client_capability clientCaps{};
> 		clientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> 
> 		ret = ioctl(VIDIOC_SUBDEV_S_CLIENT_CAP, &clientCaps);
> 
> And allowing users to use the pad-based API would make stream == 0
> implicitly. It's a lot of work though, not sure it's worth it ?

Most devices don't use streams, that's why I decided to overload the
get/set functions and offer users a pad-based interface and a
stream-based interface. It's low-maintenance on the V4L2Subdevice side
(at least for now), and allows callers that deal with stream-unaware
devices only not to have to specify the stream number. I think the code
gets more readable that way.

> >  	}
> >

Hi Laurent

On Wed, Feb 28, 2024 at 12:51:19PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Feb 27, 2024 at 05:48:35PM +0100, Jacopo Mondi wrote:
> > On Tue, Feb 27, 2024 at 04:09:51PM +0200, Laurent Pinchart wrote:
> > > Extend the V4L2Subdevice API with stream support for the functions that
> > > get and set formats and selection rectangles. Add a Stream structure to
> > > identify a subdev pad and stream, and use it to extend the V4L2Subdevice
> > > functions that get and set formats and selection rectangles with stream
> > > support.
> > >
> > > To preserve the existing pad-based API, implement overloaded functions
> > > that wrap the new stream-based API. This allows callers that are not
> > > stream-aware to use a simpler pad-based API, instead of having to
> > > explicitly set the stream number to 0 in all API calls.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  include/libcamera/internal/v4l2_subdevice.h |  41 +++-
> > >  src/libcamera/v4l2_subdevice.cpp            | 232 ++++++++++++++------
> > >  2 files changed, 202 insertions(+), 71 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> > > index 1115cfa55594..65003394a984 100644
> > > --- a/include/libcamera/internal/v4l2_subdevice.h
> > > +++ b/include/libcamera/internal/v4l2_subdevice.h
> > > @@ -80,6 +80,11 @@ public:
> > >  		ActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,
> > >  	};
> > >
> > > +	struct Stream {
> > > +		unsigned int pad;
> > > +		unsigned int stream;
> > > +	};
> > > +
> > >  	class Routing : public std::vector<struct v4l2_subdev_route>
> > >  	{
> > >  	public:
> > > @@ -93,17 +98,39 @@ public:
> > >
> > >  	const MediaEntity *entity() const { return entity_; }
> > >
> > > -	int getSelection(unsigned int pad, unsigned int target,
> > > +	int getSelection(const Stream &stream, unsigned int target,
> > >  			 Rectangle *rect);
> > > -	int setSelection(unsigned int pad, unsigned int target,
> > > +	int getSelection(unsigned int pad, unsigned int target, Rectangle *rect)
> > > +	{
> > > +		return getSelection({ pad, 0 }, target, rect);
> > > +	}
> > > +	int setSelection(const Stream &stream, unsigned int target,
> > >  			 Rectangle *rect);
> > > +	int setSelection(unsigned int pad, unsigned int target, Rectangle *rect)
> > > +	{
> > > +		return setSelection({ pad, 0 }, target, rect);
> > > +	}
> > >
> > > -	Formats formats(unsigned int pad);
> > > +	Formats formats(const Stream &stream);
> > > +	Formats formats(unsigned int pad)
> > > +	{
> > > +		return formats({ pad, 0 });
> > > +	}
> > >
> > > +	int getFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> > > +		      Whence whence = ActiveFormat);
> > >  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > +		      Whence whence = ActiveFormat)
> > > +	{
> > > +		return getFormat({ pad, 0 }, format, whence);
> > > +	}
> > > +	int setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> > >  		      Whence whence = ActiveFormat);
> > >  	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > -		      Whence whence = ActiveFormat);
> > > +		      Whence whence = ActiveFormat)
> > > +	{
> > > +		return setFormat({ pad, 0 }, format, whence);
> > > +	}
> > >
> > >  	int getRouting(Routing *routing, Whence whence = ActiveFormat);
> > >  	int setRouting(Routing *routing, Whence whence = ActiveFormat);
> > > @@ -123,8 +150,8 @@ private:
> > >  	std::optional<ColorSpace>
> > >  	toColorSpace(const v4l2_mbus_framefmt &format) const;
> > >
> > > -	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> > > -	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > > +	std::vector<unsigned int> enumPadCodes(const Stream &stream);
> > > +	std::vector<SizeRange> enumPadSizes(const Stream &stream,
> > >  					    unsigned int code);
> > >
> > >  	const MediaEntity *entity_;
> > > @@ -133,4 +160,6 @@ private:
> > >  	struct V4L2SubdeviceCapability caps_;
> > >  };
> > >
> > > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);
> > > +
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 19ddecf714c6..efe8b0f793e9 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -814,6 +814,35 @@ std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f)
> > >   * \brief The format operation applies to TRY formats
> > >   */
> > >
> > > +/**
> > > + * \class V4L2Subdevice::Stream
> > > + * \brief V4L2 subdevice stream
> > > + *
> > > + * This class identifies a subdev stream, by bundling the pad number with the
> > > + * stream number. It is used in all stream-aware functions of the V4L2Subdevice
> > > + * class to identify the stream the functions operate on.
> > > + *
> > > + * \var V4L2Subdevice::Stream::pad
> > > + * \brief The 0-indexed pad number
> > > + *
> > > + * \var V4L2Subdevice::Stream::stream
> > > + * \brief The stream number
> > > + */
> > > +
> > > +/**
> > > + * \brief Insert a text representation of a V4L2Subdevice::Stream into an
> > > + *	output stream
> >
> > What is the alignment rule when we break \brief ? I see different ones
> > being used in this patch
>
> Good question :-) Let's decide. Does anybody have a preference between
>
>  * \brief Insert a text representation of a V4L2Subdevice::Stream into an
>  *	output stream
>
> (single tab after the '*', no space)
>
> and
>
>  * \brief Insert a text representation of a V4L2Subdevice::Stream into an
>  * output stream
>
> (single space after the '*')
>

My vote for the second option, but whatever, really

> ?
>
> > > + * \param[in] out The output stream
> > > + * \param[in] stream The V4L2Subdevice::Stream
> > > + * \return The output stream \a out
> > > + */
> > > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)
> > > +{
> > > +	out << stream.pad << "/" << stream.stream;
> > > +
> > > +	return out;
> > > +}
> > > +
> > >  /**
> > >   * \class V4L2Subdevice::Routing
> > >   * \brief V4L2 subdevice routing table
> > > @@ -879,7 +908,10 @@ int V4L2Subdevice::open()
> > >  		return ret;
> > >  	}
> > >
> > > -	/* If the subdev supports streams, enable the streams API. */
> > > +	/*
> > > +	 * If the subdev supports streams, enable the streams API, and retrieve
> > > +	 * and cache the routing table.
> >
> > Does it happen in this patch ?
>
> Indeed not. I was planning to do so and then didn't. I'll drop the
> comment change.
>
> > > +	 */
> > >  	if (caps_.hasStreams()) {
> > >  		struct v4l2_subdev_client_capability clientCaps{};
> > >  		clientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> > > @@ -905,7 +937,7 @@ int V4L2Subdevice::open()
> > >
> > >  /**
> > >   * \brief Get selection rectangle \a rect for \a target
> > > - * \param[in] pad The 0-indexed pad number the rectangle is retrieved from
> > > + * \param[in] stream The stream the rectangle is retrieved from
> > >   * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > >   * \param[out] rect The retrieved selection rectangle
> > >   *
> > > @@ -913,13 +945,14 @@ int V4L2Subdevice::open()
> > >   *
> > >   * \return 0 on success or a negative error code otherwise
> > >   */
> > > -int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > > +int V4L2Subdevice::getSelection(const Stream &stream, unsigned int target,
> > >  				Rectangle *rect)
> > >  {
> > >  	struct v4l2_subdev_selection sel = {};
> > >
> > >  	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > -	sel.pad = pad;
> > > +	sel.pad = stream.pad;
> > > +	sel.stream = stream.stream;
> > >  	sel.target = target;
> > >  	sel.flags = 0;
> > >
> > > @@ -927,7 +960,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > >  	if (ret < 0) {
> > >  		LOG(V4L2, Error)
> > >  			<< "Unable to get rectangle " << target << " on pad "
> > > -			<< pad << ": " << strerror(-ret);
> > > +			<< stream.pad << "/" << stream.stream << ": "
> >
> > Can't you use operator<<(Stream) ?
>
> I'll fix it, and below too.
>
> > > +			<< strerror(-ret);
> > >  		return ret;
> > >  	}
> > >
> > > @@ -939,9 +973,20 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > >  	return 0;
> > >  }
> > >
> > > +/**
> > > + * \fn V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > > + *	Rectangle *rect)
> > > + * \brief Get selection rectangle \a rect for \a target
> > > + * \param[in] pad The 0-indexed pad number the rectangle is retrieved from
> > > + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > > + * \param[out] rect The retrieved selection rectangle
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +
> > >  /**
> > >   * \brief Set selection rectangle \a rect for \a target
> > > - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > > + * \param[in] stream The stream the rectangle is to be applied to
> > >   * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > >   * \param[inout] rect The selection rectangle to be applied
> > >   *
> > > @@ -949,13 +994,14 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > >   *
> > >   * \return 0 on success or a negative error code otherwise
> > >   */
> > > -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > > +int V4L2Subdevice::setSelection(const Stream &stream, unsigned int target,
> > >  				Rectangle *rect)
> > >  {
> > >  	struct v4l2_subdev_selection sel = {};
> > >
> > >  	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > -	sel.pad = pad;
> > > +	sel.pad = stream.pad;
> > > +	sel.stream = stream.stream;
> > >  	sel.target = target;
> > >  	sel.flags = 0;
> > >
> > > @@ -968,7 +1014,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > >  	if (ret < 0) {
> > >  		LOG(V4L2, Error)
> > >  			<< "Unable to set rectangle " << target << " on pad "
> > > -			<< pad << ": " << strerror(-ret);
> > > +			<< stream.pad << "/" << stream.stream << ": "
> >
> > ditto
> >
> > > +			<< strerror(-ret);
> > >  		return ret;
> > >  	}
> > >
> > > @@ -979,26 +1026,40 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > >
> > >  	return 0;
> > >  }
> > > +
> > >  /**
> > > - * \brief Enumerate all media bus codes and frame sizes on a \a pad
> > > - * \param[in] pad The 0-indexed pad number to enumerate formats on
> > > + * \fn V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > > + *	Rectangle *rect)
> > > + * \brief Set selection rectangle \a rect for \a target
> > > + * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > > + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > > + * \param[inout] rect The selection rectangle to be applied
> > > + *
> > > + * \todo Define a V4L2SelectionTarget enum for the selection target
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +
> > > +/**
> > > + * \brief Enumerate all media bus codes and frame sizes on a \a stream
> > > + * \param[in] stream The stream to enumerate formats for
> > >   *
> > >   * Enumerate all media bus codes and frame sizes supported by the subdevice on
> > > - * a \a pad.
> > > + * a \a stream.
> > >   *
> > >   * \return A list of the supported device formats
> > >   */
> > > -V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> > > +V4L2Subdevice::Formats V4L2Subdevice::formats(const Stream &stream)
> > >  {
> > >  	Formats formats;
> > >
> > > -	if (pad >= entity_->pads().size()) {
> > > -		LOG(V4L2, Error) << "Invalid pad: " << pad;
> > > +	if (stream.pad >= entity_->pads().size()) {
> > > +		LOG(V4L2, Error) << "Invalid pad: " << stream.pad;
> > >  		return {};
> > >  	}
> > >
> > > -	for (unsigned int code : enumPadCodes(pad)) {
> > > -		std::vector<SizeRange> sizes = enumPadSizes(pad, code);
> > > +	for (unsigned int code : enumPadCodes(stream)) {
> > > +		std::vector<SizeRange> sizes = enumPadSizes(stream, code);
> > >  		if (sizes.empty())
> > >  			return {};
> > >
> > > @@ -1006,7 +1067,7 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> > >  		if (!inserted.second) {
> > >  			LOG(V4L2, Error)
> > >  				<< "Could not add sizes for media bus code "
> > > -				<< code << " on pad " << pad;
> > > +				<< code << " on pad " << stream.pad;
> > >  			return {};
> > >  		}
> > >  	}
> > > @@ -1014,6 +1075,17 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> > >  	return formats;
> > >  }
> > >
> > > +/**
> > > + * \fn V4L2Subdevice::formats(unsigned int pad)
> > > + * \brief Enumerate all media bus codes and frame sizes on a \a pad
> > > + * \param[in] pad The 0-indexed pad number to enumerate formats on
> > > + *
> > > + * Enumerate all media bus codes and frame sizes supported by the subdevice on
> > > + * a \a pad
> > > + *
> > > + * \return A list of the supported device formats
> > > + */
> > > +
> > >  std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const
> > >  {
> > >  	/*
> > > @@ -1045,25 +1117,26 @@ std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &
> > >  }
> > >
> > >  /**
> > > - * \brief Retrieve the image format set on one of the V4L2 subdevice pads
> > > - * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> > > + * \brief Retrieve the image format set on one of the V4L2 subdevice streams
> > > + * \param[in] stream The stream the format is to be retrieved from
> > >   * \param[out] format The image bus format
> > >   * \param[in] whence The format to get, \ref V4L2Subdevice::ActiveFormat
> > >   * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> > >   * \return 0 on success or a negative error code otherwise
> > >   */
> > > -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > +int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> > >  			     Whence whence)
> > >  {
> > >  	struct v4l2_subdev_format subdevFmt = {};
> > >  	subdevFmt.which = whence;
> > > -	subdevFmt.pad = pad;
> > > +	subdevFmt.pad = stream.pad;
> > > +	subdevFmt.stream = stream.stream;
> > >
> > >  	int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > >  	if (ret) {
> > >  		LOG(V4L2, Error)
> > > -			<< "Unable to get format on pad " << pad
> > > -			<< ": " << strerror(-ret);
> > > +			<< "Unable to get format on pad " << stream.pad << "/"
> > > +			<< stream.stream << ": " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > >
> > > @@ -1076,6 +1149,66 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > >  }
> > >
> > >  /**
> > > + * \fn V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > + *	Whence whence)
> > > + * \brief Retrieve the image format set on one of the V4L2 subdevice pads
> > > + * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> > > + * \param[out] format The image bus format
> > > + * \param[in] whence The format to get, \ref V4L2Subdevice::ActiveFormat
> > > + * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +
> > > +/**
> > > + * \brief Set an image format on one of the V4L2 subdevice pads
> > > + * \param[in] stream The stream the format is to be applied to
> > > + * \param[inout] format The image bus format to apply to the stream
> > > + * \param[in] whence The format to set, \ref V4L2Subdevice::ActiveFormat
> > > + * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> > > + *
> > > + * Apply the requested image format to the desired stream and return the
> > > + * actually applied format parameters, as getFormat() would do.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> > > +			     Whence whence)
> > > +{
> > > +	struct v4l2_subdev_format subdevFmt = {};
> > > +	subdevFmt.which = whence;
> > > +	subdevFmt.pad = stream.pad;
> > > +	subdevFmt.stream = stream.stream;
> > > +	subdevFmt.format.width = format->size.width;
> > > +	subdevFmt.format.height = format->size.height;
> > > +	subdevFmt.format.code = format->code;
> > > +	subdevFmt.format.field = V4L2_FIELD_NONE;
> > > +	if (format->colorSpace) {
> > > +		fromColorSpace(format->colorSpace, subdevFmt.format);
> > > +
> > > +		/* The CSC flag is only applicable to source pads. */
> > > +		if (entity_->pads()[stream.pad]->flags() & MEDIA_PAD_FL_SOURCE)
> > > +			subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> > > +	}
> > > +
> > > +	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > > +	if (ret) {
> > > +		LOG(V4L2, Error)
> > > +			<< "Unable to set format on pad " << stream.pad << "/"
> > > +			<< stream.stream << ": " << strerror(-ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	format->size.width = subdevFmt.format.width;
> > > +	format->size.height = subdevFmt.format.height;
> > > +	format->code = subdevFmt.format.code;
> > > +	format->colorSpace = toColorSpace(subdevFmt.format);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \fn V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > + *	Whence whence)
> > >   * \brief Set an image format on one of the V4L2 subdevice pads
> > >   * \param[in] pad The 0-indexed pad number the format is to be applied to
> > >   * \param[inout] format The image bus format to apply to the subdevice's pad
> > > @@ -1087,39 +1220,6 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > >   *
> > >   * \return 0 on success or a negative error code otherwise
> > >   */
> > > -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > -			     Whence whence)
> > > -{
> > > -	struct v4l2_subdev_format subdevFmt = {};
> > > -	subdevFmt.which = whence;
> > > -	subdevFmt.pad = pad;
> > > -	subdevFmt.format.width = format->size.width;
> > > -	subdevFmt.format.height = format->size.height;
> > > -	subdevFmt.format.code = format->code;
> > > -	subdevFmt.format.field = V4L2_FIELD_NONE;
> > > -	if (format->colorSpace) {
> > > -		fromColorSpace(format->colorSpace, subdevFmt.format);
> > > -
> > > -		/* The CSC flag is only applicable to source pads. */
> > > -		if (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)
> > > -			subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> > > -	}
> > > -
> > > -	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > > -	if (ret) {
> > > -		LOG(V4L2, Error)
> > > -			<< "Unable to set format on pad " << pad
> > > -			<< ": " << strerror(-ret);
> > > -		return ret;
> > > -	}
> > > -
> > > -	format->size.width = subdevFmt.format.width;
> > > -	format->size.height = subdevFmt.format.height;
> > > -	format->code = subdevFmt.format.code;
> > > -	format->colorSpace = toColorSpace(subdevFmt.format);
> > > -
> > > -	return 0;
> > > -}
> > >
> > >  /**
> > >   * \brief Retrieve the subdevice's internal routing table
> > > @@ -1282,14 +1382,15 @@ std::string V4L2Subdevice::logPrefix() const
> > >  	return "'" + entity_->name() + "'";
> > >  }
> > >
> > > -std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > > +std::vector<unsigned int> V4L2Subdevice::enumPadCodes(const Stream &stream)
> > >  {
> > >  	std::vector<unsigned int> codes;
> > >  	int ret;
> > >
> > >  	for (unsigned int index = 0; ; index++) {
> > >  		struct v4l2_subdev_mbus_code_enum mbusEnum = {};
> > > -		mbusEnum.pad = pad;
> > > +		mbusEnum.pad = stream.pad;
> > > +		mbusEnum.stream = stream.stream;
> > >  		mbusEnum.index = index;
> > >  		mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >
> > > @@ -1302,15 +1403,15 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > >
> > >  	if (ret < 0 && ret != -EINVAL) {
> > >  		LOG(V4L2, Error)
> > > -			<< "Unable to enumerate formats on pad " << pad
> > > -			<< ": " << strerror(-ret);
> > > +			<< "Unable to enumerate formats on pad " << stream.pad
> > > +			<< "/" << stream.stream << ": " << strerror(-ret);
> > >  		return {};
> > >  	}
> > >
> > >  	return codes;
> > >  }
> > >
> > > -std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > > +std::vector<SizeRange> V4L2Subdevice::enumPadSizes(const Stream &stream,
> > >  						   unsigned int code)
> > >  {
> > >  	std::vector<SizeRange> sizes;
> > > @@ -1319,7 +1420,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > >  	for (unsigned int index = 0;; index++) {
> > >  		struct v4l2_subdev_frame_size_enum sizeEnum = {};
> > >  		sizeEnum.index = index;
> > > -		sizeEnum.pad = pad;
> > > +		sizeEnum.pad = stream.pad;
> > > +		sizeEnum.stream = stream.stream;
> > >  		sizeEnum.code = code;
> > >  		sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >
> > > @@ -1333,8 +1435,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > >
> > >  	if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> > >  		LOG(V4L2, Error)
> > > -			<< "Unable to enumerate sizes on pad " << pad
> > > -			<< ": " << strerror(-ret);
> > > +			<< "Unable to enumerate sizes on pad " << stream.pad
> > > +			<< "/" << stream.stream << ": " << strerror(-ret);
> > >  		return {};
> >
> > The only question I have is if we should allow the V4L2Subdevice API
> > to expose a pad-only interface or we should bite the bullet and move
> > it to use Stream. The reason is that, a open() time
> >
> > 	/* If the subdev supports streams, enable the streams API. */
> > 	if (caps_.hasStreams()) {
> > 		struct v4l2_subdev_client_capability clientCaps{};
> > 		clientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> >
> > 		ret = ioctl(VIDIOC_SUBDEV_S_CLIENT_CAP, &clientCaps);
> >
> > And allowing users to use the pad-based API would make stream == 0
> > implicitly. It's a lot of work though, not sure it's worth it ?
>
> Most devices don't use streams, that's why I decided to overload the
> get/set functions and offer users a pad-based interface and a
> stream-based interface. It's low-maintenance on the V4L2Subdevice side
> (at least for now), and allows callers that deal with stream-unaware
> devices only not to have to specify the stream number. I think the code

That's my concern, not having to specify the stream number would
default it to 0 on devices which are stream aware, as we enable the
stream API at open() time unconditionally, and if the kernel drivers
gets updated to use streams and the libcamera support doesn't this
might cause unexpected issues. However, even if we make Stream usage
mandatory, the existing implementations would have to use 0 anyway, so
this probably doesn't make any practical difference ?


> gets more readable that way.
>
> > >  	}
> > >
>
> --
> Regards,
>
> Laurent Pinchart

On Wed, Feb 28, 2024 at 12:18:47PM +0100, Jacopo Mondi wrote:
> On Wed, Feb 28, 2024 at 12:51:19PM +0200, Laurent Pinchart wrote:
> > On Tue, Feb 27, 2024 at 05:48:35PM +0100, Jacopo Mondi wrote:
> > > On Tue, Feb 27, 2024 at 04:09:51PM +0200, Laurent Pinchart wrote:
> > > > Extend the V4L2Subdevice API with stream support for the functions that
> > > > get and set formats and selection rectangles. Add a Stream structure to
> > > > identify a subdev pad and stream, and use it to extend the V4L2Subdevice
> > > > functions that get and set formats and selection rectangles with stream
> > > > support.
> > > >
> > > > To preserve the existing pad-based API, implement overloaded functions
> > > > that wrap the new stream-based API. This allows callers that are not
> > > > stream-aware to use a simpler pad-based API, instead of having to
> > > > explicitly set the stream number to 0 in all API calls.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  include/libcamera/internal/v4l2_subdevice.h |  41 +++-
> > > >  src/libcamera/v4l2_subdevice.cpp            | 232 ++++++++++++++------
> > > >  2 files changed, 202 insertions(+), 71 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> > > > index 1115cfa55594..65003394a984 100644
> > > > --- a/include/libcamera/internal/v4l2_subdevice.h
> > > > +++ b/include/libcamera/internal/v4l2_subdevice.h
> > > > @@ -80,6 +80,11 @@ public:
> > > >  		ActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,
> > > >  	};
> > > >
> > > > +	struct Stream {
> > > > +		unsigned int pad;
> > > > +		unsigned int stream;
> > > > +	};
> > > > +
> > > >  	class Routing : public std::vector<struct v4l2_subdev_route>
> > > >  	{
> > > >  	public:
> > > > @@ -93,17 +98,39 @@ public:
> > > >
> > > >  	const MediaEntity *entity() const { return entity_; }
> > > >
> > > > -	int getSelection(unsigned int pad, unsigned int target,
> > > > +	int getSelection(const Stream &stream, unsigned int target,
> > > >  			 Rectangle *rect);
> > > > -	int setSelection(unsigned int pad, unsigned int target,
> > > > +	int getSelection(unsigned int pad, unsigned int target, Rectangle *rect)
> > > > +	{
> > > > +		return getSelection({ pad, 0 }, target, rect);
> > > > +	}
> > > > +	int setSelection(const Stream &stream, unsigned int target,
> > > >  			 Rectangle *rect);
> > > > +	int setSelection(unsigned int pad, unsigned int target, Rectangle *rect)
> > > > +	{
> > > > +		return setSelection({ pad, 0 }, target, rect);
> > > > +	}
> > > >
> > > > -	Formats formats(unsigned int pad);
> > > > +	Formats formats(const Stream &stream);
> > > > +	Formats formats(unsigned int pad)
> > > > +	{
> > > > +		return formats({ pad, 0 });
> > > > +	}
> > > >
> > > > +	int getFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> > > > +		      Whence whence = ActiveFormat);
> > > >  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > > +		      Whence whence = ActiveFormat)
> > > > +	{
> > > > +		return getFormat({ pad, 0 }, format, whence);
> > > > +	}
> > > > +	int setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> > > >  		      Whence whence = ActiveFormat);
> > > >  	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > > -		      Whence whence = ActiveFormat);
> > > > +		      Whence whence = ActiveFormat)
> > > > +	{
> > > > +		return setFormat({ pad, 0 }, format, whence);
> > > > +	}
> > > >
> > > >  	int getRouting(Routing *routing, Whence whence = ActiveFormat);
> > > >  	int setRouting(Routing *routing, Whence whence = ActiveFormat);
> > > > @@ -123,8 +150,8 @@ private:
> > > >  	std::optional<ColorSpace>
> > > >  	toColorSpace(const v4l2_mbus_framefmt &format) const;
> > > >
> > > > -	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> > > > -	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > > > +	std::vector<unsigned int> enumPadCodes(const Stream &stream);
> > > > +	std::vector<SizeRange> enumPadSizes(const Stream &stream,
> > > >  					    unsigned int code);
> > > >
> > > >  	const MediaEntity *entity_;
> > > > @@ -133,4 +160,6 @@ private:
> > > >  	struct V4L2SubdeviceCapability caps_;
> > > >  };
> > > >
> > > > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);
> > > > +
> > > >  } /* namespace libcamera */
> > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > > index 19ddecf714c6..efe8b0f793e9 100644
> > > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > > @@ -814,6 +814,35 @@ std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f)
> > > >   * \brief The format operation applies to TRY formats
> > > >   */
> > > >
> > > > +/**
> > > > + * \class V4L2Subdevice::Stream
> > > > + * \brief V4L2 subdevice stream
> > > > + *
> > > > + * This class identifies a subdev stream, by bundling the pad number with the
> > > > + * stream number. It is used in all stream-aware functions of the V4L2Subdevice
> > > > + * class to identify the stream the functions operate on.
> > > > + *
> > > > + * \var V4L2Subdevice::Stream::pad
> > > > + * \brief The 0-indexed pad number
> > > > + *
> > > > + * \var V4L2Subdevice::Stream::stream
> > > > + * \brief The stream number
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Insert a text representation of a V4L2Subdevice::Stream into an
> > > > + *	output stream
> > >
> > > What is the alignment rule when we break \brief ? I see different ones
> > > being used in this patch
> >
> > Good question :-) Let's decide. Does anybody have a preference between
> >
> >  * \brief Insert a text representation of a V4L2Subdevice::Stream into an
> >  *	output stream
> >
> > (single tab after the '*', no space)
> >
> > and
> >
> >  * \brief Insert a text representation of a V4L2Subdevice::Stream into an
> >  * output stream
> >
> > (single space after the '*')
> >
> 
> My vote for the second option, but whatever, really
> 
> > ?
> >
> > > > + * \param[in] out The output stream
> > > > + * \param[in] stream The V4L2Subdevice::Stream
> > > > + * \return The output stream \a out
> > > > + */
> > > > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)
> > > > +{
> > > > +	out << stream.pad << "/" << stream.stream;
> > > > +
> > > > +	return out;
> > > > +}
> > > > +
> > > >  /**
> > > >   * \class V4L2Subdevice::Routing
> > > >   * \brief V4L2 subdevice routing table
> > > > @@ -879,7 +908,10 @@ int V4L2Subdevice::open()
> > > >  		return ret;
> > > >  	}
> > > >
> > > > -	/* If the subdev supports streams, enable the streams API. */
> > > > +	/*
> > > > +	 * If the subdev supports streams, enable the streams API, and retrieve
> > > > +	 * and cache the routing table.
> > >
> > > Does it happen in this patch ?
> >
> > Indeed not. I was planning to do so and then didn't. I'll drop the
> > comment change.
> >
> > > > +	 */
> > > >  	if (caps_.hasStreams()) {
> > > >  		struct v4l2_subdev_client_capability clientCaps{};
> > > >  		clientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> > > > @@ -905,7 +937,7 @@ int V4L2Subdevice::open()
> > > >
> > > >  /**
> > > >   * \brief Get selection rectangle \a rect for \a target
> > > > - * \param[in] pad The 0-indexed pad number the rectangle is retrieved from
> > > > + * \param[in] stream The stream the rectangle is retrieved from
> > > >   * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > > >   * \param[out] rect The retrieved selection rectangle
> > > >   *
> > > > @@ -913,13 +945,14 @@ int V4L2Subdevice::open()
> > > >   *
> > > >   * \return 0 on success or a negative error code otherwise
> > > >   */
> > > > -int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > > > +int V4L2Subdevice::getSelection(const Stream &stream, unsigned int target,
> > > >  				Rectangle *rect)
> > > >  {
> > > >  	struct v4l2_subdev_selection sel = {};
> > > >
> > > >  	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > > -	sel.pad = pad;
> > > > +	sel.pad = stream.pad;
> > > > +	sel.stream = stream.stream;
> > > >  	sel.target = target;
> > > >  	sel.flags = 0;
> > > >
> > > > @@ -927,7 +960,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > > >  	if (ret < 0) {
> > > >  		LOG(V4L2, Error)
> > > >  			<< "Unable to get rectangle " << target << " on pad "
> > > > -			<< pad << ": " << strerror(-ret);
> > > > +			<< stream.pad << "/" << stream.stream << ": "
> > >
> > > Can't you use operator<<(Stream) ?
> >
> > I'll fix it, and below too.
> >
> > > > +			<< strerror(-ret);
> > > >  		return ret;
> > > >  	}
> > > >
> > > > @@ -939,9 +973,20 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > > >  	return 0;
> > > >  }
> > > >
> > > > +/**
> > > > + * \fn V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > > > + *	Rectangle *rect)
> > > > + * \brief Get selection rectangle \a rect for \a target
> > > > + * \param[in] pad The 0-indexed pad number the rectangle is retrieved from
> > > > + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > > > + * \param[out] rect The retrieved selection rectangle
> > > > + *
> > > > + * \return 0 on success or a negative error code otherwise
> > > > + */
> > > > +
> > > >  /**
> > > >   * \brief Set selection rectangle \a rect for \a target
> > > > - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > > > + * \param[in] stream The stream the rectangle is to be applied to
> > > >   * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > > >   * \param[inout] rect The selection rectangle to be applied
> > > >   *
> > > > @@ -949,13 +994,14 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> > > >   *
> > > >   * \return 0 on success or a negative error code otherwise
> > > >   */
> > > > -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > > > +int V4L2Subdevice::setSelection(const Stream &stream, unsigned int target,
> > > >  				Rectangle *rect)
> > > >  {
> > > >  	struct v4l2_subdev_selection sel = {};
> > > >
> > > >  	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > > -	sel.pad = pad;
> > > > +	sel.pad = stream.pad;
> > > > +	sel.stream = stream.stream;
> > > >  	sel.target = target;
> > > >  	sel.flags = 0;
> > > >
> > > > @@ -968,7 +1014,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > > >  	if (ret < 0) {
> > > >  		LOG(V4L2, Error)
> > > >  			<< "Unable to set rectangle " << target << " on pad "
> > > > -			<< pad << ": " << strerror(-ret);
> > > > +			<< stream.pad << "/" << stream.stream << ": "
> > >
> > > ditto
> > >
> > > > +			<< strerror(-ret);
> > > >  		return ret;
> > > >  	}
> > > >
> > > > @@ -979,26 +1026,40 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > > >
> > > >  	return 0;
> > > >  }
> > > > +
> > > >  /**
> > > > - * \brief Enumerate all media bus codes and frame sizes on a \a pad
> > > > - * \param[in] pad The 0-indexed pad number to enumerate formats on
> > > > + * \fn V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > > > + *	Rectangle *rect)
> > > > + * \brief Set selection rectangle \a rect for \a target
> > > > + * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > > > + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > > > + * \param[inout] rect The selection rectangle to be applied
> > > > + *
> > > > + * \todo Define a V4L2SelectionTarget enum for the selection target
> > > > + *
> > > > + * \return 0 on success or a negative error code otherwise
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Enumerate all media bus codes and frame sizes on a \a stream
> > > > + * \param[in] stream The stream to enumerate formats for
> > > >   *
> > > >   * Enumerate all media bus codes and frame sizes supported by the subdevice on
> > > > - * a \a pad.
> > > > + * a \a stream.
> > > >   *
> > > >   * \return A list of the supported device formats
> > > >   */
> > > > -V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> > > > +V4L2Subdevice::Formats V4L2Subdevice::formats(const Stream &stream)
> > > >  {
> > > >  	Formats formats;
> > > >
> > > > -	if (pad >= entity_->pads().size()) {
> > > > -		LOG(V4L2, Error) << "Invalid pad: " << pad;
> > > > +	if (stream.pad >= entity_->pads().size()) {
> > > > +		LOG(V4L2, Error) << "Invalid pad: " << stream.pad;
> > > >  		return {};
> > > >  	}
> > > >
> > > > -	for (unsigned int code : enumPadCodes(pad)) {
> > > > -		std::vector<SizeRange> sizes = enumPadSizes(pad, code);
> > > > +	for (unsigned int code : enumPadCodes(stream)) {
> > > > +		std::vector<SizeRange> sizes = enumPadSizes(stream, code);
> > > >  		if (sizes.empty())
> > > >  			return {};
> > > >
> > > > @@ -1006,7 +1067,7 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> > > >  		if (!inserted.second) {
> > > >  			LOG(V4L2, Error)
> > > >  				<< "Could not add sizes for media bus code "
> > > > -				<< code << " on pad " << pad;
> > > > +				<< code << " on pad " << stream.pad;
> > > >  			return {};
> > > >  		}
> > > >  	}
> > > > @@ -1014,6 +1075,17 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> > > >  	return formats;
> > > >  }
> > > >
> > > > +/**
> > > > + * \fn V4L2Subdevice::formats(unsigned int pad)
> > > > + * \brief Enumerate all media bus codes and frame sizes on a \a pad
> > > > + * \param[in] pad The 0-indexed pad number to enumerate formats on
> > > > + *
> > > > + * Enumerate all media bus codes and frame sizes supported by the subdevice on
> > > > + * a \a pad
> > > > + *
> > > > + * \return A list of the supported device formats
> > > > + */
> > > > +
> > > >  std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const
> > > >  {
> > > >  	/*
> > > > @@ -1045,25 +1117,26 @@ std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &
> > > >  }
> > > >
> > > >  /**
> > > > - * \brief Retrieve the image format set on one of the V4L2 subdevice pads
> > > > - * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> > > > + * \brief Retrieve the image format set on one of the V4L2 subdevice streams
> > > > + * \param[in] stream The stream the format is to be retrieved from
> > > >   * \param[out] format The image bus format
> > > >   * \param[in] whence The format to get, \ref V4L2Subdevice::ActiveFormat
> > > >   * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> > > >   * \return 0 on success or a negative error code otherwise
> > > >   */
> > > > -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > > +int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> > > >  			     Whence whence)
> > > >  {
> > > >  	struct v4l2_subdev_format subdevFmt = {};
> > > >  	subdevFmt.which = whence;
> > > > -	subdevFmt.pad = pad;
> > > > +	subdevFmt.pad = stream.pad;
> > > > +	subdevFmt.stream = stream.stream;
> > > >
> > > >  	int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > > >  	if (ret) {
> > > >  		LOG(V4L2, Error)
> > > > -			<< "Unable to get format on pad " << pad
> > > > -			<< ": " << strerror(-ret);
> > > > +			<< "Unable to get format on pad " << stream.pad << "/"
> > > > +			<< stream.stream << ": " << strerror(-ret);
> > > >  		return ret;
> > > >  	}
> > > >
> > > > @@ -1076,6 +1149,66 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > >  }
> > > >
> > > >  /**
> > > > + * \fn V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > > + *	Whence whence)
> > > > + * \brief Retrieve the image format set on one of the V4L2 subdevice pads
> > > > + * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> > > > + * \param[out] format The image bus format
> > > > + * \param[in] whence The format to get, \ref V4L2Subdevice::ActiveFormat
> > > > + * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> > > > + * \return 0 on success or a negative error code otherwise
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Set an image format on one of the V4L2 subdevice pads
> > > > + * \param[in] stream The stream the format is to be applied to
> > > > + * \param[inout] format The image bus format to apply to the stream
> > > > + * \param[in] whence The format to set, \ref V4L2Subdevice::ActiveFormat
> > > > + * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> > > > + *
> > > > + * Apply the requested image format to the desired stream and return the
> > > > + * actually applied format parameters, as getFormat() would do.
> > > > + *
> > > > + * \return 0 on success or a negative error code otherwise
> > > > + */
> > > > +int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> > > > +			     Whence whence)
> > > > +{
> > > > +	struct v4l2_subdev_format subdevFmt = {};
> > > > +	subdevFmt.which = whence;
> > > > +	subdevFmt.pad = stream.pad;
> > > > +	subdevFmt.stream = stream.stream;
> > > > +	subdevFmt.format.width = format->size.width;
> > > > +	subdevFmt.format.height = format->size.height;
> > > > +	subdevFmt.format.code = format->code;
> > > > +	subdevFmt.format.field = V4L2_FIELD_NONE;
> > > > +	if (format->colorSpace) {
> > > > +		fromColorSpace(format->colorSpace, subdevFmt.format);
> > > > +
> > > > +		/* The CSC flag is only applicable to source pads. */
> > > > +		if (entity_->pads()[stream.pad]->flags() & MEDIA_PAD_FL_SOURCE)
> > > > +			subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> > > > +	}
> > > > +
> > > > +	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > > > +	if (ret) {
> > > > +		LOG(V4L2, Error)
> > > > +			<< "Unable to set format on pad " << stream.pad << "/"
> > > > +			<< stream.stream << ": " << strerror(-ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	format->size.width = subdevFmt.format.width;
> > > > +	format->size.height = subdevFmt.format.height;
> > > > +	format->code = subdevFmt.format.code;
> > > > +	format->colorSpace = toColorSpace(subdevFmt.format);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * \fn V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > > + *	Whence whence)
> > > >   * \brief Set an image format on one of the V4L2 subdevice pads
> > > >   * \param[in] pad The 0-indexed pad number the format is to be applied to
> > > >   * \param[inout] format The image bus format to apply to the subdevice's pad
> > > > @@ -1087,39 +1220,6 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > >   *
> > > >   * \return 0 on success or a negative error code otherwise
> > > >   */
> > > > -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> > > > -			     Whence whence)
> > > > -{
> > > > -	struct v4l2_subdev_format subdevFmt = {};
> > > > -	subdevFmt.which = whence;
> > > > -	subdevFmt.pad = pad;
> > > > -	subdevFmt.format.width = format->size.width;
> > > > -	subdevFmt.format.height = format->size.height;
> > > > -	subdevFmt.format.code = format->code;
> > > > -	subdevFmt.format.field = V4L2_FIELD_NONE;
> > > > -	if (format->colorSpace) {
> > > > -		fromColorSpace(format->colorSpace, subdevFmt.format);
> > > > -
> > > > -		/* The CSC flag is only applicable to source pads. */
> > > > -		if (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)
> > > > -			subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> > > > -	}
> > > > -
> > > > -	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > > > -	if (ret) {
> > > > -		LOG(V4L2, Error)
> > > > -			<< "Unable to set format on pad " << pad
> > > > -			<< ": " << strerror(-ret);
> > > > -		return ret;
> > > > -	}
> > > > -
> > > > -	format->size.width = subdevFmt.format.width;
> > > > -	format->size.height = subdevFmt.format.height;
> > > > -	format->code = subdevFmt.format.code;
> > > > -	format->colorSpace = toColorSpace(subdevFmt.format);
> > > > -
> > > > -	return 0;
> > > > -}
> > > >
> > > >  /**
> > > >   * \brief Retrieve the subdevice's internal routing table
> > > > @@ -1282,14 +1382,15 @@ std::string V4L2Subdevice::logPrefix() const
> > > >  	return "'" + entity_->name() + "'";
> > > >  }
> > > >
> > > > -std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > > > +std::vector<unsigned int> V4L2Subdevice::enumPadCodes(const Stream &stream)
> > > >  {
> > > >  	std::vector<unsigned int> codes;
> > > >  	int ret;
> > > >
> > > >  	for (unsigned int index = 0; ; index++) {
> > > >  		struct v4l2_subdev_mbus_code_enum mbusEnum = {};
> > > > -		mbusEnum.pad = pad;
> > > > +		mbusEnum.pad = stream.pad;
> > > > +		mbusEnum.stream = stream.stream;
> > > >  		mbusEnum.index = index;
> > > >  		mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > >
> > > > @@ -1302,15 +1403,15 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > > >
> > > >  	if (ret < 0 && ret != -EINVAL) {
> > > >  		LOG(V4L2, Error)
> > > > -			<< "Unable to enumerate formats on pad " << pad
> > > > -			<< ": " << strerror(-ret);
> > > > +			<< "Unable to enumerate formats on pad " << stream.pad
> > > > +			<< "/" << stream.stream << ": " << strerror(-ret);
> > > >  		return {};
> > > >  	}
> > > >
> > > >  	return codes;
> > > >  }
> > > >
> > > > -std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > > > +std::vector<SizeRange> V4L2Subdevice::enumPadSizes(const Stream &stream,
> > > >  						   unsigned int code)
> > > >  {
> > > >  	std::vector<SizeRange> sizes;
> > > > @@ -1319,7 +1420,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > > >  	for (unsigned int index = 0;; index++) {
> > > >  		struct v4l2_subdev_frame_size_enum sizeEnum = {};
> > > >  		sizeEnum.index = index;
> > > > -		sizeEnum.pad = pad;
> > > > +		sizeEnum.pad = stream.pad;
> > > > +		sizeEnum.stream = stream.stream;
> > > >  		sizeEnum.code = code;
> > > >  		sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > >
> > > > @@ -1333,8 +1435,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > > >
> > > >  	if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> > > >  		LOG(V4L2, Error)
> > > > -			<< "Unable to enumerate sizes on pad " << pad
> > > > -			<< ": " << strerror(-ret);
> > > > +			<< "Unable to enumerate sizes on pad " << stream.pad
> > > > +			<< "/" << stream.stream << ": " << strerror(-ret);
> > > >  		return {};
> > >
> > > The only question I have is if we should allow the V4L2Subdevice API
> > > to expose a pad-only interface or we should bite the bullet and move
> > > it to use Stream. The reason is that, a open() time
> > >
> > > 	/* If the subdev supports streams, enable the streams API. */
> > > 	if (caps_.hasStreams()) {
> > > 		struct v4l2_subdev_client_capability clientCaps{};
> > > 		clientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> > >
> > > 		ret = ioctl(VIDIOC_SUBDEV_S_CLIENT_CAP, &clientCaps);
> > >
> > > And allowing users to use the pad-based API would make stream == 0
> > > implicitly. It's a lot of work though, not sure it's worth it ?
> >
> > Most devices don't use streams, that's why I decided to overload the
> > get/set functions and offer users a pad-based interface and a
> > stream-based interface. It's low-maintenance on the V4L2Subdevice side
> > (at least for now), and allows callers that deal with stream-unaware
> > devices only not to have to specify the stream number. I think the code
> 
> That's my concern, not having to specify the stream number would
> default it to 0 on devices which are stream aware, as we enable the
> stream API at open() time unconditionally, and if the kernel drivers
> gets updated to use streams and the libcamera support doesn't this
> might cause unexpected issues. However, even if we make Stream usage
> mandatory, the existing implementations would have to use 0 anyway, so
> this probably doesn't make any practical difference ?

Drivers are supposed to preserve backward-compatibility. For instance, a
sensor driver that grows support for embedded data streams should use
stream 0 as the image stream. I don't think it would thus make any
practical difference.

> > gets more readable that way.
> >
> > > >  	}
> > > >