[{"id":19650,"web_url":"https://patchwork.libcamera.org/comment/19650/","msgid":"","date":"2021-09-14T02:54:00","subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3\n\tclass interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Mon, Sep 13, 2021 at 04:58:00PM +0200, Jean-Michel Hautbois wrote:\n> The IPU3 IPA is maturing to a modular and extensible system capable of\n> handling specific algorithms for the accelleration clusters on the ImgU.\n\ns/accelleration/acceleration/\n\nWhere does the term \"acceleration cluster\" come from ? It sounds weird\nto me, it's the first time I come across it for ISPs. Have I missed\nsomething ? Or should we say \"processing blocks\", \"processing\noperations\" or something similar ?\n\n> Provide a top-level class documentation to provide an overview of the\n> IPA, detailing what events are used and what algorithms are currently\n> supported, as well as the limitations currently imposed.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/ipu3/ipu3.cpp | 64 +++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 64 insertions(+)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index c925cf64..90053069 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -145,6 +145,70 @@ LOG_DEFINE_CATEGORY(IPAIPU3)\n> \n> namespace ipa::ipu3 {\n> \n> +/**\n> + * \\brief The IPU3 IPA implementation\n> + *\n> + * The IPU3 Pipeline defines an IPU3 specific interface for communication\n\ns/IPU3 specific/IPU3-specific/\n\nAs far as I know, a hyphen is used for compound adjectives (an\nIPU3-specific interface) but not for predicates (the interface is IPU3\nspecific).\n\nThere are three other instances below.\n\n> + * between the PipelineHandler, and the IPA module.\n\ns/PipelineHandler,/PipelineHandler/\n\n> + *\n> + * We extend the IPAIPU3Interface to implement our algorithms and handle events\n> + * from the IPU3 PipelineHandler to satisfy requests from the application.\n> + *\n> + * At initialisation time, a CameraSensorHelper is instantiated to support\n> + * camera specific calculations, while the default controls are computed, and\n> + * the algorithms are constructed and placed in an ordered list.\n> + *\n> + * The IPU3 ImgU operates with a grid layout to divide the overall frame into\n> + * rectangular cells of pixels. When the IPA is configured, we determine the\n> + * best grid for the statistics based on the pipeline handler Bayer Down Scaler\n> + * output size.\n\nOn a side note, I wonder if the IPA should be involved to compute the\nImgU configuration. It's a topic for future research.\n\n> + *\n> + * Two main events are then handled to facilitate the operation of the IPU3 ImgU\n\nI think we do a bit more than \"facilitating\" ImgU operation :-)\n\n> + * by populating its parameter buffer, and adapting the settings of the sensor\n> + * attached to the IPU3 CIO2 through sensor specific V4L2 controls.\n> + *\n> + * When the event \\a EventFillParams occurs we populate the ImgU parameter\n> + * buffer with settings to configure the device in preparation for handling the\n> + * frame queued in the Request.\n> + *\n> + * When the frame has completed processing, the ImgU will generate a statistics\n> + * buffer which is given to the IPA as part of the \\a EventStatReady event. At\n> + * this event we run the algorithms to parse the statistics and cache any\n> + * results for the next \\a EventFillParams event.\n> + *\n> + * The individual algorithms are split into modular components that are called\n> + * iteratively to allow them to process statistics from the ImgU in a defined\n> + * order.\n> + *\n> + * The current implementation supports three core algorithms:\n> + * - Automatic white balance (AWB)\n> + * - Automatic gain and exposure control (AGC)\n> + * - Tonemapping (Gamma)\n\ns/Tonemapping/Tone mapping/\n\nSame below.\n\n> + *\n> + * AWB is implemented using a Greyworld algorithm, and calculates the red and\n> + * blue gains to apply to generate a neutral grey frame overall.\n> + *\n> + * AGC is handled by calculating a histogram of the green channel to estimate an\n> + * analogue gain and shutter time which will provide a well exposed frame. An\n> + * IIR filter is used to smooth the changes to the sensor to reduce perceivable\n\nI'd say \"low-pass filter\" (of possibly \"low-pass IIR filter\") instead of\n\"IIR filter\" here, as the infinite impulse response isn't what really\nmatters.\n\n> + * steps.\n> + *\n> + * The Tonemapping algorithm provides a gamma correction table to improve the\n> + * contrast of the scene.\n> + *\n> + * The IPU3 ImgU has further accellerator clusters to support image quality\n\ns/accellerator/accelerator/\n\nSame comment as for the commit message.\n\n> + * improvements through bayer and temporal noise reductions, however those are\n> + * not supported in the current implementation, and will use default settings as\n> + * provided by the kernel driver.\n> + *\n> + * Demosaicing is operating on the default values and could be further optimised\n\nI'm not totally sure what \"values\" mean here, did you mean \"operating\nwith the default parameters\" ?\n\n> + * to provide improved sharpening coefficients, checker artifact removal, and\n> + * false color correction.\n> + *\n> + * Additional image enhancements can be made by providing lens and sensor\n> + * specific tuning to adapt for Black Level compensation (BLC), Lens shading\n> + * correction (SHD) and Color correction (CCM)\n\ns/$/./\n\n> + */\n> class IPAIPU3 : public IPAIPU3Interface\n> {\n> public:","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 B3A69BDB1D\n\tfor ;\n\tTue, 14 Sep 2021 02:54:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0AC6B69187;\n\tTue, 14 Sep 2021 04:54:26 +0200 (CEST)","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 DE4BD6916F\n\tfor ;\n\tTue, 14 Sep 2021 04:54:24 +0200 (CEST)","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 50BF82A5;\n\tTue, 14 Sep 2021 04:54:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bjOhw+pv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631588064;\n\tbh=82ahnWjwgE+73EV4LdQYro2S0b8IobTv9BtgmXaqLR4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bjOhw+pvpkaoZgEKXlrKREZrlwS84IjUQyU2VrpxL16ss8TvvCeNANV52owlz/RmK\n\tcPUTgELBgzXRD8KqHlzR8T6Q577j8f/5F/fkhcwGzLs0leXY1nbpYkNhfPGy3KEpCb\n\tq3JyY4uSUb+ckApTFVuJoyEGFkQzO3cAX1N/AKDs=","Date":"Tue, 14 Sep 2021 05:54:00 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210913145810.66515-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913145810.66515-2-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210913145810.66515-2-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3\n\tclass interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19656,"web_url":"https://patchwork.libcamera.org/comment/19656/","msgid":"","date":"2021-09-14T03:48:56","subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3\n\tclass interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Sep 14, 2021 at 05:54:01AM +0300, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Mon, Sep 13, 2021 at 04:58:00PM +0200, Jean-Michel Hautbois wrote:\n> > The IPU3 IPA is maturing to a modular and extensible system capable of\n> > handling specific algorithms for the accelleration clusters on the ImgU.\n> \n> s/accelleration/acceleration/\n> \n> Where does the term \"acceleration cluster\" come from ? It sounds weird\n> to me, it's the first time I come across it for ISPs. Have I missed\n> something ? Or should we say \"processing blocks\", \"processing\n> operations\" or something similar ?\n> \n> > Provide a top-level class documentation to provide an overview of the\n> > IPA, detailing what events are used and what algorithms are currently\n> > supported, as well as the limitations currently imposed.\n> > \n> > Signed-off-by: Jean-Michel Hautbois \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/ipa/ipu3/ipu3.cpp | 64 +++++++++++++++++++++++++++++++++++++++++++\n> > 1 file changed, 64 insertions(+)\n> > \n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index c925cf64..90053069 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -145,6 +145,70 @@ LOG_DEFINE_CATEGORY(IPAIPU3)\n> > \n> > namespace ipa::ipu3 {\n> > \n> > +/**\n> > + * \\brief The IPU3 IPA implementation\n> > + *\n> > + * The IPU3 Pipeline defines an IPU3 specific interface for communication\n> \n> s/IPU3 specific/IPU3-specific/\n> \n> As far as I know, a hyphen is used for compound adjectives (an\n> IPU3-specific interface) but not for predicates (the interface is IPU3\n> specific).\n> \n> There are three other instances below.\n> \n> > + * between the PipelineHandler, and the IPA module.\n> \n> s/PipelineHandler,/PipelineHandler/\n> \n> > + *\n> > + * We extend the IPAIPU3Interface to implement our algorithms and handle events\n> > + * from the IPU3 PipelineHandler to satisfy requests from the application.\n> > + *\n> > + * At initialisation time, a CameraSensorHelper is instantiated to support\n> > + * camera specific calculations, while the default controls are computed, and\n> > + * the algorithms are constructed and placed in an ordered list.\n> > + *\n> > + * The IPU3 ImgU operates with a grid layout to divide the overall frame into\n> > + * rectangular cells of pixels. When the IPA is configured, we determine the\n> > + * best grid for the statistics based on the pipeline handler Bayer Down Scaler\n> > + * output size.\n> \n> On a side note, I wonder if the IPA should be involved to compute the\n> ImgU configuration. It's a topic for future research.\n> \n> > + *\n> > + * Two main events are then handled to facilitate the operation of the IPU3 ImgU\n> \n> I think we do a bit more than \"facilitating\" ImgU operation :-)\n> \n> > + * by populating its parameter buffer, and adapting the settings of the sensor\n> > + * attached to the IPU3 CIO2 through sensor specific V4L2 controls.\n> > + *\n> > + * When the event \\a EventFillParams occurs we populate the ImgU parameter\n> > + * buffer with settings to configure the device in preparation for handling the\n> > + * frame queued in the Request.\n> > + *\n> > + * When the frame has completed processing, the ImgU will generate a statistics\n> > + * buffer which is given to the IPA as part of the \\a EventStatReady event. At\n> > + * this event we run the algorithms to parse the statistics and cache any\n> > + * results for the next \\a EventFillParams event.\n> > + *\n> > + * The individual algorithms are split into modular components that are called\n> > + * iteratively to allow them to process statistics from the ImgU in a defined\n> > + * order.\n> > + *\n> > + * The current implementation supports three core algorithms:\n> > + * - Automatic white balance (AWB)\n> > + * - Automatic gain and exposure control (AGC)\n> > + * - Tonemapping (Gamma)\n> \n> s/Tonemapping/Tone mapping/\n> \n> Same below.\n> \n> > + *\n> > + * AWB is implemented using a Greyworld algorithm, and calculates the red and\n> > + * blue gains to apply to generate a neutral grey frame overall.\n> > + *\n> > + * AGC is handled by calculating a histogram of the green channel to estimate an\n> > + * analogue gain and shutter time which will provide a well exposed frame. An\n> > + * IIR filter is used to smooth the changes to the sensor to reduce perceivable\n> \n> I'd say \"low-pass filter\" (of possibly \"low-pass IIR filter\") instead of\n> \"IIR filter\" here, as the infinite impulse response isn't what really\n> matters.\n\nBy the way, are you aware that the output of the filter is actually\nunused ?\n\n> > + * steps.\n> > + *\n> > + * The Tonemapping algorithm provides a gamma correction table to improve the\n> > + * contrast of the scene.\n> > + *\n> > + * The IPU3 ImgU has further accellerator clusters to support image quality\n> \n> s/accellerator/accelerator/\n> \n> Same comment as for the commit message.\n> \n> > + * improvements through bayer and temporal noise reductions, however those are\n> > + * not supported in the current implementation, and will use default settings as\n> > + * provided by the kernel driver.\n> > + *\n> > + * Demosaicing is operating on the default values and could be further optimised\n> \n> I'm not totally sure what \"values\" mean here, did you mean \"operating\n> with the default parameters\" ?\n> \n> > + * to provide improved sharpening coefficients, checker artifact removal, and\n> > + * false color correction.\n> > + *\n> > + * Additional image enhancements can be made by providing lens and sensor\n> > + * specific tuning to adapt for Black Level compensation (BLC), Lens shading\n> > + * correction (SHD) and Color correction (CCM)\n> \n> s/$/./\n> \n> > + */\n> > class IPAIPU3 : public IPAIPU3Interface\n> > {\n> > public:","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 CF5AFBDB1D\n\tfor ;\n\tTue, 14 Sep 2021 03:49:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 252A469188;\n\tTue, 14 Sep 2021 05:49:23 +0200 (CEST)","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 2F20B69181\n\tfor ;\n\tTue, 14 Sep 2021 05:49:21 +0200 (CEST)","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 9AD992A5;\n\tTue, 14 Sep 2021 05:49:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZzMCmg3+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631591360;\n\tbh=d3InqrQH71ToJlrW4WpZwvyN2UbWBw93EPcpAkzCXCM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZzMCmg3+R+TaXmJpCg6m6S6G9tqWzuULXsgbtHDtXmzJMf4cInCR5Zq6Yr4mspipn\n\tDqvjQL/Pq9bePJMt9OHJetdpMrf8nLl5XgAnoxbr3SCKjj8iHDiuXoq+zIDxOoapJ6\n\tZEb0fmQZhznIKLKdOtJ6B/z1MiFXlNEqX6cO1wro=","Date":"Tue, 14 Sep 2021 06:48:56 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210913145810.66515-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913145810.66515-2-jeanmichel.hautbois@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3\n\tclass interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20377,"web_url":"https://patchwork.libcamera.org/comment/20377/","msgid":"<654e0ffc-f6c8-6fcf-ad57-ec5d80d83eeb@ideasonboard.com>","date":"2021-10-22T08:24:16","subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3\n\tclass interface","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nAs I am rebasing this series with the proper branch, I noticed a few \ncomments I still not know how to address now) :\n\nOn 14/09/2021 04:54, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Mon, Sep 13, 2021 at 04:58:00PM +0200, Jean-Michel Hautbois wrote:\n>> The IPU3 IPA is maturing to a modular and extensible system capable of\n>> handling specific algorithms for the accelleration clusters on the ImgU.\n> \n> s/accelleration/acceleration/\n> \n> Where does the term \"acceleration cluster\" come from ? It sounds weird\n> to me, it's the first time I come across it for ISPs. Have I missed\n> something ? Or should we say \"processing blocks\", \"processing\n> operations\" or something similar ?\n> \n>> Provide a top-level class documentation to provide an overview of the\n>> IPA, detailing what events are used and what algorithms are currently\n>> supported, as well as the limitations currently imposed.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/ipa/ipu3/ipu3.cpp | 64 +++++++++++++++++++++++++++++++++++++++++++\n>> 1 file changed, 64 insertions(+)\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index c925cf64..90053069 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -145,6 +145,70 @@ LOG_DEFINE_CATEGORY(IPAIPU3)\n>> \n>> namespace ipa::ipu3 {\n>> \n>> +/**\n>> + * \\brief The IPU3 IPA implementation\n>> + *\n>> + * The IPU3 Pipeline defines an IPU3 specific interface for communication\n> \n> s/IPU3 specific/IPU3-specific/\n> \n> As far as I know, a hyphen is used for compound adjectives (an\n> IPU3-specific interface) but not for predicates (the interface is IPU3\n> specific).\n> \n> There are three other instances below.\n> \n>> + * between the PipelineHandler, and the IPA module.\n> \n> s/PipelineHandler,/PipelineHandler/\n> \n>> + *\n>> + * We extend the IPAIPU3Interface to implement our algorithms and handle events\n>> + * from the IPU3 PipelineHandler to satisfy requests from the application.\n>> + *\n>> + * At initialisation time, a CameraSensorHelper is instantiated to support\n>> + * camera specific calculations, while the default controls are computed, and\n>> + * the algorithms are constructed and placed in an ordered list.\n>> + *\n>> + * The IPU3 ImgU operates with a grid layout to divide the overall frame into\n>> + * rectangular cells of pixels. When the IPA is configured, we determine the\n>> + * best grid for the statistics based on the pipeline handler Bayer Down Scaler\n>> + * output size.\n> \n> On a side note, I wonder if the IPA should be involved to compute the\n> ImgU configuration. It's a topic for future research.\n\nShould I add a \\todo ?\n\n> \n>> + *\n>> + * Two main events are then handled to facilitate the operation of the IPU3 ImgU\n> \n> I think we do a bit more than \"facilitating\" ImgU operation :-)\n\nThis is Kieran's term, and I have changed it to:\ns/facilitate the operation of /operate/\n\n> \n>> + * by populating its parameter buffer, and adapting the settings of the sensor\n>> + * attached to the IPU3 CIO2 through sensor specific V4L2 controls.\n>> + *\n>> + * When the event \\a EventFillParams occurs we populate the ImgU parameter\n>> + * buffer with settings to configure the device in preparation for handling the\n>> + * frame queued in the Request.\n>> + *\n>> + * When the frame has completed processing, the ImgU will generate a statistics\n>> + * buffer which is given to the IPA as part of the \\a EventStatReady event. At\n>> + * this event we run the algorithms to parse the statistics and cache any\n>> + * results for the next \\a EventFillParams event.\n>> + *\n>> + * The individual algorithms are split into modular components that are called\n>> + * iteratively to allow them to process statistics from the ImgU in a defined\n>> + * order.\n>> + *\n>> + * The current implementation supports three core algorithms:\n>> + * - Automatic white balance (AWB)\n>> + * - Automatic gain and exposure control (AGC)\n>> + * - Tonemapping (Gamma)\n> \n> s/Tonemapping/Tone mapping/\n> \n> Same below.\n> \n>> + *\n>> + * AWB is implemented using a Greyworld algorithm, and calculates the red and\n>> + * blue gains to apply to generate a neutral grey frame overall.\n>> + *\n>> + * AGC is handled by calculating a histogram of the green channel to estimate an\n>> + * analogue gain and shutter time which will provide a well exposed frame. An\n>> + * IIR filter is used to smooth the changes to the sensor to reduce perceivable\n> \n> I'd say \"low-pass filter\" (of possibly \"low-pass IIR filter\") instead of\n> \"IIR filter\" here, as the infinite impulse response isn't what really\n> matters.\n> \n>> + * steps.\n>> + *\n>> + * The Tonemapping algorithm provides a gamma correction table to improve the\n>> + * contrast of the scene.\n>> + *\n>> + * The IPU3 ImgU has further accellerator clusters to support image quality\n> \n> s/accellerator/accelerator/\n> \n> Same comment as for the commit message.\n> \n>> + * improvements through bayer and temporal noise reductions, however those are\n>> + * not supported in the current implementation, and will use default settings as\n>> + * provided by the kernel driver.\n>> + *\n>> + * Demosaicing is operating on the default values and could be further optimised\n> \n> I'm not totally sure what \"values\" mean here, did you mean \"operating\n> with the default parameters\" ?\n> \n>> + * to provide improved sharpening coefficients, checker artifact removal, and\n>> + * false color correction.\n>> + *\n>> + * Additional image enhancements can be made by providing lens and sensor\n>> + * specific tuning to adapt for Black Level compensation (BLC), Lens shading\n>> + * correction (SHD) and Color correction (CCM)\n> \n> s/$/./\n> \n>> + */\n>> class IPAIPU3 : public IPAIPU3Interface\n>> {\n>> public:\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7ABFABF415\n\tfor ;\n\tFri, 22 Oct 2021 08:24:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E88DF68F59;\n\tFri, 22 Oct 2021 10:24:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC85F60128\n\tfor ;\n\tFri, 22 Oct 2021 10:24:18 +0200 (CEST)","from [IPV6:2a01:e0a:169:7140:22cc:3af6:5ccb:8367] (unknown\n\t[IPv6:2a01:e0a:169:7140:22cc:3af6:5ccb:8367])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 50E7B51D;\n\tFri, 22 Oct 2021 10:24:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MkdtFLK9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634891058;\n\tbh=3l86+pyt1nC9CBffkSdr2ehwroPhWWY+qXXhVYDQqB0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=MkdtFLK9VLmzY0TDboKY3hYIWmQl9pxEXz6pTaPVw9hhEcFGvR/jC7lKUsssIAfN8\n\tuLDBU7V2wtrjc4uogj9ahAWZ53bCWbQmp2TkZG6YqQvP83cQ07nuXtT0Qaj/lu5RhJ\n\t+LF6HjGKWdwC+QKeaW1H5ZFPuQwMfeM/XTcT5Jqg=","Message-ID":"<654e0ffc-f6c8-6fcf-ad57-ec5d80d83eeb@ideasonboard.com>","Date":"Fri, 22 Oct 2021 10:24:16 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.1.2","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20210913145810.66515-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913145810.66515-2-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3\n\tclass interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20381,"web_url":"https://patchwork.libcamera.org/comment/20381/","msgid":"","date":"2021-10-22T11:59:17","subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3\n\tclass interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Fri, Oct 22, 2021 at 10:24:16AM +0200, Jean-Michel Hautbois wrote:\n> Hi Laurent,\n> \n> As I am rebasing this series with the proper branch, I noticed a few \n> comments I still not know how to address now) :\n> \n> On 14/09/2021 04:54, Laurent Pinchart wrote:\n> > On Mon, Sep 13, 2021 at 04:58:00PM +0200, Jean-Michel Hautbois wrote:\n> >> The IPU3 IPA is maturing to a modular and extensible system capable of\n> >> handling specific algorithms for the accelleration clusters on the ImgU.\n> > \n> > s/accelleration/acceleration/\n> > \n> > Where does the term \"acceleration cluster\" come from ? It sounds weird\n> > to me, it's the first time I come across it for ISPs. Have I missed\n> > something ? Or should we say \"processing blocks\", \"processing\n> > operations\" or something similar ?\n> > \n> >> Provide a top-level class documentation to provide an overview of the\n> >> IPA, detailing what events are used and what algorithms are currently\n> >> supported, as well as the limitations currently imposed.\n> >>\n> >> Signed-off-by: Jean-Michel Hautbois \n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> src/ipa/ipu3/ipu3.cpp | 64 +++++++++++++++++++++++++++++++++++++++++++\n> >> 1 file changed, 64 insertions(+)\n> >>\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index c925cf64..90053069 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -145,6 +145,70 @@ LOG_DEFINE_CATEGORY(IPAIPU3)\n> >> \n> >> namespace ipa::ipu3 {\n> >> \n> >> +/**\n> >> + * \\brief The IPU3 IPA implementation\n> >> + *\n> >> + * The IPU3 Pipeline defines an IPU3 specific interface for communication\n> > \n> > s/IPU3 specific/IPU3-specific/\n> > \n> > As far as I know, a hyphen is used for compound adjectives (an\n> > IPU3-specific interface) but not for predicates (the interface is IPU3\n> > specific).\n> > \n> > There are three other instances below.\n> > \n> >> + * between the PipelineHandler, and the IPA module.\n> > \n> > s/PipelineHandler,/PipelineHandler/\n> > \n> >> + *\n> >> + * We extend the IPAIPU3Interface to implement our algorithms and handle events\n> >> + * from the IPU3 PipelineHandler to satisfy requests from the application.\n> >> + *\n> >> + * At initialisation time, a CameraSensorHelper is instantiated to support\n> >> + * camera specific calculations, while the default controls are computed, and\n> >> + * the algorithms are constructed and placed in an ordered list.\n> >> + *\n> >> + * The IPU3 ImgU operates with a grid layout to divide the overall frame into\n> >> + * rectangular cells of pixels. When the IPA is configured, we determine the\n> >> + * best grid for the statistics based on the pipeline handler Bayer Down Scaler\n> >> + * output size.\n> > \n> > On a side note, I wonder if the IPA should be involved to compute the\n> > ImgU configuration. It's a topic for future research.\n> \n> Should I add a \\todo ?\n\nNo, I think you can leave it as-is, it's fine.\n\n> >> + *\n> >> + * Two main events are then handled to facilitate the operation of the IPU3 ImgU\n> > \n> > I think we do a bit more than \"facilitating\" ImgU operation :-)\n> \n> This is Kieran's term, and I have changed it to:\n> s/facilitate the operation of /operate/\n\nSounds good to me.\n\n> >> + * by populating its parameter buffer, and adapting the settings of the sensor\n> >> + * attached to the IPU3 CIO2 through sensor specific V4L2 controls.\n> >> + *\n> >> + * When the event \\a EventFillParams occurs we populate the ImgU parameter\n> >> + * buffer with settings to configure the device in preparation for handling the\n> >> + * frame queued in the Request.\n> >> + *\n> >> + * When the frame has completed processing, the ImgU will generate a statistics\n> >> + * buffer which is given to the IPA as part of the \\a EventStatReady event. At\n> >> + * this event we run the algorithms to parse the statistics and cache any\n> >> + * results for the next \\a EventFillParams event.\n> >> + *\n> >> + * The individual algorithms are split into modular components that are called\n> >> + * iteratively to allow them to process statistics from the ImgU in a defined\n> >> + * order.\n> >> + *\n> >> + * The current implementation supports three core algorithms:\n> >> + * - Automatic white balance (AWB)\n> >> + * - Automatic gain and exposure control (AGC)\n> >> + * - Tonemapping (Gamma)\n> > \n> > s/Tonemapping/Tone mapping/\n> > \n> > Same below.\n> > \n> >> + *\n> >> + * AWB is implemented using a Greyworld algorithm, and calculates the red and\n> >> + * blue gains to apply to generate a neutral grey frame overall.\n> >> + *\n> >> + * AGC is handled by calculating a histogram of the green channel to estimate an\n> >> + * analogue gain and shutter time which will provide a well exposed frame. An\n> >> + * IIR filter is used to smooth the changes to the sensor to reduce perceivable\n> > \n> > I'd say \"low-pass filter\" (of possibly \"low-pass IIR filter\") instead of\n> > \"IIR filter\" here, as the infinite impulse response isn't what really\n> > matters.\n> > \n> >> + * steps.\n> >> + *\n> >> + * The Tonemapping algorithm provides a gamma correction table to improve the\n> >> + * contrast of the scene.\n> >> + *\n> >> + * The IPU3 ImgU has further accellerator clusters to support image quality\n> > \n> > s/accellerator/accelerator/\n> > \n> > Same comment as for the commit message.\n> > \n> >> + * improvements through bayer and temporal noise reductions, however those are\n> >> + * not supported in the current implementation, and will use default settings as\n> >> + * provided by the kernel driver.\n> >> + *\n> >> + * Demosaicing is operating on the default values and could be further optimised\n> > \n> > I'm not totally sure what \"values\" mean here, did you mean \"operating\n> > with the default parameters\" ?\n> > \n> >> + * to provide improved sharpening coefficients, checker artifact removal, and\n> >> + * false color correction.\n> >> + *\n> >> + * Additional image enhancements can be made by providing lens and sensor\n> >> + * specific tuning to adapt for Black Level compensation (BLC), Lens shading\n> >> + * correction (SHD) and Color correction (CCM)\n> > \n> > s/$/./\n> > \n> >> + */\n> >> class IPAIPU3 : public IPAIPU3Interface\n> >> {\n> >> public:","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 CCA80BDB1C\n\tfor ;\n\tFri, 22 Oct 2021 11:59:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4881A68F59;\n\tFri, 22 Oct 2021 13:59:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A95386012A\n\tfor ;\n\tFri, 22 Oct 2021 13:59:38 +0200 (CEST)","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 1912751D;\n\tFri, 22 Oct 2021 13:59:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FbPV3gKO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634903978;\n\tbh=Ked3z9cc3BVvnfEe9YL/qjrTsexOxmb+Ua6ru3qedEk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FbPV3gKOuDR1vLzhknZBM+j3BfyZy80q43XvsL2996mYKER9qvhB1I0MlKlPZ1hHX\n\ttZ1UFad6jIhaLgnG+2iUAfTX47IzDk+hWCCPWYt5uqklvAsbOL1iRX8/cYNaFNKFKD\n\tfUilGrBEtnpriYWsj5SwCEubEg+Dre9L0paXxMNE=","Date":"Fri, 22 Oct 2021 14:59:17 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210913145810.66515-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913145810.66515-2-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<654e0ffc-f6c8-6fcf-ad57-ec5d80d83eeb@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<654e0ffc-f6c8-6fcf-ad57-ec5d80d83eeb@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3\n\tclass interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]