[{"id":13922,"web_url":"https://patchwork.libcamera.org/comment/13922/","msgid":"<20201126150255.GO3905@pendragon.ideasonboard.com>","date":"2020-11-26T15:02:55","subject":"Re: [libcamera-devel] [PATCH v4 25/37] ipa: raspberrypi: Add mojom\n\tdata definition file","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 Fri, Nov 06, 2020 at 07:36:55PM +0900, Paul Elder wrote:\n> Add a mojom data definition for raspberrypi pipeline handler's IPAs.\n> This is a direct translation of what the raspberrypi pipeline handler\n> and IPA was using before with IPAOperationData.\n\ns/was/were/\n\n> \n> Also move the enums from raspberrypi.h to raspberrypi.mojom\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v4:\n> - rename IPARPiCallbackInterface to IPARPiEventInterface\n> \n> Changes in v3:\n> - remove stray comment about how our compiler will generate code\n> - add ipa.rpi namespace\n> - not ipa.RPi, because namespace conflict\n> - remove RPi prefix from struct and enum names\n> - add transform parameter to struct ConfigInput\n> \n> Changes in v2:\n> - rebased on \"libcamera: pipeline: ipa: raspberrypi: Rework drop frame\n> signalling\"\n> - add license\n> - move generic documentation to core.mojom\n> - move documentation from IPA interface\n> - customize the RPi IPA interface to make the pipeline and IPA code\n> cleaner\n> ---\n> include/libcamera/ipa/meson.build | 4 +-\n> include/libcamera/ipa/raspberrypi.h | 18 ---\n> include/libcamera/ipa/raspberrypi.mojom | 158 ++++++++++++++++++++++++\n> 3 files changed, 161 insertions(+), 19 deletions(-)\n> create mode 100644 include/libcamera/ipa/raspberrypi.mojom\n> \n> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\n> index b7caec95..d49dff7b 100644\n> --- a/include/libcamera/ipa/meson.build\n> +++ b/include/libcamera/ipa/meson.build\n> @@ -24,7 +24,9 @@ ipa_mojom_core = custom_target(core_mojom_file.split('.')[0] + '_mojom_module',\n> '--mojoms', '@INPUT@'\n> ])\n> \n> -ipa_mojom_files = []\n> +ipa_mojom_files = [\n> + 'raspberrypi.mojom',\n> +]\n> \n> ipa_mojoms = []\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 2b55dbfc..df30b4a2 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -16,24 +16,6 @@ namespace libcamera {\n> \n> namespace RPi {\n> \n> -enum ConfigParameters {\n> -\tIPA_CONFIG_LS_TABLE = (1 << 0),\n> -\tIPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> -\tIPA_CONFIG_SENSOR = (1 << 2),\n> -\tIPA_CONFIG_DROP_FRAMES = (1 << 3),\n> -};\n> -\n> -enum Operations {\n> -\tIPA_ACTION_V4L2_SET_STAGGERED = 1,\n> -\tIPA_ACTION_V4L2_SET_ISP,\n> -\tIPA_ACTION_STATS_METADATA_COMPLETE,\n> -\tIPA_ACTION_RUN_ISP,\n> -\tIPA_ACTION_EMBEDDED_COMPLETE,\n> -\tIPA_EVENT_SIGNAL_STAT_READY,\n> -\tIPA_EVENT_SIGNAL_ISP_PREPARE,\n> -\tIPA_EVENT_QUEUE_REQUEST,\n> -};\n> -\n> enum BufferMask {\n> \tID\t\t= 0x00ffff,\n> \tSTATS\t\t= 0x010000,\n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> new file mode 100644\n> index 00000000..ec261569\n> --- /dev/null\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -0,0 +1,158 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +\n> +module ipa.rpi;\n> +\n> +import \"include/libcamera/ipa/core.mojom\";\n> +\n> +interface IPARPiInterface {\n> +/**\n> + * \\fn 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\nHmmmm... I really wonder how we should handle documentation here. This\nwon't be parsed by doxygen, which isn't an issue (at least for now) as\nit's really an internal API. It however has the drawback of not catching\nissues in the documentation at compile time. I doubt there's much we\ncould do here for now. Longer term extracting the comment and including\nit in the generated sources could be a good way forward.\n\nAnother question is what we should document here, for the functions that\nare generic (that's init(), start() and stop() for now). It's a bit\npointless to duplicate that in all mojom files.\n\nFinally, should this be indented by one tab to align it with the\nfunctions ?\n\n> +\tinit(IPASettings settings) => (int32 ret);\n> +\n> +/**\n> + * \\fn start()\n> + * \\brief Start the IPA\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> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\tstart() => (int32 ret);\n> +\n> +/**\n> + * \\fn 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> +\tstop();\n> +\n> +/**\n> + * \\fn 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\nThe mention of IPA protocol made sense when the IPAInterface was the\nsame for all IPAs, but that's now outdated. *This* is the IPA protocol\nfor RPi :-) The documentation should be reworked accordingly. Generic\nparts could be preserved in the IPA guide, to explain how to create an\nIPA protocol.\n\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> +\tconfigure(CameraSensorInfo sensorInfo,\n> +\t\t map streamConfig,\n> +\t\t map entityControls,\n> +\t\t ConfigInput ipaConfig)\n> +\t\t=> (ConfigOutput results);\n> +\n> +/**\n> + * \\fn 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\nSame here, there's lots of generic documentation that doesn't belong to\na pipeline handler-specific file.\n\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> +\tmapBuffers(array buffers);\n> +\n> +/**\n> + * \\fn 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> + */\n> +\tunmapBuffers(array ids);\n> +\n> +\t[async] signalStatReady(uint32 bufferId);\n> +\t[async] signalQueueRequest(ControlList controls);\n> +\t[async] signalIspPrepare(IspPreparePayload data);\n> +};\n> +\n> +interface IPARPiEventInterface {\n> +\tstatsMetadataComplete(uint32 bufferId, ControlList controls);\n> +\trunIsp(uint32 bufferId);\n> +\tembeddedComplete(uint32 bufferId);\n> +\tsetIsp(ControlList controls);\n> +\tsetStaggered(ControlList controls);\n> +};\n> +\n> +\n> +enum ConfigParameters {\n> +\tRPI_IPA_CONFIG_LS_TABLE = 0x01,\n> +\tRPI_IPA_CONFIG_STAGGERED_WRITE = 0x02,\n> +\tRPI_IPA_CONFIG_SENSOR = 0x04,\n> +\tRPI_IPA_CONFIG_DROP_FRAMES = 0x08,\n\nAs this will be qualified by the ipa::rpi:: namespace, should we drop\nthe RPI_IPA_ prefix ?\n\nWhile at it, would it make sense to move to our usual coding style of\nCamelCase for enum values ?\n\n> +};\n> +\n> +struct StaggeredWritePayload {\n\ns/Payload/Config/ ? Payload really sounds like a networking term, and\nwhile data is serialized to a payload, that shouldn't appear in the API.\nActually, wouls SensorConfig be a better name ? The name \"staggered\nwrite\" disappears with Niklas' patches that move the helper to the\nlibcamera core.\n\n> +\tuint32 gainDelay;\n> +\tuint32 exposureDelay;\n> +\tuint32 sensorMetadata;\n> +};\n> +\n> +struct IspPreparePayload {\n\ns/IspPreparePayload/ISPConfig/ ?\n\n> +\tuint32 embeddedbufferId;\n> +\tuint32 bayerbufferId;\n> +};\n> +\n> +struct ConfigInput {\n> +\tuint32 op;\n\nI think you can drop this field (and the RPI_IPA_CONFIG_LS_TABLE enum)\nand rely on lsTableHandle being valid instead. It can be done on later\nin the series, up to you.\n\n> +\tuint32 transform;\n> +\tFileDescriptor lsTableHandle;\n> +\tint32 lsTableHandleStatic = -1;\n> +};\n> +\n> +struct ConfigOutput {\n> +\tuint32 op;\n\nHere too I'd like to drop the op field if possible.\nRPI_IPA_CONFIG_DROP_FRAMES can be dropped as it's always set. For\nRPI_IPA_CONFIG_SENSOR, we can check if controls is empty.\nRPI_IPA_CONFIG_STAGGERED_WRITE is the annoying one, as ideally it should\nbe implemented through support for nullable fields. Mojo supports this\nby turning fields of struct type to pointers, but we don't, as we embed\nthem instead. I think it would make sense doing the same, but not yet,\nas that would require too many changes to this series. In the meantime,\none simple option would be to ignore staggeredWriteResult in the\npipelien handler when !gainDelay && !exposureDelay, as that's an invalid\ncase.\n\n> +\tStaggeredWritePayload staggeredWriteResult;\n\nstaggeredWriteResult sounds a bit cryptic. If we rename the structure to\nSensorConfig, sensorConfig would be a better field name.\n\n> +\tControlList controls;\n\nShould this be named sensorControls ?\n\n> +\tint32 dropFrameCount;\n> +};\n\nEven if the mojo IDL doesn't require this, should the enums and struct\nbe moved before the interfaces ? I think it would improve readability.","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 947ECBE176\n\tfor ;\n\tThu, 26 Nov 2020 15:03:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 179C563477;\n\tThu, 26 Nov 2020 16:03:06 +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 7C5FE63469\n\tfor ;\n\tThu, 26 Nov 2020 16:03:04 +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 F216AA1B;\n\tThu, 26 Nov 2020 16:03:03 +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=\"dIxV6kA6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606402984;\n\tbh=H4OFRi74FIhin3MbUvDDxqQ2WIr3oSG1D+zpeq6Ks3E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dIxV6kA6pgG2nBpzdzPrnl3BxUW4fCvFOC/1xUXHDlKczHnVjERTah8jxUYxKbeWo\n\twZLyiwRcBMnpx29tAFoYtZSIpo/t8ZHX6FhODFbefc1hXlZOAWREzEEh5OMN+pzfOv\n\t2B+kM7Iwcmb95lHdWI8MpH51iswvr818dZzuJZvM=","Date":"Thu, 26 Nov 2020 17:02:55 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20201126150255.GO3905@pendragon.ideasonboard.com>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-26-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201106103707.49660-26-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 25/37] ipa: raspberrypi: Add mojom\n\tdata definition file","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14074,"web_url":"https://patchwork.libcamera.org/comment/14074/","msgid":"<20201205092010.GF2050@pyrite.rasen.tech>","date":"2020-12-05T09:20:10","subject":"Re: [libcamera-devel] [PATCH v4 25/37] ipa: raspberrypi: Add mojom\n\tdata definition file","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Thu, Nov 26, 2020 at 05:02:55PM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Fri, Nov 06, 2020 at 07:36:55PM +0900, Paul Elder wrote:\n> > Add a mojom data definition for raspberrypi pipeline handler's IPAs.\n> > This is a direct translation of what the raspberrypi pipeline handler\n> > and IPA was using before with IPAOperationData.\n> \n> s/was/were/\n> \n> > \n> > Also move the enums from raspberrypi.h to raspberrypi.mojom\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v4:\n> > - rename IPARPiCallbackInterface to IPARPiEventInterface\n> > \n> > Changes in v3:\n> > - remove stray comment about how our compiler will generate code\n> > - add ipa.rpi namespace\n> > - not ipa.RPi, because namespace conflict\n> > - remove RPi prefix from struct and enum names\n> > - add transform parameter to struct ConfigInput\n> > \n> > Changes in v2:\n> > - rebased on \"libcamera: pipeline: ipa: raspberrypi: Rework drop frame\n> > signalling\"\n> > - add license\n> > - move generic documentation to core.mojom\n> > - move documentation from IPA interface\n> > - customize the RPi IPA interface to make the pipeline and IPA code\n> > cleaner\n> > ---\n> > include/libcamera/ipa/meson.build | 4 +-\n> > include/libcamera/ipa/raspberrypi.h | 18 ---\n> > include/libcamera/ipa/raspberrypi.mojom | 158 ++++++++++++++++++++++++\n> > 3 files changed, 161 insertions(+), 19 deletions(-)\n> > create mode 100644 include/libcamera/ipa/raspberrypi.mojom\n> > \n> > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\n> > index b7caec95..d49dff7b 100644\n> > --- a/include/libcamera/ipa/meson.build\n> > +++ b/include/libcamera/ipa/meson.build\n> > @@ -24,7 +24,9 @@ ipa_mojom_core = custom_target(core_mojom_file.split('.')[0] + '_mojom_module',\n> > '--mojoms', '@INPUT@'\n> > ])\n> > \n> > -ipa_mojom_files = []\n> > +ipa_mojom_files = [\n> > + 'raspberrypi.mojom',\n> > +]\n> > \n> > ipa_mojoms = []\n> > \n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index 2b55dbfc..df30b4a2 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -16,24 +16,6 @@ namespace libcamera {\n> > \n> > namespace RPi {\n> > \n> > -enum ConfigParameters {\n> > -\tIPA_CONFIG_LS_TABLE = (1 << 0),\n> > -\tIPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> > -\tIPA_CONFIG_SENSOR = (1 << 2),\n> > -\tIPA_CONFIG_DROP_FRAMES = (1 << 3),\n> > -};\n> > -\n> > -enum Operations {\n> > -\tIPA_ACTION_V4L2_SET_STAGGERED = 1,\n> > -\tIPA_ACTION_V4L2_SET_ISP,\n> > -\tIPA_ACTION_STATS_METADATA_COMPLETE,\n> > -\tIPA_ACTION_RUN_ISP,\n> > -\tIPA_ACTION_EMBEDDED_COMPLETE,\n> > -\tIPA_EVENT_SIGNAL_STAT_READY,\n> > -\tIPA_EVENT_SIGNAL_ISP_PREPARE,\n> > -\tIPA_EVENT_QUEUE_REQUEST,\n> > -};\n> > -\n> > enum BufferMask {\n> > \tID\t\t= 0x00ffff,\n> > \tSTATS\t\t= 0x010000,\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > new file mode 100644\n> > index 00000000..ec261569\n> > --- /dev/null\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -0,0 +1,158 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +\n> > +module ipa.rpi;\n> > +\n> > +import \"include/libcamera/ipa/core.mojom\";\n> > +\n> > +interface IPARPiInterface {\n> > +/**\n> > + * \\fn 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> Hmmmm... I really wonder how we should handle documentation here. This\n> won't be parsed by doxygen, which isn't an issue (at least for now) as\n> it's really an internal API. It however has the drawback of not catching\n> issues in the documentation at compile time. I doubt there's much we\n> could do here for now. Longer term extracting the comment and including\n> it in the generated sources could be a good way forward.\n\nYeah :/\n\n> Another question is what we should document here, for the functions that\n> are generic (that's init(), start() and stop() for now). It's a bit\n> pointless to duplicate that in all mojom files.\n\nI'll move the generic ones to core.mojom for now. That's where I\nintended them to be to begin with. Maybe we can get doxygen to extract\ndocs from there?\n\n> Finally, should this be indented by one tab to align it with the\n> functions ?\n\nYeah that's better.\n\n> > +\tinit(IPASettings settings) => (int32 ret);\n> > +\n> > +/**\n> > + * \\fn start()\n> > + * \\brief Start the IPA\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> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\tstart() => (int32 ret);\n> > +\n> > +/**\n> > + * \\fn 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> > +\tstop();\n> > +\n> > +/**\n> > + * \\fn 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 mention of IPA protocol made sense when the IPAInterface was the\n> same for all IPAs, but that's now outdated. *This* is the IPA protocol\n> for RPi :-) The documentation should be reworked accordingly. Generic\n> parts could be preserved in the IPA guide, to explain how to create an\n> IPA protocol.\n\nAh yes.\n\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> > +\tconfigure(CameraSensorInfo sensorInfo,\n> > +\t\t map streamConfig,\n> > +\t\t map entityControls,\n> > +\t\t ConfigInput ipaConfig)\n> > +\t\t=> (ConfigOutput results);\n> > +\n> > +/**\n> > + * \\fn 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> Same here, there's lots of generic documentation that doesn't belong to\n> a pipeline handler-specific file.\n> \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> > +\tmapBuffers(array buffers);\n> > +\n> > +/**\n> > + * \\fn 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> > + */\n> > +\tunmapBuffers(array ids);\n> > +\n> > +\t[async] signalStatReady(uint32 bufferId);\n> > +\t[async] signalQueueRequest(ControlList controls);\n> > +\t[async] signalIspPrepare(IspPreparePayload data);\n> > +};\n> > +\n> > +interface IPARPiEventInterface {\n> > +\tstatsMetadataComplete(uint32 bufferId, ControlList controls);\n> > +\trunIsp(uint32 bufferId);\n> > +\tembeddedComplete(uint32 bufferId);\n> > +\tsetIsp(ControlList controls);\n> > +\tsetStaggered(ControlList controls);\n> > +};\n> > +\n> > +\n> > +enum ConfigParameters {\n> > +\tRPI_IPA_CONFIG_LS_TABLE = 0x01,\n> > +\tRPI_IPA_CONFIG_STAGGERED_WRITE = 0x02,\n> > +\tRPI_IPA_CONFIG_SENSOR = 0x04,\n> > +\tRPI_IPA_CONFIG_DROP_FRAMES = 0x08,\n> \n> As this will be qualified by the ipa::rpi:: namespace, should we drop\n> the RPI_IPA_ prefix ?\n\nHere, yeah, we can do that.\n\n> While at it, would it make sense to move to our usual coding style of\n> CamelCase for enum values ?\n\nYeah.\n\n> > +};\n> > +\n> > +struct StaggeredWritePayload {\n> \n> s/Payload/Config/ ? Payload really sounds like a networking term, and\n> while data is serialized to a payload, that shouldn't appear in the API.\n> Actually, wouls SensorConfig be a better name ? The name \"staggered\n> write\" disappears with Niklas' patches that move the helper to the\n> libcamera core.\n\nSensorConfig it is.\n\n> > +\tuint32 gainDelay;\n> > +\tuint32 exposureDelay;\n> > +\tuint32 sensorMetadata;\n> > +};\n> > +\n> > +struct IspPreparePayload {\n> \n> s/IspPreparePayload/ISPConfig/ ?\n\nYep.\n\n> > +\tuint32 embeddedbufferId;\n> > +\tuint32 bayerbufferId;\n> > +};\n> > +\n> > +struct ConfigInput {\n> > +\tuint32 op;\n> \n> I think you can drop this field (and the RPI_IPA_CONFIG_LS_TABLE enum)\n> and rely on lsTableHandle being valid instead. It can be done on later\n> in the series, up to you.\n> \n> > +\tuint32 transform;\n> > +\tFileDescriptor lsTableHandle;\n> > +\tint32 lsTableHandleStatic = -1;\n> > +};\n> > +\n> > +struct ConfigOutput {\n> > +\tuint32 op;\n> \n> Here too I'd like to drop the op field if possible.\n> RPI_IPA_CONFIG_DROP_FRAMES can be dropped as it's always set. For\n> RPI_IPA_CONFIG_SENSOR, we can check if controls is empty.\n> RPI_IPA_CONFIG_STAGGERED_WRITE is the annoying one, as ideally it should\n> be implemented through support for nullable fields. Mojo supports this\n> by turning fields of struct type to pointers, but we don't, as we embed\n> them instead. I think it would make sense doing the same, but not yet,\n> as that would require too many changes to this series. In the meantime,\n> one simple option would be to ignore staggeredWriteResult in the\n> pipelien handler when !gainDelay && !exposureDelay, as that's an invalid\n> case.\n\nI'll change around the ops and the ops processing in v6.\n\n> > +\tStaggeredWritePayload staggeredWriteResult;\n> \n> staggeredWriteResult sounds a bit cryptic. If we rename the structure to\n> SensorConfig, sensorConfig would be a better field name.\n\nYeah.\n\n> > +\tControlList controls;\n> \n> Should this be named sensorControls ?\n\nI thought controls was just fine...\n\n> > +\tint32 dropFrameCount;\n> > +};\n> \n> Even if the mojo IDL doesn't require this, should the enums and struct\n> be moved before the interfaces ? I think it would improve readability.\n\nYeah.\n\n\nThanks,\n\nPaul","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 ABEDDBDB1F\n\tfor ;\n\tSat, 5 Dec 2020 09:20:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FC42635EF;\n\tSat, 5 Dec 2020 10:20:20 +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 90E0C60327\n\tfor ;\n\tSat, 5 Dec 2020 10:20:18 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E32562A4;\n\tSat, 5 Dec 2020 10:20:16 +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=\"MXkrsvQb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607160018;\n\tbh=Sq5KN4yN4GRNCzZT9bK1qNjMuH0Z0f6mS6EZgRKqTHE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MXkrsvQb9kNzxHNAeJOw/O8/MonD2y6afDphvRpSFv1JqNzjP2Ha1hr4KeDflu9EW\n\tKyIqcIR86UHCAdVqw8VVJ7DlBAmOWGVrlmeeMDYSvPHnfzfMkStN7IRehb6vXHjP5h\n\twW+KvsB9Ws60dS/ja7XUegMCJt6XiU0C49PYxf2w=","Date":"Sat, 5 Dec 2020 18:20:10 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20201205092010.GF2050@pyrite.rasen.tech>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-26-paul.elder@ideasonboard.com>\n\t<20201126150255.GO3905@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201126150255.GO3905@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 25/37] ipa: raspberrypi: Add mojom\n\tdata definition file","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14077,"web_url":"https://patchwork.libcamera.org/comment/14077/","msgid":"","date":"2020-12-05T14:34:09","subject":"Re: [libcamera-devel] [PATCH v4 25/37] ipa: raspberrypi: Add mojom\n\tdata definition file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Sat, Dec 05, 2020 at 06:20:10PM +0900, paul.elder@ideasonboard.com wrote:\n> On Thu, Nov 26, 2020 at 05:02:55PM +0200, Laurent Pinchart wrote:\n> > On Fri, Nov 06, 2020 at 07:36:55PM +0900, Paul Elder wrote:\n> > > Add a mojom data definition for raspberrypi pipeline handler's IPAs.\n> > > This is a direct translation of what the raspberrypi pipeline handler\n> > > and IPA was using before with IPAOperationData.\n> > \n> > s/was/were/\n> > \n> > > \n> > > Also move the enums from raspberrypi.h to raspberrypi.mojom\n> > > \n> > > Signed-off-by: Paul Elder \n> > > \n> > > ---\n> > > Changes in v4:\n> > > - rename IPARPiCallbackInterface to IPARPiEventInterface\n> > > \n> > > Changes in v3:\n> > > - remove stray comment about how our compiler will generate code\n> > > - add ipa.rpi namespace\n> > > - not ipa.RPi, because namespace conflict\n> > > - remove RPi prefix from struct and enum names\n> > > - add transform parameter to struct ConfigInput\n> > > \n> > > Changes in v2:\n> > > - rebased on \"libcamera: pipeline: ipa: raspberrypi: Rework drop frame\n> > > signalling\"\n> > > - add license\n> > > - move generic documentation to core.mojom\n> > > - move documentation from IPA interface\n> > > - customize the RPi IPA interface to make the pipeline and IPA code\n> > > cleaner\n> > > ---\n> > > include/libcamera/ipa/meson.build | 4 +-\n> > > include/libcamera/ipa/raspberrypi.h | 18 ---\n> > > include/libcamera/ipa/raspberrypi.mojom | 158 ++++++++++++++++++++++++\n> > > 3 files changed, 161 insertions(+), 19 deletions(-)\n> > > create mode 100644 include/libcamera/ipa/raspberrypi.mojom\n\n[snip]\n\n> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > > new file mode 100644\n> > > index 00000000..ec261569\n> > > --- /dev/null\n> > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > @@ -0,0 +1,158 @@\n\n[snip]\n\n> > > +struct ConfigOutput {\n> > > +\tuint32 op;\n> > \n> > Here too I'd like to drop the op field if possible.\n> > RPI_IPA_CONFIG_DROP_FRAMES can be dropped as it's always set. For\n> > RPI_IPA_CONFIG_SENSOR, we can check if controls is empty.\n> > RPI_IPA_CONFIG_STAGGERED_WRITE is the annoying one, as ideally it should\n> > be implemented through support for nullable fields. Mojo supports this\n> > by turning fields of struct type to pointers, but we don't, as we embed\n> > them instead. I think it would make sense doing the same, but not yet,\n> > as that would require too many changes to this series. In the meantime,\n> > one simple option would be to ignore staggeredWriteResult in the\n> > pipelien handler when !gainDelay && !exposureDelay, as that's an invalid\n> > case.\n> \n> I'll change around the ops and the ops processing in v6.\n> \n> > > +\tStaggeredWritePayload staggeredWriteResult;\n> > \n> > staggeredWriteResult sounds a bit cryptic. If we rename the structure to\n> > SensorConfig, sensorConfig would be a better field name.\n> \n> Yeah.\n> \n> > > +\tControlList controls;\n> > \n> > Should this be named sensorControls ?\n> \n> I thought controls was just fine...\n\n\"controls\" isn't bad :-) I thought it could be a bit more descriptive,\nreading the code would likely leave one wonder what controls these are.\nWe of course need to find a proper balance to avoid descriptive names\nthat would be 100 characters long :-) I trust you to decide what's best\nin this case.\n\n> > > +\tint32 dropFrameCount;\n> > > +};\n> > \n> > Even if the mojo IDL doesn't require this, should the enums and struct\n> > be moved before the interfaces ? I think it would improve readability.\n> \n> Yeah.","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 ED7C1BDB1F\n\tfor ;\n\tSat, 5 Dec 2020 14:34:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BA3A635F2;\n\tSat, 5 Dec 2020 15:34:12 +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 482DA635F0\n\tfor ;\n\tSat, 5 Dec 2020 15:34:11 +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 B0B40B95;\n\tSat, 5 Dec 2020 15:34: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=\"loT9hjgd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607178850;\n\tbh=tiEGgbXVsXm/BYUyG2MmhwlxsoydZWUD1hGtpQSpcnI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=loT9hjgdWWbxM2C6ByZYlRWuusq1ZUjIr4xcUAQlZ5xz1GURoLVJyf2J+EsR0wwkA\n\tBbPIKNudryt4k45kjj54sLHHSYq/exfi4bO+mnfC0YfWv0yYrd2eu3DWKBIT0V0f2H\n\tXMTQmmTrQX6TQ8BqiHOj5PPhmANZTFuA6G1IuNwU=","Date":"Sat, 5 Dec 2020 16:34:09 +0200","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-26-paul.elder@ideasonboard.com>\n\t<20201126150255.GO3905@pendragon.ideasonboard.com>\n\t<20201205092010.GF2050@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201205092010.GF2050@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v4 25/37] ipa: raspberrypi: Add mojom\n\tdata definition file","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]