[libcamera-devel,10/13] libcamera: ipa: Extend to support IPA interactions

Message ID 20190828011710.32128-11-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: ipa: Add basic IPA support
Related show

Commit Message

Niklas Söderlund Aug. 28, 2019, 1:17 a.m. UTC
The IPA interface needs to support interactions with the pipeline, add
interfaces to control the sensor and handling of request ISP parameters
and statistics.

The interface needs to be extended further to support attaching the
result of the statistic analyses to the request before the request
completes to the user. It also needs to modified to allow proper per
frame control of capture parameters.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/ipa/ipa_interface.h   | 17 +++++-
 src/ipa/ipa_dummy.cpp                   | 11 ++--
 src/ipa/ipa_dummy_isolate.cpp           | 11 ++--
 src/libcamera/ipa_interface.cpp         | 72 ++++++++++++++++++++++++-
 src/libcamera/proxy/ipa_proxy_linux.cpp | 12 ++---
 5 files changed, 98 insertions(+), 25 deletions(-)

Comments

Kieran Bingham Aug. 29, 2019, 3:42 p.m. UTC | #1
Hi Niklas,

On 28/08/2019 02:17, Niklas Söderlund wrote:
> The IPA interface needs to support interactions with the pipeline, add
> interfaces to control the sensor and handling of request ISP parameters
> and statistics.
> 
> The interface needs to be extended further to support attaching the
> result of the statistic analyses to the request before the request
> completes to the user. It also needs to modified to allow proper per
> frame control of capture parameters.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/ipa/ipa_interface.h   | 17 +++++-
>  src/ipa/ipa_dummy.cpp                   | 11 ++--
>  src/ipa/ipa_dummy_isolate.cpp           | 11 ++--
>  src/libcamera/ipa_interface.cpp         | 72 ++++++++++++++++++++++++-
>  src/libcamera/proxy/ipa_proxy_linux.cpp | 12 ++---
>  5 files changed, 98 insertions(+), 25 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> index 2c5eb1fd524311cb..38e16ff37214f7b9 100644
> --- a/include/libcamera/ipa/ipa_interface.h
> +++ b/include/libcamera/ipa/ipa_interface.h
> @@ -7,14 +7,29 @@
>  #ifndef __LIBCAMERA_IPA_INTERFACE_H__
>  #define __LIBCAMERA_IPA_INTERFACE_H__
>  
> +#include <libcamera/controls.h>
> +#include <libcamera/signal.h>
> +
> +#include "v4l2_controls.h"
> +
>  namespace libcamera {
>  
> +class Buffer;
> +class Request;
> +
>  class IPAInterface
>  {
>  public:
>  	virtual ~IPAInterface() {}
>  
> -	virtual int init() = 0;

I see now why you don't call init() in patch 2/13 - but I think to keep
the patches 'correct' then if you are removing init() here, you should
add the call in to 2/13, and then remove it here. Tedious I know - but
it will keep the patches consistent.



> +	virtual int initSensor(const V4L2ControlInfoMap &controls) = 0;


Should an IPA know anything about V4L2Controls?

My initial thought here is that this should be a ControlList being passed.

Will we need an initISP as well?

I wonder if all controls will go to the Sensor, or if perhaps some
controls from the loop will be handled by the ISP as well ...

That might be a pipelinehandler decision.

In fact, if we have a video stabilisation IPA, then that will most
certainly be updating a cropping parameter on the ISP rather than the
Sensor ... so it would be up to the Pipeline handler to interpret that.


> +	virtual void processRequest(const void *cookie,
> +				    const ControlList &controls,
> +				    Buffer &parameters) = 0;
> +	virtual void updateStatistics(const void *cookie, Buffer &statistics) = 0;
> +
> +	Signal<V4L2ControlList> updateSensor;
> +	Signal<const void *> queueRequest;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
> index b0e944a17fc5cffb..11dcc54ce4ebc6f4 100644
> --- a/src/ipa/ipa_dummy.cpp
> +++ b/src/ipa/ipa_dummy.cpp
> @@ -15,15 +15,12 @@ namespace libcamera {
>  class IPADummy : public IPAInterface
>  {
>  public:
> -	int init();
> +	int initSensor(const V4L2ControlInfoMap &controls) { return 0; }
> +	void processRequest(const void *cookie, const ControlList &controls,
> +			    Buffer &parameters) {}
> +	void updateStatistics(const void *cookie, Buffer &statistics) {}
>  };
>  
> -int IPADummy::init()
> -{
> -	std::cout << "initializing dummy IPA!" << std::endl;
> -	return 0;
> -}
> -
>  /*
>   * External IPA module interface
>   */
> diff --git a/src/ipa/ipa_dummy_isolate.cpp b/src/ipa/ipa_dummy_isolate.cpp
> index 24434e85d31809e2..4d49c0e372466af6 100644
> --- a/src/ipa/ipa_dummy_isolate.cpp
> +++ b/src/ipa/ipa_dummy_isolate.cpp
> @@ -16,15 +16,12 @@ namespace libcamera {
>  class IPADummyIsolate : public IPAInterface
>  {
>  public:
> -	int init();
> +	int initSensor(const V4L2ControlInfoMap &controls) { return 0; }
> +	void processRequest(const void *cookie, const ControlList &controls,
> +			    Buffer &parameters) {}
> +	void updateStatistics(const void *cookie, Buffer &statistics) {}
>  };
>  
> -int IPADummyIsolate::init()
> -{
> -	std::cout << "initializing isolated dummy IPA!" << std::endl;
> -	return 0;
> -}

Should we keep a basic init() call and have that called when the IPA is
created?

Or perhaps we should move these messages to the IPA constructors?


> -
>  /*
>   * External IPA module interface
>   */
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index 9d30da29228fabb7..4edcebbe84d5a0d0 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -20,8 +20,76 @@ namespace libcamera {
>   */
>  
>  /**
> - * \fn IPAInterface::init()
> - * \brief Initialise the IPAInterface
> + * \fn IPAInterface::initSensor()
> + * \brief Initialize the IPA sensor settings
> + * \param[in] controls List of controls provided by the sensor
> + *
> + * This function is called when a pipeline attaches to an IPA to inform the IPA
> + * of the controls and limits the sensor in the video pipeline supports. The IPA

I think that's what Camera controls do already ?

Would we ever expose a control available in the sensor to the IPA - but
*not* to the application?

> + * have the option to set controls of the sensor by emitting the updateSensor
> + * signal.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \fn IPAInterface::processRequest()
> + * \brief Provide IPA with a parameter buffer to fill
> + * \param[in] cookie Cookie for the request
> + * \param[in] controls List of controls associated with the request
> + * \param[in,out] parameters Buffer containing an empty parameters buffer
> + *
> + * This function is called by a pipeline handler when it has a request to
> + * process. The IPA shall fill in the \a parameters buffer to achieve the
> + * capture result described in \a controls. The \a cookie value identifiers

s/identifiers/identifies/

So this allows a specific block of control data to be written which will
be applied to the ISP?

I feel like this is also the opportunity to provide an updated/second
ControlList which will be queued in the Request to apply any other
controls at the correct time.


> + * the request the controls and parameters buffer corresponds to and can be
> + * matched in updateStatistics() once the request have been processed by the
> + * hardware. The cookie value is only valid from that processRequest() is
> + * called and until updateStatistics() return.
> + *
> + * When the \a parameters buffer is filled in and ready to be queued to hardware
> + * the IPA shall signal pipeline handler using the queueRequest signal with the
> + * cookie value corresponding to the request the parameters belong to.
> + */
> +
> +/**
> + * \fn IPAInterface::updateStatistics()
> + * \brief Provide IPA with statistic buffer to examine
> + * \param[in] cookie Cookie for the request
> + * \param[in] statistics Buffer containing a filled in statistic buffer
> + *
> + * This function is called once a statistic buffer is completed and have been
> + * dequeue. The IPA may inspect the buffer and update its internal view of the
> + * capture conditions. The \a cookie value can be used to associate the dequeued
> + * statistics buffer with the parameters buffer filled in by the IPA in
> + * processRequest().
> + *
> + * \todo Extend this function to return or signal the result of the statistic
> + * examination by the IPA.
> + */
> +
> +/**
> + * \var IPAInterface::updateSensor
> + * \brief Signal emitted when the IPA wish to update sensor V4L2 controls
> + *
> + * This signal is emitted when the IPA wish to update one or more V4L2 control
> + * of the sensor in the video pipeline. The list of controls to modify is passed
> + * as a parameter.

I fear this is running asynchronous to the ISP parameters queued up in
the Request.

At the moment, my initial thought is that an extra ControlList should be
added or appended to by the IPA in the request which will allow the
pipeline handler to set controls at the appropriate time based on any
request queue depth.

Using a signal here feels like we will lose all possibility for any
synchronisation required..

> + *
> + * \todo Extend this function to work with per-frame control setting of
> + * controls.

Indeed - I don't think it's a case of "extend" I think this works
directly against our needs and instead should be something put back in
to the Request.

Deailing with per-frame-controls and timings will be handled at the
pipeline handler layer, by the pipeline being able to peek into the
queue of Requests and see if it needs to set a control early if it will
take a known duration.

But to handle that - it needs to be done at the pipeline handler.


> + */
> +
> +/**
> + * \var IPAInterface::queueRequest
> + * \brief Signal emitted when the IPA is done preparing a request
> + *
> + * This signal is emitted then the IPA have finished filling in the parameters
> + * buffer and is ready for the request to be committed to hardware for capture.
> + * The request cookie is passed as a parameter.
> + *
> + * \todo Extend this function to work with per-frame control setting of
> + * controls.

so a pipeline handler will block the processing of a Request until this
signal is received?

(That sounds like it needs to, yes)


>   */
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index f881aab5b2e1178a..bb7aeedf12779c66 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -26,7 +26,10 @@ public:
>  	IPAProxyLinux(IPAModule *ipam);
>  	~IPAProxyLinux();
>  
> -	int init();
> +	int initSensor(const V4L2ControlInfoMap &controls) { return 0; }
> +	void processRequest(const void *cookie, const ControlList &controls,
> +			    Buffer &parameters) {}
> +	void updateStatistics(const void *cookie, Buffer &statistics) {}
>  
>  private:
>  	void readyRead(IPCUnixSocket *ipc);
> @@ -36,13 +39,6 @@ private:
>  	IPCUnixSocket *socket_;
>  };
>  
> -int IPAProxyLinux::init()
> -{
> -	LOG(IPAProxy, Debug) << "initializing IPA via dummy proxy!";
> -
> -	return 0;
> -}
> -
>  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)
>  {
>  	LOG(IPAProxy, Debug)
>
Niklas Söderlund Aug. 29, 2019, 9:39 p.m. UTC | #2
Hi Kieran,

Thanks for your feedback.

On 2019-08-29 16:42:53 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 28/08/2019 02:17, Niklas Söderlund wrote:
> > The IPA interface needs to support interactions with the pipeline, add
> > interfaces to control the sensor and handling of request ISP parameters
> > and statistics.
> > 
> > The interface needs to be extended further to support attaching the
> > result of the statistic analyses to the request before the request
> > completes to the user. It also needs to modified to allow proper per
> > frame control of capture parameters.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/libcamera/ipa/ipa_interface.h   | 17 +++++-
> >  src/ipa/ipa_dummy.cpp                   | 11 ++--
> >  src/ipa/ipa_dummy_isolate.cpp           | 11 ++--
> >  src/libcamera/ipa_interface.cpp         | 72 ++++++++++++++++++++++++-
> >  src/libcamera/proxy/ipa_proxy_linux.cpp | 12 ++---
> >  5 files changed, 98 insertions(+), 25 deletions(-)
> > 
> > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> > index 2c5eb1fd524311cb..38e16ff37214f7b9 100644
> > --- a/include/libcamera/ipa/ipa_interface.h
> > +++ b/include/libcamera/ipa/ipa_interface.h
> > @@ -7,14 +7,29 @@
> >  #ifndef __LIBCAMERA_IPA_INTERFACE_H__
> >  #define __LIBCAMERA_IPA_INTERFACE_H__
> >  
> > +#include <libcamera/controls.h>
> > +#include <libcamera/signal.h>
> > +
> > +#include "v4l2_controls.h"
> > +
> >  namespace libcamera {
> >  
> > +class Buffer;
> > +class Request;
> > +
> >  class IPAInterface
> >  {
> >  public:
> >  	virtual ~IPAInterface() {}
> >  
> > -	virtual int init() = 0;
> 
> I see now why you don't call init() in patch 2/13 - but I think to keep
> the patches 'correct' then if you are removing init() here, you should
> add the call in to 2/13, and then remove it here. Tedious I know - but
> it will keep the patches consistent.

I chose to remove the init() call as a new first patch in this series to 
sidestep the problem ;-)

> 
> 
> 
> > +	virtual int initSensor(const V4L2ControlInfoMap &controls) = 0;
> 
> 
> Should an IPA know anything about V4L2Controls?

That is a good question.

In this series I drawn the boundary where V4L2 knowledge enters the 
stage at the specific pipeline handler implementation. And I vision the 
IPA module as an extension of the pipeline handler. Acting at a low 
level where I think we can safely assume using V4L2 will not hurt this 
moving forward.

If we ever get another video backend we will need a new IPA interface to 
facilitate it, but we will also need a new pipeline handler interface so 
that is no issue. And I don't see an IPA implementation working with 
different kernel video systems. The important part is that the 
application view should be the same in both cases.

> 
> My initial thought here is that this should be a ControlList being passed.
> 
> Will we need an initISP as well?

I don't think we need that. My current understanding of the ISPs we have 
seen is that they are stateless. You give it all parameters at the same 
time as the buffer and there is no "timing" involved.

> 
> I wonder if all controls will go to the Sensor, or if perhaps some
> controls from the loop will be handled by the ISP as well ...

We will likely need controls for the ISP at some point, but as we don't 
have such drivers today I fear trying to model that now will do us no 
good.

> 
> That might be a pipelinehandler decision.
> 
> In fact, if we have a video stabilisation IPA, then that will most
> certainly be updating a cropping parameter on the ISP rather than the
> Sensor ... so it would be up to the Pipeline handler to interpret that.

True.

> 
> 
> > +	virtual void processRequest(const void *cookie,
> > +				    const ControlList &controls,
> > +				    Buffer &parameters) = 0;
> > +	virtual void updateStatistics(const void *cookie, Buffer &statistics) = 0;
> > +
> > +	Signal<V4L2ControlList> updateSensor;
> > +	Signal<const void *> queueRequest;
> >  };
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
> > index b0e944a17fc5cffb..11dcc54ce4ebc6f4 100644
> > --- a/src/ipa/ipa_dummy.cpp
> > +++ b/src/ipa/ipa_dummy.cpp
> > @@ -15,15 +15,12 @@ namespace libcamera {
> >  class IPADummy : public IPAInterface
> >  {
> >  public:
> > -	int init();
> > +	int initSensor(const V4L2ControlInfoMap &controls) { return 0; }
> > +	void processRequest(const void *cookie, const ControlList &controls,
> > +			    Buffer &parameters) {}
> > +	void updateStatistics(const void *cookie, Buffer &statistics) {}
> >  };
> >  
> > -int IPADummy::init()
> > -{
> > -	std::cout << "initializing dummy IPA!" << std::endl;
> > -	return 0;
> > -}
> > -
> >  /*
> >   * External IPA module interface
> >   */
> > diff --git a/src/ipa/ipa_dummy_isolate.cpp b/src/ipa/ipa_dummy_isolate.cpp
> > index 24434e85d31809e2..4d49c0e372466af6 100644
> > --- a/src/ipa/ipa_dummy_isolate.cpp
> > +++ b/src/ipa/ipa_dummy_isolate.cpp
> > @@ -16,15 +16,12 @@ namespace libcamera {
> >  class IPADummyIsolate : public IPAInterface
> >  {
> >  public:
> > -	int init();
> > +	int initSensor(const V4L2ControlInfoMap &controls) { return 0; }
> > +	void processRequest(const void *cookie, const ControlList &controls,
> > +			    Buffer &parameters) {}
> > +	void updateStatistics(const void *cookie, Buffer &statistics) {}
> >  };
> >  
> > -int IPADummyIsolate::init()
> > -{
> > -	std::cout << "initializing isolated dummy IPA!" << std::endl;
> > -	return 0;
> > -}
> 
> Should we keep a basic init() call and have that called when the IPA is
> created?
> 
> Or perhaps we should move these messages to the IPA constructors?

I see no need for a init call that just prints a string, we can always 
add an init() when/if it's needed.

> 
> 
> > -
> >  /*
> >   * External IPA module interface
> >   */
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index 9d30da29228fabb7..4edcebbe84d5a0d0 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -20,8 +20,76 @@ namespace libcamera {
> >   */
> >  
> >  /**
> > - * \fn IPAInterface::init()
> > - * \brief Initialise the IPAInterface
> > + * \fn IPAInterface::initSensor()
> > + * \brief Initialize the IPA sensor settings
> > + * \param[in] controls List of controls provided by the sensor
> > + *
> > + * This function is called when a pipeline attaches to an IPA to inform the IPA
> > + * of the controls and limits the sensor in the video pipeline supports. The IPA
> 
> I think that's what Camera controls do already ?

They do, but this is to inform the IPA of what controls are available on 
the sensor and what values are valid there. Remember the IPA can run in 
an separate process and can not inspect the Camera for this.

> 
> Would we ever expose a control available in the sensor to the IPA - but
> *not* to the application?

I don't think so.

> 
> > + * have the option to set controls of the sensor by emitting the updateSensor
> > + * signal.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::processRequest()
> > + * \brief Provide IPA with a parameter buffer to fill
> > + * \param[in] cookie Cookie for the request
> > + * \param[in] controls List of controls associated with the request
> > + * \param[in,out] parameters Buffer containing an empty parameters buffer
> > + *
> > + * This function is called by a pipeline handler when it has a request to
> > + * process. The IPA shall fill in the \a parameters buffer to achieve the
> > + * capture result described in \a controls. The \a cookie value identifiers
> 
> s/identifiers/identifies/
> 
> So this allows a specific block of control data to be written which will
> be applied to the ISP?

Yes, this allows the IPA to fill the ISP parameters buffer before the 
request is queued to hardware.

> 
> I feel like this is also the opportunity to provide an updated/second
> ControlList which will be queued in the Request to apply any other
> controls at the correct time.

I agree, the IPA can emit the updateSensor() signal her to set controls 
on the sensor. Of course the updateSensor() singnal needs to be extended 
to support per frame control once we start working on that for this to 
be truly effective. At this point it's just on a best effort basis.

As you talk about in the commit message, ISP V4L2 controls is a candiate 
we might need to handle here once/if we need them.

> 
> 
> > + * the request the controls and parameters buffer corresponds to and can be
> > + * matched in updateStatistics() once the request have been processed by the
> > + * hardware. The cookie value is only valid from that processRequest() is
> > + * called and until updateStatistics() return.
> > + *
> > + * When the \a parameters buffer is filled in and ready to be queued to hardware
> > + * the IPA shall signal pipeline handler using the queueRequest signal with the
> > + * cookie value corresponding to the request the parameters belong to.
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::updateStatistics()
> > + * \brief Provide IPA with statistic buffer to examine
> > + * \param[in] cookie Cookie for the request
> > + * \param[in] statistics Buffer containing a filled in statistic buffer
> > + *
> > + * This function is called once a statistic buffer is completed and have been
> > + * dequeue. The IPA may inspect the buffer and update its internal view of the
> > + * capture conditions. The \a cookie value can be used to associate the dequeued
> > + * statistics buffer with the parameters buffer filled in by the IPA in
> > + * processRequest().
> > + *
> > + * \todo Extend this function to return or signal the result of the statistic
> > + * examination by the IPA.
> > + */
> > +
> > +/**
> > + * \var IPAInterface::updateSensor
> > + * \brief Signal emitted when the IPA wish to update sensor V4L2 controls
> > + *
> > + * This signal is emitted when the IPA wish to update one or more V4L2 control
> > + * of the sensor in the video pipeline. The list of controls to modify is passed
> > + * as a parameter.
> 
> I fear this is running asynchronous to the ISP parameters queued up in
> the Request.

Yes, this needs to be solved once we start working on per frame control.  
I envision this being extended with either a frame number or timestamp 
of when in the future the controls in the updateSensor signal should be 
applied. But I'm sure we find something better once we tackle per frame 
controls for real.

For now this is the best I think we can do.

> 
> At the moment, my initial thought is that an extra ControlList should be
> added or appended to by the IPA in the request which will allow the
> pipeline handler to set controls at the appropriate time based on any
> request queue depth.
> 
> Using a signal here feels like we will lose all possibility for any
> synchronisation required..

The interface between pipeline and IPA needs to be asynchronous to 
facilitate IPA isolation. The synchronisation will need to be built into 
how we choose to solve per frame controls.

> 
> > + *
> > + * \todo Extend this function to work with per-frame control setting of
> > + * controls.
> 
> Indeed - I don't think it's a case of "extend" I think this works
> directly against our needs and instead should be something put back in
> to the Request.

The IPA can't access the request object as it will be updated 
asynchronously in a different thread/process by the pipeline manager who 
at this point is the owner of the request object.

> 
> Deailing with per-frame-controls and timings will be handled at the
> pipeline handler layer, by the pipeline being able to peek into the
> queue of Requests and see if it needs to set a control early if it will
> take a known duration.

I think it's the IPA who will peek into the future and emit signals with 
a timestamp or other synchronisation tool, while the pipeline is 
responsible for managing the timeline and applying controls and queuing 
buffers at the right time.

It would be nice if the interaction between pipeline and IPA could be 
frame number based. While the internal of the pipeline deals with 
timestamps.

The IPA knows it needs to apply controls X frames in advance (pipe 
depth) and emits signals with a frame number (in the future) where it 
needs a control to have a certain valie. Then the pipeline handler 
translates that to a timestamp and makes sure the control is applied on 
time.

> 
> But to handle that - it needs to be done at the pipeline handler.
> 
> 
> > + */
> > +
> > +/**
> > + * \var IPAInterface::queueRequest
> > + * \brief Signal emitted when the IPA is done preparing a request
> > + *
> > + * This signal is emitted then the IPA have finished filling in the parameters
> > + * buffer and is ready for the request to be committed to hardware for capture.
> > + * The request cookie is passed as a parameter.
> > + *
> > + * \todo Extend this function to work with per-frame control setting of
> > + * controls.
> 
> so a pipeline handler will block the processing of a Request until this
> signal is received?
> 
> (That sounds like it needs to, yes)

Yes, the IPA will need to tell the pipeline handler when it wants a 
request to be committed to hardware.

> 
> 
> >   */
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index f881aab5b2e1178a..bb7aeedf12779c66 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -26,7 +26,10 @@ public:
> >  	IPAProxyLinux(IPAModule *ipam);
> >  	~IPAProxyLinux();
> >  
> > -	int init();
> > +	int initSensor(const V4L2ControlInfoMap &controls) { return 0; }
> > +	void processRequest(const void *cookie, const ControlList &controls,
> > +			    Buffer &parameters) {}
> > +	void updateStatistics(const void *cookie, Buffer &statistics) {}
> >  
> >  private:
> >  	void readyRead(IPCUnixSocket *ipc);
> > @@ -36,13 +39,6 @@ private:
> >  	IPCUnixSocket *socket_;
> >  };
> >  
> > -int IPAProxyLinux::init()
> > -{
> > -	LOG(IPAProxy, Debug) << "initializing IPA via dummy proxy!";
> > -
> > -	return 0;
> > -}
> > -
> >  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)
> >  {
> >  	LOG(IPAProxy, Debug)
> > 
> 
> -- 
> Regards
> --
> Kieran

Patch

diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
index 2c5eb1fd524311cb..38e16ff37214f7b9 100644
--- a/include/libcamera/ipa/ipa_interface.h
+++ b/include/libcamera/ipa/ipa_interface.h
@@ -7,14 +7,29 @@ 
 #ifndef __LIBCAMERA_IPA_INTERFACE_H__
 #define __LIBCAMERA_IPA_INTERFACE_H__
 
+#include <libcamera/controls.h>
+#include <libcamera/signal.h>
+
+#include "v4l2_controls.h"
+
 namespace libcamera {
 
+class Buffer;
+class Request;
+
 class IPAInterface
 {
 public:
 	virtual ~IPAInterface() {}
 
-	virtual int init() = 0;
+	virtual int initSensor(const V4L2ControlInfoMap &controls) = 0;
+	virtual void processRequest(const void *cookie,
+				    const ControlList &controls,
+				    Buffer &parameters) = 0;
+	virtual void updateStatistics(const void *cookie, Buffer &statistics) = 0;
+
+	Signal<V4L2ControlList> updateSensor;
+	Signal<const void *> queueRequest;
 };
 
 } /* namespace libcamera */
diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
index b0e944a17fc5cffb..11dcc54ce4ebc6f4 100644
--- a/src/ipa/ipa_dummy.cpp
+++ b/src/ipa/ipa_dummy.cpp
@@ -15,15 +15,12 @@  namespace libcamera {
 class IPADummy : public IPAInterface
 {
 public:
-	int init();
+	int initSensor(const V4L2ControlInfoMap &controls) { return 0; }
+	void processRequest(const void *cookie, const ControlList &controls,
+			    Buffer &parameters) {}
+	void updateStatistics(const void *cookie, Buffer &statistics) {}
 };
 
-int IPADummy::init()
-{
-	std::cout << "initializing dummy IPA!" << std::endl;
-	return 0;
-}
-
 /*
  * External IPA module interface
  */
diff --git a/src/ipa/ipa_dummy_isolate.cpp b/src/ipa/ipa_dummy_isolate.cpp
index 24434e85d31809e2..4d49c0e372466af6 100644
--- a/src/ipa/ipa_dummy_isolate.cpp
+++ b/src/ipa/ipa_dummy_isolate.cpp
@@ -16,15 +16,12 @@  namespace libcamera {
 class IPADummyIsolate : public IPAInterface
 {
 public:
-	int init();
+	int initSensor(const V4L2ControlInfoMap &controls) { return 0; }
+	void processRequest(const void *cookie, const ControlList &controls,
+			    Buffer &parameters) {}
+	void updateStatistics(const void *cookie, Buffer &statistics) {}
 };
 
-int IPADummyIsolate::init()
-{
-	std::cout << "initializing isolated dummy IPA!" << std::endl;
-	return 0;
-}
-
 /*
  * External IPA module interface
  */
diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
index 9d30da29228fabb7..4edcebbe84d5a0d0 100644
--- a/src/libcamera/ipa_interface.cpp
+++ b/src/libcamera/ipa_interface.cpp
@@ -20,8 +20,76 @@  namespace libcamera {
  */
 
 /**
- * \fn IPAInterface::init()
- * \brief Initialise the IPAInterface
+ * \fn IPAInterface::initSensor()
+ * \brief Initialize the IPA sensor settings
+ * \param[in] controls List of controls provided by the sensor
+ *
+ * This function is called when a pipeline attaches to an IPA to inform the IPA
+ * of the controls and limits the sensor in the video pipeline supports. The IPA
+ * have the option to set controls of the sensor by emitting the updateSensor
+ * signal.
+ *
+ * \return 0 on success or a negative error code otherwise
+ */
+
+/**
+ * \fn IPAInterface::processRequest()
+ * \brief Provide IPA with a parameter buffer to fill
+ * \param[in] cookie Cookie for the request
+ * \param[in] controls List of controls associated with the request
+ * \param[in,out] parameters Buffer containing an empty parameters buffer
+ *
+ * This function is called by a pipeline handler when it has a request to
+ * process. The IPA shall fill in the \a parameters buffer to achieve the
+ * capture result described in \a controls. The \a cookie value identifiers
+ * the request the controls and parameters buffer corresponds to and can be
+ * matched in updateStatistics() once the request have been processed by the
+ * hardware. The cookie value is only valid from that processRequest() is
+ * called and until updateStatistics() return.
+ *
+ * When the \a parameters buffer is filled in and ready to be queued to hardware
+ * the IPA shall signal pipeline handler using the queueRequest signal with the
+ * cookie value corresponding to the request the parameters belong to.
+ */
+
+/**
+ * \fn IPAInterface::updateStatistics()
+ * \brief Provide IPA with statistic buffer to examine
+ * \param[in] cookie Cookie for the request
+ * \param[in] statistics Buffer containing a filled in statistic buffer
+ *
+ * This function is called once a statistic buffer is completed and have been
+ * dequeue. The IPA may inspect the buffer and update its internal view of the
+ * capture conditions. The \a cookie value can be used to associate the dequeued
+ * statistics buffer with the parameters buffer filled in by the IPA in
+ * processRequest().
+ *
+ * \todo Extend this function to return or signal the result of the statistic
+ * examination by the IPA.
+ */
+
+/**
+ * \var IPAInterface::updateSensor
+ * \brief Signal emitted when the IPA wish to update sensor V4L2 controls
+ *
+ * This signal is emitted when the IPA wish to update one or more V4L2 control
+ * of the sensor in the video pipeline. The list of controls to modify is passed
+ * as a parameter.
+ *
+ * \todo Extend this function to work with per-frame control setting of
+ * controls.
+ */
+
+/**
+ * \var IPAInterface::queueRequest
+ * \brief Signal emitted when the IPA is done preparing a request
+ *
+ * This signal is emitted then the IPA have finished filling in the parameters
+ * buffer and is ready for the request to be committed to hardware for capture.
+ * The request cookie is passed as a parameter.
+ *
+ * \todo Extend this function to work with per-frame control setting of
+ * controls.
  */
 
 } /* namespace libcamera */
diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
index f881aab5b2e1178a..bb7aeedf12779c66 100644
--- a/src/libcamera/proxy/ipa_proxy_linux.cpp
+++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
@@ -26,7 +26,10 @@  public:
 	IPAProxyLinux(IPAModule *ipam);
 	~IPAProxyLinux();
 
-	int init();
+	int initSensor(const V4L2ControlInfoMap &controls) { return 0; }
+	void processRequest(const void *cookie, const ControlList &controls,
+			    Buffer &parameters) {}
+	void updateStatistics(const void *cookie, Buffer &statistics) {}
 
 private:
 	void readyRead(IPCUnixSocket *ipc);
@@ -36,13 +39,6 @@  private:
 	IPCUnixSocket *socket_;
 };
 
-int IPAProxyLinux::init()
-{
-	LOG(IPAProxy, Debug) << "initializing IPA via dummy proxy!";
-
-	return 0;
-}
-
 IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)
 {
 	LOG(IPAProxy, Debug)