[{"id":2773,"web_url":"https://patchwork.libcamera.org/comment/2773/","msgid":"<20191004055833.GA9925@pendragon.ideasonboard.com>","date":"2019-10-04T05:58:33","subject":"Re: [libcamera-devel] [PATCH v4 06/11] libcamera: ipa: Extend to\n\tsupport IPA interactions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Thu, Oct 03, 2019 at 07:49:36PM +0200, Niklas Söderlund wrote:\n> The IPA interface needs to support interactions with the pipeline, add\n> interfaces to control the sensor and handling of request ISP parameters\n> and statistics.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  include/ipa/ipa_interface.h             |  37 +++++++\n>  src/ipa/ipa_dummy.cpp                   |   7 +-\n>  src/libcamera/ipa_interface.cpp         | 132 ++++++++++++++++++++++++\n>  src/libcamera/proxy/ipa_proxy_linux.cpp |  14 ++-\n>  4 files changed, 181 insertions(+), 9 deletions(-)\n> \n> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> index 2c5eb1fd524311cb..2c715314c7a418f0 100644\n> --- a/include/ipa/ipa_interface.h\n> +++ b/include/ipa/ipa_interface.h\n> @@ -7,14 +7,51 @@\n>  #ifndef __LIBCAMERA_IPA_INTERFACE_H__\n>  #define __LIBCAMERA_IPA_INTERFACE_H__\n>  \n> +#include <map>\n> +#include <vector>\n> +\n> +#include <libcamera/buffer.h>\n> +#include <libcamera/controls.h>\n> +#include <libcamera/geometry.h>\n> +#include <libcamera/signal.h>\n> +\n> +#include \"v4l2_controls.h\"\n> +\n>  namespace libcamera {\n>  \n> +struct IPAStream {\n> +\tunsigned int pixelFormat;\n> +\tSize size;\n> +};\n> +\n> +struct IPABuffer {\n> +\tunsigned int id;\n> +\tunsigned int type;\n> +\tBufferMemory buffer;\n> +};\n> +\n> +struct IPAOperationData {\n> +\tunsigned int operation;\n> +\tstd::vector<uint32_t> data;\n> +\tstd::vector<ControlList> controls;\n> +\t/* \\todo: Add more vectors for data types used in pipa<->IPA interactions. */\n\nIf I'm not mistaken you use this to set sensor parameters. What's the\nreason for not including a V4L2ControlList already ? Do you plan to add\none later ? Or is the plan to find a way to pass V4L2 controls through a\nControlList ?\n\n> +};\n> +\n>  class IPAInterface\n>  {\n>  public:\n>  \tvirtual ~IPAInterface() {}\n>  \n>  \tvirtual int init() = 0;\n> +\n> +\tvirtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +\t\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) = 0;\n\nThe data types are getting a bit long, an alias might be nice, but I\nthink we'll end up refactoring this anyway, so it's fine for now in my\nopinion.\n\n> +\n> +\tvirtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;\n> +\tvirtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;\n\nI like this :-)\n\n> +\n> +\tvirtual void processEvent(const IPAOperationData &event) = 0;\n\nDo you think we should split queueRequest() to a dedicated method, as we\nknow we'll always need it ? Or do you think we should wait ? If so, when\ndo you think would be a good time ?\n\n> +\tSignal<const IPAOperationData &> queueFrameAction;\n>  };\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp\n> index 9d0cbdc8b1ad5565..08f74c4952f3923a 100644\n> --- a/src/ipa/ipa_dummy.cpp\n> +++ b/src/ipa/ipa_dummy.cpp\n> @@ -15,7 +15,12 @@ namespace libcamera {\n>  class IPADummy : public IPAInterface\n>  {\n>  public:\n> -\tint init();\n> +\tint init() override;\n> +\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}\n> +\tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> +\tvoid unmapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> +\tvoid processEvent(const IPAOperationData &event) override {}\n>  };\n>  \n>  int IPADummy::init()\n> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> index d7d8ca8881efcf66..c0f2004f3ec993b2 100644\n> --- a/src/libcamera/ipa_interface.cpp\n> +++ b/src/libcamera/ipa_interface.cpp\n> @@ -14,6 +14,68 @@\n>  \n>  namespace libcamera {\n>  \n> +\n\nOne blank line is enough.\n\n> +/**\n> + * \\struct IPAStream\n> + * \\brief Hold IPA description of a stream.\n\nI think you should detail this a bit. Maybe something like the following\n?\n\n * \\brief Stream configuration for the IPA API\n *\n * This structure mirrors StreamConfiguration for the IPA API. It stores\n * the configuration of a stream.\n\n> + */\n> +\n> +/**\n> + * \\var IPAStream::pixelFormat\n> + * \\brief The streams pixel format\n\ns/streams/stream's/ or just \"The pixel format\"\n\n> + */\n> +\n> +/**\n> + * \\var IPAStream::size\n> + * \\brief The streams size\n\nSame here.\n\n> + */\n> +\n> +/**\n> + * \\struct IPABuffer\n> + * \\brief Hold IPA description of a buffer\n> + */\n> +\n> +/**\n> + * \\var IPABuffer::id\n> + * \\brief The pipeline to IPA id cookie for the buffer\n> + */\n> +\n> +/**\n> + * \\var IPABuffer::type\n> + * \\brief The pipeline to IPA type for the buffer\n> + */\n> +\n> +/**\n> + * \\var IPABuffer::buffer\n> + * \\brief The hardware description of the buffer shared between pipeline and IPA\n> + */\n\nAll this is very terse. I know what this structure and its fields are\nused for because we've discussed it, but reading the documentation would\nleave the reader puzzled :-S\n\n> +\n> +/**\n> + * \\struct IPAOperationData\n> + * \\brief Hold data exchanged between pipeline and IPA\n\nJust drop the verb from the brief description. \"Data container for\nparameters to IPA operations\"\n\n> + *\n> + * This is the information carrier between pipeline and IPA. The is is used\n> + * to transport the pipeline specific protocol between the two. Core libcamera\n> + * components do not try to interpret the protocol and it's up to the pipeline\n> + * handler to uses the members of this struct in any way they see fit, and to\n> + * document it so multiple IPA implementations for the same pipeline may use it.\n\nSame here, I don't think this would make any sense to someone who hasn't\nfollowed the development closely.\n\n> + */\n> +\n> +/**\n> + * \\var IPAOperationData::operation\n> + * \\brief Intended as the operation code in the pipeline to IPA protocol\n> + */\n> +\n> +/**\n> + * \\var IPAOperationData::data\n> + * \\brief Intended as the generic data carrier in the pipeline to IPA protocol\n> + */\n> +\n> +/**\n> + * \\var IPAOperationData::controls\n> + * \\brief Intended as the ControlList carrier in the pipeline to IPA protocol\n> + */\n\nAnd same for the three fields too.\n\n> +\n>  /**\n>   * \\class IPAInterface\n>   * \\brief Interface for IPA implementation\n> @@ -24,4 +86,74 @@ namespace libcamera {\n>   * \\brief Initialise the IPAInterface\n>   */\n>  \n> +/**\n> + * \\fn IPAInterface::configure()\n> + * \\brief Configure the IPA stream and sensor settings\n> + * \\param[in] streamConfig List of stream configuration descriptions\n> + * \\param[in] entityControls List of controls provided by the pipeline entities\n> + *\n> + * This function is called when a pipeline attaches to an IPA to inform the IPA\n> + * of the streams and controls limits the entities(s) in the video pipeline\n> + * supports.\n\nThat's not correct, this is called when the streams are configured.\nFurthermore there's no \"attach\" operation defined anywhere. We really\nneed to improve this documentation I'm afraid. I'll leave the\ndocumentation below unreviewed.\n\n> + */\n> +\n> +/**\n> + * \\fn IPAInterface::mapBuffers()\n> + * \\brief Map the buffers shared by the pipeline to the IPA\n> + * \\param[in] buffers List of buffers to map\n> + *\n> + * This function is called when a pipeline handler wants to inform the IPA of\n> + * which buffers it has mapped which the IPA can make use of. All buffers shared\n> + * between these two object's needs to be shared using this function prior to\n> + * use.\n> + *\n> + * After the buffers have been initialized a specific buffer can be referenced\n> + * using the numerical \\a type and \\a id provided when the buffers where mapped.\n> + *\n> + * The numerical values for type and id used to describe a buffer have no\n> + * meaning in the core libcamera code and the interface is pipeline handler\n> + * specific.\n> + *\n> + * \\sa unmapBuffers()\n> + * \\todo Can we make this a generic function?\n> + */\n> +\n> +/**\n> + * \\fn IPAInterface::unmapBuffers()\n> + * \\brief Unmap the buffers shared by the pipeline to the IPA\n> + * \\param[in] buffers List of buffers to unmap\n> + *\n> + * This function is called when a pipeline handler wants to the IPA to unmap\n> + * all or some of the buffer it have mapped.\n> + *\n> + * \\sa mapBuffers()\n> + * \\todo Can we make this a generic function?\n> + */\n> +\n> +/**\n> + * \\fn IPAInterface::processEvent()\n> + * \\brief Process an event from the pipeline handler\n> + * \\param[in] event Event to process\n> + *\n> + * The pipeline handler can send events to the IPA to notify it of what is\n> + * going on. This is the entry point on the IPA side for those messages.\n> + *\n> + * The protocol between pipeline and IPA are hilly specific for each pipeline\n> + * and needs to be documented separately. Libcamera reserves no operation\n> + * numbers and make no attempt to interpret the protocol.\n> + */\n> +\n> +/**\n> + * \\fn IPAInterface::queueFrameAction()\n> + * \\brief Signal emitted when the IPA wish to queue a FrameAction\n> + * \\param[in] frame The frame number for the request\n> + * \\param[in] controls List of controls associated with the request\n> + *\n> + * This signal is emitted when the IPA wish 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> + * The IPA operation describing the frame action is passed as a parameter.\n> + */\n> +\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> index 62fcb529e1c7e4ff..14e8bb6067623fc6 100644\n> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> @@ -26,7 +26,12 @@ public:\n>  \tIPAProxyLinux(IPAModule *ipam);\n>  \t~IPAProxyLinux();\n>  \n> -\tint init();\n> +\tint init() override { return 0; }\n> +\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}\n> +\tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> +\tvoid unmapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> +\tvoid processEvent(const IPAOperationData &event) override {}\n\nVery clearly a hack :-) That's fine, we'll fix it.\n\n>  \n>  private:\n>  \tvoid readyRead(IPCUnixSocket *ipc);\n> @@ -36,13 +41,6 @@ private:\n>  \tIPCUnixSocket *socket_;\n>  };\n>  \n> -int IPAProxyLinux::init()\n> -{\n> -\tLOG(IPAProxy, Debug) << \"initializing IPA via dummy proxy!\";\n> -\n> -\treturn 0;\n> -}\n> -\n>  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)\n>  \t: proc_(nullptr), socket_(nullptr)\n>  {","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9519860E1E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  4 Oct 2019 07:58:47 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(modemcable151.96-160-184.mc.videotron.ca [184.160.96.151])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9251B2E5;\n\tFri,  4 Oct 2019 07:58:46 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570168727;\n\tbh=gc7os59AjNUgCB0haT1+HS3cpgKI7G5Aniz7taGf0Og=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RPbGIZP/JzA5ezMqQc8LOH1ILHLYBH7K0ZQlI5yJDOq5Kt0ovz84PE0Pc1orRtoLo\n\tq6W7Q0J/taaMwbOpMiUJ2VdY6i9HMfIQAH1CvGfQbXY1cGE2xvhjHL/o8U/RIB6a29\n\tFUpfzxKbnevwGk1hpBr9ReCc5elq+5g83uv4MBL0=","Date":"Fri, 4 Oct 2019 08:58:33 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191004055833.GA9925@pendragon.ideasonboard.com>","References":"<20191003174941.1296988-1-niklas.soderlund@ragnatech.se>\n\t<20191003174941.1296988-7-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191003174941.1296988-7-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 06/11] libcamera: ipa: Extend to\n\tsupport IPA interactions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 04 Oct 2019 05:58:47 -0000"}},{"id":2777,"web_url":"https://patchwork.libcamera.org/comment/2777/","msgid":"<20191004100320.zobxwtiifz2r2t3n@uno.localdomain>","date":"2019-10-04T10:03:20","subject":"Re: [libcamera-devel] [PATCH v4 06/11] libcamera: ipa: Extend to\n\tsupport IPA interactions","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas, Laurent,\n   a few comments\n\nOn Fri, Oct 04, 2019 at 08:58:33AM +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n>\n> Thank you for the patch.\n>\n> On Thu, Oct 03, 2019 at 07:49:36PM +0200, Niklas Söderlund wrote:\n> > The IPA interface needs to support interactions with the pipeline, add\n> > interfaces to control the sensor and handling of request ISP parameters\n> > and statistics.\n> >\n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  include/ipa/ipa_interface.h             |  37 +++++++\n> >  src/ipa/ipa_dummy.cpp                   |   7 +-\n> >  src/libcamera/ipa_interface.cpp         | 132 ++++++++++++++++++++++++\n> >  src/libcamera/proxy/ipa_proxy_linux.cpp |  14 ++-\n> >  4 files changed, 181 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> > index 2c5eb1fd524311cb..2c715314c7a418f0 100644\n> > --- a/include/ipa/ipa_interface.h\n> > +++ b/include/ipa/ipa_interface.h\n> > @@ -7,14 +7,51 @@\n> >  #ifndef __LIBCAMERA_IPA_INTERFACE_H__\n> >  #define __LIBCAMERA_IPA_INTERFACE_H__\n> >\n> > +#include <map>\n> > +#include <vector>\n> > +\n> > +#include <libcamera/buffer.h>\n> > +#include <libcamera/controls.h>\n> > +#include <libcamera/geometry.h>\n> > +#include <libcamera/signal.h>\n> > +\n> > +#include \"v4l2_controls.h\"\n> > +\n> >  namespace libcamera {\n> >\n> > +struct IPAStream {\n> > +\tunsigned int pixelFormat;\n> > +\tSize size;\n> > +};\n> > +\n> > +struct IPABuffer {\n> > +\tunsigned int id;\n> > +\tunsigned int type;\n> > +\tBufferMemory buffer;\n> > +};\n> > +\n> > +struct IPAOperationData {\n> > +\tunsigned int operation;\n> > +\tstd::vector<uint32_t> data;\n> > +\tstd::vector<ControlList> controls;\n> > +\t/* \\todo: Add more vectors for data types used in pipa<->IPA interactions. */\n>\n> If I'm not mistaken you use this to set sensor parameters. What's the\n> reason for not including a V4L2ControlList already ? Do you plan to add\n> one later ? Or is the plan to find a way to pass V4L2 controls through a\n> ControlList ?\n>\n\nJust to add the you include the v4l2_controls.h already to pass the\nV4L2ControlInfoMap at configure time, so I see no reasons why there\nshouldn't be a V4L2CotrolList here.\n\n> > +};\n> > +\n> >  class IPAInterface\n> >  {\n> >  public:\n> >  \tvirtual ~IPAInterface() {}\n> >\n> >  \tvirtual int init() = 0;\n> > +\n> > +\tvirtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +\t\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) = 0;\n>\n> The data types are getting a bit long, an alias might be nice, but I\n> think we'll end up refactoring this anyway, so it's fine for now in my\n> opinion.\n>\n\nHow would you keep track of the entities in the entityControls map ?\nAre pipelines and associated IPA supposed to know what entity 0,1,2..\nare ?\n\nShould we provide a <unsigned int cookie, std::string entityName> at\ninit() time ?\n\n> > +\n> > +\tvirtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;\n> > +\tvirtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;\n>\n> I like this :-)\n>\n> > +\n> > +\tvirtual void processEvent(const IPAOperationData &event) = 0;\n>\n> Do you think we should split queueRequest() to a dedicated method, as we\n> know we'll always need it ? Or do you think we should wait ? If so, when\n> do you think would be a good time ?\n>\n\nSame questions actually.\n\nI know the argument against have an explicit queueRequest() is that by\nusing the IPAOperationData argument of processEvent() to transport its\nparameters we have a more flexible interace, as pipe can use data[] to\ntransport arbitrary data.\n\nFor this first version I would anyhow keep queueRequest() separate, as\nit's a corner-stone operation of pipe/IPA interaction, with possibly a\ndata[] in its parameter with the canonical ControlList & so we get the\nsame amount of flexibility but we have the operation already broken\nout.\n\n----------------------------------------------------------------------\nProbably for later, don't let this delay this work, but:\ngoing forward, I still think we should find a way to define\noperations signature of IPAInterface that use a pure virtual base\nclasses, and ask pipe/IPA implementations to use their own derived types\nwith an associated serialization procedure.\n\nNot to push for what I had, but the pure virtual base class should\ninherit from a Serializable interface and derived classes should\ndefine their own serialize() and deserialize() operations. When going\nthrough IPC the operation's parameters would be serialized, when\nshort-circuiting and going through direct calls on the IPAInterface\nthe derived classes instances would be down-casted from the base class\npointer.\n\nSomething like:\n\nclass IPARequest : public Serializable\n{\nprivate:\n        unisgned int frame;\n        const ControlList &controls;\n};\n\nclass IPAInterface\n{\n        virtual int queueRequest(IPARequest *request);\n};\n\n--------------------------------------------------\n\nclass RKISP1Request : public IPARequest\n{\n        /* Augment the class with the required data/operations\n\n        /* Provide a serialize/deserialize operations. */\n        int serialize() override;\n        int deserialize(int8_t *data) override;\n};\n\nclass RKISP1IPA\n{\n        int queueRequest(IPARequest *request)\n        {\n                RKISP1Request *r = reinterpret_cast<RKISP1Request *>(request);\n                ....\n        };\n}\n---------------------------------------------------\n\nWhen going through IPC the IPAInterfaceWrapper would call\nthe virtual serialize/deserialize operations and reduce all to the\nequivalent of data[] when going through the C interface.\n\n---------------------------------------------------\n\n> > +\tSignal<const IPAOperationData &> queueFrameAction;\n> >  };\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp\n> > index 9d0cbdc8b1ad5565..08f74c4952f3923a 100644\n> > --- a/src/ipa/ipa_dummy.cpp\n> > +++ b/src/ipa/ipa_dummy.cpp\n> > @@ -15,7 +15,12 @@ namespace libcamera {\n> >  class IPADummy : public IPAInterface\n> >  {\n> >  public:\n> > -\tint init();\n> > +\tint init() override;\n> > +\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}\n> > +\tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> > +\tvoid unmapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> > +\tvoid processEvent(const IPAOperationData &event) override {}\n> >  };\n> >\n> >  int IPADummy::init()\n> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> > index d7d8ca8881efcf66..c0f2004f3ec993b2 100644\n> > --- a/src/libcamera/ipa_interface.cpp\n> > +++ b/src/libcamera/ipa_interface.cpp\n> > @@ -14,6 +14,68 @@\n> >\n> >  namespace libcamera {\n> >\n> > +\n>\n> One blank line is enough.\n>\n> > +/**\n> > + * \\struct IPAStream\n> > + * \\brief Hold IPA description of a stream.\n>\n> I think you should detail this a bit. Maybe something like the following\n> ?\n>\n>  * \\brief Stream configuration for the IPA API\n>  *\n>  * This structure mirrors StreamConfiguration for the IPA API. It stores\n>  * the configuration of a stream.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAStream::pixelFormat\n> > + * \\brief The streams pixel format\n>\n> s/streams/stream's/ or just \"The pixel format\"\n>\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAStream::size\n> > + * \\brief The streams size\n>\n> Same here.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\struct IPABuffer\n> > + * \\brief Hold IPA description of a buffer\n> > + */\n> > +\n> > +/**\n> > + * \\var IPABuffer::id\n> > + * \\brief The pipeline to IPA id cookie for the buffer\n> > + */\n> > +\n> > +/**\n> > + * \\var IPABuffer::type\n> > + * \\brief The pipeline to IPA type for the buffer\n> > + */\n> > +\n> > +/**\n> > + * \\var IPABuffer::buffer\n> > + * \\brief The hardware description of the buffer shared between pipeline and IPA\n> > + */\n>\n> All this is very terse. I know what this structure and its fields are\n> used for because we've discussed it, but reading the documentation would\n> leave the reader puzzled :-S\n>\n> > +\n> > +/**\n> > + * \\struct IPAOperationData\n> > + * \\brief Hold data exchanged between pipeline and IPA\n>\n> Just drop the verb from the brief description. \"Data container for\n> parameters to IPA operations\"\n>\n> > + *\n> > + * This is the information carrier between pipeline and IPA. The is is used\n> > + * to transport the pipeline specific protocol between the two. Core libcamera\n> > + * components do not try to interpret the protocol and it's up to the pipeline\n> > + * handler to uses the members of this struct in any way they see fit, and to\n> > + * document it so multiple IPA implementations for the same pipeline may use it.\n>\n> Same here, I don't think this would make any sense to someone who hasn't\n> followed the development closely.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAOperationData::operation\n> > + * \\brief Intended as the operation code in the pipeline to IPA protocol\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAOperationData::data\n> > + * \\brief Intended as the generic data carrier in the pipeline to IPA protocol\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAOperationData::controls\n> > + * \\brief Intended as the ControlList carrier in the pipeline to IPA protocol\n> > + */\n>\n> And same for the three fields too.\n>\n> > +\n> >  /**\n> >   * \\class IPAInterface\n> >   * \\brief Interface for IPA implementation\n> > @@ -24,4 +86,74 @@ namespace libcamera {\n> >   * \\brief Initialise the IPAInterface\n> >   */\n> >\n> > +/**\n> > + * \\fn IPAInterface::configure()\n> > + * \\brief Configure the IPA stream and sensor settings\n> > + * \\param[in] streamConfig List of stream configuration descriptions\n> > + * \\param[in] entityControls List of controls provided by the pipeline entities\n> > + *\n> > + * This function is called when a pipeline attaches to an IPA to inform the IPA\n> > + * of the streams and controls limits the entities(s) in the video pipeline\n> > + * supports.\n>\n> That's not correct, this is called when the streams are configured.\n> Furthermore there's no \"attach\" operation defined anywhere. We really\n> need to improve this documentation I'm afraid. I'll leave the\n> documentation below unreviewed.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn IPAInterface::mapBuffers()\n> > + * \\brief Map the buffers shared by the pipeline to the IPA\n> > + * \\param[in] buffers List of buffers to map\n> > + *\n> > + * This function is called when a pipeline handler wants to inform the IPA of\n> > + * which buffers it has mapped which the IPA can make use of. All buffers shared\n\ndouble which.\n\nI would\n\"This operation is used to inform the IPA module of the memory buffers\nset up by pipeline handlers and associate to each of them a numerical\ncookie id, that will be used to identify the buffer in all sequent\noperations.\"\n\n> > + * between these two object's needs to be shared using this function prior to\n> > + * use.\n> > + *\n> > + * After the buffers have been initialized a specific buffer can be referenced\n> > + * using the numerical \\a type and \\a id provided when the buffers where mapped.\n> > + *\n> > + * The numerical values for type and id used to describe a buffer have no\n> > + * meaning in the core libcamera code and the interface is pipeline handler\n> > + * specific.\n> > + *\n> > + * \\sa unmapBuffers()\n> > + * \\todo Can we make this a generic function?\n> > + */\n> > +\n> > +/**\n> > + * \\fn IPAInterface::unmapBuffers()\n> > + * \\brief Unmap the buffers shared by the pipeline to the IPA\n> > + * \\param[in] buffers List of buffers to unmap\n> > + *\n> > + * This function is called when a pipeline handler wants to the IPA to unmap\n> > + * all or some of the buffer it have mapped.\n> > + *\n\n\"remove the mapping set up with mapBuffers\"\n\n> > + * \\sa mapBuffers()\n> > + * \\todo Can we make this a generic function?\n> > + */\n> > +\n> > +/**\n> > + * \\fn IPAInterface::processEvent()\n> > + * \\brief Process an event from the pipeline handler\n> > + * \\param[in] event Event to process\n> > + *\n> > + * The pipeline handler can send events to the IPA to notify it of what is\n> > + * going on. This is the entry point on the IPA side for those messages.\n\n\"what is going on\" is pretty generic.\n\n\"This operation is used by pipeline handlers to inform the IPA\nmodule of events occurred during the on-going capture operation.\n\nEach \\a event notified by the pipeline handler with this method is\nhandled by the IPAModule which interprets the operation parameters in\nan implementation specific way, which needs to be separately\ndocumented.\"\n\nOr something like that...\n\n> > + *\n> > + * The protocol between pipeline and IPA are hilly specific for each pipeline\n\nhilly ?\n\n> > + * and needs to be documented separately. Libcamera reserves no operation\n> > + * numbers and make no attempt to interpret the protocol.\n> > + */\n> > +\n> > +/**\n> > + * \\fn IPAInterface::queueFrameAction()\n> > + * \\brief Signal emitted when the IPA wish to queue a FrameAction\n\n\"Queue an action associated with a frame to the pipeline handler\"\n\n> > + * \\param[in] frame The frame number for the request\n\ns/request/action\n\n> > + * \\param[in] controls List of controls associated with the request\n\ns/request/action\n\n> > + *\n> > + * This signal is emitted when the IPA wish 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> > + * The IPA operation describing the frame action is passed as a parameter.\n> > + */\n> > +\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > index 62fcb529e1c7e4ff..14e8bb6067623fc6 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > @@ -26,7 +26,12 @@ public:\n> >  \tIPAProxyLinux(IPAModule *ipam);\n> >  \t~IPAProxyLinux();\n> >\n> > -\tint init();\n> > +\tint init() override { return 0; }\n> > +\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}\n> > +\tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> > +\tvoid unmapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> > +\tvoid processEvent(const IPAOperationData &event) override {}\n>\n> Very clearly a hack :-) That's fine, we'll fix it.\n>\n> >\n> >  private:\n> >  \tvoid readyRead(IPCUnixSocket *ipc);\n> > @@ -36,13 +41,6 @@ private:\n> >  \tIPCUnixSocket *socket_;\n> >  };\n> >\n> > -int IPAProxyLinux::init()\n> > -{\n> > -\tLOG(IPAProxy, Debug) << \"initializing IPA via dummy proxy!\";\n> > -\n> > -\treturn 0;\n> > -}\n> > -\n> >  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)\n> >  \t: proc_(nullptr), socket_(nullptr)\n> >  {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 177F36101A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  4 Oct 2019 12:01:37 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 63972FF80A;\n\tFri,  4 Oct 2019 10:01:36 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 4 Oct 2019 12:03:20 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20191004100320.zobxwtiifz2r2t3n@uno.localdomain>","References":"<20191003174941.1296988-1-niklas.soderlund@ragnatech.se>\n\t<20191003174941.1296988-7-niklas.soderlund@ragnatech.se>\n\t<20191004055833.GA9925@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"5uhhsxxthegd36ij\"","Content-Disposition":"inline","In-Reply-To":"<20191004055833.GA9925@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 06/11] libcamera: ipa: Extend to\n\tsupport IPA interactions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 04 Oct 2019 10:01:37 -0000"}},{"id":2793,"web_url":"https://patchwork.libcamera.org/comment/2793/","msgid":"<20191005222717.GW1322@bigcity.dyn.berto.se>","date":"2019-10-05T22:27:17","subject":"Re: [libcamera-devel] [PATCH v4 06/11] libcamera: ipa: Extend to\n\tsupport IPA interactions","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo and Laurent,\n\nThanks for your feedback.\n\nOn 2019-10-04 12:03:20 +0200, Jacopo Mondi wrote:\n> Hi Niklas, Laurent,\n>    a few comments\n> \n> On Fri, Oct 04, 2019 at 08:58:33AM +0300, Laurent Pinchart wrote:\n> > Hi Niklas,\n> >\n> > Thank you for the patch.\n> >\n> > On Thu, Oct 03, 2019 at 07:49:36PM +0200, Niklas Söderlund wrote:\n> > > The IPA interface needs to support interactions with the pipeline, add\n> > > interfaces to control the sensor and handling of request ISP parameters\n> > > and statistics.\n> > >\n> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > > ---\n> > >  include/ipa/ipa_interface.h             |  37 +++++++\n> > >  src/ipa/ipa_dummy.cpp                   |   7 +-\n> > >  src/libcamera/ipa_interface.cpp         | 132 ++++++++++++++++++++++++\n> > >  src/libcamera/proxy/ipa_proxy_linux.cpp |  14 ++-\n> > >  4 files changed, 181 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> > > index 2c5eb1fd524311cb..2c715314c7a418f0 100644\n> > > --- a/include/ipa/ipa_interface.h\n> > > +++ b/include/ipa/ipa_interface.h\n> > > @@ -7,14 +7,51 @@\n> > >  #ifndef __LIBCAMERA_IPA_INTERFACE_H__\n> > >  #define __LIBCAMERA_IPA_INTERFACE_H__\n> > >\n> > > +#include <map>\n> > > +#include <vector>\n> > > +\n> > > +#include <libcamera/buffer.h>\n> > > +#include <libcamera/controls.h>\n> > > +#include <libcamera/geometry.h>\n> > > +#include <libcamera/signal.h>\n> > > +\n> > > +#include \"v4l2_controls.h\"\n> > > +\n> > >  namespace libcamera {\n> > >\n> > > +struct IPAStream {\n> > > +\tunsigned int pixelFormat;\n> > > +\tSize size;\n> > > +};\n> > > +\n> > > +struct IPABuffer {\n> > > +\tunsigned int id;\n> > > +\tunsigned int type;\n> > > +\tBufferMemory buffer;\n> > > +};\n> > > +\n> > > +struct IPAOperationData {\n> > > +\tunsigned int operation;\n> > > +\tstd::vector<uint32_t> data;\n> > > +\tstd::vector<ControlList> controls;\n> > > +\t/* \\todo: Add more vectors for data types used in pipa<->IPA interactions. */\n> >\n> > If I'm not mistaken you use this to set sensor parameters. What's the\n> > reason for not including a V4L2ControlList already ? Do you plan to add\n> > one later ? Or is the plan to find a way to pass V4L2 controls through a\n> > ControlList ?\n> >\n> \n> Just to add the you include the v4l2_controls.h already to pass the\n> V4L2ControlInfoMap at configure time, so I see no reasons why there\n> shouldn't be a V4L2CotrolList here.\n> \n\nIt was debated on irc while this series was written that V4L2CotrolList \nwould be replaced by a ControlList. As the result of that was not clear \nI opted to keep what already was in this series. I will switch back to \nV4L2CotrolList in next version.\n\n> > > +};\n> > > +\n> > >  class IPAInterface\n> > >  {\n> > >  public:\n> > >  \tvirtual ~IPAInterface() {}\n> > >\n> > >  \tvirtual int init() = 0;\n> > > +\n> > > +\tvirtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +\t\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) = 0;\n> >\n> > The data types are getting a bit long, an alias might be nice, but I\n> > think we'll end up refactoring this anyway, so it's fine for now in my\n> > opinion.\n> >\n> \n> How would you keep track of the entities in the entityControls map ?\n> Are pipelines and associated IPA supposed to know what entity 0,1,2..\n> are ?\n\nThat is up to each pipeline handler to specify, which numerical ID will \nrepresent which media entity. This of course needs to be documented in a \npipeline/IPA protocol description.\n\n> \n> Should we provide a <unsigned int cookie, std::string entityName> at\n> init() time ?\n\nI think that is a bad idea. If we want to use the entity name instead of \na numerical id we can make the map<std::string, V4L2ControlInfoMap>. I'm \nopen for this.\n\n> \n> > > +\n> > > +\tvirtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;\n> > > +\tvirtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;\n> >\n> > I like this :-)\n> >\n> > > +\n> > > +\tvirtual void processEvent(const IPAOperationData &event) = 0;\n> >\n> > Do you think we should split queueRequest() to a dedicated method, as we\n> > know we'll always need it ? Or do you think we should wait ? If so, when\n> > do you think would be a good time ?\n> >\n> \n> Same questions actually.\n\nWe had this in v3, then it was argued that there might be pipelines that \nneeds more information than frame number and the request control list.  \nAnd that queueRequest() should be merged with the new processEvent() \noperation.\n\nI have no strong feelings and can se pros and cons with both. But the \nkiller for me as we are still debating this is that it's easier to go \nfrom processEvent() -> queueRequest() later then the other way around. I \nwould prefer to keep this as is.\n\n> \n> I know the argument against have an explicit queueRequest() is that by\n> using the IPAOperationData argument of processEvent() to transport its\n> parameters we have a more flexible interace, as pipe can use data[] to\n> transport arbitrary data.\n> \n> For this first version I would anyhow keep queueRequest() separate, as\n> it's a corner-stone operation of pipe/IPA interaction, with possibly a\n> data[] in its parameter with the canonical ControlList & so we get the\n> same amount of flexibility but we have the operation already broken\n> out.\n> \n> ----------------------------------------------------------------------\n> Probably for later, don't let this delay this work, but:\n> going forward, I still think we should find a way to define\n> operations signature of IPAInterface that use a pure virtual base\n> classes, and ask pipe/IPA implementations to use their own derived types\n> with an associated serialization procedure.\n> \n> Not to push for what I had, but the pure virtual base class should\n> inherit from a Serializable interface and derived classes should\n> define their own serialize() and deserialize() operations. When going\n> through IPC the operation's parameters would be serialized, when\n> short-circuiting and going through direct calls on the IPAInterface\n> the derived classes instances would be down-casted from the base class\n> pointer.\n> \n> Something like:\n> \n> class IPARequest : public Serializable\n> {\n> private:\n>         unisgned int frame;\n>         const ControlList &controls;\n> };\n> \n> class IPAInterface\n> {\n>         virtual int queueRequest(IPARequest *request);\n> };\n> \n> --------------------------------------------------\n> \n> class RKISP1Request : public IPARequest\n> {\n>         /* Augment the class with the required data/operations\n> \n>         /* Provide a serialize/deserialize operations. */\n>         int serialize() override;\n>         int deserialize(int8_t *data) override;\n> };\n> \n> class RKISP1IPA\n> {\n>         int queueRequest(IPARequest *request)\n>         {\n>                 RKISP1Request *r = reinterpret_cast<RKISP1Request *>(request);\n>                 ....\n>         };\n> }\n> ---------------------------------------------------\n> \n> When going through IPC the IPAInterfaceWrapper would call\n> the virtual serialize/deserialize operations and reduce all to the\n> equivalent of data[] when going through the C interface.\n> \n> ---------------------------------------------------\n> \n> > > +\tSignal<const IPAOperationData &> queueFrameAction;\n> > >  };\n> > >\n> > >  } /* namespace libcamera */\n> > > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp\n> > > index 9d0cbdc8b1ad5565..08f74c4952f3923a 100644\n> > > --- a/src/ipa/ipa_dummy.cpp\n> > > +++ b/src/ipa/ipa_dummy.cpp\n> > > @@ -15,7 +15,12 @@ namespace libcamera {\n> > >  class IPADummy : public IPAInterface\n> > >  {\n> > >  public:\n> > > -\tint init();\n> > > +\tint init() override;\n> > > +\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}\n> > > +\tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> > > +\tvoid unmapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> > > +\tvoid processEvent(const IPAOperationData &event) override {}\n> > >  };\n> > >\n> > >  int IPADummy::init()\n> > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> > > index d7d8ca8881efcf66..c0f2004f3ec993b2 100644\n> > > --- a/src/libcamera/ipa_interface.cpp\n> > > +++ b/src/libcamera/ipa_interface.cpp\n> > > @@ -14,6 +14,68 @@\n> > >\n> > >  namespace libcamera {\n> > >\n> > > +\n> >\n> > One blank line is enough.\n> >\n> > > +/**\n> > > + * \\struct IPAStream\n> > > + * \\brief Hold IPA description of a stream.\n> >\n> > I think you should detail this a bit. Maybe something like the following\n> > ?\n> >\n> >  * \\brief Stream configuration for the IPA API\n> >  *\n> >  * This structure mirrors StreamConfiguration for the IPA API. It stores\n> >  * the configuration of a stream.\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPAStream::pixelFormat\n> > > + * \\brief The streams pixel format\n> >\n> > s/streams/stream's/ or just \"The pixel format\"\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPAStream::size\n> > > + * \\brief The streams size\n> >\n> > Same here.\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\struct IPABuffer\n> > > + * \\brief Hold IPA description of a buffer\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPABuffer::id\n> > > + * \\brief The pipeline to IPA id cookie for the buffer\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPABuffer::type\n> > > + * \\brief The pipeline to IPA type for the buffer\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPABuffer::buffer\n> > > + * \\brief The hardware description of the buffer shared between pipeline and IPA\n> > > + */\n> >\n> > All this is very terse. I know what this structure and its fields are\n> > used for because we've discussed it, but reading the documentation would\n> > leave the reader puzzled :-S\n> >\n> > > +\n> > > +/**\n> > > + * \\struct IPAOperationData\n> > > + * \\brief Hold data exchanged between pipeline and IPA\n> >\n> > Just drop the verb from the brief description. \"Data container for\n> > parameters to IPA operations\"\n> >\n> > > + *\n> > > + * This is the information carrier between pipeline and IPA. The is is used\n> > > + * to transport the pipeline specific protocol between the two. Core libcamera\n> > > + * components do not try to interpret the protocol and it's up to the pipeline\n> > > + * handler to uses the members of this struct in any way they see fit, and to\n> > > + * document it so multiple IPA implementations for the same pipeline may use it.\n> >\n> > Same here, I don't think this would make any sense to someone who hasn't\n> > followed the development closely.\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPAOperationData::operation\n> > > + * \\brief Intended as the operation code in the pipeline to IPA protocol\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPAOperationData::data\n> > > + * \\brief Intended as the generic data carrier in the pipeline to IPA protocol\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPAOperationData::controls\n> > > + * \\brief Intended as the ControlList carrier in the pipeline to IPA protocol\n> > > + */\n> >\n> > And same for the three fields too.\n> >\n> > > +\n> > >  /**\n> > >   * \\class IPAInterface\n> > >   * \\brief Interface for IPA implementation\n> > > @@ -24,4 +86,74 @@ namespace libcamera {\n> > >   * \\brief Initialise the IPAInterface\n> > >   */\n> > >\n> > > +/**\n> > > + * \\fn IPAInterface::configure()\n> > > + * \\brief Configure the IPA stream and sensor settings\n> > > + * \\param[in] streamConfig List of stream configuration descriptions\n> > > + * \\param[in] entityControls List of controls provided by the pipeline entities\n> > > + *\n> > > + * This function is called when a pipeline attaches to an IPA to inform the IPA\n> > > + * of the streams and controls limits the entities(s) in the video pipeline\n> > > + * supports.\n> >\n> > That's not correct, this is called when the streams are configured.\n> > Furthermore there's no \"attach\" operation defined anywhere. We really\n> > need to improve this documentation I'm afraid. I'll leave the\n> > documentation below unreviewed.\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn IPAInterface::mapBuffers()\n> > > + * \\brief Map the buffers shared by the pipeline to the IPA\n> > > + * \\param[in] buffers List of buffers to map\n> > > + *\n> > > + * This function is called when a pipeline handler wants to inform the IPA of\n> > > + * which buffers it has mapped which the IPA can make use of. All buffers shared\n> \n> double which.\n> \n> I would\n> \"This operation is used to inform the IPA module of the memory buffers\n> set up by pipeline handlers and associate to each of them a numerical\n> cookie id, that will be used to identify the buffer in all sequent\n> operations.\"\n> \n> > > + * between these two object's needs to be shared using this function prior to\n> > > + * use.\n> > > + *\n> > > + * After the buffers have been initialized a specific buffer can be referenced\n> > > + * using the numerical \\a type and \\a id provided when the buffers where mapped.\n> > > + *\n> > > + * The numerical values for type and id used to describe a buffer have no\n> > > + * meaning in the core libcamera code and the interface is pipeline handler\n> > > + * specific.\n> > > + *\n> > > + * \\sa unmapBuffers()\n> > > + * \\todo Can we make this a generic function?\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn IPAInterface::unmapBuffers()\n> > > + * \\brief Unmap the buffers shared by the pipeline to the IPA\n> > > + * \\param[in] buffers List of buffers to unmap\n> > > + *\n> > > + * This function is called when a pipeline handler wants to the IPA to unmap\n> > > + * all or some of the buffer it have mapped.\n> > > + *\n> \n> \"remove the mapping set up with mapBuffers\"\n> \n> > > + * \\sa mapBuffers()\n> > > + * \\todo Can we make this a generic function?\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn IPAInterface::processEvent()\n> > > + * \\brief Process an event from the pipeline handler\n> > > + * \\param[in] event Event to process\n> > > + *\n> > > + * The pipeline handler can send events to the IPA to notify it of what is\n> > > + * going on. This is the entry point on the IPA side for those messages.\n> \n> \"what is going on\" is pretty generic.\n> \n> \"This operation is used by pipeline handlers to inform the IPA\n> module of events occurred during the on-going capture operation.\n> \n> Each \\a event notified by the pipeline handler with this method is\n> handled by the IPAModule which interprets the operation parameters in\n> an implementation specific way, which needs to be separately\n> documented.\"\n> \n> Or something like that...\n> \n> > > + *\n> > > + * The protocol between pipeline and IPA are hilly specific for each pipeline\n> \n> hilly ?\n> \n> > > + * and needs to be documented separately. Libcamera reserves no operation\n> > > + * numbers and make no attempt to interpret the protocol.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn IPAInterface::queueFrameAction()\n> > > + * \\brief Signal emitted when the IPA wish to queue a FrameAction\n> \n> \"Queue an action associated with a frame to the pipeline handler\"\n> \n> > > + * \\param[in] frame The frame number for the request\n> \n> s/request/action\n> \n> > > + * \\param[in] controls List of controls associated with the request\n> \n> s/request/action\n> \n> > > + *\n> > > + * This signal is emitted when the IPA wish 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> > > + * The IPA operation describing the frame action is passed as a parameter.\n> > > + */\n> > > +\n> > >  } /* namespace libcamera */\n> > > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > > index 62fcb529e1c7e4ff..14e8bb6067623fc6 100644\n> > > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > > @@ -26,7 +26,12 @@ public:\n> > >  \tIPAProxyLinux(IPAModule *ipam);\n> > >  \t~IPAProxyLinux();\n> > >\n> > > -\tint init();\n> > > +\tint init() override { return 0; }\n> > > +\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}\n> > > +\tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> > > +\tvoid unmapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> > > +\tvoid processEvent(const IPAOperationData &event) override {}\n> >\n> > Very clearly a hack :-) That's fine, we'll fix it.\n> >\n> > >\n> > >  private:\n> > >  \tvoid readyRead(IPCUnixSocket *ipc);\n> > > @@ -36,13 +41,6 @@ private:\n> > >  \tIPCUnixSocket *socket_;\n> > >  };\n> > >\n> > > -int IPAProxyLinux::init()\n> > > -{\n> > > -\tLOG(IPAProxy, Debug) << \"initializing IPA via dummy proxy!\";\n> > > -\n> > > -\treturn 0;\n> > > -}\n> > > -\n> > >  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)\n> > >  \t: proc_(nullptr), socket_(nullptr)\n> > >  {\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16FD96165B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  6 Oct 2019 00:27:21 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id f5so9930369ljg.8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 05 Oct 2019 15:27:21 -0700 (PDT)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tt3sm1965198lfd.92.2019.10.05.15.27.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 05 Oct 2019 15:27:18 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=SrGhDnEKVvOCEXGXWpwtQ/U8qUBblHUZmp4h5v6pSqU=;\n\tb=lGr7MjN37PfO1I83XnGoTov3Wf3NhnlyHy1kb3bqCh7Lz3aipUQ/qrcHLgd1Hhj92W\n\tkEF+cIWTot9KkcPAKWn3VohD+HEF6/rDARhhHgiRrSs69QUR/1S7dF5hc6j29vqcaR7k\n\tsWJCl66wldAu1cqg4DsKMV9I4IIB4QK/8aMJvnrFqX6Th22q2wzO0M6T2IjA23kZnO21\n\tcxIGnrK0jUW+Q/j+QSx1w4U8vjjYVNiv9RPxZpR17f3WUMA2+wfmiZgyRWYu/kbs9UCw\n\tGfXgI01+kQs+XoAgHgQc/t0ygJWOpICtsEY5CbmTWsWE6FRXc/fECN06IWRKICUG8qhF\n\tKWug==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=SrGhDnEKVvOCEXGXWpwtQ/U8qUBblHUZmp4h5v6pSqU=;\n\tb=ckvAuOXKaa42FJbL5qZfW3Or9SQ9d0o01J15gJ4HbF2EHYRie/2khHlkrp5MRYnGcu\n\tz2wcpGJgPujk01rdLLTjn7J572DL0D0mdPr3StrAuHRZ1a54Z+2dBmcaM9nwVfwdkklk\n\t5Wo+2a0SK8jROJERD1Ab6ksMVUr+djOH1kVxDed1ZsSb01qfuW3dIOmAjx+G7ycoSA1s\n\t/frkm4/CuAVOvmsCkhq6ix+8NDhClVHIAmSBFbmQf5UpQzsGCN8JZOp3qAFzqp7Cc6AM\n\tDwOsj0LSwTl6QhymFpsUbIVj/kjMm/cfY4l/U++pxAcq4/jhXS7LgWdZ2pLt6Ao5WBiF\n\tS6RA==","X-Gm-Message-State":"APjAAAXulgSvN6+T0SVlukoMcUdrYL2DJujKhNxo6oe/66ZMgrvrMdwO\n\tvQCx8k0wphQgcdWUzuzGbNKjKQ==","X-Google-Smtp-Source":"APXvYqxSC3KVzTGKejI1RuswJZHCroGU3KF1yXU5MYnCSEn+2g7xO0DvBU38rAIAHIoI6JrTSh6XEA==","X-Received":"by 2002:a2e:a17b:: with SMTP id\n\tu27mr13101848ljl.65.1570314439761; \n\tSat, 05 Oct 2019 15:27:19 -0700 (PDT)","Date":"Sun, 6 Oct 2019 00:27:17 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>,\n\tLaurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191005222717.GW1322@bigcity.dyn.berto.se>","References":"<20191003174941.1296988-1-niklas.soderlund@ragnatech.se>\n\t<20191003174941.1296988-7-niklas.soderlund@ragnatech.se>\n\t<20191004055833.GA9925@pendragon.ideasonboard.com>\n\t<20191004100320.zobxwtiifz2r2t3n@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191004100320.zobxwtiifz2r2t3n@uno.localdomain>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v4 06/11] libcamera: ipa: Extend to\n\tsupport IPA interactions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 05 Oct 2019 22:27:21 -0000"}},{"id":2794,"web_url":"https://patchwork.libcamera.org/comment/2794/","msgid":"<20191005224057.GA20839@pendragon.ideasonboard.com>","date":"2019-10-05T22:40:57","subject":"Re: [libcamera-devel] [PATCH v4 06/11] libcamera: ipa: Extend to\n\tsupport IPA interactions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Sun, Oct 06, 2019 at 12:27:17AM +0200, Niklas Söderlund wrote:\n> On 2019-10-04 12:03:20 +0200, Jacopo Mondi wrote:\n> > On Fri, Oct 04, 2019 at 08:58:33AM +0300, Laurent Pinchart wrote:\n> >> On Thu, Oct 03, 2019 at 07:49:36PM +0200, Niklas Söderlund wrote:\n> >>> The IPA interface needs to support interactions with the pipeline, add\n> >>> interfaces to control the sensor and handling of request ISP parameters\n> >>> and statistics.\n> >>>\n> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> >>> ---\n> >>>  include/ipa/ipa_interface.h             |  37 +++++++\n> >>>  src/ipa/ipa_dummy.cpp                   |   7 +-\n> >>>  src/libcamera/ipa_interface.cpp         | 132 ++++++++++++++++++++++++\n> >>>  src/libcamera/proxy/ipa_proxy_linux.cpp |  14 ++-\n> >>>  4 files changed, 181 insertions(+), 9 deletions(-)\n> >>>\n> >>> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> >>> index 2c5eb1fd524311cb..2c715314c7a418f0 100644\n> >>> --- a/include/ipa/ipa_interface.h\n> >>> +++ b/include/ipa/ipa_interface.h\n> >>> @@ -7,14 +7,51 @@\n> >>>  #ifndef __LIBCAMERA_IPA_INTERFACE_H__\n> >>>  #define __LIBCAMERA_IPA_INTERFACE_H__\n> >>>\n> >>> +#include <map>\n> >>> +#include <vector>\n> >>> +\n> >>> +#include <libcamera/buffer.h>\n> >>> +#include <libcamera/controls.h>\n> >>> +#include <libcamera/geometry.h>\n> >>> +#include <libcamera/signal.h>\n> >>> +\n> >>> +#include \"v4l2_controls.h\"\n> >>> +\n> >>>  namespace libcamera {\n> >>>\n> >>> +struct IPAStream {\n> >>> +\tunsigned int pixelFormat;\n> >>> +\tSize size;\n> >>> +};\n> >>> +\n> >>> +struct IPABuffer {\n> >>> +\tunsigned int id;\n> >>> +\tunsigned int type;\n> >>> +\tBufferMemory buffer;\n> >>> +};\n> >>> +\n> >>> +struct IPAOperationData {\n> >>> +\tunsigned int operation;\n> >>> +\tstd::vector<uint32_t> data;\n> >>> +\tstd::vector<ControlList> controls;\n> >>> +\t/* \\todo: Add more vectors for data types used in pipa<->IPA interactions. */\n> >>\n> >> If I'm not mistaken you use this to set sensor parameters. What's the\n> >> reason for not including a V4L2ControlList already ? Do you plan to add\n> >> one later ? Or is the plan to find a way to pass V4L2 controls through a\n> >> ControlList ?\n> > \n> > Just to add the you include the v4l2_controls.h already to pass the\n> > V4L2ControlInfoMap at configure time, so I see no reasons why there\n> > shouldn't be a V4L2CotrolList here.\n> \n> It was debated on irc while this series was written that V4L2CotrolList \n> would be replaced by a ControlList. As the result of that was not clear \n> I opted to keep what already was in this series. I will switch back to \n> V4L2CotrolList in next version.\n> \n> >>> +};\n> >>> +\n> >>>  class IPAInterface\n> >>>  {\n> >>>  public:\n> >>>  \tvirtual ~IPAInterface() {}\n> >>>\n> >>>  \tvirtual int init() = 0;\n> >>> +\n> >>> +\tvirtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> >>> +\t\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) = 0;\n> >>\n> >> The data types are getting a bit long, an alias might be nice, but I\n> >> think we'll end up refactoring this anyway, so it's fine for now in my\n> >> opinion.\n> > \n> > How would you keep track of the entities in the entityControls map ?\n> > Are pipelines and associated IPA supposed to know what entity 0,1,2..\n> > are ?\n> \n> That is up to each pipeline handler to specify, which numerical ID will \n> represent which media entity. This of course needs to be documented in a \n> pipeline/IPA protocol description.\n> \n> > Should we provide a <unsigned int cookie, std::string entityName> at\n> > init() time ?\n> \n> I think that is a bad idea. If we want to use the entity name instead of \n> a numerical id we can make the map<std::string, V4L2ControlInfoMap>. I'm \n> open for this.\n> \n> >>> +\n> >>> +\tvirtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;\n> >>> +\tvirtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;\n> >>\n> >> I like this :-)\n> >>\n> >>> +\n> >>> +\tvirtual void processEvent(const IPAOperationData &event) = 0;\n> >>\n> >> Do you think we should split queueRequest() to a dedicated method, as we\n> >> know we'll always need it ? Or do you think we should wait ? If so, when\n> >> do you think would be a good time ?\n> > \n> > Same questions actually.\n> \n> We had this in v3, then it was argued that there might be pipelines that \n> needs more information than frame number and the request control list.  \n> And that queueRequest() should be merged with the new processEvent() \n> operation.\n> \n> I have no strong feelings and can se pros and cons with both. But the \n> killer for me as we are still debating this is that it's easier to go \n> from processEvent() -> queueRequest() later then the other way around. I \n> would prefer to keep this as is.\n> \n> > I know the argument against have an explicit queueRequest() is that by\n> > using the IPAOperationData argument of processEvent() to transport its\n> > parameters we have a more flexible interace, as pipe can use data[] to\n> > transport arbitrary data.\n> > \n> > For this first version I would anyhow keep queueRequest() separate, as\n> > it's a corner-stone operation of pipe/IPA interaction, with possibly a\n> > data[] in its parameter with the canonical ControlList & so we get the\n> > same amount of flexibility but we have the operation already broken\n> > out.\n> > \n> > ----------------------------------------------------------------------\n> > Probably for later, don't let this delay this work, but:\n> > going forward, I still think we should find a way to define\n> > operations signature of IPAInterface that use a pure virtual base\n> > classes, and ask pipe/IPA implementations to use their own derived types\n> > with an associated serialization procedure.\n> > \n> > Not to push for what I had, but the pure virtual base class should\n> > inherit from a Serializable interface and derived classes should\n> > define their own serialize() and deserialize() operations. When going\n> > through IPC the operation's parameters would be serialized, when\n> > short-circuiting and going through direct calls on the IPAInterface\n> > the derived classes instances would be down-casted from the base class\n> > pointer.\n> > \n> > Something like:\n> > \n> > class IPARequest : public Serializable\n> > {\n> > private:\n> >         unisgned int frame;\n> >         const ControlList &controls;\n> > };\n> > \n> > class IPAInterface\n> > {\n> >         virtual int queueRequest(IPARequest *request);\n> > };\n> > \n> > --------------------------------------------------\n> > \n> > class RKISP1Request : public IPARequest\n> > {\n> >         /* Augment the class with the required data/operations\n> > \n> >         /* Provide a serialize/deserialize operations. */\n> >         int serialize() override;\n> >         int deserialize(int8_t *data) override;\n> > };\n> > \n> > class RKISP1IPA\n> > {\n> >         int queueRequest(IPARequest *request)\n> >         {\n> >                 RKISP1Request *r = reinterpret_cast<RKISP1Request *>(request);\n\nThis should be a static_cast, not a reinterpret_cast.\n\n> >                 ....\n> >         };\n> > }\n> > ---------------------------------------------------\n> > \n> > When going through IPC the IPAInterfaceWrapper would call\n> > the virtual serialize/deserialize operations and reduce all to the\n> > equivalent of data[] when going through the C interface.\n> > \n> > ---------------------------------------------------\n\nIt's not a bad idea on paper, let me sleep over it :-)\n\n> >>> +\tSignal<const IPAOperationData &> queueFrameAction;\n> >>>  };\n> >>>\n> >>>  } /* namespace libcamera */\n> >>> diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp\n> >>> index 9d0cbdc8b1ad5565..08f74c4952f3923a 100644\n> >>> --- a/src/ipa/ipa_dummy.cpp\n> >>> +++ b/src/ipa/ipa_dummy.cpp\n> >>> @@ -15,7 +15,12 @@ namespace libcamera {\n> >>>  class IPADummy : public IPAInterface\n> >>>  {\n> >>>  public:\n> >>> -\tint init();\n> >>> +\tint init() override;\n> >>> +\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> >>> +\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}\n> >>> +\tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> >>> +\tvoid unmapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> >>> +\tvoid processEvent(const IPAOperationData &event) override {}\n> >>>  };\n> >>>\n> >>>  int IPADummy::init()\n> >>> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> >>> index d7d8ca8881efcf66..c0f2004f3ec993b2 100644\n> >>> --- a/src/libcamera/ipa_interface.cpp\n> >>> +++ b/src/libcamera/ipa_interface.cpp\n> >>> @@ -14,6 +14,68 @@\n> >>>\n> >>>  namespace libcamera {\n> >>>\n> >>> +\n> >>\n> >> One blank line is enough.\n> >>\n> >>> +/**\n> >>> + * \\struct IPAStream\n> >>> + * \\brief Hold IPA description of a stream.\n> >>\n> >> I think you should detail this a bit. Maybe something like the following\n> >> ?\n> >>\n> >>  * \\brief Stream configuration for the IPA API\n> >>  *\n> >>  * This structure mirrors StreamConfiguration for the IPA API. It stores\n> >>  * the configuration of a stream.\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var IPAStream::pixelFormat\n> >>> + * \\brief The streams pixel format\n> >>\n> >> s/streams/stream's/ or just \"The pixel format\"\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var IPAStream::size\n> >>> + * \\brief The streams size\n> >>\n> >> Same here.\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\struct IPABuffer\n> >>> + * \\brief Hold IPA description of a buffer\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var IPABuffer::id\n> >>> + * \\brief The pipeline to IPA id cookie for the buffer\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var IPABuffer::type\n> >>> + * \\brief The pipeline to IPA type for the buffer\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var IPABuffer::buffer\n> >>> + * \\brief The hardware description of the buffer shared between pipeline and IPA\n> >>> + */\n> >>\n> >> All this is very terse. I know what this structure and its fields are\n> >> used for because we've discussed it, but reading the documentation would\n> >> leave the reader puzzled :-S\n> >>\n> >>> +\n> >>> +/**\n> >>> + * \\struct IPAOperationData\n> >>> + * \\brief Hold data exchanged between pipeline and IPA\n> >>\n> >> Just drop the verb from the brief description. \"Data container for\n> >> parameters to IPA operations\"\n> >>\n> >>> + *\n> >>> + * This is the information carrier between pipeline and IPA. The is is used\n> >>> + * to transport the pipeline specific protocol between the two. Core libcamera\n> >>> + * components do not try to interpret the protocol and it's up to the pipeline\n> >>> + * handler to uses the members of this struct in any way they see fit, and to\n> >>> + * document it so multiple IPA implementations for the same pipeline may use it.\n> >>\n> >> Same here, I don't think this would make any sense to someone who hasn't\n> >> followed the development closely.\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var IPAOperationData::operation\n> >>> + * \\brief Intended as the operation code in the pipeline to IPA protocol\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var IPAOperationData::data\n> >>> + * \\brief Intended as the generic data carrier in the pipeline to IPA protocol\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var IPAOperationData::controls\n> >>> + * \\brief Intended as the ControlList carrier in the pipeline to IPA protocol\n> >>> + */\n> >>\n> >> And same for the three fields too.\n> >>\n> >>> +\n> >>>  /**\n> >>>   * \\class IPAInterface\n> >>>   * \\brief Interface for IPA implementation\n> >>> @@ -24,4 +86,74 @@ namespace libcamera {\n> >>>   * \\brief Initialise the IPAInterface\n> >>>   */\n> >>>\n> >>> +/**\n> >>> + * \\fn IPAInterface::configure()\n> >>> + * \\brief Configure the IPA stream and sensor settings\n> >>> + * \\param[in] streamConfig List of stream configuration descriptions\n> >>> + * \\param[in] entityControls List of controls provided by the pipeline entities\n> >>> + *\n> >>> + * This function is called when a pipeline attaches to an IPA to inform the IPA\n> >>> + * of the streams and controls limits the entities(s) in the video pipeline\n> >>> + * supports.\n> >>\n> >> That's not correct, this is called when the streams are configured.\n> >> Furthermore there's no \"attach\" operation defined anywhere. We really\n> >> need to improve this documentation I'm afraid. I'll leave the\n> >> documentation below unreviewed.\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn IPAInterface::mapBuffers()\n> >>> + * \\brief Map the buffers shared by the pipeline to the IPA\n> >>> + * \\param[in] buffers List of buffers to map\n> >>> + *\n> >>> + * This function is called when a pipeline handler wants to inform the IPA of\n> >>> + * which buffers it has mapped which the IPA can make use of. All buffers shared\n> > \n> > double which.\n> > \n> > I would\n> > \"This operation is used to inform the IPA module of the memory buffers\n> > set up by pipeline handlers and associate to each of them a numerical\n> > cookie id, that will be used to identify the buffer in all sequent\n> > operations.\"\n> > \n> >>> + * between these two object's needs to be shared using this function prior to\n> >>> + * use.\n> >>> + *\n> >>> + * After the buffers have been initialized a specific buffer can be referenced\n> >>> + * using the numerical \\a type and \\a id provided when the buffers where mapped.\n> >>> + *\n> >>> + * The numerical values for type and id used to describe a buffer have no\n> >>> + * meaning in the core libcamera code and the interface is pipeline handler\n> >>> + * specific.\n> >>> + *\n> >>> + * \\sa unmapBuffers()\n> >>> + * \\todo Can we make this a generic function?\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn IPAInterface::unmapBuffers()\n> >>> + * \\brief Unmap the buffers shared by the pipeline to the IPA\n> >>> + * \\param[in] buffers List of buffers to unmap\n> >>> + *\n> >>> + * This function is called when a pipeline handler wants to the IPA to unmap\n> >>> + * all or some of the buffer it have mapped.\n> >>> + *\n> > \n> > \"remove the mapping set up with mapBuffers\"\n> > \n> >>> + * \\sa mapBuffers()\n> >>> + * \\todo Can we make this a generic function?\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn IPAInterface::processEvent()\n> >>> + * \\brief Process an event from the pipeline handler\n> >>> + * \\param[in] event Event to process\n> >>> + *\n> >>> + * The pipeline handler can send events to the IPA to notify it of what is\n> >>> + * going on. This is the entry point on the IPA side for those messages.\n> > \n> > \"what is going on\" is pretty generic.\n> > \n> > \"This operation is used by pipeline handlers to inform the IPA\n> > module of events occurred during the on-going capture operation.\n> > \n> > Each \\a event notified by the pipeline handler with this method is\n> > handled by the IPAModule which interprets the operation parameters in\n> > an implementation specific way, which needs to be separately\n> > documented.\"\n> > \n> > Or something like that...\n> > \n> >>> + *\n> >>> + * The protocol between pipeline and IPA are hilly specific for each pipeline\n> > \n> > hilly ?\n> > \n> >>> + * and needs to be documented separately. Libcamera reserves no operation\n> >>> + * numbers and make no attempt to interpret the protocol.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn IPAInterface::queueFrameAction()\n> >>> + * \\brief Signal emitted when the IPA wish to queue a FrameAction\n> > \n> > \"Queue an action associated with a frame to the pipeline handler\"\n> > \n> >>> + * \\param[in] frame The frame number for the request\n> > \n> > s/request/action\n> > \n> >>> + * \\param[in] controls List of controls associated with the request\n> > \n> > s/request/action\n> > \n> >>> + *\n> >>> + * This signal is emitted when the IPA wish 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> >>> + * The IPA operation describing the frame action is passed as a parameter.\n> >>> + */\n> >>> +\n> >>>  } /* namespace libcamera */\n> >>> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> >>> index 62fcb529e1c7e4ff..14e8bb6067623fc6 100644\n> >>> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> >>> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> >>> @@ -26,7 +26,12 @@ public:\n> >>>  \tIPAProxyLinux(IPAModule *ipam);\n> >>>  \t~IPAProxyLinux();\n> >>>\n> >>> -\tint init();\n> >>> +\tint init() override { return 0; }\n> >>> +\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> >>> +\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}\n> >>> +\tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> >>> +\tvoid unmapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> >>> +\tvoid processEvent(const IPAOperationData &event) override {}\n> >>\n> >> Very clearly a hack :-) That's fine, we'll fix it.\n> >>\n> >>>\n> >>>  private:\n> >>>  \tvoid readyRead(IPCUnixSocket *ipc);\n> >>> @@ -36,13 +41,6 @@ private:\n> >>>  \tIPCUnixSocket *socket_;\n> >>>  };\n> >>>\n> >>> -int IPAProxyLinux::init()\n> >>> -{\n> >>> -\tLOG(IPAProxy, Debug) << \"initializing IPA via dummy proxy!\";\n> >>> -\n> >>> -\treturn 0;\n> >>> -}\n> >>> -\n> >>>  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)\n> >>>  \t: proc_(nullptr), socket_(nullptr)\n> >>>  {","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 626EC6165B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  6 Oct 2019 00:41:01 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(modemcable151.96-160-184.mc.videotron.ca [184.160.96.151])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 36230DD;\n\tSun,  6 Oct 2019 00:41:00 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570315260;\n\tbh=HdOi08T8g2W8kzzENLFkcURUHCt97WGOnZCGFaw5TAo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e5iZAF22ItqXq+ddJYakqFSlw+wtZ6AKyKwFbN8+EQ4e2IL0Vp5oAIDiHwIzqPkc4\n\tsGAmZKdaM80eM4nmd7QCdqmz2zqT/yCxY4aJ8xwlz2Ut21i6Mqa9liMYiT7SNpe0hX\n\tNLdIjH6cKXP5Viv9xQhtDazSoFuoeWrATGIUFhCE=","Date":"Sun, 6 Oct 2019 01:40:57 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org","Message-ID":"<20191005224057.GA20839@pendragon.ideasonboard.com>","References":"<20191003174941.1296988-1-niklas.soderlund@ragnatech.se>\n\t<20191003174941.1296988-7-niklas.soderlund@ragnatech.se>\n\t<20191004055833.GA9925@pendragon.ideasonboard.com>\n\t<20191004100320.zobxwtiifz2r2t3n@uno.localdomain>\n\t<20191005222717.GW1322@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191005222717.GW1322@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 06/11] libcamera: ipa: Extend to\n\tsupport IPA interactions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 05 Oct 2019 22:41:01 -0000"}}]