[{"id":14898,"web_url":"https://patchwork.libcamera.org/comment/14898/","msgid":"","date":"2021-02-02T02:49:48","subject":"Re: [libcamera-devel] [PATCH v6 9/9] ipa: Add core.mojom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, Dec 24, 2020 at 05:15:34PM +0900, Paul Elder wrote:\n> Add a base mojom file to contain empty mojom definitions of libcamera\n> objects, as well as documentation for the IPA interfaces that need to be\n> defined in the mojom files.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Niklas Söderlund \n> \n> ---\n> Changes in v6:\n> - expand documentation on what can and can't be done in mojom\n> - add definitions for geometry.h structs, and the structs that used to\n> be in ipa_interface.h, including their documentation\n> - remove documentation for start()\n> \n> Changes in v5:\n> - add todo for defining some libcamera ipa structs in mojom\n> - remove ipa_mojom_core from dependencies of everything in the\n> generator stage\n> - add documentation for the base IPA functions (init, stop, start)\n> \n> Changes in v4:\n> - move docs to IPA guide\n> \n> Changes in v3:\n> - add doc that structs need to be defined\n> - add doc to recommend namespacing\n> - change indentation\n> - add requirement that base controls *must* be defined in\n> libcamera::{pipeline_name}::Controls\n> \n> New in v2\n> ---\n> include/libcamera/ipa/core.mojom | 208 +++++++++++++++++++++++++++++++\n> 1 file changed, 208 insertions(+)\n> create mode 100644 include/libcamera/ipa/core.mojom\n> \n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> new file mode 100644\n> index 00000000..d707508b\n> --- /dev/null\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -0,0 +1,208 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +\n> +/*\n> + * Things that can be defined here (and in other mojom files):\n> + * - consts\n> + * - enums\n> + * - structs\n> + *\n> + * Attributes:\n> + * - genHeader - structs only\n> + * - designate that this struct needs a C++ header definition to be generated\n> + * - not necessary if this struct is already defined in a C++ header\n> + * - genSerdes - structs only\n> + * - designate that this struct needs a (de)serializer to be generated\n\nWould it make sense to do it the other way around, having an attribute\nto skip generation of the serdes ? All but three structures in this\nfiles set the genSerdes attribute.\n\nI'm also tempted to do the same for genHeader, to mark the structures\nthat already have a C++ version. Up to you.\n\n> + * - all fields need a (de)serializer to be defined, either hand-written\n> + * in ipa_data_serializer.h, or designated here with genSerdes\n\nShould you document that those two attributes are only valid in\ncore.mojom, and ignored in other files ?\n\n> + * - hasFd - struct fields or empty structs only\n> + * - designate that this field or empty struct contains a FileDescriptor\n> + *\n> + * Rules:\n> + * - Any struct that is used in a struct definition in mojom must also be\n> + * defined in mojom\n> + * - If the struct has both a definition in a C++ header and a (de)serializer\n> + * in ipa_data_serializer.h, then the struct shall be declared as empty\n> + * - If the struct only has a definition in a C++ header, but no\n> + * (de)serializer, then the struct definition should have the [genSerdes]\n> + * attribute\n> + * - If the struct has neither a definition in a C++ header nor a\n> + * (de)serializer, then the struct definition should have both the\n> + * [genHeader] and [genSerdes] attributes\n> + * - Nested structures (eg. FrameBuffer::Plane) cannot be defined in mojom.\n\ns/eg./e.g./\n\n> + * - Avoid them, by defining them in a header in C++ and a (de)serializer in\n> + * ipa_data_serializer.h\n> + * - If a struct is in an array/map inside a struct, then the struct that is\n> + * the member of the array/map does not need a mojom definition.\n\ns/definition./definition if it is defined in a C++ header./ ?\n\n> + * - This can be used to embed nested structures. The C++ double dolon is\n\ns/dolon/colon/\n\n> + * replaced with a dot (eg. FrameBuffer::Plane -> FrameBuffer.Plane)\n\ns/eg./e.g./\n\n> + * - The struct must still be defined in a header in C++ and a (de)serializer\n> + * implemented in ipa_data_serializer.h, as it cannot be defined in mojom\n> + * - [genHeader] and [genSerdes] only work here in core.mojom. Any struct defined\n> + * in other mojom files will implicitly have both attributes.\n\nAh here we go :-) Maybe this should be moved above ?\n\n> + * - If a struct definition does not have genHeader, then the header where the\n> + * struct is defined must be #included (or the struct forward-declared) in\n> + * ipa_interface.h\n> + * - If a field in a struct has a FileDescriptor, but is not explicitly\n> + * defined so in mojom, then the field must be marked with the [hasFd]\n> + * attribute.\n> + */\n> +struct ControlInfoMap {};\n> +struct ControlList {};\n> +struct FileDescriptor {};\n> +\n> +[genSerdes] struct Point {\n> +\tint32 x;\n> +\tint32 y;\n> +};\n> +\n> +[genSerdes] struct Size {\n> +\tuint32 width;\n> +\tuint32 height;\n> +};\n> +\n> +[genSerdes] struct SizeRange {\n> +\tSize min;\n> +\tSize max;\n> +\tuint32 hStep;\n> +\tuint32 vStep;\n> +};\n> +\n> +[genSerdes] struct Rectangle {\n> +\tint32 x;\n> +\tint32 y;\n> +\tuint32 width;\n> +\tuint32 height;\n> +};\n> +\n> +[genSerdes] struct CameraSensorInfo {\n> +\tstring model;\n> +\n> +\tuint32 bitsPerPixel;\n> +\n> +\tSize activeAreaSize;\n> +\tRectangle analogCrop;\n> +\tSize outputSize;\n> +\n> +\tuint64 pixelRate;\n> +\tuint32 lineLength;\n> +};\n> +\n> +/**\n> + * \\struct IPABuffer\n> + * \\brief Buffer information for the IPA interface\n> + *\n> + * The IPABuffer structure associates buffer memory with a unique ID. It is\n> + * used to map buffers to the IPA with IPAInterface::mapBuffers(), after which\n> + * buffers will be identified by their ID in the IPA interface.\n> + */\n> +\n> +/**\n> + * \\var IPABuffer::id\n> + * \\brief The buffer unique ID\n> + *\n> + * Buffers mapped to the IPA are identified by numerical unique IDs. The IDs\n> + * are chosen by the pipeline handler to fulfil the following constraints:\n> + *\n> + * - IDs shall be positive integers different than zero\n> + * - IDs shall be unique among all mapped buffers\n> + *\n> + * When buffers are unmapped with IPAInterface::unmapBuffers() their IDs are\n> + * freed and may be reused for new buffer mappings.\n> + */\n> +\n> +/**\n> + * \\var IPABuffer::planes\n> + * \\brief The buffer planes description\n> + *\n> + * Stores the dmabuf handle and length for each plane of the buffer.\n> + */\n> +\n> +/**\n> + * \\class IPAInterface\n> + * \\brief C++ Interface for IPA implementation\n> + *\n> + * This pure virtual class defines a skeletal C++ API for IPA modules.\n> + * Specializations of this class must be defined in a mojom file in\n> + * include/libcamera/ipa/ (see the IPA Writers Guide for details\n> + * on how to do so).\n> + *\n> + * Due to process isolation all arguments to the IPAInterface methods and\n> + * signals may need to be transferred over IPC. The class thus uses serializable\n> + * data types only. The IPA C++ interface defines custom data structures that\n> + * mirror core libcamera structures when the latter are not suitable, such as\n> + * IPAStream to carry StreamConfiguration data.\n> + *\n> + * Custom data structures may also be defined in the mojom file, in which case\n> + * the (de)serialization will automatically be generated. If any other libcamera\n> + * structures are to be used as parameters, then a de/serializer for them must\n\ns/de\\/(de)/ ?\n\n> + * be implemented in IPADataSerializer.\n> + *\n> + * The pipeline handler shall use the IPAManager to locate a compatible\n\ns/handler/handlers/\n\n> + * IPAInterface. The interface may then be used to interact with the IPA module.\n> + */\n\nI'd move this to the end of the file, before the init() and stop()\nfunctions.\n\n> +[genHeader, genSerdes] struct IPABuffer {\n> +\tuint32 id;\n> +\t[hasFd] array planes;\n> +};\n> +\n> +/**\n> + * \\struct IPASettings\n> + * \\brief IPA interface initialization settings\n> + *\n> + * The IPASettings structure stores data passed to the IPAInterface::init()\n> + * function. The data contains settings that don't depend on a particular camera\n> + * or pipeline configuration and are valid for the whole life time of the IPA\n> + * interface.\n> + */\n> +\n> +/**\n> + * \\var IPASettings::configurationFile\n> + * \\brief The name of the IPA configuration file\n> + *\n> + * This field may be an empty string if the IPA doesn't require a configuration\n> + * file.\n> + */\n> +[genHeader, genSerdes] struct IPASettings {\n> +\tstring configurationFile;\n> +};\n> +\n> +/**\n> + * \\struct IPAStream\n> + * \\brief Stream configuration for the IPA interface\n> + *\n> + * The IPAStream structure stores stream configuration parameters needed by the\n> + * IPAInterface::configure() method. It mirrors the StreamConfiguration class\n> + * that is not suitable for this purpose due to not being serializable.\n> + */\n> +\n> +/**\n> + * \\var IPAStream::pixelFormat\n> + * \\brief The stream pixel format\n> + */\n> +\n> +/**\n> + * \\var IPAStream::size\n> + * \\brief The stream size in pixels\n> + */\n> +[genHeader, genSerdes] struct IPAStream {\n> +\tuint32 pixelFormat;\n> +\tSize size;\n> +};\n> +\n> +/**\n> + * \\fn init()\n\ns/init/IPAInterface::init/\n\n> + * \\brief Initialise the IPAInterface\n> + * \\param[in] settings The IPA initialization settings\n> + *\n> + * This function initializes the IPA interface. It shall be called before any\n> + * other function of the IPAInterface. The \\a settings carry initialization\n> + * parameters that are valid for the whole life time of the IPA interface.\n> + */\n> +\n> +/**\n> + * \\fn stop()\n\nSame here.\n\n> + * \\brief Stop the IPA\n> + *\n> + * This method informs the IPA module that the camera is stopped. The IPA module\n> + * shall release resources prepared in start().\n> + */\n\nI wonder where to store this. We can't document those two functions in\nipa_interface.cpp, as the base IPAInterface class doesn't define init()\nand stop() functions. We can fix this later.\n\nReviewed-by: 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 DEF11C33BB\n\tfor ;\n\tTue, 2 Feb 2021 02:50:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CD836841D;\n\tTue, 2 Feb 2021 03:50:14 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D44160106\n\tfor ;\n\tTue, 2 Feb 2021 03:50:12 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7D106556;\n\tTue, 2 Feb 2021 03:50:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NleP/xWy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612234210;\n\tbh=NPy9ULzNtbKMsQ16InAILTq9RgZokqy7nMOzjDDLybM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NleP/xWyh/Zw2LsieJjg88vieZOOtZorTMg+Qf2bXD8+3fzA4BCyPVcv9+7s+jgYs\n\ty/2VXrVH5b79W6O7DsPpAAwjC8jBA686Dk/v5zcYDxHeZnTwurzRqv6QfiKpaoBYMv\n\t4kOLpJuVvvCWL0i0Fb3E9lwlDB+kcu2bfN/CuWj8=","Date":"Tue, 2 Feb 2021 04:49:48 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20201224081534.41601-1-paul.elder@ideasonboard.com>\n\t<20201224081534.41601-10-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201224081534.41601-10-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 9/9] ipa: Add core.mojom","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14900,"web_url":"https://patchwork.libcamera.org/comment/14900/","msgid":"","date":"2021-02-02T02:55:38","subject":"Re: [libcamera-devel] [PATCH v6 9/9] ipa: Add core.mojom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nAnother comment.\n\nOn Tue, Feb 02, 2021 at 04:49:52AM +0200, Laurent Pinchart wrote:\n> On Thu, Dec 24, 2020 at 05:15:34PM +0900, Paul Elder wrote:\n> > Add a base mojom file to contain empty mojom definitions of libcamera\n> > objects, as well as documentation for the IPA interfaces that need to be\n> > defined in the mojom files.\n> > \n> > Signed-off-by: Paul Elder \n> > Reviewed-by: Niklas Söderlund \n> > \n> > ---\n> > Changes in v6:\n> > - expand documentation on what can and can't be done in mojom\n> > - add definitions for geometry.h structs, and the structs that used to\n> > be in ipa_interface.h, including their documentation\n> > - remove documentation for start()\n> > \n> > Changes in v5:\n> > - add todo for defining some libcamera ipa structs in mojom\n> > - remove ipa_mojom_core from dependencies of everything in the\n> > generator stage\n> > - add documentation for the base IPA functions (init, stop, start)\n> > \n> > Changes in v4:\n> > - move docs to IPA guide\n> > \n> > Changes in v3:\n> > - add doc that structs need to be defined\n> > - add doc to recommend namespacing\n> > - change indentation\n> > - add requirement that base controls *must* be defined in\n> > libcamera::{pipeline_name}::Controls\n> > \n> > New in v2\n> > ---\n> > include/libcamera/ipa/core.mojom | 208 +++++++++++++++++++++++++++++++\n> > 1 file changed, 208 insertions(+)\n> > create mode 100644 include/libcamera/ipa/core.mojom\n> > \n> > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> > new file mode 100644\n> > index 00000000..d707508b\n> > --- /dev/null\n> > +++ b/include/libcamera/ipa/core.mojom\n> > @@ -0,0 +1,208 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +\n> > +/*\n> > + * Things that can be defined here (and in other mojom files):\n> > + * - consts\n> > + * - enums\n> > + * - structs\n> > + *\n> > + * Attributes:\n> > + * - genHeader - structs only\n> > + * - designate that this struct needs a C++ header definition to be generated\n> > + * - not necessary if this struct is already defined in a C++ header\n> > + * - genSerdes - structs only\n> > + * - designate that this struct needs a (de)serializer to be generated\n> \n> Would it make sense to do it the other way around, having an attribute\n> to skip generation of the serdes ? All but three structures in this\n> files set the genSerdes attribute.\n> \n> I'm also tempted to do the same for genHeader, to mark the structures\n> that already have a C++ version. Up to you.\n> \n> > + * - all fields need a (de)serializer to be defined, either hand-written\n> > + * in ipa_data_serializer.h, or designated here with genSerdes\n> \n> Should you document that those two attributes are only valid in\n> core.mojom, and ignored in other files ?\n> \n> > + * - hasFd - struct fields or empty structs only\n> > + * - designate that this field or empty struct contains a FileDescriptor\n> > + *\n> > + * Rules:\n> > + * - Any struct that is used in a struct definition in mojom must also be\n> > + * defined in mojom\n> > + * - If the struct has both a definition in a C++ header and a (de)serializer\n> > + * in ipa_data_serializer.h, then the struct shall be declared as empty\n> > + * - If the struct only has a definition in a C++ header, but no\n> > + * (de)serializer, then the struct definition should have the [genSerdes]\n> > + * attribute\n> > + * - If the struct has neither a definition in a C++ header nor a\n> > + * (de)serializer, then the struct definition should have both the\n> > + * [genHeader] and [genSerdes] attributes\n> > + * - Nested structures (eg. FrameBuffer::Plane) cannot be defined in mojom.\n> \n> s/eg./e.g./\n> \n> > + * - Avoid them, by defining them in a header in C++ and a (de)serializer in\n> > + * ipa_data_serializer.h\n> > + * - If a struct is in an array/map inside a struct, then the struct that is\n> > + * the member of the array/map does not need a mojom definition.\n> \n> s/definition./definition if it is defined in a C++ header./ ?\n> \n> > + * - This can be used to embed nested structures. The C++ double dolon is\n> \n> s/dolon/colon/\n> \n> > + * replaced with a dot (eg. FrameBuffer::Plane -> FrameBuffer.Plane)\n> \n> s/eg./e.g./\n> \n> > + * - The struct must still be defined in a header in C++ and a (de)serializer\n> > + * implemented in ipa_data_serializer.h, as it cannot be defined in mojom\n> > + * - [genHeader] and [genSerdes] only work here in core.mojom. Any struct defined\n> > + * in other mojom files will implicitly have both attributes.\n> \n> Ah here we go :-) Maybe this should be moved above ?\n> \n> > + * - If a struct definition does not have genHeader, then the header where the\n> > + * struct is defined must be #included (or the struct forward-declared) in\n> > + * ipa_interface.h\n> > + * - If a field in a struct has a FileDescriptor, but is not explicitly\n> > + * defined so in mojom, then the field must be marked with the [hasFd]\n> > + * attribute.\n\n * \\todo Generate documentation from Doxygen comments in .mojom files\n\n> > + */\n> > +struct ControlInfoMap {};\n> > +struct ControlList {};\n> > +struct FileDescriptor {};\n> > +\n> > +[genSerdes] struct Point {\n> > +\tint32 x;\n> > +\tint32 y;\n> > +};\n> > +\n> > +[genSerdes] struct Size {\n> > +\tuint32 width;\n> > +\tuint32 height;\n> > +};\n> > +\n> > +[genSerdes] struct SizeRange {\n> > +\tSize min;\n> > +\tSize max;\n> > +\tuint32 hStep;\n> > +\tuint32 vStep;\n> > +};\n> > +\n> > +[genSerdes] struct Rectangle {\n> > +\tint32 x;\n> > +\tint32 y;\n> > +\tuint32 width;\n> > +\tuint32 height;\n> > +};\n> > +\n> > +[genSerdes] struct CameraSensorInfo {\n> > +\tstring model;\n> > +\n> > +\tuint32 bitsPerPixel;\n> > +\n> > +\tSize activeAreaSize;\n> > +\tRectangle analogCrop;\n> > +\tSize outputSize;\n> > +\n> > +\tuint64 pixelRate;\n> > +\tuint32 lineLength;\n> > +};\n> > +\n> > +/**\n> > + * \\struct IPABuffer\n> > + * \\brief Buffer information for the IPA interface\n> > + *\n> > + * The IPABuffer structure associates buffer memory with a unique ID. It is\n> > + * used to map buffers to the IPA with IPAInterface::mapBuffers(), after which\n> > + * buffers will be identified by their ID in the IPA interface.\n> > + */\n> > +\n> > +/**\n> > + * \\var IPABuffer::id\n> > + * \\brief The buffer unique ID\n> > + *\n> > + * Buffers mapped to the IPA are identified by numerical unique IDs. The IDs\n> > + * are chosen by the pipeline handler to fulfil the following constraints:\n> > + *\n> > + * - IDs shall be positive integers different than zero\n> > + * - IDs shall be unique among all mapped buffers\n> > + *\n> > + * When buffers are unmapped with IPAInterface::unmapBuffers() their IDs are\n> > + * freed and may be reused for new buffer mappings.\n> > + */\n> > +\n> > +/**\n> > + * \\var IPABuffer::planes\n> > + * \\brief The buffer planes description\n> > + *\n> > + * Stores the dmabuf handle and length for each plane of the buffer.\n> > + */\n> > +\n> > +/**\n> > + * \\class IPAInterface\n> > + * \\brief C++ Interface for IPA implementation\n> > + *\n> > + * This pure virtual class defines a skeletal C++ API for IPA modules.\n> > + * Specializations of this class must be defined in a mojom file in\n> > + * include/libcamera/ipa/ (see the IPA Writers Guide for details\n> > + * on how to do so).\n> > + *\n> > + * Due to process isolation all arguments to the IPAInterface methods and\n> > + * signals may need to be transferred over IPC. The class thus uses serializable\n> > + * data types only. The IPA C++ interface defines custom data structures that\n> > + * mirror core libcamera structures when the latter are not suitable, such as\n> > + * IPAStream to carry StreamConfiguration data.\n> > + *\n> > + * Custom data structures may also be defined in the mojom file, in which case\n> > + * the (de)serialization will automatically be generated. If any other libcamera\n> > + * structures are to be used as parameters, then a de/serializer for them must\n> \n> s/de\\/(de)/ ?\n> \n> > + * be implemented in IPADataSerializer.\n> > + *\n> > + * The pipeline handler shall use the IPAManager to locate a compatible\n> \n> s/handler/handlers/\n> \n> > + * IPAInterface. The interface may then be used to interact with the IPA module.\n> > + */\n> \n> I'd move this to the end of the file, before the init() and stop()\n> functions.\n> \n> > +[genHeader, genSerdes] struct IPABuffer {\n> > +\tuint32 id;\n> > +\t[hasFd] array planes;\n> > +};\n> > +\n> > +/**\n> > + * \\struct IPASettings\n> > + * \\brief IPA interface initialization settings\n> > + *\n> > + * The IPASettings structure stores data passed to the IPAInterface::init()\n> > + * function. The data contains settings that don't depend on a particular camera\n> > + * or pipeline configuration and are valid for the whole life time of the IPA\n> > + * interface.\n> > + */\n> > +\n> > +/**\n> > + * \\var IPASettings::configurationFile\n> > + * \\brief The name of the IPA configuration file\n> > + *\n> > + * This field may be an empty string if the IPA doesn't require a configuration\n> > + * file.\n> > + */\n> > +[genHeader, genSerdes] struct IPASettings {\n> > +\tstring configurationFile;\n> > +};\n> > +\n> > +/**\n> > + * \\struct IPAStream\n> > + * \\brief Stream configuration for the IPA interface\n> > + *\n> > + * The IPAStream structure stores stream configuration parameters needed by the\n> > + * IPAInterface::configure() method. It mirrors the StreamConfiguration class\n> > + * that is not suitable for this purpose due to not being serializable.\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAStream::pixelFormat\n> > + * \\brief The stream pixel format\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAStream::size\n> > + * \\brief The stream size in pixels\n> > + */\n> > +[genHeader, genSerdes] struct IPAStream {\n> > +\tuint32 pixelFormat;\n> > +\tSize size;\n> > +};\n> > +\n> > +/**\n> > + * \\fn init()\n> \n> s/init/IPAInterface::init/\n> \n> > + * \\brief Initialise the IPAInterface\n> > + * \\param[in] settings The IPA initialization settings\n> > + *\n> > + * This function initializes the IPA interface. It shall be called before any\n> > + * other function of the IPAInterface. The \\a settings carry initialization\n> > + * parameters that are valid for the whole life time of the IPA interface.\n> > + */\n> > +\n> > +/**\n> > + * \\fn stop()\n> \n> Same here.\n> \n> > + * \\brief Stop the IPA\n> > + *\n> > + * This method informs the IPA module that the camera is stopped. The IPA module\n> > + * shall release resources prepared in start().\n> > + */\n> \n> I wonder where to store this. We can't document those two functions in\n> ipa_interface.cpp, as the base IPAInterface class doesn't define init()\n> and stop() functions. We can fix this later.\n> \n> Reviewed-by: 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 21F07BD808\n\tfor ;\n\tTue, 2 Feb 2021 02:56:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9373268422;\n\tTue, 2 Feb 2021 03:56:01 +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 636DD60106\n\tfor ;\n\tTue, 2 Feb 2021 03:56:00 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C68AA556;\n\tTue, 2 Feb 2021 03:55:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YKBz/MsB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612234560;\n\tbh=xGmEoh3BH7jmOGGVudxJCPSplY1JYh6xqc+GKln6jEs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YKBz/MsB8+SdO66E3pXnAzb5nMgoNuiIdDqkKF48ZsCN4A58kDpmt8iVwVi+x9zuL\n\tBLB7Aq0iJYqDA+b4himq7JkjU0u+kPklz+LIAcVLVTMEL19zE1EocDNXlLucZIM05/\n\ttoauAofI7j2YDNXWWUi46Q6gASTBIs6X8fs5Wigg=","Date":"Tue, 2 Feb 2021 04:55:38 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20201224081534.41601-1-paul.elder@ideasonboard.com>\n\t<20201224081534.41601-10-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 9/9] ipa: Add core.mojom","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]