| Message ID | 20211026095534.90348-18-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jean-Michel, Thank you for the patch. On Tue, Oct 26, 2021 at 11:55:32AM +0200, Jean-Michel Hautbois wrote: > While the stop() function does not currently perform any action, it forms > part of the IPA interface and is a public function in the class. > > Promote it to a full (but basic) function implementation and begin the > documentation accordingly so that there is an appropriate stub to > perform stop operations if they come up. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/ipu3/ipu3.cpp | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index a10fdd4a..5c51607d 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -135,7 +135,7 @@ public: > ControlInfoMap *ipaControls) override; > > int start() override; > - void stop() override {} > + void stop() override; > > int configure(const IPAConfigInfo &configInfo, > ControlInfoMap *ipaControls) override; > @@ -323,6 +323,13 @@ int IPAIPU3::start() > return 0; > } > > +/** > + * \brief Ensure that all processing has completed > + */ > +void IPAIPU3::stop() > +{ > +} > + > /** > * \brief Calculate a grid for the AWB statistics > *
Hi JM, On 10/26/21 3:25 PM, Jean-Michel Hautbois wrote: > While the stop() function does not currently perform any action, it forms > part of the IPA interface and is a public function in the class. > > Promote it to a full (but basic) function implementation and begin the > documentation accordingly so that there is an appropriate stub to > perform stop operations if they come up. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/ipu3/ipu3.cpp | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index a10fdd4a..5c51607d 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -135,7 +135,7 @@ public: > ControlInfoMap *ipaControls) override; > > int start() override; > - void stop() override {} > + void stop() override; > > int configure(const IPAConfigInfo &configInfo, > ControlInfoMap *ipaControls) override; > @@ -323,6 +323,13 @@ int IPAIPU3::start() > return 0; > } > > +/** > + * \brief Ensure that all processing has completed This sounds less like a \brief but more like a \todo. Since, it's a stub function, I am not sure how well we can document it as of now. Do we have some "stopping" criteria which needs to be satisfied to ensure we have stopped the IPA? > + */ > +void IPAIPU3::stop() > +{ > +} > + > /** > * \brief Calculate a grid for the AWB statistics > *
Hi Umang, On 26/10/2021 12:13, Umang Jain wrote: > Hi JM, > > On 10/26/21 3:25 PM, Jean-Michel Hautbois wrote: >> While the stop() function does not currently perform any action, it forms >> part of the IPA interface and is a public function in the class. >> >> Promote it to a full (but basic) function implementation and begin the >> documentation accordingly so that there is an appropriate stub to >> perform stop operations if they come up. >> >> Signed-off-by: Jean-Michel Hautbois >> <jeanmichel.hautbois@ideasonboard.com> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/ipa/ipu3/ipu3.cpp | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index a10fdd4a..5c51607d 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -135,7 +135,7 @@ public: >> ControlInfoMap *ipaControls) override; >> int start() override; >> - void stop() override {} >> + void stop() override; >> int configure(const IPAConfigInfo &configInfo, >> ControlInfoMap *ipaControls) override; >> @@ -323,6 +323,13 @@ int IPAIPU3::start() >> return 0; >> } >> +/** >> + * \brief Ensure that all processing has completed > > > This sounds less like a \brief but more like a \todo. Since, it's a stub > function, I am not sure how well we can document it as of now. Indeed, but this is a reference documentation, so we are telling the purpose of the function, even if not implemented yet or empty. > > Do we have some "stopping" criteria which needs to be satisfied to > ensure we have stopped the IPA? Right now, no, but in the future, maybe (not really thought about it yet) > >> + */ >> +void IPAIPU3::stop() >> +{ >> +} >> + >> /** >> * \brief Calculate a grid for the AWB statistics >> *
Hi JM. On 10/26/21 4:00 PM, Jean-Michel Hautbois wrote: > Hi Umang, > > On 26/10/2021 12:13, Umang Jain wrote: >> Hi JM, >> >> On 10/26/21 3:25 PM, Jean-Michel Hautbois wrote: >>> While the stop() function does not currently perform any action, it >>> forms >>> part of the IPA interface and is a public function in the class. >>> >>> Promote it to a full (but basic) function implementation and begin the >>> documentation accordingly so that there is an appropriate stub to >>> perform stop operations if they come up. >>> >>> Signed-off-by: Jean-Michel Hautbois >>> <jeanmichel.hautbois@ideasonboard.com> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> src/ipa/ipu3/ipu3.cpp | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>> index a10fdd4a..5c51607d 100644 >>> --- a/src/ipa/ipu3/ipu3.cpp >>> +++ b/src/ipa/ipu3/ipu3.cpp >>> @@ -135,7 +135,7 @@ public: >>> ControlInfoMap *ipaControls) override; >>> int start() override; >>> - void stop() override {} >>> + void stop() override; >>> int configure(const IPAConfigInfo &configInfo, >>> ControlInfoMap *ipaControls) override; >>> @@ -323,6 +323,13 @@ int IPAIPU3::start() >>> return 0; >>> } >>> +/** >>> + * \brief Ensure that all processing has completed >> >> >> This sounds less like a \brief but more like a \todo. Since, it's a >> stub function, I am not sure how well we can document it as of now. > > Indeed, but this is a reference documentation, so we are telling the > purpose of the function, even if not implemented yet or empty. Ok, sounds okay to me then Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > >> >> Do we have some "stopping" criteria which needs to be satisfied to >> ensure we have stopped the IPA? > > Right now, no, but in the future, maybe (not really thought about it yet) > >> >>> + */ >>> +void IPAIPU3::stop() >>> +{ >>> +} >>> + >>> /** >>> * \brief Calculate a grid for the AWB statistics >>> *
Quoting Umang Jain (2021-10-26 11:31:33) > Hi JM. > > On 10/26/21 4:00 PM, Jean-Michel Hautbois wrote: > > Hi Umang, > > > > On 26/10/2021 12:13, Umang Jain wrote: > >> Hi JM, > >> > >> On 10/26/21 3:25 PM, Jean-Michel Hautbois wrote: > >>> While the stop() function does not currently perform any action, it > >>> forms > >>> part of the IPA interface and is a public function in the class. > >>> > >>> Promote it to a full (but basic) function implementation and begin the > >>> documentation accordingly so that there is an appropriate stub to > >>> perform stop operations if they come up. > >>> > >>> Signed-off-by: Jean-Michel Hautbois > >>> <jeanmichel.hautbois@ideasonboard.com> > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- > >>> src/ipa/ipu3/ipu3.cpp | 9 ++++++++- > >>> 1 file changed, 8 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >>> index a10fdd4a..5c51607d 100644 > >>> --- a/src/ipa/ipu3/ipu3.cpp > >>> +++ b/src/ipa/ipu3/ipu3.cpp > >>> @@ -135,7 +135,7 @@ public: > >>> ControlInfoMap *ipaControls) override; > >>> int start() override; > >>> - void stop() override {} > >>> + void stop() override; > >>> int configure(const IPAConfigInfo &configInfo, > >>> ControlInfoMap *ipaControls) override; > >>> @@ -323,6 +323,13 @@ int IPAIPU3::start() > >>> return 0; > >>> } > >>> +/** > >>> + * \brief Ensure that all processing has completed > >> > >> > >> This sounds less like a \brief but more like a \todo. Since, it's a > >> stub function, I am not sure how well we can document it as of now. > > > > Indeed, but this is a reference documentation, so we are telling the > > purpose of the function, even if not implemented yet or empty. > > > Ok, sounds okay to me then > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > > >> > >> Do we have some "stopping" criteria which needs to be satisfied to > >> ensure we have stopped the IPA? > > > > Right now, no, but in the future, maybe (not really thought about it yet) This IPA doesn't currently have any requirements to stop anything as nothing is run asynchronously. However - the libcamera IPA interface /requires/ that after stop() is called, the IPA is not allowed to do anything. (Think back to those IPU3 bugs I had to fix) So I'm quite happy that this is here as a clear statement to say "This is where you must stop anything that has started". > > > >> > >>> + */ > >>> +void IPAIPU3::stop() > >>> +{ > >>> +} > >>> + > >>> /** > >>> * \brief Calculate a grid for the AWB statistics > >>> *
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index a10fdd4a..5c51607d 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -135,7 +135,7 @@ public: ControlInfoMap *ipaControls) override; int start() override; - void stop() override {} + void stop() override; int configure(const IPAConfigInfo &configInfo, ControlInfoMap *ipaControls) override; @@ -323,6 +323,13 @@ int IPAIPU3::start() return 0; } +/** + * \brief Ensure that all processing has completed + */ +void IPAIPU3::stop() +{ +} + /** * \brief Calculate a grid for the AWB statistics *