[{"id":28767,"web_url":"https://patchwork.libcamera.org/comment/28767/","msgid":"","date":"2024-02-27T16:48:35","subject":"Re: [PATCH 7/9] libcamera: v4l2_subdevice: Add stream support to\n\tget/set functions","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Tue, Feb 27, 2024 at 04:09:51PM +0200, Laurent Pinchart wrote:\n> Extend the V4L2Subdevice API with stream support for the functions that\n> get and set formats and selection rectangles. Add a Stream structure to\n> identify a subdev pad and stream, and use it to extend the V4L2Subdevice\n> functions that get and set formats and selection rectangles with stream\n> support.\n>\n> To preserve the existing pad-based API, implement overloaded functions\n> that wrap the new stream-based API. This allows callers that are not\n> stream-aware to use a simpler pad-based API, instead of having to\n> explicitly set the stream number to 0 in all API calls.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/v4l2_subdevice.h | 41 +++-\n> src/libcamera/v4l2_subdevice.cpp | 232 ++++++++++++++------\n> 2 files changed, 202 insertions(+), 71 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> index 1115cfa55594..65003394a984 100644\n> --- a/include/libcamera/internal/v4l2_subdevice.h\n> +++ b/include/libcamera/internal/v4l2_subdevice.h\n> @@ -80,6 +80,11 @@ public:\n> \t\tActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,\n> \t};\n>\n> +\tstruct Stream {\n> +\t\tunsigned int pad;\n> +\t\tunsigned int stream;\n> +\t};\n> +\n> \tclass Routing : public std::vector\n> \t{\n> \tpublic:\n> @@ -93,17 +98,39 @@ public:\n>\n> \tconst MediaEntity *entity() const { return entity_; }\n>\n> -\tint getSelection(unsigned int pad, unsigned int target,\n> +\tint getSelection(const Stream &stream, unsigned int target,\n> \t\t\t Rectangle *rect);\n> -\tint setSelection(unsigned int pad, unsigned int target,\n> +\tint getSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> +\t{\n> +\t\treturn getSelection({ pad, 0 }, target, rect);\n> +\t}\n> +\tint setSelection(const Stream &stream, unsigned int target,\n> \t\t\t Rectangle *rect);\n> +\tint setSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> +\t{\n> +\t\treturn setSelection({ pad, 0 }, target, rect);\n> +\t}\n>\n> -\tFormats formats(unsigned int pad);\n> +\tFormats formats(const Stream &stream);\n> +\tFormats formats(unsigned int pad)\n> +\t{\n> +\t\treturn formats({ pad, 0 });\n> +\t}\n>\n> +\tint getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> +\t\t Whence whence = ActiveFormat);\n> \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> +\t\t Whence whence = ActiveFormat)\n> +\t{\n> +\t\treturn getFormat({ pad, 0 }, format, whence);\n> +\t}\n> +\tint setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> \t\t Whence whence = ActiveFormat);\n> \tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> -\t\t Whence whence = ActiveFormat);\n> +\t\t Whence whence = ActiveFormat)\n> +\t{\n> +\t\treturn setFormat({ pad, 0 }, format, whence);\n> +\t}\n>\n> \tint getRouting(Routing *routing, Whence whence = ActiveFormat);\n> \tint setRouting(Routing *routing, Whence whence = ActiveFormat);\n> @@ -123,8 +150,8 @@ private:\n> \tstd::optional\n> \ttoColorSpace(const v4l2_mbus_framefmt &format) const;\n>\n> -\tstd::vector enumPadCodes(unsigned int pad);\n> -\tstd::vector enumPadSizes(unsigned int pad,\n> +\tstd::vector enumPadCodes(const Stream &stream);\n> +\tstd::vector enumPadSizes(const Stream &stream,\n> \t\t\t\t\t unsigned int code);\n>\n> \tconst MediaEntity *entity_;\n> @@ -133,4 +160,6 @@ private:\n> \tstruct V4L2SubdeviceCapability caps_;\n> };\n>\n> +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 19ddecf714c6..efe8b0f793e9 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -814,6 +814,35 @@ std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f)\n> * \\brief The format operation applies to TRY formats\n> */\n>\n> +/**\n> + * \\class V4L2Subdevice::Stream\n> + * \\brief V4L2 subdevice stream\n> + *\n> + * This class identifies a subdev stream, by bundling the pad number with the\n> + * stream number. It is used in all stream-aware functions of the V4L2Subdevice\n> + * class to identify the stream the functions operate on.\n> + *\n> + * \\var V4L2Subdevice::Stream::pad\n> + * \\brief The 0-indexed pad number\n> + *\n> + * \\var V4L2Subdevice::Stream::stream\n> + * \\brief The stream number\n> + */\n> +\n> +/**\n> + * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n> + *\toutput stream\n\nWhat is the alignment rule when we break \\brief ? I see different ones\nbeing used in this patch\n\n> + * \\param[in] out The output stream\n> + * \\param[in] stream The V4L2Subdevice::Stream\n> + * \\return The output stream \\a out\n> + */\n> +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n> +{\n> +\tout << stream.pad << \"/\" << stream.stream;\n> +\n> +\treturn out;\n> +}\n> +\n> /**\n> * \\class V4L2Subdevice::Routing\n> * \\brief V4L2 subdevice routing table\n> @@ -879,7 +908,10 @@ int V4L2Subdevice::open()\n> \t\treturn ret;\n> \t}\n>\n> -\t/* If the subdev supports streams, enable the streams API. */\n> +\t/*\n> +\t * If the subdev supports streams, enable the streams API, and retrieve\n> +\t * and cache the routing table.\n\nDoes it happen in this patch ?\n\n> +\t */\n> \tif (caps_.hasStreams()) {\n> \t\tstruct v4l2_subdev_client_capability clientCaps{};\n> \t\tclientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;\n> @@ -905,7 +937,7 @@ int V4L2Subdevice::open()\n>\n> /**\n> * \\brief Get selection rectangle \\a rect for \\a target\n> - * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> + * \\param[in] stream The stream the rectangle is retrieved from\n> * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> * \\param[out] rect The retrieved selection rectangle\n> *\n> @@ -913,13 +945,14 @@ int V4L2Subdevice::open()\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> +int V4L2Subdevice::getSelection(const Stream &stream, unsigned int target,\n> \t\t\t\tRectangle *rect)\n> {\n> \tstruct v4l2_subdev_selection sel = {};\n>\n> \tsel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> -\tsel.pad = pad;\n> +\tsel.pad = stream.pad;\n> +\tsel.stream = stream.stream;\n> \tsel.target = target;\n> \tsel.flags = 0;\n>\n> @@ -927,7 +960,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> \tif (ret < 0) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Unable to get rectangle \" << target << \" on pad \"\n> -\t\t\t<< pad << \": \" << strerror(-ret);\n> +\t\t\t<< stream.pad << \"/\" << stream.stream << \": \"\n\nCan't you use operator<<(Stream) ?\n\n> +\t\t\t<< strerror(-ret);\n> \t\treturn ret;\n> \t}\n>\n> @@ -939,9 +973,20 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> \treturn 0;\n> }\n>\n> +/**\n> + * \\fn V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> + *\tRectangle *rect)\n> + * \\brief Get selection rectangle \\a rect for \\a target\n> + * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> + * \\param[out] rect The retrieved selection rectangle\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> /**\n> * \\brief Set selection rectangle \\a rect for \\a target\n> - * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> + * \\param[in] stream The stream the rectangle is to be applied to\n> * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> * \\param[inout] rect The selection rectangle to be applied\n> *\n> @@ -949,13 +994,14 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> +int V4L2Subdevice::setSelection(const Stream &stream, unsigned int target,\n> \t\t\t\tRectangle *rect)\n> {\n> \tstruct v4l2_subdev_selection sel = {};\n>\n> \tsel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> -\tsel.pad = pad;\n> +\tsel.pad = stream.pad;\n> +\tsel.stream = stream.stream;\n> \tsel.target = target;\n> \tsel.flags = 0;\n>\n> @@ -968,7 +1014,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> \tif (ret < 0) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Unable to set rectangle \" << target << \" on pad \"\n> -\t\t\t<< pad << \": \" << strerror(-ret);\n> +\t\t\t<< stream.pad << \"/\" << stream.stream << \": \"\n\nditto\n\n> +\t\t\t<< strerror(-ret);\n> \t\treturn ret;\n> \t}\n>\n> @@ -979,26 +1026,40 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n>\n> \treturn 0;\n> }\n> +\n> /**\n> - * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> - * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> + * \\fn V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> + *\tRectangle *rect)\n> + * \\brief Set selection rectangle \\a rect for \\a target\n> + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> + * \\param[inout] rect The selection rectangle to be applied\n> + *\n> + * \\todo Define a V4L2SelectionTarget enum for the selection target\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\brief Enumerate all media bus codes and frame sizes on a \\a stream\n> + * \\param[in] stream The stream to enumerate formats for\n> *\n> * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> - * a \\a pad.\n> + * a \\a stream.\n> *\n> * \\return A list of the supported device formats\n> */\n> -V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> +V4L2Subdevice::Formats V4L2Subdevice::formats(const Stream &stream)\n> {\n> \tFormats formats;\n>\n> -\tif (pad >= entity_->pads().size()) {\n> -\t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> +\tif (stream.pad >= entity_->pads().size()) {\n> +\t\tLOG(V4L2, Error) << \"Invalid pad: \" << stream.pad;\n> \t\treturn {};\n> \t}\n>\n> -\tfor (unsigned int code : enumPadCodes(pad)) {\n> -\t\tstd::vector sizes = enumPadSizes(pad, code);\n> +\tfor (unsigned int code : enumPadCodes(stream)) {\n> +\t\tstd::vector sizes = enumPadSizes(stream, code);\n> \t\tif (sizes.empty())\n> \t\t\treturn {};\n>\n> @@ -1006,7 +1067,7 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> \t\tif (!inserted.second) {\n> \t\t\tLOG(V4L2, Error)\n> \t\t\t\t<< \"Could not add sizes for media bus code \"\n> -\t\t\t\t<< code << \" on pad \" << pad;\n> +\t\t\t\t<< code << \" on pad \" << stream.pad;\n> \t\t\treturn {};\n> \t\t}\n> \t}\n> @@ -1014,6 +1075,17 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> \treturn formats;\n> }\n>\n> +/**\n> + * \\fn V4L2Subdevice::formats(unsigned int pad)\n> + * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> + * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> + *\n> + * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> + * a \\a pad\n> + *\n> + * \\return A list of the supported device formats\n> + */\n> +\n> std::optional V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const\n> {\n> \t/*\n> @@ -1045,25 +1117,26 @@ std::optional V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &\n> }\n>\n> /**\n> - * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> - * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> + * \\brief Retrieve the image format set on one of the V4L2 subdevice streams\n> + * \\param[in] stream The stream the format is to be retrieved from\n> * \\param[out] format The image bus format\n> * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n> * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> +int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> \t\t\t Whence whence)\n> {\n> \tstruct v4l2_subdev_format subdevFmt = {};\n> \tsubdevFmt.which = whence;\n> -\tsubdevFmt.pad = pad;\n> +\tsubdevFmt.pad = stream.pad;\n> +\tsubdevFmt.stream = stream.stream;\n>\n> \tint ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n> \tif (ret) {\n> \t\tLOG(V4L2, Error)\n> -\t\t\t<< \"Unable to get format on pad \" << pad\n> -\t\t\t<< \": \" << strerror(-ret);\n> +\t\t\t<< \"Unable to get format on pad \" << stream.pad << \"/\"\n> +\t\t\t<< stream.stream << \": \" << strerror(-ret);\n> \t\treturn ret;\n> \t}\n>\n> @@ -1076,6 +1149,66 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> }\n>\n> /**\n> + * \\fn V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> + *\tWhence whence)\n> + * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> + * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> + * \\param[out] format The image bus format\n> + * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n> + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\brief Set an image format on one of the V4L2 subdevice pads\n> + * \\param[in] stream The stream the format is to be applied to\n> + * \\param[inout] format The image bus format to apply to the stream\n> + * \\param[in] whence The format to set, \\ref V4L2Subdevice::ActiveFormat\n> + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> + *\n> + * Apply the requested image format to the desired stream and return the\n> + * actually applied format parameters, as getFormat() would do.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> +\t\t\t Whence whence)\n> +{\n> +\tstruct v4l2_subdev_format subdevFmt = {};\n> +\tsubdevFmt.which = whence;\n> +\tsubdevFmt.pad = stream.pad;\n> +\tsubdevFmt.stream = stream.stream;\n> +\tsubdevFmt.format.width = format->size.width;\n> +\tsubdevFmt.format.height = format->size.height;\n> +\tsubdevFmt.format.code = format->code;\n> +\tsubdevFmt.format.field = V4L2_FIELD_NONE;\n> +\tif (format->colorSpace) {\n> +\t\tfromColorSpace(format->colorSpace, subdevFmt.format);\n> +\n> +\t\t/* The CSC flag is only applicable to source pads. */\n> +\t\tif (entity_->pads()[stream.pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> +\t\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> +\t}\n> +\n> +\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> +\tif (ret) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Unable to set format on pad \" << stream.pad << \"/\"\n> +\t\t\t<< stream.stream << \": \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tformat->size.width = subdevFmt.format.width;\n> +\tformat->size.height = subdevFmt.format.height;\n> +\tformat->code = subdevFmt.format.code;\n> +\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> + *\tWhence whence)\n> * \\brief Set an image format on one of the V4L2 subdevice pads\n> * \\param[in] pad The 0-indexed pad number the format is to be applied to\n> * \\param[inout] format The image bus format to apply to the subdevice's pad\n> @@ -1087,39 +1220,6 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> -\t\t\t Whence whence)\n> -{\n> -\tstruct v4l2_subdev_format subdevFmt = {};\n> -\tsubdevFmt.which = whence;\n> -\tsubdevFmt.pad = pad;\n> -\tsubdevFmt.format.width = format->size.width;\n> -\tsubdevFmt.format.height = format->size.height;\n> -\tsubdevFmt.format.code = format->code;\n> -\tsubdevFmt.format.field = V4L2_FIELD_NONE;\n> -\tif (format->colorSpace) {\n> -\t\tfromColorSpace(format->colorSpace, subdevFmt.format);\n> -\n> -\t\t/* The CSC flag is only applicable to source pads. */\n> -\t\tif (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> -\t\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> -\t}\n> -\n> -\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> -\tif (ret) {\n> -\t\tLOG(V4L2, Error)\n> -\t\t\t<< \"Unable to set format on pad \" << pad\n> -\t\t\t<< \": \" << strerror(-ret);\n> -\t\treturn ret;\n> -\t}\n> -\n> -\tformat->size.width = subdevFmt.format.width;\n> -\tformat->size.height = subdevFmt.format.height;\n> -\tformat->code = subdevFmt.format.code;\n> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> -\n> -\treturn 0;\n> -}\n>\n> /**\n> * \\brief Retrieve the subdevice's internal routing table\n> @@ -1282,14 +1382,15 @@ std::string V4L2Subdevice::logPrefix() const\n> \treturn \"'\" + entity_->name() + \"'\";\n> }\n>\n> -std::vector V4L2Subdevice::enumPadCodes(unsigned int pad)\n> +std::vector V4L2Subdevice::enumPadCodes(const Stream &stream)\n> {\n> \tstd::vector codes;\n> \tint ret;\n>\n> \tfor (unsigned int index = 0; ; index++) {\n> \t\tstruct v4l2_subdev_mbus_code_enum mbusEnum = {};\n> -\t\tmbusEnum.pad = pad;\n> +\t\tmbusEnum.pad = stream.pad;\n> +\t\tmbusEnum.stream = stream.stream;\n> \t\tmbusEnum.index = index;\n> \t\tmbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n>\n> @@ -1302,15 +1403,15 @@ std::vector V4L2Subdevice::enumPadCodes(unsigned int pad)\n>\n> \tif (ret < 0 && ret != -EINVAL) {\n> \t\tLOG(V4L2, Error)\n> -\t\t\t<< \"Unable to enumerate formats on pad \" << pad\n> -\t\t\t<< \": \" << strerror(-ret);\n> +\t\t\t<< \"Unable to enumerate formats on pad \" << stream.pad\n> +\t\t\t<< \"/\" << stream.stream << \": \" << strerror(-ret);\n> \t\treturn {};\n> \t}\n>\n> \treturn codes;\n> }\n>\n> -std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> +std::vector V4L2Subdevice::enumPadSizes(const Stream &stream,\n> \t\t\t\t\t\t unsigned int code)\n> {\n> \tstd::vector sizes;\n> @@ -1319,7 +1420,8 @@ std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> \tfor (unsigned int index = 0;; index++) {\n> \t\tstruct v4l2_subdev_frame_size_enum sizeEnum = {};\n> \t\tsizeEnum.index = index;\n> -\t\tsizeEnum.pad = pad;\n> +\t\tsizeEnum.pad = stream.pad;\n> +\t\tsizeEnum.stream = stream.stream;\n> \t\tsizeEnum.code = code;\n> \t\tsizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n>\n> @@ -1333,8 +1435,8 @@ std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n>\n> \tif (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {\n> \t\tLOG(V4L2, Error)\n> -\t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n> -\t\t\t<< \": \" << strerror(-ret);\n> +\t\t\t<< \"Unable to enumerate sizes on pad \" << stream.pad\n> +\t\t\t<< \"/\" << stream.stream << \": \" << strerror(-ret);\n> \t\treturn {};\n\nThe only question I have is if we should allow the V4L2Subdevice API\nto expose a pad-only interface or we should bite the bullet and move\nit to use Stream. The reason is that, a open() time\n\n\t/* If the subdev supports streams, enable the streams API. */\n\tif (caps_.hasStreams()) {\n\t\tstruct v4l2_subdev_client_capability clientCaps{};\n\t\tclientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;\n\n\t\tret = ioctl(VIDIOC_SUBDEV_S_CLIENT_CAP, &clientCaps);\n\nAnd allowing users to use the pad-based API would make stream == 0\nimplicitly. It's a lot of work though, not sure it's worth it ?\n\n> \t}\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 71571BD160\n\tfor ;\n\tTue, 27 Feb 2024 16:48:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C340062867;\n\tTue, 27 Feb 2024 17:48:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3FBB662806\n\tfor ;\n\tTue, 27 Feb 2024 17:48:39 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D779E8C;\n\tTue, 27 Feb 2024 17:48:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eD/C0vgU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709052507;\n\tbh=Q2f2KK9acqj2tmzbIkZyreJAceiturtMyJI6Y1rYxpc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eD/C0vgU3s2gNF5OxasNQ+miFNDx/rD3804Qo7o+4Jc9nCE5b8JA3kkj9XU3K7bxp\n\tH414oZNK5AKTZoaSlHz8exLyme8Mi1hyWVD0oJyiWHML7ox3/zLw/gL5mJRJe9zjG3\n\tj3gCSBvSz9/IRUE9rMFX+F3rI0H9iWYbD0meVts0=","Date":"Tue, 27 Feb 2024 17:48:35 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Subject":"Re: [PATCH 7/9] libcamera: v4l2_subdevice: Add stream support to\n\tget/set functions","Message-ID":"","References":"<20240227140953.26093-1-laurent.pinchart@ideasonboard.com>\n\t<20240227140953.26093-8-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240227140953.26093-8-laurent.pinchart@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28786,"web_url":"https://patchwork.libcamera.org/comment/28786/","msgid":"<20240228105119.GH3419@pendragon.ideasonboard.com>","date":"2024-02-28T10:51:19","subject":"Re: [PATCH 7/9] libcamera: v4l2_subdevice: Add stream support to\n\tget/set functions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Feb 27, 2024 at 05:48:35PM +0100, Jacopo Mondi wrote:\n> On Tue, Feb 27, 2024 at 04:09:51PM +0200, Laurent Pinchart wrote:\n> > Extend the V4L2Subdevice API with stream support for the functions that\n> > get and set formats and selection rectangles. Add a Stream structure to\n> > identify a subdev pad and stream, and use it to extend the V4L2Subdevice\n> > functions that get and set formats and selection rectangles with stream\n> > support.\n> >\n> > To preserve the existing pad-based API, implement overloaded functions\n> > that wrap the new stream-based API. This allows callers that are not\n> > stream-aware to use a simpler pad-based API, instead of having to\n> > explicitly set the stream number to 0 in all API calls.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/v4l2_subdevice.h | 41 +++-\n> > src/libcamera/v4l2_subdevice.cpp | 232 ++++++++++++++------\n> > 2 files changed, 202 insertions(+), 71 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> > index 1115cfa55594..65003394a984 100644\n> > --- a/include/libcamera/internal/v4l2_subdevice.h\n> > +++ b/include/libcamera/internal/v4l2_subdevice.h\n> > @@ -80,6 +80,11 @@ public:\n> > \t\tActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,\n> > \t};\n> >\n> > +\tstruct Stream {\n> > +\t\tunsigned int pad;\n> > +\t\tunsigned int stream;\n> > +\t};\n> > +\n> > \tclass Routing : public std::vector\n> > \t{\n> > \tpublic:\n> > @@ -93,17 +98,39 @@ public:\n> >\n> > \tconst MediaEntity *entity() const { return entity_; }\n> >\n> > -\tint getSelection(unsigned int pad, unsigned int target,\n> > +\tint getSelection(const Stream &stream, unsigned int target,\n> > \t\t\t Rectangle *rect);\n> > -\tint setSelection(unsigned int pad, unsigned int target,\n> > +\tint getSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> > +\t{\n> > +\t\treturn getSelection({ pad, 0 }, target, rect);\n> > +\t}\n> > +\tint setSelection(const Stream &stream, unsigned int target,\n> > \t\t\t Rectangle *rect);\n> > +\tint setSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> > +\t{\n> > +\t\treturn setSelection({ pad, 0 }, target, rect);\n> > +\t}\n> >\n> > -\tFormats formats(unsigned int pad);\n> > +\tFormats formats(const Stream &stream);\n> > +\tFormats formats(unsigned int pad)\n> > +\t{\n> > +\t\treturn formats({ pad, 0 });\n> > +\t}\n> >\n> > +\tint getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > +\t\t Whence whence = ActiveFormat);\n> > \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > +\t\t Whence whence = ActiveFormat)\n> > +\t{\n> > +\t\treturn getFormat({ pad, 0 }, format, whence);\n> > +\t}\n> > +\tint setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > \t\t Whence whence = ActiveFormat);\n> > \tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > -\t\t Whence whence = ActiveFormat);\n> > +\t\t Whence whence = ActiveFormat)\n> > +\t{\n> > +\t\treturn setFormat({ pad, 0 }, format, whence);\n> > +\t}\n> >\n> > \tint getRouting(Routing *routing, Whence whence = ActiveFormat);\n> > \tint setRouting(Routing *routing, Whence whence = ActiveFormat);\n> > @@ -123,8 +150,8 @@ private:\n> > \tstd::optional\n> > \ttoColorSpace(const v4l2_mbus_framefmt &format) const;\n> >\n> > -\tstd::vector enumPadCodes(unsigned int pad);\n> > -\tstd::vector enumPadSizes(unsigned int pad,\n> > +\tstd::vector enumPadCodes(const Stream &stream);\n> > +\tstd::vector enumPadSizes(const Stream &stream,\n> > \t\t\t\t\t unsigned int code);\n> >\n> > \tconst MediaEntity *entity_;\n> > @@ -133,4 +160,6 @@ private:\n> > \tstruct V4L2SubdeviceCapability caps_;\n> > };\n> >\n> > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 19ddecf714c6..efe8b0f793e9 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -814,6 +814,35 @@ std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f)\n> > * \\brief The format operation applies to TRY formats\n> > */\n> >\n> > +/**\n> > + * \\class V4L2Subdevice::Stream\n> > + * \\brief V4L2 subdevice stream\n> > + *\n> > + * This class identifies a subdev stream, by bundling the pad number with the\n> > + * stream number. It is used in all stream-aware functions of the V4L2Subdevice\n> > + * class to identify the stream the functions operate on.\n> > + *\n> > + * \\var V4L2Subdevice::Stream::pad\n> > + * \\brief The 0-indexed pad number\n> > + *\n> > + * \\var V4L2Subdevice::Stream::stream\n> > + * \\brief The stream number\n> > + */\n> > +\n> > +/**\n> > + * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n> > + *\toutput stream\n> \n> What is the alignment rule when we break \\brief ? I see different ones\n> being used in this patch\n\nGood question :-) Let's decide. Does anybody have a preference between\n\n * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n *\toutput stream\n\n(single tab after the '*', no space)\n\nand\n\n * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n * output stream\n\n(single space after the '*')\n\n?\n\n> > + * \\param[in] out The output stream\n> > + * \\param[in] stream The V4L2Subdevice::Stream\n> > + * \\return The output stream \\a out\n> > + */\n> > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n> > +{\n> > +\tout << stream.pad << \"/\" << stream.stream;\n> > +\n> > +\treturn out;\n> > +}\n> > +\n> > /**\n> > * \\class V4L2Subdevice::Routing\n> > * \\brief V4L2 subdevice routing table\n> > @@ -879,7 +908,10 @@ int V4L2Subdevice::open()\n> > \t\treturn ret;\n> > \t}\n> >\n> > -\t/* If the subdev supports streams, enable the streams API. */\n> > +\t/*\n> > +\t * If the subdev supports streams, enable the streams API, and retrieve\n> > +\t * and cache the routing table.\n> \n> Does it happen in this patch ?\n\nIndeed not. I was planning to do so and then didn't. I'll drop the\ncomment change.\n\n> > +\t */\n> > \tif (caps_.hasStreams()) {\n> > \t\tstruct v4l2_subdev_client_capability clientCaps{};\n> > \t\tclientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;\n> > @@ -905,7 +937,7 @@ int V4L2Subdevice::open()\n> >\n> > /**\n> > * \\brief Get selection rectangle \\a rect for \\a target\n> > - * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> > + * \\param[in] stream The stream the rectangle is retrieved from\n> > * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > * \\param[out] rect The retrieved selection rectangle\n> > *\n> > @@ -913,13 +945,14 @@ int V4L2Subdevice::open()\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > -int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > +int V4L2Subdevice::getSelection(const Stream &stream, unsigned int target,\n> > \t\t\t\tRectangle *rect)\n> > {\n> > \tstruct v4l2_subdev_selection sel = {};\n> >\n> > \tsel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > -\tsel.pad = pad;\n> > +\tsel.pad = stream.pad;\n> > +\tsel.stream = stream.stream;\n> > \tsel.target = target;\n> > \tsel.flags = 0;\n> >\n> > @@ -927,7 +960,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > \tif (ret < 0) {\n> > \t\tLOG(V4L2, Error)\n> > \t\t\t<< \"Unable to get rectangle \" << target << \" on pad \"\n> > -\t\t\t<< pad << \": \" << strerror(-ret);\n> > +\t\t\t<< stream.pad << \"/\" << stream.stream << \": \"\n> \n> Can't you use operator<<(Stream) ?\n\nI'll fix it, and below too.\n\n> > +\t\t\t<< strerror(-ret);\n> > \t\treturn ret;\n> > \t}\n> >\n> > @@ -939,9 +973,20 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > \treturn 0;\n> > }\n> >\n> > +/**\n> > + * \\fn V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > + *\tRectangle *rect)\n> > + * \\brief Get selection rectangle \\a rect for \\a target\n> > + * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> > + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > + * \\param[out] rect The retrieved selection rectangle\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> > /**\n> > * \\brief Set selection rectangle \\a rect for \\a target\n> > - * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > + * \\param[in] stream The stream the rectangle is to be applied to\n> > * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > * \\param[inout] rect The selection rectangle to be applied\n> > *\n> > @@ -949,13 +994,14 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > +int V4L2Subdevice::setSelection(const Stream &stream, unsigned int target,\n> > \t\t\t\tRectangle *rect)\n> > {\n> > \tstruct v4l2_subdev_selection sel = {};\n> >\n> > \tsel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > -\tsel.pad = pad;\n> > +\tsel.pad = stream.pad;\n> > +\tsel.stream = stream.stream;\n> > \tsel.target = target;\n> > \tsel.flags = 0;\n> >\n> > @@ -968,7 +1014,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > \tif (ret < 0) {\n> > \t\tLOG(V4L2, Error)\n> > \t\t\t<< \"Unable to set rectangle \" << target << \" on pad \"\n> > -\t\t\t<< pad << \": \" << strerror(-ret);\n> > +\t\t\t<< stream.pad << \"/\" << stream.stream << \": \"\n> \n> ditto\n> \n> > +\t\t\t<< strerror(-ret);\n> > \t\treturn ret;\n> > \t}\n> >\n> > @@ -979,26 +1026,40 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> >\n> > \treturn 0;\n> > }\n> > +\n> > /**\n> > - * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> > - * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> > + * \\fn V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > + *\tRectangle *rect)\n> > + * \\brief Set selection rectangle \\a rect for \\a target\n> > + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > + * \\param[inout] rect The selection rectangle to be applied\n> > + *\n> > + * \\todo Define a V4L2SelectionTarget enum for the selection target\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\brief Enumerate all media bus codes and frame sizes on a \\a stream\n> > + * \\param[in] stream The stream to enumerate formats for\n> > *\n> > * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> > - * a \\a pad.\n> > + * a \\a stream.\n> > *\n> > * \\return A list of the supported device formats\n> > */\n> > -V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> > +V4L2Subdevice::Formats V4L2Subdevice::formats(const Stream &stream)\n> > {\n> > \tFormats formats;\n> >\n> > -\tif (pad >= entity_->pads().size()) {\n> > -\t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> > +\tif (stream.pad >= entity_->pads().size()) {\n> > +\t\tLOG(V4L2, Error) << \"Invalid pad: \" << stream.pad;\n> > \t\treturn {};\n> > \t}\n> >\n> > -\tfor (unsigned int code : enumPadCodes(pad)) {\n> > -\t\tstd::vector sizes = enumPadSizes(pad, code);\n> > +\tfor (unsigned int code : enumPadCodes(stream)) {\n> > +\t\tstd::vector sizes = enumPadSizes(stream, code);\n> > \t\tif (sizes.empty())\n> > \t\t\treturn {};\n> >\n> > @@ -1006,7 +1067,7 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> > \t\tif (!inserted.second) {\n> > \t\t\tLOG(V4L2, Error)\n> > \t\t\t\t<< \"Could not add sizes for media bus code \"\n> > -\t\t\t\t<< code << \" on pad \" << pad;\n> > +\t\t\t\t<< code << \" on pad \" << stream.pad;\n> > \t\t\treturn {};\n> > \t\t}\n> > \t}\n> > @@ -1014,6 +1075,17 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> > \treturn formats;\n> > }\n> >\n> > +/**\n> > + * \\fn V4L2Subdevice::formats(unsigned int pad)\n> > + * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> > + * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> > + *\n> > + * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> > + * a \\a pad\n> > + *\n> > + * \\return A list of the supported device formats\n> > + */\n> > +\n> > std::optional V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const\n> > {\n> > \t/*\n> > @@ -1045,25 +1117,26 @@ std::optional V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &\n> > }\n> >\n> > /**\n> > - * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> > - * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> > + * \\brief Retrieve the image format set on one of the V4L2 subdevice streams\n> > + * \\param[in] stream The stream the format is to be retrieved from\n> > * \\param[out] format The image bus format\n> > * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n> > * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > +int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > \t\t\t Whence whence)\n> > {\n> > \tstruct v4l2_subdev_format subdevFmt = {};\n> > \tsubdevFmt.which = whence;\n> > -\tsubdevFmt.pad = pad;\n> > +\tsubdevFmt.pad = stream.pad;\n> > +\tsubdevFmt.stream = stream.stream;\n> >\n> > \tint ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n> > \tif (ret) {\n> > \t\tLOG(V4L2, Error)\n> > -\t\t\t<< \"Unable to get format on pad \" << pad\n> > -\t\t\t<< \": \" << strerror(-ret);\n> > +\t\t\t<< \"Unable to get format on pad \" << stream.pad << \"/\"\n> > +\t\t\t<< stream.stream << \": \" << strerror(-ret);\n> > \t\treturn ret;\n> > \t}\n> >\n> > @@ -1076,6 +1149,66 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > }\n> >\n> > /**\n> > + * \\fn V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > + *\tWhence whence)\n> > + * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> > + * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> > + * \\param[out] format The image bus format\n> > + * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n> > + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\brief Set an image format on one of the V4L2 subdevice pads\n> > + * \\param[in] stream The stream the format is to be applied to\n> > + * \\param[inout] format The image bus format to apply to the stream\n> > + * \\param[in] whence The format to set, \\ref V4L2Subdevice::ActiveFormat\n> > + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> > + *\n> > + * Apply the requested image format to the desired stream and return the\n> > + * actually applied format parameters, as getFormat() would do.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > +\t\t\t Whence whence)\n> > +{\n> > +\tstruct v4l2_subdev_format subdevFmt = {};\n> > +\tsubdevFmt.which = whence;\n> > +\tsubdevFmt.pad = stream.pad;\n> > +\tsubdevFmt.stream = stream.stream;\n> > +\tsubdevFmt.format.width = format->size.width;\n> > +\tsubdevFmt.format.height = format->size.height;\n> > +\tsubdevFmt.format.code = format->code;\n> > +\tsubdevFmt.format.field = V4L2_FIELD_NONE;\n> > +\tif (format->colorSpace) {\n> > +\t\tfromColorSpace(format->colorSpace, subdevFmt.format);\n> > +\n> > +\t\t/* The CSC flag is only applicable to source pads. */\n> > +\t\tif (entity_->pads()[stream.pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> > +\t\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> > +\t}\n> > +\n> > +\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> > +\tif (ret) {\n> > +\t\tLOG(V4L2, Error)\n> > +\t\t\t<< \"Unable to set format on pad \" << stream.pad << \"/\"\n> > +\t\t\t<< stream.stream << \": \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tformat->size.width = subdevFmt.format.width;\n> > +\tformat->size.height = subdevFmt.format.height;\n> > +\tformat->code = subdevFmt.format.code;\n> > +\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > + *\tWhence whence)\n> > * \\brief Set an image format on one of the V4L2 subdevice pads\n> > * \\param[in] pad The 0-indexed pad number the format is to be applied to\n> > * \\param[inout] format The image bus format to apply to the subdevice's pad\n> > @@ -1087,39 +1220,6 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > -\t\t\t Whence whence)\n> > -{\n> > -\tstruct v4l2_subdev_format subdevFmt = {};\n> > -\tsubdevFmt.which = whence;\n> > -\tsubdevFmt.pad = pad;\n> > -\tsubdevFmt.format.width = format->size.width;\n> > -\tsubdevFmt.format.height = format->size.height;\n> > -\tsubdevFmt.format.code = format->code;\n> > -\tsubdevFmt.format.field = V4L2_FIELD_NONE;\n> > -\tif (format->colorSpace) {\n> > -\t\tfromColorSpace(format->colorSpace, subdevFmt.format);\n> > -\n> > -\t\t/* The CSC flag is only applicable to source pads. */\n> > -\t\tif (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> > -\t\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> > -\t}\n> > -\n> > -\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> > -\tif (ret) {\n> > -\t\tLOG(V4L2, Error)\n> > -\t\t\t<< \"Unable to set format on pad \" << pad\n> > -\t\t\t<< \": \" << strerror(-ret);\n> > -\t\treturn ret;\n> > -\t}\n> > -\n> > -\tformat->size.width = subdevFmt.format.width;\n> > -\tformat->size.height = subdevFmt.format.height;\n> > -\tformat->code = subdevFmt.format.code;\n> > -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> > -\n> > -\treturn 0;\n> > -}\n> >\n> > /**\n> > * \\brief Retrieve the subdevice's internal routing table\n> > @@ -1282,14 +1382,15 @@ std::string V4L2Subdevice::logPrefix() const\n> > \treturn \"'\" + entity_->name() + \"'\";\n> > }\n> >\n> > -std::vector V4L2Subdevice::enumPadCodes(unsigned int pad)\n> > +std::vector V4L2Subdevice::enumPadCodes(const Stream &stream)\n> > {\n> > \tstd::vector codes;\n> > \tint ret;\n> >\n> > \tfor (unsigned int index = 0; ; index++) {\n> > \t\tstruct v4l2_subdev_mbus_code_enum mbusEnum = {};\n> > -\t\tmbusEnum.pad = pad;\n> > +\t\tmbusEnum.pad = stream.pad;\n> > +\t\tmbusEnum.stream = stream.stream;\n> > \t\tmbusEnum.index = index;\n> > \t\tmbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> >\n> > @@ -1302,15 +1403,15 @@ std::vector V4L2Subdevice::enumPadCodes(unsigned int pad)\n> >\n> > \tif (ret < 0 && ret != -EINVAL) {\n> > \t\tLOG(V4L2, Error)\n> > -\t\t\t<< \"Unable to enumerate formats on pad \" << pad\n> > -\t\t\t<< \": \" << strerror(-ret);\n> > +\t\t\t<< \"Unable to enumerate formats on pad \" << stream.pad\n> > +\t\t\t<< \"/\" << stream.stream << \": \" << strerror(-ret);\n> > \t\treturn {};\n> > \t}\n> >\n> > \treturn codes;\n> > }\n> >\n> > -std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > +std::vector V4L2Subdevice::enumPadSizes(const Stream &stream,\n> > \t\t\t\t\t\t unsigned int code)\n> > {\n> > \tstd::vector sizes;\n> > @@ -1319,7 +1420,8 @@ std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > \tfor (unsigned int index = 0;; index++) {\n> > \t\tstruct v4l2_subdev_frame_size_enum sizeEnum = {};\n> > \t\tsizeEnum.index = index;\n> > -\t\tsizeEnum.pad = pad;\n> > +\t\tsizeEnum.pad = stream.pad;\n> > +\t\tsizeEnum.stream = stream.stream;\n> > \t\tsizeEnum.code = code;\n> > \t\tsizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> >\n> > @@ -1333,8 +1435,8 @@ std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> >\n> > \tif (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {\n> > \t\tLOG(V4L2, Error)\n> > -\t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n> > -\t\t\t<< \": \" << strerror(-ret);\n> > +\t\t\t<< \"Unable to enumerate sizes on pad \" << stream.pad\n> > +\t\t\t<< \"/\" << stream.stream << \": \" << strerror(-ret);\n> > \t\treturn {};\n> \n> The only question I have is if we should allow the V4L2Subdevice API\n> to expose a pad-only interface or we should bite the bullet and move\n> it to use Stream. The reason is that, a open() time\n> \n> \t/* If the subdev supports streams, enable the streams API. */\n> \tif (caps_.hasStreams()) {\n> \t\tstruct v4l2_subdev_client_capability clientCaps{};\n> \t\tclientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;\n> \n> \t\tret = ioctl(VIDIOC_SUBDEV_S_CLIENT_CAP, &clientCaps);\n> \n> And allowing users to use the pad-based API would make stream == 0\n> implicitly. It's a lot of work though, not sure it's worth it ?\n\nMost devices don't use streams, that's why I decided to overload the\nget/set functions and offer users a pad-based interface and a\nstream-based interface. It's low-maintenance on the V4L2Subdevice side\n(at least for now), and allows callers that deal with stream-unaware\ndevices only not to have to specify the stream number. I think the code\ngets more readable that way.\n\n> > \t}\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DA091BD80A\n\tfor ;\n\tWed, 28 Feb 2024 10:51:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA4CF62868;\n\tWed, 28 Feb 2024 11:51:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B3835627F9\n\tfor ;\n\tWed, 28 Feb 2024 11:51:17 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BB8EB672;\n\tWed, 28 Feb 2024 11:51:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"i8mX9CNg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709117465;\n\tbh=BNYezdlh8zX7Sdnu2QirqcRCAnTew4Vptuq4sid4uBs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=i8mX9CNg5cRJbpjfJF1EeX8dn+fBpCYj059wo4hvY0YuDXDCPXaVSAuJeY8FmGqa5\n\tZwHOY+bPrhcv74XmTv2TLltScYEevBJX9tbI9pcje+imXoLSx19/zsy7KIcVMUGV5v\n\tfuRF1bOIVPC6b6qIpdMTvJMdDHuM0cs5Vl+IHP4I=","Date":"Wed, 28 Feb 2024 12:51:19 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Subject":"Re: [PATCH 7/9] libcamera: v4l2_subdevice: Add stream support to\n\tget/set functions","Message-ID":"<20240228105119.GH3419@pendragon.ideasonboard.com>","References":"<20240227140953.26093-1-laurent.pinchart@ideasonboard.com>\n\t<20240227140953.26093-8-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28791,"web_url":"https://patchwork.libcamera.org/comment/28791/","msgid":"<4mswhhito7dgbwk27oanrtlwoph7qzpuwiao6o7gfelx57kjc4@3k33law6dubx>","date":"2024-02-28T11:18:47","subject":"Re: [PATCH 7/9] libcamera: v4l2_subdevice: Add stream support to\n\tget/set functions","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Wed, Feb 28, 2024 at 12:51:19PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Tue, Feb 27, 2024 at 05:48:35PM +0100, Jacopo Mondi wrote:\n> > On Tue, Feb 27, 2024 at 04:09:51PM +0200, Laurent Pinchart wrote:\n> > > Extend the V4L2Subdevice API with stream support for the functions that\n> > > get and set formats and selection rectangles. Add a Stream structure to\n> > > identify a subdev pad and stream, and use it to extend the V4L2Subdevice\n> > > functions that get and set formats and selection rectangles with stream\n> > > support.\n> > >\n> > > To preserve the existing pad-based API, implement overloaded functions\n> > > that wrap the new stream-based API. This allows callers that are not\n> > > stream-aware to use a simpler pad-based API, instead of having to\n> > > explicitly set the stream number to 0 in all API calls.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/internal/v4l2_subdevice.h | 41 +++-\n> > > src/libcamera/v4l2_subdevice.cpp | 232 ++++++++++++++------\n> > > 2 files changed, 202 insertions(+), 71 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> > > index 1115cfa55594..65003394a984 100644\n> > > --- a/include/libcamera/internal/v4l2_subdevice.h\n> > > +++ b/include/libcamera/internal/v4l2_subdevice.h\n> > > @@ -80,6 +80,11 @@ public:\n> > > \t\tActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,\n> > > \t};\n> > >\n> > > +\tstruct Stream {\n> > > +\t\tunsigned int pad;\n> > > +\t\tunsigned int stream;\n> > > +\t};\n> > > +\n> > > \tclass Routing : public std::vector\n> > > \t{\n> > > \tpublic:\n> > > @@ -93,17 +98,39 @@ public:\n> > >\n> > > \tconst MediaEntity *entity() const { return entity_; }\n> > >\n> > > -\tint getSelection(unsigned int pad, unsigned int target,\n> > > +\tint getSelection(const Stream &stream, unsigned int target,\n> > > \t\t\t Rectangle *rect);\n> > > -\tint setSelection(unsigned int pad, unsigned int target,\n> > > +\tint getSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> > > +\t{\n> > > +\t\treturn getSelection({ pad, 0 }, target, rect);\n> > > +\t}\n> > > +\tint setSelection(const Stream &stream, unsigned int target,\n> > > \t\t\t Rectangle *rect);\n> > > +\tint setSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> > > +\t{\n> > > +\t\treturn setSelection({ pad, 0 }, target, rect);\n> > > +\t}\n> > >\n> > > -\tFormats formats(unsigned int pad);\n> > > +\tFormats formats(const Stream &stream);\n> > > +\tFormats formats(unsigned int pad)\n> > > +\t{\n> > > +\t\treturn formats({ pad, 0 });\n> > > +\t}\n> > >\n> > > +\tint getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > > +\t\t Whence whence = ActiveFormat);\n> > > \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > +\t\t Whence whence = ActiveFormat)\n> > > +\t{\n> > > +\t\treturn getFormat({ pad, 0 }, format, whence);\n> > > +\t}\n> > > +\tint setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > > \t\t Whence whence = ActiveFormat);\n> > > \tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > -\t\t Whence whence = ActiveFormat);\n> > > +\t\t Whence whence = ActiveFormat)\n> > > +\t{\n> > > +\t\treturn setFormat({ pad, 0 }, format, whence);\n> > > +\t}\n> > >\n> > > \tint getRouting(Routing *routing, Whence whence = ActiveFormat);\n> > > \tint setRouting(Routing *routing, Whence whence = ActiveFormat);\n> > > @@ -123,8 +150,8 @@ private:\n> > > \tstd::optional\n> > > \ttoColorSpace(const v4l2_mbus_framefmt &format) const;\n> > >\n> > > -\tstd::vector enumPadCodes(unsigned int pad);\n> > > -\tstd::vector enumPadSizes(unsigned int pad,\n> > > +\tstd::vector enumPadCodes(const Stream &stream);\n> > > +\tstd::vector enumPadSizes(const Stream &stream,\n> > > \t\t\t\t\t unsigned int code);\n> > >\n> > > \tconst MediaEntity *entity_;\n> > > @@ -133,4 +160,6 @@ private:\n> > > \tstruct V4L2SubdeviceCapability caps_;\n> > > };\n> > >\n> > > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);\n> > > +\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > index 19ddecf714c6..efe8b0f793e9 100644\n> > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > @@ -814,6 +814,35 @@ std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f)\n> > > * \\brief The format operation applies to TRY formats\n> > > */\n> > >\n> > > +/**\n> > > + * \\class V4L2Subdevice::Stream\n> > > + * \\brief V4L2 subdevice stream\n> > > + *\n> > > + * This class identifies a subdev stream, by bundling the pad number with the\n> > > + * stream number. It is used in all stream-aware functions of the V4L2Subdevice\n> > > + * class to identify the stream the functions operate on.\n> > > + *\n> > > + * \\var V4L2Subdevice::Stream::pad\n> > > + * \\brief The 0-indexed pad number\n> > > + *\n> > > + * \\var V4L2Subdevice::Stream::stream\n> > > + * \\brief The stream number\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n> > > + *\toutput stream\n> >\n> > What is the alignment rule when we break \\brief ? I see different ones\n> > being used in this patch\n>\n> Good question :-) Let's decide. Does anybody have a preference between\n>\n> * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n> *\toutput stream\n>\n> (single tab after the '*', no space)\n>\n> and\n>\n> * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n> * output stream\n>\n> (single space after the '*')\n>\n\nMy vote for the second option, but whatever, really\n\n> ?\n>\n> > > + * \\param[in] out The output stream\n> > > + * \\param[in] stream The V4L2Subdevice::Stream\n> > > + * \\return The output stream \\a out\n> > > + */\n> > > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n> > > +{\n> > > +\tout << stream.pad << \"/\" << stream.stream;\n> > > +\n> > > +\treturn out;\n> > > +}\n> > > +\n> > > /**\n> > > * \\class V4L2Subdevice::Routing\n> > > * \\brief V4L2 subdevice routing table\n> > > @@ -879,7 +908,10 @@ int V4L2Subdevice::open()\n> > > \t\treturn ret;\n> > > \t}\n> > >\n> > > -\t/* If the subdev supports streams, enable the streams API. */\n> > > +\t/*\n> > > +\t * If the subdev supports streams, enable the streams API, and retrieve\n> > > +\t * and cache the routing table.\n> >\n> > Does it happen in this patch ?\n>\n> Indeed not. I was planning to do so and then didn't. I'll drop the\n> comment change.\n>\n> > > +\t */\n> > > \tif (caps_.hasStreams()) {\n> > > \t\tstruct v4l2_subdev_client_capability clientCaps{};\n> > > \t\tclientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;\n> > > @@ -905,7 +937,7 @@ int V4L2Subdevice::open()\n> > >\n> > > /**\n> > > * \\brief Get selection rectangle \\a rect for \\a target\n> > > - * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> > > + * \\param[in] stream The stream the rectangle is retrieved from\n> > > * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > > * \\param[out] rect The retrieved selection rectangle\n> > > *\n> > > @@ -913,13 +945,14 @@ int V4L2Subdevice::open()\n> > > *\n> > > * \\return 0 on success or a negative error code otherwise\n> > > */\n> > > -int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > > +int V4L2Subdevice::getSelection(const Stream &stream, unsigned int target,\n> > > \t\t\t\tRectangle *rect)\n> > > {\n> > > \tstruct v4l2_subdev_selection sel = {};\n> > >\n> > > \tsel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > > -\tsel.pad = pad;\n> > > +\tsel.pad = stream.pad;\n> > > +\tsel.stream = stream.stream;\n> > > \tsel.target = target;\n> > > \tsel.flags = 0;\n> > >\n> > > @@ -927,7 +960,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > > \tif (ret < 0) {\n> > > \t\tLOG(V4L2, Error)\n> > > \t\t\t<< \"Unable to get rectangle \" << target << \" on pad \"\n> > > -\t\t\t<< pad << \": \" << strerror(-ret);\n> > > +\t\t\t<< stream.pad << \"/\" << stream.stream << \": \"\n> >\n> > Can't you use operator<<(Stream) ?\n>\n> I'll fix it, and below too.\n>\n> > > +\t\t\t<< strerror(-ret);\n> > > \t\treturn ret;\n> > > \t}\n> > >\n> > > @@ -939,9 +973,20 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > > \treturn 0;\n> > > }\n> > >\n> > > +/**\n> > > + * \\fn V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > > + *\tRectangle *rect)\n> > > + * \\brief Get selection rectangle \\a rect for \\a target\n> > > + * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> > > + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > > + * \\param[out] rect The retrieved selection rectangle\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +\n> > > /**\n> > > * \\brief Set selection rectangle \\a rect for \\a target\n> > > - * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > > + * \\param[in] stream The stream the rectangle is to be applied to\n> > > * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > > * \\param[inout] rect The selection rectangle to be applied\n> > > *\n> > > @@ -949,13 +994,14 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > > *\n> > > * \\return 0 on success or a negative error code otherwise\n> > > */\n> > > -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > +int V4L2Subdevice::setSelection(const Stream &stream, unsigned int target,\n> > > \t\t\t\tRectangle *rect)\n> > > {\n> > > \tstruct v4l2_subdev_selection sel = {};\n> > >\n> > > \tsel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > > -\tsel.pad = pad;\n> > > +\tsel.pad = stream.pad;\n> > > +\tsel.stream = stream.stream;\n> > > \tsel.target = target;\n> > > \tsel.flags = 0;\n> > >\n> > > @@ -968,7 +1014,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > \tif (ret < 0) {\n> > > \t\tLOG(V4L2, Error)\n> > > \t\t\t<< \"Unable to set rectangle \" << target << \" on pad \"\n> > > -\t\t\t<< pad << \": \" << strerror(-ret);\n> > > +\t\t\t<< stream.pad << \"/\" << stream.stream << \": \"\n> >\n> > ditto\n> >\n> > > +\t\t\t<< strerror(-ret);\n> > > \t\treturn ret;\n> > > \t}\n> > >\n> > > @@ -979,26 +1026,40 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > >\n> > > \treturn 0;\n> > > }\n> > > +\n> > > /**\n> > > - * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> > > - * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> > > + * \\fn V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > + *\tRectangle *rect)\n> > > + * \\brief Set selection rectangle \\a rect for \\a target\n> > > + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > > + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > > + * \\param[inout] rect The selection rectangle to be applied\n> > > + *\n> > > + * \\todo Define a V4L2SelectionTarget enum for the selection target\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Enumerate all media bus codes and frame sizes on a \\a stream\n> > > + * \\param[in] stream The stream to enumerate formats for\n> > > *\n> > > * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> > > - * a \\a pad.\n> > > + * a \\a stream.\n> > > *\n> > > * \\return A list of the supported device formats\n> > > */\n> > > -V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> > > +V4L2Subdevice::Formats V4L2Subdevice::formats(const Stream &stream)\n> > > {\n> > > \tFormats formats;\n> > >\n> > > -\tif (pad >= entity_->pads().size()) {\n> > > -\t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> > > +\tif (stream.pad >= entity_->pads().size()) {\n> > > +\t\tLOG(V4L2, Error) << \"Invalid pad: \" << stream.pad;\n> > > \t\treturn {};\n> > > \t}\n> > >\n> > > -\tfor (unsigned int code : enumPadCodes(pad)) {\n> > > -\t\tstd::vector sizes = enumPadSizes(pad, code);\n> > > +\tfor (unsigned int code : enumPadCodes(stream)) {\n> > > +\t\tstd::vector sizes = enumPadSizes(stream, code);\n> > > \t\tif (sizes.empty())\n> > > \t\t\treturn {};\n> > >\n> > > @@ -1006,7 +1067,7 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> > > \t\tif (!inserted.second) {\n> > > \t\t\tLOG(V4L2, Error)\n> > > \t\t\t\t<< \"Could not add sizes for media bus code \"\n> > > -\t\t\t\t<< code << \" on pad \" << pad;\n> > > +\t\t\t\t<< code << \" on pad \" << stream.pad;\n> > > \t\t\treturn {};\n> > > \t\t}\n> > > \t}\n> > > @@ -1014,6 +1075,17 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> > > \treturn formats;\n> > > }\n> > >\n> > > +/**\n> > > + * \\fn V4L2Subdevice::formats(unsigned int pad)\n> > > + * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> > > + * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> > > + *\n> > > + * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> > > + * a \\a pad\n> > > + *\n> > > + * \\return A list of the supported device formats\n> > > + */\n> > > +\n> > > std::optional V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const\n> > > {\n> > > \t/*\n> > > @@ -1045,25 +1117,26 @@ std::optional V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &\n> > > }\n> > >\n> > > /**\n> > > - * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> > > - * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> > > + * \\brief Retrieve the image format set on one of the V4L2 subdevice streams\n> > > + * \\param[in] stream The stream the format is to be retrieved from\n> > > * \\param[out] format The image bus format\n> > > * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n> > > * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> > > * \\return 0 on success or a negative error code otherwise\n> > > */\n> > > -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > +int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > > \t\t\t Whence whence)\n> > > {\n> > > \tstruct v4l2_subdev_format subdevFmt = {};\n> > > \tsubdevFmt.which = whence;\n> > > -\tsubdevFmt.pad = pad;\n> > > +\tsubdevFmt.pad = stream.pad;\n> > > +\tsubdevFmt.stream = stream.stream;\n> > >\n> > > \tint ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n> > > \tif (ret) {\n> > > \t\tLOG(V4L2, Error)\n> > > -\t\t\t<< \"Unable to get format on pad \" << pad\n> > > -\t\t\t<< \": \" << strerror(-ret);\n> > > +\t\t\t<< \"Unable to get format on pad \" << stream.pad << \"/\"\n> > > +\t\t\t<< stream.stream << \": \" << strerror(-ret);\n> > > \t\treturn ret;\n> > > \t}\n> > >\n> > > @@ -1076,6 +1149,66 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > }\n> > >\n> > > /**\n> > > + * \\fn V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > + *\tWhence whence)\n> > > + * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> > > + * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> > > + * \\param[out] format The image bus format\n> > > + * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n> > > + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Set an image format on one of the V4L2 subdevice pads\n> > > + * \\param[in] stream The stream the format is to be applied to\n> > > + * \\param[inout] format The image bus format to apply to the stream\n> > > + * \\param[in] whence The format to set, \\ref V4L2Subdevice::ActiveFormat\n> > > + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> > > + *\n> > > + * Apply the requested image format to the desired stream and return the\n> > > + * actually applied format parameters, as getFormat() would do.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > > +\t\t\t Whence whence)\n> > > +{\n> > > +\tstruct v4l2_subdev_format subdevFmt = {};\n> > > +\tsubdevFmt.which = whence;\n> > > +\tsubdevFmt.pad = stream.pad;\n> > > +\tsubdevFmt.stream = stream.stream;\n> > > +\tsubdevFmt.format.width = format->size.width;\n> > > +\tsubdevFmt.format.height = format->size.height;\n> > > +\tsubdevFmt.format.code = format->code;\n> > > +\tsubdevFmt.format.field = V4L2_FIELD_NONE;\n> > > +\tif (format->colorSpace) {\n> > > +\t\tfromColorSpace(format->colorSpace, subdevFmt.format);\n> > > +\n> > > +\t\t/* The CSC flag is only applicable to source pads. */\n> > > +\t\tif (entity_->pads()[stream.pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> > > +\t\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> > > +\t}\n> > > +\n> > > +\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> > > +\tif (ret) {\n> > > +\t\tLOG(V4L2, Error)\n> > > +\t\t\t<< \"Unable to set format on pad \" << stream.pad << \"/\"\n> > > +\t\t\t<< stream.stream << \": \" << strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tformat->size.width = subdevFmt.format.width;\n> > > +\tformat->size.height = subdevFmt.format.height;\n> > > +\tformat->code = subdevFmt.format.code;\n> > > +\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > + *\tWhence whence)\n> > > * \\brief Set an image format on one of the V4L2 subdevice pads\n> > > * \\param[in] pad The 0-indexed pad number the format is to be applied to\n> > > * \\param[inout] format The image bus format to apply to the subdevice's pad\n> > > @@ -1087,39 +1220,6 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > *\n> > > * \\return 0 on success or a negative error code otherwise\n> > > */\n> > > -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > -\t\t\t Whence whence)\n> > > -{\n> > > -\tstruct v4l2_subdev_format subdevFmt = {};\n> > > -\tsubdevFmt.which = whence;\n> > > -\tsubdevFmt.pad = pad;\n> > > -\tsubdevFmt.format.width = format->size.width;\n> > > -\tsubdevFmt.format.height = format->size.height;\n> > > -\tsubdevFmt.format.code = format->code;\n> > > -\tsubdevFmt.format.field = V4L2_FIELD_NONE;\n> > > -\tif (format->colorSpace) {\n> > > -\t\tfromColorSpace(format->colorSpace, subdevFmt.format);\n> > > -\n> > > -\t\t/* The CSC flag is only applicable to source pads. */\n> > > -\t\tif (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> > > -\t\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> > > -\t}\n> > > -\n> > > -\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> > > -\tif (ret) {\n> > > -\t\tLOG(V4L2, Error)\n> > > -\t\t\t<< \"Unable to set format on pad \" << pad\n> > > -\t\t\t<< \": \" << strerror(-ret);\n> > > -\t\treturn ret;\n> > > -\t}\n> > > -\n> > > -\tformat->size.width = subdevFmt.format.width;\n> > > -\tformat->size.height = subdevFmt.format.height;\n> > > -\tformat->code = subdevFmt.format.code;\n> > > -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> > > -\n> > > -\treturn 0;\n> > > -}\n> > >\n> > > /**\n> > > * \\brief Retrieve the subdevice's internal routing table\n> > > @@ -1282,14 +1382,15 @@ std::string V4L2Subdevice::logPrefix() const\n> > > \treturn \"'\" + entity_->name() + \"'\";\n> > > }\n> > >\n> > > -std::vector V4L2Subdevice::enumPadCodes(unsigned int pad)\n> > > +std::vector V4L2Subdevice::enumPadCodes(const Stream &stream)\n> > > {\n> > > \tstd::vector codes;\n> > > \tint ret;\n> > >\n> > > \tfor (unsigned int index = 0; ; index++) {\n> > > \t\tstruct v4l2_subdev_mbus_code_enum mbusEnum = {};\n> > > -\t\tmbusEnum.pad = pad;\n> > > +\t\tmbusEnum.pad = stream.pad;\n> > > +\t\tmbusEnum.stream = stream.stream;\n> > > \t\tmbusEnum.index = index;\n> > > \t\tmbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > >\n> > > @@ -1302,15 +1403,15 @@ std::vector V4L2Subdevice::enumPadCodes(unsigned int pad)\n> > >\n> > > \tif (ret < 0 && ret != -EINVAL) {\n> > > \t\tLOG(V4L2, Error)\n> > > -\t\t\t<< \"Unable to enumerate formats on pad \" << pad\n> > > -\t\t\t<< \": \" << strerror(-ret);\n> > > +\t\t\t<< \"Unable to enumerate formats on pad \" << stream.pad\n> > > +\t\t\t<< \"/\" << stream.stream << \": \" << strerror(-ret);\n> > > \t\treturn {};\n> > > \t}\n> > >\n> > > \treturn codes;\n> > > }\n> > >\n> > > -std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > > +std::vector V4L2Subdevice::enumPadSizes(const Stream &stream,\n> > > \t\t\t\t\t\t unsigned int code)\n> > > {\n> > > \tstd::vector sizes;\n> > > @@ -1319,7 +1420,8 @@ std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > > \tfor (unsigned int index = 0;; index++) {\n> > > \t\tstruct v4l2_subdev_frame_size_enum sizeEnum = {};\n> > > \t\tsizeEnum.index = index;\n> > > -\t\tsizeEnum.pad = pad;\n> > > +\t\tsizeEnum.pad = stream.pad;\n> > > +\t\tsizeEnum.stream = stream.stream;\n> > > \t\tsizeEnum.code = code;\n> > > \t\tsizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > >\n> > > @@ -1333,8 +1435,8 @@ std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > >\n> > > \tif (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {\n> > > \t\tLOG(V4L2, Error)\n> > > -\t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n> > > -\t\t\t<< \": \" << strerror(-ret);\n> > > +\t\t\t<< \"Unable to enumerate sizes on pad \" << stream.pad\n> > > +\t\t\t<< \"/\" << stream.stream << \": \" << strerror(-ret);\n> > > \t\treturn {};\n> >\n> > The only question I have is if we should allow the V4L2Subdevice API\n> > to expose a pad-only interface or we should bite the bullet and move\n> > it to use Stream. The reason is that, a open() time\n> >\n> > \t/* If the subdev supports streams, enable the streams API. */\n> > \tif (caps_.hasStreams()) {\n> > \t\tstruct v4l2_subdev_client_capability clientCaps{};\n> > \t\tclientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;\n> >\n> > \t\tret = ioctl(VIDIOC_SUBDEV_S_CLIENT_CAP, &clientCaps);\n> >\n> > And allowing users to use the pad-based API would make stream == 0\n> > implicitly. It's a lot of work though, not sure it's worth it ?\n>\n> Most devices don't use streams, that's why I decided to overload the\n> get/set functions and offer users a pad-based interface and a\n> stream-based interface. It's low-maintenance on the V4L2Subdevice side\n> (at least for now), and allows callers that deal with stream-unaware\n> devices only not to have to specify the stream number. I think the code\n\nThat's my concern, not having to specify the stream number would\ndefault it to 0 on devices which are stream aware, as we enable the\nstream API at open() time unconditionally, and if the kernel drivers\ngets updated to use streams and the libcamera support doesn't this\nmight cause unexpected issues. However, even if we make Stream usage\nmandatory, the existing implementations would have to use 0 anyway, so\nthis probably doesn't make any practical difference ?\n\n\n> gets more readable that way.\n>\n> > > \t}\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3F9CFBD80A\n\tfor ;\n\tWed, 28 Feb 2024 11:18:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 712966286D;\n\tWed, 28 Feb 2024 12:18:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2DEC8627F9\n\tfor ;\n\tWed, 28 Feb 2024 12:18:51 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 510F42B3;\n\tWed, 28 Feb 2024 12:18:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ARM40xy3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709119118;\n\tbh=GXeyBycM3Slcx3TLvzc/aG7OkjJpt3/q94z4p7319Ho=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ARM40xy3A2SfCdKI1GlATK3TgM5D/Flk2fwzFIJHsUkx8gTbObEEgb5EPg/rUXQ86\n\tEPkEv/vTE7aZ2Uk2CTTeKA7a2yx315s3OOTKTaCbqCISD8ZJzHscDaCxCJ6nz1+ho0\n\tcpgLa3NKDsIuNP0AjmTNEsETK8+Eulf2WUlfU8xw=","Date":"Wed, 28 Feb 2024 12:18:47 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Subject":"Re: [PATCH 7/9] libcamera: v4l2_subdevice: Add stream support to\n\tget/set functions","Message-ID":"<4mswhhito7dgbwk27oanrtlwoph7qzpuwiao6o7gfelx57kjc4@3k33law6dubx>","References":"<20240227140953.26093-1-laurent.pinchart@ideasonboard.com>\n\t<20240227140953.26093-8-laurent.pinchart@ideasonboard.com>\n\t\n\t<20240228105119.GH3419@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240228105119.GH3419@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28792,"web_url":"https://patchwork.libcamera.org/comment/28792/","msgid":"<20240228112423.GL3419@pendragon.ideasonboard.com>","date":"2024-02-28T11:24:23","subject":"Re: [PATCH 7/9] libcamera: v4l2_subdevice: Add stream support to\n\tget/set functions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Feb 28, 2024 at 12:18:47PM +0100, Jacopo Mondi wrote:\n> On Wed, Feb 28, 2024 at 12:51:19PM +0200, Laurent Pinchart wrote:\n> > On Tue, Feb 27, 2024 at 05:48:35PM +0100, Jacopo Mondi wrote:\n> > > On Tue, Feb 27, 2024 at 04:09:51PM +0200, Laurent Pinchart wrote:\n> > > > Extend the V4L2Subdevice API with stream support for the functions that\n> > > > get and set formats and selection rectangles. Add a Stream structure to\n> > > > identify a subdev pad and stream, and use it to extend the V4L2Subdevice\n> > > > functions that get and set formats and selection rectangles with stream\n> > > > support.\n> > > >\n> > > > To preserve the existing pad-based API, implement overloaded functions\n> > > > that wrap the new stream-based API. This allows callers that are not\n> > > > stream-aware to use a simpler pad-based API, instead of having to\n> > > > explicitly set the stream number to 0 in all API calls.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > include/libcamera/internal/v4l2_subdevice.h | 41 +++-\n> > > > src/libcamera/v4l2_subdevice.cpp | 232 ++++++++++++++------\n> > > > 2 files changed, 202 insertions(+), 71 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> > > > index 1115cfa55594..65003394a984 100644\n> > > > --- a/include/libcamera/internal/v4l2_subdevice.h\n> > > > +++ b/include/libcamera/internal/v4l2_subdevice.h\n> > > > @@ -80,6 +80,11 @@ public:\n> > > > \t\tActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,\n> > > > \t};\n> > > >\n> > > > +\tstruct Stream {\n> > > > +\t\tunsigned int pad;\n> > > > +\t\tunsigned int stream;\n> > > > +\t};\n> > > > +\n> > > > \tclass Routing : public std::vector\n> > > > \t{\n> > > > \tpublic:\n> > > > @@ -93,17 +98,39 @@ public:\n> > > >\n> > > > \tconst MediaEntity *entity() const { return entity_; }\n> > > >\n> > > > -\tint getSelection(unsigned int pad, unsigned int target,\n> > > > +\tint getSelection(const Stream &stream, unsigned int target,\n> > > > \t\t\t Rectangle *rect);\n> > > > -\tint setSelection(unsigned int pad, unsigned int target,\n> > > > +\tint getSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> > > > +\t{\n> > > > +\t\treturn getSelection({ pad, 0 }, target, rect);\n> > > > +\t}\n> > > > +\tint setSelection(const Stream &stream, unsigned int target,\n> > > > \t\t\t Rectangle *rect);\n> > > > +\tint setSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> > > > +\t{\n> > > > +\t\treturn setSelection({ pad, 0 }, target, rect);\n> > > > +\t}\n> > > >\n> > > > -\tFormats formats(unsigned int pad);\n> > > > +\tFormats formats(const Stream &stream);\n> > > > +\tFormats formats(unsigned int pad)\n> > > > +\t{\n> > > > +\t\treturn formats({ pad, 0 });\n> > > > +\t}\n> > > >\n> > > > +\tint getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > > > +\t\t Whence whence = ActiveFormat);\n> > > > \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > > +\t\t Whence whence = ActiveFormat)\n> > > > +\t{\n> > > > +\t\treturn getFormat({ pad, 0 }, format, whence);\n> > > > +\t}\n> > > > +\tint setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > > > \t\t Whence whence = ActiveFormat);\n> > > > \tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > > -\t\t Whence whence = ActiveFormat);\n> > > > +\t\t Whence whence = ActiveFormat)\n> > > > +\t{\n> > > > +\t\treturn setFormat({ pad, 0 }, format, whence);\n> > > > +\t}\n> > > >\n> > > > \tint getRouting(Routing *routing, Whence whence = ActiveFormat);\n> > > > \tint setRouting(Routing *routing, Whence whence = ActiveFormat);\n> > > > @@ -123,8 +150,8 @@ private:\n> > > > \tstd::optional\n> > > > \ttoColorSpace(const v4l2_mbus_framefmt &format) const;\n> > > >\n> > > > -\tstd::vector enumPadCodes(unsigned int pad);\n> > > > -\tstd::vector enumPadSizes(unsigned int pad,\n> > > > +\tstd::vector enumPadCodes(const Stream &stream);\n> > > > +\tstd::vector enumPadSizes(const Stream &stream,\n> > > > \t\t\t\t\t unsigned int code);\n> > > >\n> > > > \tconst MediaEntity *entity_;\n> > > > @@ -133,4 +160,6 @@ private:\n> > > > \tstruct V4L2SubdeviceCapability caps_;\n> > > > };\n> > > >\n> > > > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);\n> > > > +\n> > > > } /* namespace libcamera */\n> > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > > index 19ddecf714c6..efe8b0f793e9 100644\n> > > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > > @@ -814,6 +814,35 @@ std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f)\n> > > > * \\brief The format operation applies to TRY formats\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\class V4L2Subdevice::Stream\n> > > > + * \\brief V4L2 subdevice stream\n> > > > + *\n> > > > + * This class identifies a subdev stream, by bundling the pad number with the\n> > > > + * stream number. It is used in all stream-aware functions of the V4L2Subdevice\n> > > > + * class to identify the stream the functions operate on.\n> > > > + *\n> > > > + * \\var V4L2Subdevice::Stream::pad\n> > > > + * \\brief The 0-indexed pad number\n> > > > + *\n> > > > + * \\var V4L2Subdevice::Stream::stream\n> > > > + * \\brief The stream number\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n> > > > + *\toutput stream\n> > >\n> > > What is the alignment rule when we break \\brief ? I see different ones\n> > > being used in this patch\n> >\n> > Good question :-) Let's decide. Does anybody have a preference between\n> >\n> > * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n> > *\toutput stream\n> >\n> > (single tab after the '*', no space)\n> >\n> > and\n> >\n> > * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n> > * output stream\n> >\n> > (single space after the '*')\n> >\n> \n> My vote for the second option, but whatever, really\n> \n> > ?\n> >\n> > > > + * \\param[in] out The output stream\n> > > > + * \\param[in] stream The V4L2Subdevice::Stream\n> > > > + * \\return The output stream \\a out\n> > > > + */\n> > > > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n> > > > +{\n> > > > +\tout << stream.pad << \"/\" << stream.stream;\n> > > > +\n> > > > +\treturn out;\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\class V4L2Subdevice::Routing\n> > > > * \\brief V4L2 subdevice routing table\n> > > > @@ -879,7 +908,10 @@ int V4L2Subdevice::open()\n> > > > \t\treturn ret;\n> > > > \t}\n> > > >\n> > > > -\t/* If the subdev supports streams, enable the streams API. */\n> > > > +\t/*\n> > > > +\t * If the subdev supports streams, enable the streams API, and retrieve\n> > > > +\t * and cache the routing table.\n> > >\n> > > Does it happen in this patch ?\n> >\n> > Indeed not. I was planning to do so and then didn't. I'll drop the\n> > comment change.\n> >\n> > > > +\t */\n> > > > \tif (caps_.hasStreams()) {\n> > > > \t\tstruct v4l2_subdev_client_capability clientCaps{};\n> > > > \t\tclientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;\n> > > > @@ -905,7 +937,7 @@ int V4L2Subdevice::open()\n> > > >\n> > > > /**\n> > > > * \\brief Get selection rectangle \\a rect for \\a target\n> > > > - * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> > > > + * \\param[in] stream The stream the rectangle is retrieved from\n> > > > * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > > > * \\param[out] rect The retrieved selection rectangle\n> > > > *\n> > > > @@ -913,13 +945,14 @@ int V4L2Subdevice::open()\n> > > > *\n> > > > * \\return 0 on success or a negative error code otherwise\n> > > > */\n> > > > -int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > > > +int V4L2Subdevice::getSelection(const Stream &stream, unsigned int target,\n> > > > \t\t\t\tRectangle *rect)\n> > > > {\n> > > > \tstruct v4l2_subdev_selection sel = {};\n> > > >\n> > > > \tsel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > > > -\tsel.pad = pad;\n> > > > +\tsel.pad = stream.pad;\n> > > > +\tsel.stream = stream.stream;\n> > > > \tsel.target = target;\n> > > > \tsel.flags = 0;\n> > > >\n> > > > @@ -927,7 +960,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > > > \tif (ret < 0) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > \t\t\t<< \"Unable to get rectangle \" << target << \" on pad \"\n> > > > -\t\t\t<< pad << \": \" << strerror(-ret);\n> > > > +\t\t\t<< stream.pad << \"/\" << stream.stream << \": \"\n> > >\n> > > Can't you use operator<<(Stream) ?\n> >\n> > I'll fix it, and below too.\n> >\n> > > > +\t\t\t<< strerror(-ret);\n> > > > \t\treturn ret;\n> > > > \t}\n> > > >\n> > > > @@ -939,9 +973,20 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > > > \treturn 0;\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\fn V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > > > + *\tRectangle *rect)\n> > > > + * \\brief Get selection rectangle \\a rect for \\a target\n> > > > + * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> > > > + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > > > + * \\param[out] rect The retrieved selection rectangle\n> > > > + *\n> > > > + * \\return 0 on success or a negative error code otherwise\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\brief Set selection rectangle \\a rect for \\a target\n> > > > - * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > > > + * \\param[in] stream The stream the rectangle is to be applied to\n> > > > * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > > > * \\param[inout] rect The selection rectangle to be applied\n> > > > *\n> > > > @@ -949,13 +994,14 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > > > *\n> > > > * \\return 0 on success or a negative error code otherwise\n> > > > */\n> > > > -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > > +int V4L2Subdevice::setSelection(const Stream &stream, unsigned int target,\n> > > > \t\t\t\tRectangle *rect)\n> > > > {\n> > > > \tstruct v4l2_subdev_selection sel = {};\n> > > >\n> > > > \tsel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > > > -\tsel.pad = pad;\n> > > > +\tsel.pad = stream.pad;\n> > > > +\tsel.stream = stream.stream;\n> > > > \tsel.target = target;\n> > > > \tsel.flags = 0;\n> > > >\n> > > > @@ -968,7 +1014,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > > \tif (ret < 0) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > \t\t\t<< \"Unable to set rectangle \" << target << \" on pad \"\n> > > > -\t\t\t<< pad << \": \" << strerror(-ret);\n> > > > +\t\t\t<< stream.pad << \"/\" << stream.stream << \": \"\n> > >\n> > > ditto\n> > >\n> > > > +\t\t\t<< strerror(-ret);\n> > > > \t\treturn ret;\n> > > > \t}\n> > > >\n> > > > @@ -979,26 +1026,40 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > >\n> > > > \treturn 0;\n> > > > }\n> > > > +\n> > > > /**\n> > > > - * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> > > > - * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> > > > + * \\fn V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > > + *\tRectangle *rect)\n> > > > + * \\brief Set selection rectangle \\a rect for \\a target\n> > > > + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > > > + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > > > + * \\param[inout] rect The selection rectangle to be applied\n> > > > + *\n> > > > + * \\todo Define a V4L2SelectionTarget enum for the selection target\n> > > > + *\n> > > > + * \\return 0 on success or a negative error code otherwise\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Enumerate all media bus codes and frame sizes on a \\a stream\n> > > > + * \\param[in] stream The stream to enumerate formats for\n> > > > *\n> > > > * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> > > > - * a \\a pad.\n> > > > + * a \\a stream.\n> > > > *\n> > > > * \\return A list of the supported device formats\n> > > > */\n> > > > -V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> > > > +V4L2Subdevice::Formats V4L2Subdevice::formats(const Stream &stream)\n> > > > {\n> > > > \tFormats formats;\n> > > >\n> > > > -\tif (pad >= entity_->pads().size()) {\n> > > > -\t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> > > > +\tif (stream.pad >= entity_->pads().size()) {\n> > > > +\t\tLOG(V4L2, Error) << \"Invalid pad: \" << stream.pad;\n> > > > \t\treturn {};\n> > > > \t}\n> > > >\n> > > > -\tfor (unsigned int code : enumPadCodes(pad)) {\n> > > > -\t\tstd::vector sizes = enumPadSizes(pad, code);\n> > > > +\tfor (unsigned int code : enumPadCodes(stream)) {\n> > > > +\t\tstd::vector sizes = enumPadSizes(stream, code);\n> > > > \t\tif (sizes.empty())\n> > > > \t\t\treturn {};\n> > > >\n> > > > @@ -1006,7 +1067,7 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> > > > \t\tif (!inserted.second) {\n> > > > \t\t\tLOG(V4L2, Error)\n> > > > \t\t\t\t<< \"Could not add sizes for media bus code \"\n> > > > -\t\t\t\t<< code << \" on pad \" << pad;\n> > > > +\t\t\t\t<< code << \" on pad \" << stream.pad;\n> > > > \t\t\treturn {};\n> > > > \t\t}\n> > > > \t}\n> > > > @@ -1014,6 +1075,17 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> > > > \treturn formats;\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\fn V4L2Subdevice::formats(unsigned int pad)\n> > > > + * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> > > > + * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> > > > + *\n> > > > + * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> > > > + * a \\a pad\n> > > > + *\n> > > > + * \\return A list of the supported device formats\n> > > > + */\n> > > > +\n> > > > std::optional V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const\n> > > > {\n> > > > \t/*\n> > > > @@ -1045,25 +1117,26 @@ std::optional V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &\n> > > > }\n> > > >\n> > > > /**\n> > > > - * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> > > > - * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> > > > + * \\brief Retrieve the image format set on one of the V4L2 subdevice streams\n> > > > + * \\param[in] stream The stream the format is to be retrieved from\n> > > > * \\param[out] format The image bus format\n> > > > * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n> > > > * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> > > > * \\return 0 on success or a negative error code otherwise\n> > > > */\n> > > > -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > > +int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > > > \t\t\t Whence whence)\n> > > > {\n> > > > \tstruct v4l2_subdev_format subdevFmt = {};\n> > > > \tsubdevFmt.which = whence;\n> > > > -\tsubdevFmt.pad = pad;\n> > > > +\tsubdevFmt.pad = stream.pad;\n> > > > +\tsubdevFmt.stream = stream.stream;\n> > > >\n> > > > \tint ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n> > > > \tif (ret) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > -\t\t\t<< \"Unable to get format on pad \" << pad\n> > > > -\t\t\t<< \": \" << strerror(-ret);\n> > > > +\t\t\t<< \"Unable to get format on pad \" << stream.pad << \"/\"\n> > > > +\t\t\t<< stream.stream << \": \" << strerror(-ret);\n> > > > \t\treturn ret;\n> > > > \t}\n> > > >\n> > > > @@ -1076,6 +1149,66 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > > }\n> > > >\n> > > > /**\n> > > > + * \\fn V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > > + *\tWhence whence)\n> > > > + * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> > > > + * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> > > > + * \\param[out] format The image bus format\n> > > > + * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n> > > > + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> > > > + * \\return 0 on success or a negative error code otherwise\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Set an image format on one of the V4L2 subdevice pads\n> > > > + * \\param[in] stream The stream the format is to be applied to\n> > > > + * \\param[inout] format The image bus format to apply to the stream\n> > > > + * \\param[in] whence The format to set, \\ref V4L2Subdevice::ActiveFormat\n> > > > + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> > > > + *\n> > > > + * Apply the requested image format to the desired stream and return the\n> > > > + * actually applied format parameters, as getFormat() would do.\n> > > > + *\n> > > > + * \\return 0 on success or a negative error code otherwise\n> > > > + */\n> > > > +int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > > > +\t\t\t Whence whence)\n> > > > +{\n> > > > +\tstruct v4l2_subdev_format subdevFmt = {};\n> > > > +\tsubdevFmt.which = whence;\n> > > > +\tsubdevFmt.pad = stream.pad;\n> > > > +\tsubdevFmt.stream = stream.stream;\n> > > > +\tsubdevFmt.format.width = format->size.width;\n> > > > +\tsubdevFmt.format.height = format->size.height;\n> > > > +\tsubdevFmt.format.code = format->code;\n> > > > +\tsubdevFmt.format.field = V4L2_FIELD_NONE;\n> > > > +\tif (format->colorSpace) {\n> > > > +\t\tfromColorSpace(format->colorSpace, subdevFmt.format);\n> > > > +\n> > > > +\t\t/* The CSC flag is only applicable to source pads. */\n> > > > +\t\tif (entity_->pads()[stream.pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> > > > +\t\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> > > > +\t}\n> > > > +\n> > > > +\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> > > > +\tif (ret) {\n> > > > +\t\tLOG(V4L2, Error)\n> > > > +\t\t\t<< \"Unable to set format on pad \" << stream.pad << \"/\"\n> > > > +\t\t\t<< stream.stream << \": \" << strerror(-ret);\n> > > > +\t\treturn ret;\n> > > > +\t}\n> > > > +\n> > > > +\tformat->size.width = subdevFmt.format.width;\n> > > > +\tformat->size.height = subdevFmt.format.height;\n> > > > +\tformat->code = subdevFmt.format.code;\n> > > > +\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > > + *\tWhence whence)\n> > > > * \\brief Set an image format on one of the V4L2 subdevice pads\n> > > > * \\param[in] pad The 0-indexed pad number the format is to be applied to\n> > > > * \\param[inout] format The image bus format to apply to the subdevice's pad\n> > > > @@ -1087,39 +1220,6 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > > *\n> > > > * \\return 0 on success or a negative error code otherwise\n> > > > */\n> > > > -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > > > -\t\t\t Whence whence)\n> > > > -{\n> > > > -\tstruct v4l2_subdev_format subdevFmt = {};\n> > > > -\tsubdevFmt.which = whence;\n> > > > -\tsubdevFmt.pad = pad;\n> > > > -\tsubdevFmt.format.width = format->size.width;\n> > > > -\tsubdevFmt.format.height = format->size.height;\n> > > > -\tsubdevFmt.format.code = format->code;\n> > > > -\tsubdevFmt.format.field = V4L2_FIELD_NONE;\n> > > > -\tif (format->colorSpace) {\n> > > > -\t\tfromColorSpace(format->colorSpace, subdevFmt.format);\n> > > > -\n> > > > -\t\t/* The CSC flag is only applicable to source pads. */\n> > > > -\t\tif (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> > > > -\t\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> > > > -\t}\n> > > > -\n> > > > -\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> > > > -\tif (ret) {\n> > > > -\t\tLOG(V4L2, Error)\n> > > > -\t\t\t<< \"Unable to set format on pad \" << pad\n> > > > -\t\t\t<< \": \" << strerror(-ret);\n> > > > -\t\treturn ret;\n> > > > -\t}\n> > > > -\n> > > > -\tformat->size.width = subdevFmt.format.width;\n> > > > -\tformat->size.height = subdevFmt.format.height;\n> > > > -\tformat->code = subdevFmt.format.code;\n> > > > -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> > > > -\n> > > > -\treturn 0;\n> > > > -}\n> > > >\n> > > > /**\n> > > > * \\brief Retrieve the subdevice's internal routing table\n> > > > @@ -1282,14 +1382,15 @@ std::string V4L2Subdevice::logPrefix() const\n> > > > \treturn \"'\" + entity_->name() + \"'\";\n> > > > }\n> > > >\n> > > > -std::vector V4L2Subdevice::enumPadCodes(unsigned int pad)\n> > > > +std::vector V4L2Subdevice::enumPadCodes(const Stream &stream)\n> > > > {\n> > > > \tstd::vector codes;\n> > > > \tint ret;\n> > > >\n> > > > \tfor (unsigned int index = 0; ; index++) {\n> > > > \t\tstruct v4l2_subdev_mbus_code_enum mbusEnum = {};\n> > > > -\t\tmbusEnum.pad = pad;\n> > > > +\t\tmbusEnum.pad = stream.pad;\n> > > > +\t\tmbusEnum.stream = stream.stream;\n> > > > \t\tmbusEnum.index = index;\n> > > > \t\tmbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > > >\n> > > > @@ -1302,15 +1403,15 @@ std::vector V4L2Subdevice::enumPadCodes(unsigned int pad)\n> > > >\n> > > > \tif (ret < 0 && ret != -EINVAL) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > -\t\t\t<< \"Unable to enumerate formats on pad \" << pad\n> > > > -\t\t\t<< \": \" << strerror(-ret);\n> > > > +\t\t\t<< \"Unable to enumerate formats on pad \" << stream.pad\n> > > > +\t\t\t<< \"/\" << stream.stream << \": \" << strerror(-ret);\n> > > > \t\treturn {};\n> > > > \t}\n> > > >\n> > > > \treturn codes;\n> > > > }\n> > > >\n> > > > -std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > > > +std::vector V4L2Subdevice::enumPadSizes(const Stream &stream,\n> > > > \t\t\t\t\t\t unsigned int code)\n> > > > {\n> > > > \tstd::vector sizes;\n> > > > @@ -1319,7 +1420,8 @@ std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > > > \tfor (unsigned int index = 0;; index++) {\n> > > > \t\tstruct v4l2_subdev_frame_size_enum sizeEnum = {};\n> > > > \t\tsizeEnum.index = index;\n> > > > -\t\tsizeEnum.pad = pad;\n> > > > +\t\tsizeEnum.pad = stream.pad;\n> > > > +\t\tsizeEnum.stream = stream.stream;\n> > > > \t\tsizeEnum.code = code;\n> > > > \t\tsizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > > >\n> > > > @@ -1333,8 +1435,8 @@ std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > > >\n> > > > \tif (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > -\t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n> > > > -\t\t\t<< \": \" << strerror(-ret);\n> > > > +\t\t\t<< \"Unable to enumerate sizes on pad \" << stream.pad\n> > > > +\t\t\t<< \"/\" << stream.stream << \": \" << strerror(-ret);\n> > > > \t\treturn {};\n> > >\n> > > The only question I have is if we should allow the V4L2Subdevice API\n> > > to expose a pad-only interface or we should bite the bullet and move\n> > > it to use Stream. The reason is that, a open() time\n> > >\n> > > \t/* If the subdev supports streams, enable the streams API. */\n> > > \tif (caps_.hasStreams()) {\n> > > \t\tstruct v4l2_subdev_client_capability clientCaps{};\n> > > \t\tclientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;\n> > >\n> > > \t\tret = ioctl(VIDIOC_SUBDEV_S_CLIENT_CAP, &clientCaps);\n> > >\n> > > And allowing users to use the pad-based API would make stream == 0\n> > > implicitly. It's a lot of work though, not sure it's worth it ?\n> >\n> > Most devices don't use streams, that's why I decided to overload the\n> > get/set functions and offer users a pad-based interface and a\n> > stream-based interface. It's low-maintenance on the V4L2Subdevice side\n> > (at least for now), and allows callers that deal with stream-unaware\n> > devices only not to have to specify the stream number. I think the code\n> \n> That's my concern, not having to specify the stream number would\n> default it to 0 on devices which are stream aware, as we enable the\n> stream API at open() time unconditionally, and if the kernel drivers\n> gets updated to use streams and the libcamera support doesn't this\n> might cause unexpected issues. However, even if we make Stream usage\n> mandatory, the existing implementations would have to use 0 anyway, so\n> this probably doesn't make any practical difference ?\n\nDrivers are supposed to preserve backward-compatibility. For instance, a\nsensor driver that grows support for embedded data streams should use\nstream 0 as the image stream. I don't think it would thus make any\npractical difference.\n\n> > gets more readable that way.\n> >\n> > > > \t}\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 268BCBD160\n\tfor ;\n\tWed, 28 Feb 2024 11:24:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 769166286D;\n\tWed, 28 Feb 2024 12:24:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 50E6B61C94\n\tfor ;\n\tWed, 28 Feb 2024 12:24:22 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 49954C67;\n\tWed, 28 Feb 2024 12:24:09 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BSVH0caS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709119449;\n\tbh=4/ZivS4N1OQ3VJuP2OhBy6gTt6MeIqrPF+K7+qFuzjs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BSVH0caS/47nK4ftF3seGK4ExAm/7gg8og1B+vSDK8Q+Qtk+HAvxl3w1dCqxxYAIQ\n\tFgPaCKQFOVQxASsVuBPPJ4gOvLyrH3eu+2hpXCrYqtJF7sXtsniyPhyb7TI2AwT/sn\n\tFnbZRTWjjS8BTDUqy9gPbqQQhvcDBa8hacfwtKbo=","Date":"Wed, 28 Feb 2024 13:24:23 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Subject":"Re: [PATCH 7/9] libcamera: v4l2_subdevice: Add stream support to\n\tget/set functions","Message-ID":"<20240228112423.GL3419@pendragon.ideasonboard.com>","References":"<20240227140953.26093-1-laurent.pinchart@ideasonboard.com>\n\t<20240227140953.26093-8-laurent.pinchart@ideasonboard.com>\n\t\n\t<20240228105119.GH3419@pendragon.ideasonboard.com>\n\t<4mswhhito7dgbwk27oanrtlwoph7qzpuwiao6o7gfelx57kjc4@3k33law6dubx>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<4mswhhito7dgbwk27oanrtlwoph7qzpuwiao6o7gfelx57kjc4@3k33law6dubx>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]