[libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce IpaBase class

[Naushir Patuck]

Hi Laurent,

On Tue, 2 May 2023 at 16:16, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Tue, May 02, 2023 at 03:54:16PM +0100, Naushir Patuck wrote:
> > Hi Laurent and Jacopo,
> >
> > On Fri, 28 Apr 2023 at 14:57, Naushir Patuck wrote:
> > >
> > > Hi Jacopo,
> >
> >  <snip>
> >
> > > > When it comes to the call order, it's the base class driving the
> > > > procesing flow, while dispatching calls to the derived classes at the
> > > > right time. Moving to your suggestion would allow derived classes to
> > > > call the base class method, which means most of the control is moved
> > > > to the derived classes with possible code duplication.
> > > >
> > > > I think to actually decide where to go, it is useful to analyze what
> > > > class implements the operations flow control
> > > >
> > > > In one case
> > > >
> > > >         Base::method()                  Derived::platformMethod()
> > > >
> > > >                 doSomeWork();
> > > >                 ....
> > > >                 platformMethod() -->    doPlaformWork();
> > > >                 ...
> > > >                 finalizeWork();
> > > >
> > > > While in the other
> > > >
> > > >         Base::method()                  Derived::method()
> > > >
> > > >                                         doSomeWork();
> > > >                                         ....
> > > >                 method()        <--     Base::method()
> > > >                                         ...
> > > >                                         finalizeWork();
> > > >
> > > > Is this an over-simplification ?
> > > >
> > > > Naush, do you expect the derived classes to control the operation
> > > > flow, or will this be common to both and could be implemented in the
> > > > base classes ?
> > >
> > > No, I see no explicit need for operation flow to be dictated by the derived
> > > class.  Hence changing things around to your suggestion and making things more
> > > consistent.
> >
> > Having looked further into this, I'm a bit more conflicted.  I initially thought
> > that I could simply revert to the original mechanism of having the control flow
> > start from the derived class and call into the base class, as the 2nd case
> > drawn above.  This does work neatly for the ipa::init() and ipa::configure().
> > However platformProcessStats() and platformPrepareIsp() require the control flow
> > from base class to derived class, as in the 1st case drawn above.
> >
> > I don't think it's sensible to choose flow 1 for some functions and flow 2 for
> > others.  So that suggests to me that flow 1 (i.e. the existing flow) is probably
> > the right approach for the IPA changes.  Do folks think that makes sense?
> > Again, I am happy to change things if it seems more appropriate.
>
> As mentioned in a different conversation, I'm fine keeping the existing
> code flow, even if I think it would be best to reverse it. We can
> improve this incrementally on top.

Agree.  I'll keep the existing flow and change if needed in the future.

Regards,
Naush

>
> --
> Regards,
>
> Laurent Pinchart