[{"id":15165,"web_url":"https://patchwork.libcamera.org/comment/15165/","msgid":"","date":"2021-02-15T03:24:14","subject":"Re: [libcamera-devel] [PATCH v8 04/12] libcamera: IPAInterface:\n\tReplace C API with the new C++-only API","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 Sat, Feb 13, 2021 at 01:22:17PM +0900, Paul Elder wrote:\n> Remove everything related to the C API, including ipa_context,\n> ipa_context_wrapper, and IPAInterfaceWrapper. Also remove relevant\n> documentation.\n> \n> ipaCreate() provided by IPA implementations, and createInterface()\n> provided by IPAModule (wrapper around IPA implementation) both now\n> return a C++ object IPAInterface instead of struct ipa_context.\n> \n> Although IPAInterfaceWrapper is the only component of libipa, the\n> skeleton and build files for libipa are retained.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> No change in v8\n> \n> No change in v7\n> \n> Changes in v6:\n> - remove definitions and documentation for IPASettings, IPAStream,\n> IPABuffer, and IPAOperationData, as the first three are moved to\n> mojom, and the last is simply superseded\n> \n> Squashed in v5\n> - change some words, remove some outdated points\n> - don't remove libipa, only remove IPAInterfaceWrapper\n> - as a result, libipa is now empty\n> - update IPAInterface documentation\n> - add todo on how to generate documentation for the new IPAInterface\n> implementations\n> \n> No change in v4\n> \n> No change in v3\n> \n> Changes in v2:\n> - add documentation for IPAModule::createInterface()\n> \n> ---\n> \n> This is a combination of 6 commits:\n> \n> ---\n> \n> libcamera: IPAModule: Replace ipa_context with IPAInterface\n> \n> With the new IPC infrastructure, we no longer need the C interface as\n> provided by struct ipa_context. Make ipaCreate_() and createInterface()\n> return IPAInterface.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Laurent Pinchart \n> \n> ---\n> \n> libcamera: ipa_context_wrapper: Remove ipa_context_wrapper\n> \n> Since ipa_context has been replaced with custom IPAInterfaces, it is not\n> longer needed. Remove it.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Laurent Pinchart \n> \n> ---\n> \n> libcamera: IPAInterface: remove ipa_context and functions from documentation\n> \n> Remove all the documentation related to ipa_context and the C IPA API,\n> as well as the documentation about the functions in the IPAInterface.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> \n> ---\n> \n> libcamera: IPAInterface: Remove all functions from IPAInterface\n> \n> Now that all the functions in the IPA interface are defined in the data\n> definition file and a specialized IPAInterface is generated per pipeline\n> handler, remove all the functions from the base IPAInterface.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Laurent Pinchart \n> \n> ---\n> \n> libcamera: IPAInterface: make ipaCreate return IPAInterface\n> \n> With the new IPC infrastructure, we no longer need the C interface as\n> provided by struct ipa_context. Make ipaCreate return IPAinterface.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Jacopo Mondi \n> \n> ---\n> \n> ipa: remove IPAInterfaceWrapper\n> \n> As every pipeline has its own proxy, IPAInterfaceWrapper is no\n> longer necessary. Remove it.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Niklas Söderlund \n> Acked-by: Jacopo Mondi \n> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Kieran Bingham \n> \n> Used to be \"ipa: remove libipa\"\n> ---\n> .../libcamera/internal/ipa_context_wrapper.h | 53 --\n> include/libcamera/internal/ipa_module.h | 4 +-\n> include/libcamera/internal/meson.build | 1 -\n> include/libcamera/ipa/ipa_interface.h | 147 +---\n> src/ipa/ipu3/ipu3.cpp | 2 -\n> src/ipa/libipa/ipa_interface_wrapper.cpp | 287 --------\n> src/ipa/libipa/ipa_interface_wrapper.h | 61 --\n> src/ipa/libipa/meson.build | 2 -\n> src/ipa/raspberrypi/raspberrypi.cpp | 2 -\n> src/ipa/rkisp1/rkisp1.cpp | 2 -\n> src/ipa/vimc/vimc.cpp | 2 -\n> src/libcamera/ipa_context_wrapper.cpp | 298 --------\n> src/libcamera/ipa_interface.cpp | 643 ++----------------\n> src/libcamera/ipa_module.cpp | 18 +-\n> src/libcamera/meson.build | 1 -\n> 15 files changed, 57 insertions(+), 1466 deletions(-)\n> delete mode 100644 include/libcamera/internal/ipa_context_wrapper.h\n> delete mode 100644 src/ipa/libipa/ipa_interface_wrapper.cpp\n> delete mode 100644 src/ipa/libipa/ipa_interface_wrapper.h\n> delete mode 100644 src/libcamera/ipa_context_wrapper.cpp\n\n[snip]\n\n> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> index 5be6f787..ae049831 100644\n> --- a/src/libcamera/ipa_interface.cpp\n> +++ b/src/libcamera/ipa_interface.cpp\n\n[snip]\n\n> @@ -516,147 +90,16 @@ namespace libcamera {\n> * mirror core libcamera structures when the latter are not suitable, such as\n> * IPAStream to carry StreamConfiguration data.\n> *\n> - * As for the functions defined in struct ipa_context_ops, the methods defined\n> - * by this class shall not return data from the IPA.\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> + * be implemented in IPADataSerializer.\n> *\n> - * The pipeline handler shall use the IPAManager to locate a compatible\n> + * The pipeline handlers shall use the IPAManager to locate a compatible\n> * IPAInterface. The interface may then be used to interact with the IPA module.\n> - */\n> -\n> -/**\n> - * \\fn IPAInterface::init()\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 IPAInterface::start()\n> - * \\brief Start the IPA\n> - * \\param[in] data Protocol-specific data for the start operation\n> - * \\param[out] result Result of the start operation\n> - *\n> - * This method informs the IPA module that the camera is about to be started.\n> - * The IPA module shall prepare any resources it needs to operate.\n> - *\n> - * The \\a data and \\a result parameters carry custom data passed by the\n> - * pipeline handler to the IPA and back. The pipeline handler may set the \\a\n> - * result parameter to null if the IPA protocol doesn't need to pass a result\n> - * back through the start() function.\n> - *\n> - * \\return 0 on success or a negative error code otherwise\n> - */\n> -\n> -/**\n> - * \\fn IPAInterface::stop()\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> -/**\n> - * \\fn IPAInterface::configure()\n> - * \\brief Configure the IPA stream and sensor settings\n> - * \\param[in] sensorInfo Camera sensor information\n> - * \\param[in] streamConfig Configuration of all active streams\n> - * \\param[in] entityControls Controls provided by the pipeline entities\n> - * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> - * \\param[out] result Pipeline-handler-specific configuration result\n> *\n> - * This method shall be called when the camera is started to inform the IPA of\n> - * the camera's streams and the sensor settings. The meaning of the numerical\n> - * keys in the \\a streamConfig and \\a entityControls maps is defined by the IPA\n> - * protocol.\n> - *\n> - * The \\a sensorInfo conveys information about the camera sensor settings that\n> - * the pipeline handler has selected for the configuration. The IPA may use\n> - * that information to tune its algorithms.\n> - *\n> - * The \\a ipaConfig and \\a result parameters carry custom data passed by the\n> - * pipeline handler to the IPA and back. The pipeline handler may set the \\a\n> - * result parameter to null if the IPA protocol doesn't need to pass a result\n> - * back through the configure() function.\n> - */\n> -\n> -/**\n> - * \\fn IPAInterface::mapBuffers()\n> - * \\brief Map buffers shared between the pipeline handler and the IPA\n> - * \\param[in] buffers List of buffers to map\n> - *\n> - * This method informs the IPA module of memory buffers set up by the pipeline\n> - * handler that the IPA needs to access. It provides dmabuf file handles for\n> - * each buffer, and associates the buffers with unique numerical IDs.\n> - *\n> - * IPAs shall map the dmabuf file handles to their address space and keep a\n> - * cache of the mappings, indexed by the buffer numerical IDs. The IDs are used\n> - * in all other IPA interface methods to refer to buffers, including the\n> - * unmapBuffers() method.\n> - *\n> - * All buffers that the pipeline handler wishes to share with an IPA shall be\n> - * mapped with this method. Buffers may be mapped all at once with a single\n> - * call, or mapped and unmapped dynamically at runtime, depending on the IPA\n> - * protocol. Regardless of the protocol, all buffers mapped at a given time\n> - * shall have unique numerical IDs.\n> - *\n> - * The numerical IDs have no meaning defined by the IPA interface, and IPA\n> - * protocols shall not give them any specific meaning either. They should be\n> - * treated as opaque handles by IPAs, with the only exception that ID zero is\n> - * invalid.\n> - *\n> - * \\sa unmapBuffers()\n> - *\n> - * \\todo Provide a generic implementation of mapBuffers and unmapBuffers for\n> - * IPAs\n> - */\n> -\n> -/**\n> - * \\fn IPAInterface::unmapBuffers()\n> - * \\brief Unmap buffers shared by the pipeline to the IPA\n> - * \\param[in] ids List of buffer IDs to unmap\n> - *\n> - * This method removes mappings set up with mapBuffers(). Buffers may be\n> - * unmapped all at once with a single call, or selectively at runtime, depending\n> - * on the IPA protocol. Numerical IDs of unmapped buffers may be reused when\n> - * mapping new buffers.\n> - *\n> - * \\sa mapBuffers()\n> + * \\todo Figure out how to generate IPAInterface documentation.\n> */\n> \n> -/**\n> - * \\fn IPAInterface::processEvent()\n> - * \\brief Process an event from the pipeline handler\n> - * \\param[in] data IPA operation data\n> - *\n> - * This operation is used by pipeline handlers to inform the IPA module of\n> - * events that occurred during the on-going capture operation.\n> - *\n> - * The event notified by the pipeline handler with this method is handled by the\n> - * IPA, which interprets the operation parameters according to the separately\n> - * documented IPA protocol.\n> - */\n> -\n> -/**\n> - * \\var IPAInterface::queueFrameAction\n> - * \\brief Queue an action associated with a frame to the pipeline handler\n> - * \\param[in] frame The frame number for the action\n> - * \\param[in] data IPA operation data\n> - *\n> - * This signal is emitted when the IPA wishes to queue a FrameAction on the\n> - * pipeline. The pipeline is still responsible for the scheduling of the action\n> - * on its timeline.\n> - *\n> - * This signal is emitted by the IPA to queue an action to be executed by the\n> - * pipeline handler on a frame. The type of action is identified by the\n> - * \\a data.operation field, as defined by the IPA protocol, and the rest of the\n> - * \\a data is interpreted accordingly. The pipeline handler shall queue the\n> - * action and execute it as appropriate.\n> - *\n> - * The signal is only emitted when the IPA is running, that is after start() and\n> - * before stop() have been called.\n> - */\n> \n\nExtra blank line.\n\nReviewed-by: Laurent Pinchart \n\n> } /* namespace libcamera */","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 D38A4BD162\n\tfor ;\n\tMon, 15 Feb 2021 03:24:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2ABD6637AA;\n\tMon, 15 Feb 2021 04:24:42 +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 CCC83602F9\n\tfor ;\n\tMon, 15 Feb 2021 04:24:40 +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 12E1D743;\n\tMon, 15 Feb 2021 04:24:39 +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=\"IeSEBixF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613359480;\n\tbh=k30KrwtbroRs0t7HiOgzcCFGvAa4bI45v0BZybeeVuM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IeSEBixFisu7dPXhYlrbVXPs4D3WOSVsqTNezDjffN137cseRbqAHlROzsv3xNOzk\n\t/SUL8XKcWxw4Ww4LQqPz4OaNvbDhUAwWieo0FMLe6hfsD0KBkv1K7iPLfx/KHA1ToY\n\tdwF+eUeo1PKiqgfIB86EVF+rePDVb5z6yX84ZM9E=","Date":"Mon, 15 Feb 2021 05:24:14 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210213042225.112477-1-paul.elder@ideasonboard.com>\n\t<20210213042225.112477-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210213042225.112477-5-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v8 04/12] libcamera: IPAInterface:\n\tReplace C API with the new C++-only API","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\" "}}]