[libcamera-devel] [PATCH v4 17/19] ipa: ipu3: Implement an empty stop() function

[Kieran Bingham]

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 at ideasonboard.com>
> >>> Reviewed-by: Kieran Bingham <kieran.bingham at 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 at 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
> >>>    *