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

2145 diff mbox series

The IPA interface needs to support interactions with the pipeline; add
interfaces to control the sensor and handling of request ISP parameters
and statistics.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/ipa/ipa_interface.h             |  38 +++++
 src/ipa/ipa_dummy.cpp                   |   7 +-
 src/libcamera/ipa_interface.cpp         | 195 ++++++++++++++++++++++++
 src/libcamera/proxy/ipa_proxy_linux.cpp |  14 +-
 4 files changed, 245 insertions(+), 9 deletions(-)

Hi Niklas,
   minor notes, mostly on comments.

Feel free to collect what you consider more opportune.

On Tue, Oct 08, 2019 at 02:45:31AM +0200, 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.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/ipa/ipa_interface.h             |  38 +++++
>  src/ipa/ipa_dummy.cpp                   |   7 +-
>  src/libcamera/ipa_interface.cpp         | 195 ++++++++++++++++++++++++
>  src/libcamera/proxy/ipa_proxy_linux.cpp |  14 +-
>  4 files changed, 245 insertions(+), 9 deletions(-)
>
> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> index 2c5eb1fd524311cb..499340dc3a8c67c2 100644
> --- a/include/ipa/ipa_interface.h
> +++ b/include/ipa/ipa_interface.h
> @@ -7,14 +7,52 @@
>  #ifndef __LIBCAMERA_IPA_INTERFACE_H__
>  #define __LIBCAMERA_IPA_INTERFACE_H__
>
> +#include <map>
> +#include <vector>
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/controls.h>
> +#include <libcamera/geometry.h>
> +#include <libcamera/signal.h>
> +
> +#include "v4l2_controls.h"
> +
>  namespace libcamera {
>
> +struct IPAStream {
> +	unsigned int pixelFormat;
> +	Size size;
> +};
> +
> +struct IPABuffer {
> +	unsigned int type;
> +	unsigned int id;
> +	BufferMemory buffer;
> +};
> +
> +struct IPAOperationData {
> +	unsigned int operation;
> +	std::vector<uint32_t> data;
> +	std::vector<ControlList> controls;
> +	std::vector<V4L2ControlList> v4l2controls;

Why vectors of control lists? Do you envision IPAs to set controls for
multiple entities in a single operation ? you need a field to identify
to which entity the controls apply to, or is it something pipe/IPA
should handle somehow ?

> +	/* \todo: Add more vectors for data types used in pipa<->IPA interactions. */
> +};
> +
>  class IPAInterface
>  {
>  public:
>  	virtual ~IPAInterface() {}
>
>  	virtual int init() = 0;
> +
> +	virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +			       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) = 0;
> +
> +	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> +	virtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> +
> +	virtual void processEvent(const IPAOperationData &event) = 0;
> +	Signal<const IPAOperationData &> queueFrameAction;
>  };
>
>  } /* namespace libcamera */
> diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
> index 9d0cbdc8b1ad5565..08f74c4952f3923a 100644
> --- a/src/ipa/ipa_dummy.cpp
> +++ b/src/ipa/ipa_dummy.cpp
> @@ -15,7 +15,12 @@ namespace libcamera {
>  class IPADummy : public IPAInterface
>  {
>  public:
> -	int init();
> +	int init() override;
> +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void processEvent(const IPAOperationData &event) override {}
>  };
>
>  int IPADummy::init()
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index d7d8ca8881efcf66..c365d14c53669c21 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -14,9 +14,136 @@
>
>  namespace libcamera {
>
> +/**
> + * \struct IPAStream
> + * \brief Stream configuration for the IPA protocol

Here and in the other briefs, I would not make "IPA protocol" the
object of the statement. I would just reuse something like what we
have for StreamConfiguration

        \brief Configuration parameters for a stream


> + *
> + * The StreamConfiguration class contains too much information to be suitable
> + * for IPA protocols. The IPAStream structure mirrors the parameters from
> + * StreamConfiguration which are useful for the IPA.

I'm not sure mentioning that StreamConfiguration is too much is
something that applies to public documentation.

> + */
> +
> +/**
> + * \var IPAStream::pixelFormat

I might be playing in the 'too verbose' field, but I would expand
these with a few words to make them a little nicer to read. In
example:

> + * \brief The pixel format

        \brief The requested stream pixel format

        The pixel format encoding is specific to the pipeline/IPA
        implementation and might differ from the one configured from
        applications on the Camera.

> + */
> +
> +/**
> + * \var IPAStream::size
> + * \brief The size
> + */

        \brief The requested stream size
> +
> +/**
> + * \struct IPABuffer
> + * \brief Buffer information for the IPA protocol
> + *
> + * Buffers allocated by a pipeline handler might be needed inside the IPA. This
> + * structure allows the pipeline handler to associate a buffer with numerical
> + * ids that uniquely identify the buffer that can be used in the IPA protocol.

A suggestion, feel free to scratch or integrate what's better:
"IPA modules need to access buffers allocated by pipeline handlers.
This structure provides fields to establish a mapping between a buffer
descriptor and a numerical id, that uniquely identifies the buffer in
the following IPA operations calls. The mapping is established by calling
IPAInterface::mapBuffers() and stays valid until it is removed with a
call to IPAInterface::unmapBuffers().

> + *
> + * The numerical identifiers are \a type and \a id. Type describes which buffer
> + * pool \a buffer belongs to and id unique identifies the buffer within that
> + * pool.

I would leave type out from here and expand it below

> + *
> + * \sa IPAInterface::mapBuffers()
> + * \sa IPAInterface::unmapBuffers()
> + */
> +
> +/**
> + * \var IPABuffer::type
> + * \brief Type of the buffer

        \brief The buffer type

        Buffer type is a numerical identifier made available for IPA
        protocol implementations to store the intended buffer usage.
        The semantic associated to the values that can be assigned
        to a type is implementation specific and not enforced by any
        libcamera core component.

> + */
> +
> +/**
> + * \var IPABuffer::id
> + * \brief ID of the buffer

Here too I will spend a few more words on the accepted values, the
validity of the mapping etc.. Here or in the IPABuffer class
documentation as I've suggested

> + */
> +
> +/**
> + * \var IPABuffer::buffer
> + * \brief Hardware description of the buffer

Now that I think about this I'm a bit skeptical on passing a
BufferMemory while we should really provide an array of file descriptors.
But I know you and Laurent discussed this already and I'm fine
whatever.

Anyway, 'Hardware description' does not play well imo. BufferMemory
contains an array of planes exported as either fds and memory pointers,
so it actually abstracts a buffer by providing information on how to
access its content. I would just drop 'Hardware' and specify this is
intended for IPA to be able to access the buffer content as you have
done here below.

I wonder if the memory mapped pointer *mem_ should not be marked as
not-accessible by IPA modules running in a different process space
somehow..

> + *
> + * The description allows the IPA to map and unmap the buffer memory to access
> + * its content.

I would have detailed that a number of planes is exported as file
descriptors and the IPA modules can map them in their memory space,
but I would be actually describing BufferMemory. Not sure if it's
appropriate.

> + */
> +
> +/**
> + * \struct IPAOperationData
> + * \brief Parameters for IPA operations
> + *
> + * A pipeline defines its IPA protocol by describing how an  IPAOperationData

Double space between "an  IPAOperationData"

> + * structure shall be filled out for each of its operations. When the structure
> + * is populated it can be sent to the other part of the protocol and decoded.

This describes the IPA protocol definition, while the struct actually
contains a number of fields that can be used for that purpose, at
least that's my understanding of what you're trying to convey.

I would
"The IPAOperationData structure provides fields that are meant to be
used in the implementation of the IPA operations between pipeline
handlers and IPA modules.

Specific implementations realize their protocol by using the
IPAOperationData members. Some members, such as ControlList, have a well
intended meaning while others, such as data[] provide space for
protocol-specific implementations, to allow pipelines and IPAs to
freely exchange data by implementing a binary protocol on top of it.

> + *
> + * The \a operation describes which operation the receiver shall perform
> + * and implicitly describes which of the other fields in the structure are
> + * populated and how they shall be interpreted. All this is the responsibility
> + * of the pipeline implementation.

I would mix this with pieces of the above text. Up to you.

> + *
> + * \sa IPAInterface::processEvent()
> + * \sa IPAInterface::queueFrameAction
> + */
> +
> +/**
> + * \var IPAOperationData::operation
> + * \brief Operation in the pipeline-to-IPA protocol

"Operation identifier code in the ... " ?

> + *
> + * The allocation of numerical values to operations are left to the
> + * implementation of the pipeline protocol.

Not allocation but I would say "the definition of operation identifier
codes... "

> + */
> +
> +/**
> + * \var IPAOperationData::data
> + * \brief Array of unsigned int values

Array of binary data. The type we use to manage it is not relevant.

> + *
> + * The interpretation and position of different values in the array are left
> + * to the implementation of the pipeline protocol.
> + */
> +
> +/**
> + * \var IPAOperationData::controls
> + * \brief Array of ControlList
> + *
> + * The interpretation and position of different values in the array are left
> + * to the implementation of the pipeline protocol.
> + */
> +
> +/**
> + * \var IPAOperationData::v4l2controls
> + * \brief Array of V4L2ControlList
> + *
> + * The interpretation and position of different values in the array are left
> + * to the implementation of the pipeline protocol.
> +
>  /**
>   * \class IPAInterface
>   * \brief Interface for IPA implementation
> + *
> + * Every pipeline implementation in libcamera may attach all or some of its
> + * cameras to an Image Processing Algorithm (IPA) plugin. An IPA plugin is
> + * developed for a specific pipeline and each pipeline might have multiple
> + * competing IPA implementations, both open and closed source.

Should this be part of the interface description ? Isn't more a
general architecture comment, probably better located in the file
description ?

> + *
> + * To allow for multiple and competing IPA plugins for the same pipeline, an
> + * interface for the pipeline and IPA communication is needed; IPAInterface
> + * is this interface. The interface defines how and what type of data can be
> + * exchanged, not how the data exchanged makes up a protocol. Each pipeline's
> + * design is unique and so is its IPA protocol. It is the pipeline's
> + * responsibility to define and document a protocol on top of the IPAInterface
> + * which can be used by multiple IPA plugins.
> + *
> + * The IPAInterface provides some methods to help the protocol design. As the
> + * requirements of the IPA plugins become more clear it is expected more
> + * common protocol operations will be identified and added.
> + * \todo Add more common operations if possible.
> + *
> + * The pipeline shall use the IPAManager to locate an IPAInterface compatible
> + * with the pipeline. The interface may then be used in the pipeline to interact
> + * with the IPA plugin to make up the protocol.
> + * \todo Add reference to how pipelines shall document their protocol.
> + *
> + * \sa IPAManager
>   */
>
>  /**
> @@ -24,4 +151,72 @@ namespace libcamera {
>   * \brief Initialise the IPAInterface
>   */
>
> +/**
> + * \fn IPAInterface::configure()
> + * \brief Configure the IPA stream and sensor settings
> + * \param[in] streamConfig List of stream configuration descriptions
> + * \param[in] entityControls List of controls provided by the pipeline entities
> + *
> + * This operation shall be called when the camera is started to inform the IPA
> + * of the camera's streams and the sensor settings.

The entityControls are a map, how should it be interpreted? Is this up
to each implementation to define that?

In any case you are referring to 'the sensor' while the parameters are
settings-per-entity. I would make this clear in the documentation,
even if I assume you would like to make the interpretation of the
entity identifier (which is a numerical ID at the moment) up to each
implementation.

> + */
> +
> +/**
> + * \fn IPAInterface::mapBuffers()
> + * \brief Map the buffers shared by the pipeline to the IPA

Map buffers reserved by pipelines in the IPA address space ?

> + * \param[in] buffers List of buffers to map
> + *
> + * This operation is used to inform the IPA module of the memory buffers set up
> + * by the pipeline handler and associate to each of them a numerical \a type and
> + * \a id. All buffers that the pipeline wishes to share with an IPA must be
> + * mapped in the IPA with this operation.
> + *
> + * After the buffers have been initialized a specific buffer can be referenced
> + * using the numerical \a type and \a id provided when the buffers were mapped.
> + *
> + * The numerical values for type and id used to describe a buffer have no
> + * meaning in the core libcamera code and the interface is pipeline handler
> + * specific.

This captures part of my comments above here. Again I will stress how
long a mapping is valid for.

> + *
> + * \sa unmapBuffers()
> + * \todo Can we make this a generic function?

Isn't this 'generic' ? What do you mean here ?

> + */
> +
> +/**
> + * \fn IPAInterface::unmapBuffers()
> + * \brief Unmap the buffers shared by the pipeline to the IPA
> + * \param[in] buffers List of buffers to unmap
> + *
> + * This operation removes the mapping set up with mapBuffers().

When should this be called by pipeline handlers ? Will other mappings
not part of 'buffers' stay valid?

> + *
> + * \sa mapBuffers()
> + * \todo Can we make this a generic function?
> + */
> +
> +/**
> + * \fn IPAInterface::processEvent()
> + * \brief Process an event from the pipeline handler
> + * \param[in] event Event to process
> + *
> + * This operation is used by pipeline handlers to inform the IPA module of
> + * events that occurred during the on-going capture operation.
> + *
> + * Each \a event notified by the pipeline handler with this method is handled
> + * by the IPA which interprets the operation parameters in an implementation
> + * specific way, which needs to be separately documented.
> + */
> +
> +/**
> + * \fn IPAInterface::queueFrameAction()
> + * \brief Queue an action associated with a frame to the pipeline handler
> + * \param[in] frame The frame number for the action
> + * \param[in] controls List of controls associated with the action
> + *
> + * This signal is emitted when the IPA wishes to queue a FrameAction on the
> + * pipeline. The pipeline is still responsible for the scheduling of the action
> + * on its timeline.

Timeline is now a rk1isp specific thing. Should it be removed from
here and just mention a generic "responsible for handling the reported
action opportunely" ?

> + *
> + * The IPA operation describing the frame action is passed as a parameter.

Didn't get this to be honest :)

Thanks
   j

> + */
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index 62fcb529e1c7e4ff..14e8bb6067623fc6 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -26,7 +26,12 @@ public:
>  	IPAProxyLinux(IPAModule *ipam);
>  	~IPAProxyLinux();
>
> -	int init();
> +	int init() override { return 0; }
> +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void processEvent(const IPAOperationData &event) override {}
>
>  private:
>  	void readyRead(IPCUnixSocket *ipc);
> @@ -36,13 +41,6 @@ private:
>  	IPCUnixSocket *socket_;
>  };
>
> -int IPAProxyLinux::init()
> -{
> -	LOG(IPAProxy, Debug) << "initializing IPA via dummy proxy!";
> -
> -	return 0;
> -}
> -
>  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)
>  	: proc_(nullptr), socket_(nullptr)
>  {
> --
> 2.23.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

Thank you for the patch.

On Tue, Oct 08, 2019 at 02:45:31AM +0200, 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.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/ipa/ipa_interface.h             |  38 +++++
>  src/ipa/ipa_dummy.cpp                   |   7 +-
>  src/libcamera/ipa_interface.cpp         | 195 ++++++++++++++++++++++++
>  src/libcamera/proxy/ipa_proxy_linux.cpp |  14 +-
>  4 files changed, 245 insertions(+), 9 deletions(-)
> 
> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> index 2c5eb1fd524311cb..499340dc3a8c67c2 100644
> --- a/include/ipa/ipa_interface.h
> +++ b/include/ipa/ipa_interface.h
> @@ -7,14 +7,52 @@
>  #ifndef __LIBCAMERA_IPA_INTERFACE_H__
>  #define __LIBCAMERA_IPA_INTERFACE_H__
>  
> +#include <map>
> +#include <vector>
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/controls.h>
> +#include <libcamera/geometry.h>
> +#include <libcamera/signal.h>
> +
> +#include "v4l2_controls.h"

I had a quick look at how this could be rebased on top of my latest
control patches, but it's not trivial and will likely require me to send
a v2. Let's get this series merged first.

>  namespace libcamera {
>  
> +struct IPAStream {
> +	unsigned int pixelFormat;
> +	Size size;
> +};
> +
> +struct IPABuffer {
> +	unsigned int type;
> +	unsigned int id;
> +	BufferMemory buffer;
> +};
> +
> +struct IPAOperationData {
> +	unsigned int operation;
> +	std::vector<uint32_t> data;
> +	std::vector<ControlList> controls;
> +	std::vector<V4L2ControlList> v4l2controls;
> +	/* \todo: Add more vectors for data types used in pipa<->IPA interactions. */

s/todo:/todo/

Is this comment still valid ? What other data types do you foresee ?

> +};
> +
>  class IPAInterface
>  {
>  public:
>  	virtual ~IPAInterface() {}
>  
>  	virtual int init() = 0;
> +
> +	virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +			       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) = 0;
> +
> +	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> +	virtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;

Just thinking out loud, I think we'll need some sort of synchronous
status operation to verify that the initialisation went fine before
starting the streams. It can wait and be implemented on top of this
series.

> +	virtual void processEvent(const IPAOperationData &event) = 0;
> +	Signal<const IPAOperationData &> queueFrameAction;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
> index 9d0cbdc8b1ad5565..08f74c4952f3923a 100644
> --- a/src/ipa/ipa_dummy.cpp
> +++ b/src/ipa/ipa_dummy.cpp
> @@ -15,7 +15,12 @@ namespace libcamera {
>  class IPADummy : public IPAInterface
>  {
>  public:
> -	int init();
> +	int init() override;
> +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void processEvent(const IPAOperationData &event) override {}
>  };

I tried to rebase your series on top of Jacopo's IPA test series, and
without surprise this changes conflicts as the file has been renamed.
Conflict resolution is trivial though.

>  int IPADummy::init()
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index d7d8ca8881efcf66..c365d14c53669c21 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -14,9 +14,136 @@

I think we need some introduction here.

@@ -10,32 +10,50 @@
 /**
  * \file ipa_interface.h
  * \brief Image Processing Algorithm interface
+ *
+ * Pipeline handlers communicate with IPAs through a C++ interface defined by
+ * the IPAInterface class. The class defines high-level methods and signals to
+ * configure the IPA, notify it of events, and receive actions back from the
+ * IPA.
+ *
+ * IPAs can be isolated in a separate process. This implies that all arguments
+ * to the IPA interface may need to be transferred over IPC. The IPA interface
+ * thus uses serialisable data types only. The IPA interface defines custom data
+ * structures that mirror core libcamera structures when the latter are not
+ * suitable, such as IPAStream to carry StreamConfiguration data.
+ *
+ * Synchronous communication between pipeline handlers and IPAs can thus be
+ * costly. For that reason, the interface operates asynchronously. This implies
+ * that methods don't return a status, and that both methods and signals may
+ * copy their arguments.
  */

>  
>  namespace libcamera {
>  
> +/**
> + * \struct IPAStream
> + * \brief Stream configuration for the IPA protocol

"IPA interface" instead of "IPA protocol" ?

> + *
> + * The StreamConfiguration class contains too much information to be suitable
> + * for IPA protocols. The IPAStream structure mirrors the parameters from
> + * StreamConfiguration which are useful for the IPA.

 * The IPAStream structure stores stream configuration parameters needed by the
 * IPAInterface::configure() method. It mirrors the StreamConfiguration class
 * that is not suitable for this purpose due to not being serialisable.

> + */
> +
> +/**
> + * \var IPAStream::pixelFormat
> + * \brief The pixel format

"The stream pixel format" ?

> + */
> +
> +/**
> + * \var IPAStream::size
> + * \brief The size

"The stream size in pixels" (to match StreamConfiguration) ?

> + */
> +
> +/**
> + * \struct IPABuffer
> + * \brief Buffer information for the IPA protocol

"IPA interface" here too ?

> + *
> + * Buffers allocated by a pipeline handler might be needed inside the IPA. This
> + * structure allows the pipeline handler to associate a buffer with numerical
> + * ids that uniquely identify the buffer that can be used in the IPA protocol.
> + *
> + * The numerical identifiers are \a type and \a id. Type describes which buffer
> + * pool \a buffer belongs to and id unique identifies the buffer within that
> + * pool.

Do we really need a type here ? Wouldn't it be better to just associate
buffer memory with a unique ID in the map and unmap operations, and, if
a type is needed, passing it through the IPAOperationData for
processEvent or queueFrameAction ?

I had a quick look at what the implications would be on the rkisp1 IPA,
and it seems fine. The only problem I see is that
IPARkISP1::queueRequest() won't have a per-type free list anymore, but I
think it's actually not the role of the IPA to keep such a free list. I
think the pipeline handler should instead pass the ID of the params and
stats buffers to RKISP1_IPA_EVENT_QUEUE_REQUEST (actually the stats
buffer shouldn't be passed at queue request time, but that's a different
issue, we can address it on top).

We may then however need to add mapping flags to IPABuffer, to indicate
whether the IPA should map the buffer for read (mostly used for stats)
or write (mostly used for params, although in that case read/write is
probably the correct mapping type).

> + *
> + * \sa IPAInterface::mapBuffers()
> + * \sa IPAInterface::unmapBuffers()
> + */
> +
> +/**
> + * \var IPABuffer::type
> + * \brief Type of the buffer
> + */
> +
> +/**
> + * \var IPABuffer::id
> + * \brief ID of the buffer
> + */
> +
> +/**
> + * \var IPABuffer::buffer
> + * \brief Hardware description of the buffer
> + *
> + * The description allows the IPA to map and unmap the buffer memory to access
> + * its content.
> + */

I'll review the rest in a little bit.

> +
> +/**
> + * \struct IPAOperationData
> + * \brief Parameters for IPA operations
> + *
> + * A pipeline defines its IPA protocol by describing how an  IPAOperationData
> + * structure shall be filled out for each of its operations. When the structure
> + * is populated it can be sent to the other part of the protocol and decoded.
> + *
> + * The \a operation describes which operation the receiver shall perform
> + * and implicitly describes which of the other fields in the structure are
> + * populated and how they shall be interpreted. All this is the responsibility
> + * of the pipeline implementation.
> + *
> + * \sa IPAInterface::processEvent()
> + * \sa IPAInterface::queueFrameAction
> + */
> +
> +/**
> + * \var IPAOperationData::operation
> + * \brief Operation in the pipeline-to-IPA protocol
> + *
> + * The allocation of numerical values to operations are left to the
> + * implementation of the pipeline protocol.
> + */
> +
> +/**
> + * \var IPAOperationData::data
> + * \brief Array of unsigned int values
> + *
> + * The interpretation and position of different values in the array are left
> + * to the implementation of the pipeline protocol.
> + */
> +
> +/**
> + * \var IPAOperationData::controls
> + * \brief Array of ControlList
> + *
> + * The interpretation and position of different values in the array are left
> + * to the implementation of the pipeline protocol.
> + */
> +
> +/**
> + * \var IPAOperationData::v4l2controls
> + * \brief Array of V4L2ControlList
> + *
> + * The interpretation and position of different values in the array are left
> + * to the implementation of the pipeline protocol.
> + */
> +
>  /**
>   * \class IPAInterface
>   * \brief Interface for IPA implementation
> + *
> + * Every pipeline implementation in libcamera may attach all or some of its
> + * cameras to an Image Processing Algorithm (IPA) plugin. An IPA plugin is
> + * developed for a specific pipeline and each pipeline might have multiple
> + * competing IPA implementations, both open and closed source.
> + *
> + * To allow for multiple and competing IPA plugins for the same pipeline, an
> + * interface for the pipeline and IPA communication is needed; IPAInterface
> + * is this interface. The interface defines how and what type of data can be
> + * exchanged, not how the data exchanged makes up a protocol. Each pipeline's
> + * design is unique and so is its IPA protocol. It is the pipeline's
> + * responsibility to define and document a protocol on top of the IPAInterface
> + * which can be used by multiple IPA plugins.
> + *
> + * The IPAInterface provides some methods to help the protocol design. As the
> + * requirements of the IPA plugins become more clear it is expected more
> + * common protocol operations will be identified and added.
> + * \todo Add more common operations if possible.
> + *
> + * The pipeline shall use the IPAManager to locate an IPAInterface compatible
> + * with the pipeline. The interface may then be used in the pipeline to interact
> + * with the IPA plugin to make up the protocol.
> + * \todo Add reference to how pipelines shall document their protocol.
> + *
> + * \sa IPAManager
>   */
>  
>  /**
> @@ -24,4 +151,72 @@ namespace libcamera {
>   * \brief Initialise the IPAInterface
>   */
>  
> +/**
> + * \fn IPAInterface::configure()
> + * \brief Configure the IPA stream and sensor settings
> + * \param[in] streamConfig List of stream configuration descriptions
> + * \param[in] entityControls List of controls provided by the pipeline entities
> + *
> + * This operation shall be called when the camera is started to inform the IPA
> + * of the camera's streams and the sensor settings.
> + */
> +
> +/**
> + * \fn IPAInterface::mapBuffers()
> + * \brief Map the buffers shared by the pipeline to the IPA
> + * \param[in] buffers List of buffers to map
> + *
> + * This operation is used to inform the IPA module of the memory buffers set up
> + * by the pipeline handler and associate to each of them a numerical \a type and
> + * \a id. All buffers that the pipeline wishes to share with an IPA must be
> + * mapped in the IPA with this operation.
> + *
> + * After the buffers have been initialized a specific buffer can be referenced
> + * using the numerical \a type and \a id provided when the buffers were mapped.
> + *
> + * The numerical values for type and id used to describe a buffer have no
> + * meaning in the core libcamera code and the interface is pipeline handler
> + * specific.
> + *
> + * \sa unmapBuffers()
> + * \todo Can we make this a generic function?
> + */
> +
> +/**
> + * \fn IPAInterface::unmapBuffers()
> + * \brief Unmap the buffers shared by the pipeline to the IPA
> + * \param[in] buffers List of buffers to unmap
> + *
> + * This operation removes the mapping set up with mapBuffers().
> + *
> + * \sa mapBuffers()
> + * \todo Can we make this a generic function?
> + */
> +
> +/**
> + * \fn IPAInterface::processEvent()
> + * \brief Process an event from the pipeline handler
> + * \param[in] event Event to process
> + *
> + * This operation is used by pipeline handlers to inform the IPA module of
> + * events that occurred during the on-going capture operation.
> + *
> + * Each \a event notified by the pipeline handler with this method is handled
> + * by the IPA which interprets the operation parameters in an implementation
> + * specific way, which needs to be separately documented.
> + */
> +
> +/**
> + * \fn IPAInterface::queueFrameAction()
> + * \brief Queue an action associated with a frame to the pipeline handler
> + * \param[in] frame The frame number for the action
> + * \param[in] controls List of controls associated with the action
> + *
> + * This signal is emitted when the IPA wishes to queue a FrameAction on the
> + * pipeline. The pipeline is still responsible for the scheduling of the action
> + * on its timeline.
> + *
> + * The IPA operation describing the frame action is passed as a parameter.
> + */
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index 62fcb529e1c7e4ff..14e8bb6067623fc6 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -26,7 +26,12 @@ public:
>  	IPAProxyLinux(IPAModule *ipam);
>  	~IPAProxyLinux();
>  
> -	int init();
> +	int init() override { return 0; }
> +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void processEvent(const IPAOperationData &event) override {}
>  
>  private:
>  	void readyRead(IPCUnixSocket *ipc);
> @@ -36,13 +41,6 @@ private:
>  	IPCUnixSocket *socket_;
>  };
>  
> -int IPAProxyLinux::init()
> -{
> -	LOG(IPAProxy, Debug) << "initializing IPA via dummy proxy!";
> -
> -	return 0;
> -}
> -
>  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)
>  	: proc_(nullptr), socket_(nullptr)
>  {

Hello,

On Tue, Oct 08, 2019 at 11:46:20AM +0200, Jacopo Mondi wrote:
> Hi Niklas,
>    minor notes, mostly on comments.
> 
> Feel free to collect what you consider more opportune.
> 
> On Tue, Oct 08, 2019 at 02:45:31AM +0200, 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.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/ipa/ipa_interface.h             |  38 +++++
> >  src/ipa/ipa_dummy.cpp                   |   7 +-
> >  src/libcamera/ipa_interface.cpp         | 195 ++++++++++++++++++++++++
> >  src/libcamera/proxy/ipa_proxy_linux.cpp |  14 +-
> >  4 files changed, 245 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> > index 2c5eb1fd524311cb..499340dc3a8c67c2 100644
> > --- a/include/ipa/ipa_interface.h
> > +++ b/include/ipa/ipa_interface.h
> > @@ -7,14 +7,52 @@
> >  #ifndef __LIBCAMERA_IPA_INTERFACE_H__
> >  #define __LIBCAMERA_IPA_INTERFACE_H__
> >
> > +#include <map>
> > +#include <vector>
> > +
> > +#include <libcamera/buffer.h>
> > +#include <libcamera/controls.h>
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/signal.h>
> > +
> > +#include "v4l2_controls.h"
> > +
> >  namespace libcamera {
> >
> > +struct IPAStream {
> > +	unsigned int pixelFormat;
> > +	Size size;
> > +};
> > +
> > +struct IPABuffer {
> > +	unsigned int type;
> > +	unsigned int id;
> > +	BufferMemory buffer;
> > +};
> > +
> > +struct IPAOperationData {
> > +	unsigned int operation;
> > +	std::vector<uint32_t> data;
> > +	std::vector<ControlList> controls;
> > +	std::vector<V4L2ControlList> v4l2controls;
> 
> Why vectors of control lists? Do you envision IPAs to set controls for
> multiple entities in a single operation ? you need a field to identify
> to which entity the controls apply to, or is it something pipe/IPA
> should handle somehow ?
> 
> > +	/* \todo: Add more vectors for data types used in pipa<->IPA interactions. */
> > +};
> > +
> >  class IPAInterface
> >  {
> >  public:
> >  	virtual ~IPAInterface() {}
> >
> >  	virtual int init() = 0;
> > +
> > +	virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +			       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) = 0;
> > +
> > +	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > +	virtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > +
> > +	virtual void processEvent(const IPAOperationData &event) = 0;
> > +	Signal<const IPAOperationData &> queueFrameAction;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
> > index 9d0cbdc8b1ad5565..08f74c4952f3923a 100644
> > --- a/src/ipa/ipa_dummy.cpp
> > +++ b/src/ipa/ipa_dummy.cpp
> > @@ -15,7 +15,12 @@ namespace libcamera {
> >  class IPADummy : public IPAInterface
> >  {
> >  public:
> > -	int init();
> > +	int init() override;
> > +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> > +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void processEvent(const IPAOperationData &event) override {}
> >  };
> >
> >  int IPADummy::init()
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index d7d8ca8881efcf66..c365d14c53669c21 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -14,9 +14,136 @@
> >
> >  namespace libcamera {
> >
> > +/**
> > + * \struct IPAStream
> > + * \brief Stream configuration for the IPA protocol
> 
> Here and in the other briefs, I would not make "IPA protocol" the
> object of the statement. I would just reuse something like what we
> have for StreamConfiguration
> 
>         \brief Configuration parameters for a stream
> 
> 
> > + *
> > + * The StreamConfiguration class contains too much information to be suitable
> > + * for IPA protocols. The IPAStream structure mirrors the parameters from
> > + * StreamConfiguration which are useful for the IPA.
> 
> I'm not sure mentioning that StreamConfiguration is too much is
> something that applies to public documentation.
> 
> > + */
> > +
> > +/**
> > + * \var IPAStream::pixelFormat
> 
> I might be playing in the 'too verbose' field, but I would expand
> these with a few words to make them a little nicer to read. In
> example:
> 
> > + * \brief The pixel format
> 
>         \brief The requested stream pixel format
> 
>         The pixel format encoding is specific to the pipeline/IPA
>         implementation and might differ from the one configured from
>         applications on the Camera.

Can it ? I thought it should be the same.

> 
> > + */
> > +
> > +/**
> > + * \var IPAStream::size
> > + * \brief The size
> > + */
> 
>         \brief The requested stream size
> > +
> > +/**
> > + * \struct IPABuffer
> > + * \brief Buffer information for the IPA protocol
> > + *
> > + * Buffers allocated by a pipeline handler might be needed inside the IPA. This
> > + * structure allows the pipeline handler to associate a buffer with numerical
> > + * ids that uniquely identify the buffer that can be used in the IPA protocol.
> 
> A suggestion, feel free to scratch or integrate what's better:
> "IPA modules need to access buffers allocated by pipeline handlers.
> This structure provides fields to establish a mapping between a buffer
> descriptor and a numerical id, that uniquely identifies the buffer in
> the following IPA operations calls. The mapping is established by calling
> IPAInterface::mapBuffers() and stays valid until it is removed with a
> call to IPAInterface::unmapBuffers().
> 
> > + *
> > + * The numerical identifiers are \a type and \a id. Type describes which buffer
> > + * pool \a buffer belongs to and id unique identifies the buffer within that
> > + * pool.
> 
> I would leave type out from here and expand it below
> 
> > + *
> > + * \sa IPAInterface::mapBuffers()
> > + * \sa IPAInterface::unmapBuffers()
> > + */
> > +
> > +/**
> > + * \var IPABuffer::type
> > + * \brief Type of the buffer
> 
>         \brief The buffer type
> 
>         Buffer type is a numerical identifier made available for IPA
>         protocol implementations to store the intended buffer usage.
>         The semantic associated to the values that can be assigned
>         to a type is implementation specific and not enforced by any
>         libcamera core component.
> 
> > + */
> > +
> > +/**
> > + * \var IPABuffer::id
> > + * \brief ID of the buffer
> 
> Here too I will spend a few more words on the accepted values, the
> validity of the mapping etc.. Here or in the IPABuffer class
> documentation as I've suggested
> 
> > + */
> > +
> > +/**
> > + * \var IPABuffer::buffer
> > + * \brief Hardware description of the buffer
> 
> Now that I think about this I'm a bit skeptical on passing a
> BufferMemory while we should really provide an array of file descriptors.
> But I know you and Laurent discussed this already and I'm fine
> whatever.
> 
> Anyway, 'Hardware description' does not play well imo. BufferMemory
> contains an array of planes exported as either fds and memory pointers,
> so it actually abstracts a buffer by providing information on how to
> access its content. I would just drop 'Hardware' and specify this is
> intended for IPA to be able to access the buffer content as you have
> done here below.
> 
> I wonder if the memory mapped pointer *mem_ should not be marked as
> not-accessible by IPA modules running in a different process space
> somehow..
> 
> > + *
> > + * The description allows the IPA to map and unmap the buffer memory to access
> > + * its content.
> 
> I would have detailed that a number of planes is exported as file
> descriptors and the IPA modules can map them in their memory space,
> but I would be actually describing BufferMemory. Not sure if it's
> appropriate.
> 
> > + */
> > +
> > +/**
> > + * \struct IPAOperationData
> > + * \brief Parameters for IPA operations
> > + *
> > + * A pipeline defines its IPA protocol by describing how an  IPAOperationData
> 
> Double space between "an  IPAOperationData"
> 
> > + * structure shall be filled out for each of its operations. When the structure
> > + * is populated it can be sent to the other part of the protocol and decoded.
> 
> This describes the IPA protocol definition, while the struct actually
> contains a number of fields that can be used for that purpose, at
> least that's my understanding of what you're trying to convey.
> 
> I would
> "The IPAOperationData structure provides fields that are meant to be
> used in the implementation of the IPA operations between pipeline
> handlers and IPA modules.
> 
> Specific implementations realize their protocol by using the
> IPAOperationData members. Some members, such as ControlList, have a well
> intended meaning while others, such as data[] provide space for
> protocol-specific implementations, to allow pipelines and IPAs to
> freely exchange data by implementing a binary protocol on top of it.
> 
> > + *
> > + * The \a operation describes which operation the receiver shall perform
> > + * and implicitly describes which of the other fields in the structure are
> > + * populated and how they shall be interpreted. All this is the responsibility
> > + * of the pipeline implementation.
> 
> I would mix this with pieces of the above text. Up to you.
> 
> > + *
> > + * \sa IPAInterface::processEvent()
> > + * \sa IPAInterface::queueFrameAction
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::operation
> > + * \brief Operation in the pipeline-to-IPA protocol
> 
> "Operation identifier code in the ... " ?
> 
> > + *
> > + * The allocation of numerical values to operations are left to the
> > + * implementation of the pipeline protocol.
> 
> Not allocation but I would say "the definition of operation identifier
> codes... "
> 
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::data
> > + * \brief Array of unsigned int values
> 
> Array of binary data. The type we use to manage it is not relevant.
> 
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::controls
> > + * \brief Array of ControlList
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::v4l2controls
> > + * \brief Array of V4L2ControlList
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> > +
> >  /**
> >   * \class IPAInterface
> >   * \brief Interface for IPA implementation
> > + *
> > + * Every pipeline implementation in libcamera may attach all or some of its
> > + * cameras to an Image Processing Algorithm (IPA) plugin. An IPA plugin is
> > + * developed for a specific pipeline and each pipeline might have multiple
> > + * competing IPA implementations, both open and closed source.
> 
> Should this be part of the interface description ? Isn't more a
> general architecture comment, probably better located in the file
> description ?
> 
> > + *
> > + * To allow for multiple and competing IPA plugins for the same pipeline, an
> > + * interface for the pipeline and IPA communication is needed; IPAInterface
> > + * is this interface. The interface defines how and what type of data can be
> > + * exchanged, not how the data exchanged makes up a protocol. Each pipeline's
> > + * design is unique and so is its IPA protocol. It is the pipeline's
> > + * responsibility to define and document a protocol on top of the IPAInterface
> > + * which can be used by multiple IPA plugins.
> > + *
> > + * The IPAInterface provides some methods to help the protocol design. As the
> > + * requirements of the IPA plugins become more clear it is expected more
> > + * common protocol operations will be identified and added.
> > + * \todo Add more common operations if possible.
> > + *
> > + * The pipeline shall use the IPAManager to locate an IPAInterface compatible
> > + * with the pipeline. The interface may then be used in the pipeline to interact
> > + * with the IPA plugin to make up the protocol.
> > + * \todo Add reference to how pipelines shall document their protocol.
> > + *
> > + * \sa IPAManager
> >   */
> >
> >  /**
> > @@ -24,4 +151,72 @@ namespace libcamera {
> >   * \brief Initialise the IPAInterface
> >   */
> >
> > +/**
> > + * \fn IPAInterface::configure()
> > + * \brief Configure the IPA stream and sensor settings
> > + * \param[in] streamConfig List of stream configuration descriptions
> > + * \param[in] entityControls List of controls provided by the pipeline entities
> > + *
> > + * This operation shall be called when the camera is started to inform the IPA
> > + * of the camera's streams and the sensor settings.
> 
> The entityControls are a map, how should it be interpreted? Is this up
> to each implementation to define that?
> 
> In any case you are referring to 'the sensor' while the parameters are
> settings-per-entity. I would make this clear in the documentation,
> even if I assume you would like to make the interpretation of the
> entity identifier (which is a numerical ID at the moment) up to each
> implementation.
> 
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::mapBuffers()
> > + * \brief Map the buffers shared by the pipeline to the IPA
> 
> Map buffers reserved by pipelines in the IPA address space ?
> 
> > + * \param[in] buffers List of buffers to map
> > + *
> > + * This operation is used to inform the IPA module of the memory buffers set up
> > + * by the pipeline handler and associate to each of them a numerical \a type and
> > + * \a id. All buffers that the pipeline wishes to share with an IPA must be
> > + * mapped in the IPA with this operation.
> > + *
> > + * After the buffers have been initialized a specific buffer can be referenced
> > + * using the numerical \a type and \a id provided when the buffers were mapped.
> > + *
> > + * The numerical values for type and id used to describe a buffer have no
> > + * meaning in the core libcamera code and the interface is pipeline handler
> > + * specific.
> 
> This captures part of my comments above here. Again I will stress how
> long a mapping is valid for.
> 
> > + *
> > + * \sa unmapBuffers()
> > + * \todo Can we make this a generic function?
> 
> Isn't this 'generic' ? What do you mean here ?
> 
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::unmapBuffers()
> > + * \brief Unmap the buffers shared by the pipeline to the IPA
> > + * \param[in] buffers List of buffers to unmap
> > + *
> > + * This operation removes the mapping set up with mapBuffers().
> 
> When should this be called by pipeline handlers ? Will other mappings
> not part of 'buffers' stay valid?
> 
> > + *
> > + * \sa mapBuffers()
> > + * \todo Can we make this a generic function?
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::processEvent()
> > + * \brief Process an event from the pipeline handler
> > + * \param[in] event Event to process
> > + *
> > + * This operation is used by pipeline handlers to inform the IPA module of
> > + * events that occurred during the on-going capture operation.
> > + *
> > + * Each \a event notified by the pipeline handler with this method is handled
> > + * by the IPA which interprets the operation parameters in an implementation
> > + * specific way, which needs to be separately documented.
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::queueFrameAction()
> > + * \brief Queue an action associated with a frame to the pipeline handler
> > + * \param[in] frame The frame number for the action
> > + * \param[in] controls List of controls associated with the action
> > + *
> > + * This signal is emitted when the IPA wishes to queue a FrameAction on the
> > + * pipeline. The pipeline is still responsible for the scheduling of the action
> > + * on its timeline.
> 
> Timeline is now a rk1isp specific thing. Should it be removed from
> here and just mention a generic "responsible for handling the reported
> action opportunely" ?
> 
> > + *
> > + * The IPA operation describing the frame action is passed as a parameter.
> 
> Didn't get this to be honest :)
> 
> > + */
> > +
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index 62fcb529e1c7e4ff..14e8bb6067623fc6 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -26,7 +26,12 @@ public:
> >  	IPAProxyLinux(IPAModule *ipam);
> >  	~IPAProxyLinux();
> >
> > -	int init();
> > +	int init() override { return 0; }
> > +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> > +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void processEvent(const IPAOperationData &event) override {}
> >
> >  private:
> >  	void readyRead(IPCUnixSocket *ipc);
> > @@ -36,13 +41,6 @@ private:
> >  	IPCUnixSocket *socket_;
> >  };
> >
> > -int IPAProxyLinux::init()
> > -{
> > -	LOG(IPAProxy, Debug) << "initializing IPA via dummy proxy!";
> > -
> > -	return 0;
> > -}
> > -
> >  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)
> >  	: proc_(nullptr), socket_(nullptr)
> >  {

Hi Laurent,

On Wed, Oct 09, 2019 at 05:53:55PM +0300, Laurent Pinchart wrote:
> Hello,
>

[snip]

> >
> > I might be playing in the 'too verbose' field, but I would expand
> > these with a few words to make them a little nicer to read. In
> > example:
> >
> > > + * \brief The pixel format
> >
> >         \brief The requested stream pixel format
> >
> >         The pixel format encoding is specific to the pipeline/IPA
> >         implementation and might differ from the one configured from
> >         applications on the Camera.
>
> Can it ? I thought it should be the same.
>

I assumed it might be possible for some pipes to use external
components to produce, say, JPEG.

Hi Jacopo,

On Wed, Oct 09, 2019 at 05:04:41PM +0200, Jacopo Mondi wrote:
> On Wed, Oct 09, 2019 at 05:53:55PM +0300, Laurent Pinchart wrote:
> > Hello,
> 
> [snip]
> 
> > > I might be playing in the 'too verbose' field, but I would expand
> > > these with a few words to make them a little nicer to read. In
> > > example:
> > >
> > > > + * \brief The pixel format
> > >
> > >         \brief The requested stream pixel format
> > >
> > >         The pixel format encoding is specific to the pipeline/IPA
> > >         implementation and might differ from the one configured from
> > >         applications on the Camera.
> >
> > Can it ? I thought it should be the same.
> 
> I assumed it might be possible for some pipes to use external
> components to produce, say, JPEG.

We don't support JPEG compression yet though :-) Longer term we will
probably want to describe more of the pipeline configuration, and extend
IPAStream for that purpose. For now I think we can restrict it to
describe the format produced by the stream.

Hi Niklas,

Second part of the review :-)

On Tue, Oct 08, 2019 at 02:45:31AM +0200, 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.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/ipa/ipa_interface.h             |  38 +++++
>  src/ipa/ipa_dummy.cpp                   |   7 +-
>  src/libcamera/ipa_interface.cpp         | 195 ++++++++++++++++++++++++
>  src/libcamera/proxy/ipa_proxy_linux.cpp |  14 +-
>  4 files changed, 245 insertions(+), 9 deletions(-)
> 
> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> index 2c5eb1fd524311cb..499340dc3a8c67c2 100644
> --- a/include/ipa/ipa_interface.h
> +++ b/include/ipa/ipa_interface.h
> @@ -7,14 +7,52 @@
>  #ifndef __LIBCAMERA_IPA_INTERFACE_H__
>  #define __LIBCAMERA_IPA_INTERFACE_H__
>  
> +#include <map>
> +#include <vector>
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/controls.h>
> +#include <libcamera/geometry.h>
> +#include <libcamera/signal.h>
> +
> +#include "v4l2_controls.h"
> +
>  namespace libcamera {
>  
> +struct IPAStream {
> +	unsigned int pixelFormat;
> +	Size size;
> +};
> +
> +struct IPABuffer {
> +	unsigned int type;
> +	unsigned int id;
> +	BufferMemory buffer;
> +};
> +
> +struct IPAOperationData {
> +	unsigned int operation;
> +	std::vector<uint32_t> data;
> +	std::vector<ControlList> controls;
> +	std::vector<V4L2ControlList> v4l2controls;
> +	/* \todo: Add more vectors for data types used in pipa<->IPA interactions. */
> +};
> +
>  class IPAInterface
>  {
>  public:
>  	virtual ~IPAInterface() {}
>  
>  	virtual int init() = 0;
> +
> +	virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +			       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) = 0;
> +
> +	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> +	virtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> +
> +	virtual void processEvent(const IPAOperationData &event) = 0;
> +	Signal<const IPAOperationData &> queueFrameAction;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
> index 9d0cbdc8b1ad5565..08f74c4952f3923a 100644
> --- a/src/ipa/ipa_dummy.cpp
> +++ b/src/ipa/ipa_dummy.cpp
> @@ -15,7 +15,12 @@ namespace libcamera {
>  class IPADummy : public IPAInterface
>  {
>  public:
> -	int init();
> +	int init() override;
> +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void processEvent(const IPAOperationData &event) override {}
>  };
>  
>  int IPADummy::init()
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index d7d8ca8881efcf66..c365d14c53669c21 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -14,9 +14,136 @@
>  
>  namespace libcamera {
>  
> +/**
> + * \struct IPAStream
> + * \brief Stream configuration for the IPA protocol
> + *
> + * The StreamConfiguration class contains too much information to be suitable
> + * for IPA protocols. The IPAStream structure mirrors the parameters from
> + * StreamConfiguration which are useful for the IPA.
> + */
> +
> +/**
> + * \var IPAStream::pixelFormat
> + * \brief The pixel format
> + */
> +
> +/**
> + * \var IPAStream::size
> + * \brief The size
> + */
> +
> +/**
> + * \struct IPABuffer
> + * \brief Buffer information for the IPA protocol
> + *
> + * Buffers allocated by a pipeline handler might be needed inside the IPA. This
> + * structure allows the pipeline handler to associate a buffer with numerical
> + * ids that uniquely identify the buffer that can be used in the IPA protocol.
> + *
> + * The numerical identifiers are \a type and \a id. Type describes which buffer
> + * pool \a buffer belongs to and id unique identifies the buffer within that
> + * pool.
> + *
> + * \sa IPAInterface::mapBuffers()
> + * \sa IPAInterface::unmapBuffers()

Following our discussion regarding removal of the type field, I would
update the above documentation as follows.

 * \struct IPABuffer
 * \brief Buffer information for the IPA interface
 *
 * The IPABuffer structure associates buffer memory with a unique ID. It is
 * used to map buffers to the IPA with IPAInterface::mapBuffers(), after which
 * buffers will be identified by their ID in the IPA interface.

(I've dropped the \sa because mapBuffers() is referenced in the text)

> + */
> +
> +/**
> + * \var IPABuffer::type
> + * \brief Type of the buffer
> + */
> +
> +/**
> + * \var IPABuffer::id
> + * \brief ID of the buffer
> + */

Let's be more precise.

/**
 * \var IPABuffer::id
 * \brief The buffer unique ID
 *
 * Buffers mapped to the IPA are identified by numerical unique IDs. The IDs
 * are chosen by the pipeline handler to fulfil the following constraints:
 *
 * - IDs shall be positive integers different than zero
 * - IDs shall be unique among all mapped buffers
 *
 * When buffers are unmapped with IPAInterface::unmapBuffers() their IDs are
 * freed and may be reused for new buffer mappings.
 */

> +
> +/**
> + * \var IPABuffer::buffer
> + * \brief Hardware description of the buffer
> + *
> + * The description allows the IPA to map and unmap the buffer memory to access
> + * its content.
> + */

How about naming this field memory instead of buffer, as it describes
buffer memory ?

/**
 * \var IPABuffer::memory
 * \brief The buffer memory description
 *
 * The memory field stores the dmabuf handle and size for each plane of the
 * buffer.
 */

Now that I wrote that, I think BufferMemory isn't a very good fit as it
also carries the mem field that the IPA shouldn't use. Let's rework this
as part of the buffer API rework.

> +
> +/**
> + * \struct IPAOperationData
> + * \brief Parameters for IPA operations
> + *
> + * A pipeline defines its IPA protocol by describing how an  IPAOperationData

s/pipeline/pipeline handler/
s/  / /

> + * structure shall be filled out for each of its operations. When the structure
> + * is populated it can be sent to the other part of the protocol and decoded.
> + *
> + * The \a operation describes which operation the receiver shall perform
> + * and implicitly describes which of the other fields in the structure are
> + * populated and how they shall be interpreted. All this is the responsibility
> + * of the pipeline implementation.
> + *
> + * \sa IPAInterface::processEvent()
> + * \sa IPAInterface::queueFrameAction
> + */

I realise now that my documentation proposal for the \file block is
missing a description of IPA operations. I propose adding the following
as the second paragraph:

 * Pipeline handlers define the set of events and actions used to communicate
 * with their IPA. These are collectively referred to ias IPA operations and
 * define the pipeline handler-specific IPA protocol. Each operation defines the
 * data that it carries, and how the data is encoded in the IPAOperationData
 * structure.

The IPAOperationData documentation could then be updated as

/**
 * \struct IPAOperationData
 * \brief Parameters for IPA operations
 *
 * The IPAOperationData structure carries parameters for the IPA operations
 * performed through the IPAInterface::processEvent() method and the
 * IPAInterface::queueFrameAction signal.
 */

> +
> +/**
> + * \var IPAOperationData::operation
> + * \brief Operation in the pipeline-to-IPA protocol
> + *
> + * The allocation of numerical values to operations are left to the
> + * implementation of the pipeline protocol.
> + */

And this block could be extended with the description of the operation
field that was previously part of the \struct IPAOperationData block.

/**
 * \var IPAOperationData::operation
 * \brief IPA protocol operation
 *
 * The operation field describes which operation the receiver shall perform. It
 * defines, through the IPA protocol, how the other fields of the structure are
 * interpreted. The protocol freely assigns numerical values to operations.
 */

> +
> +/**
> + * \var IPAOperationData::data
> + * \brief Array of unsigned int values
> + *
> + * The interpretation and position of different values in the array are left
> + * to the implementation of the pipeline protocol.

I would say "are defined by the IPA protocol" instead of "are left to
...", in order to consistently use the term IPA protocol to refer to the
pipeline handler-specific protocol. Same for the two fields below.

> + */
> +
> +/**
> + * \var IPAOperationData::controls
> + * \brief Array of ControlList
> + *
> + * The interpretation and position of different values in the array are left
> + * to the implementation of the pipeline protocol.
> + */
> +
> +/**
> + * \var IPAOperationData::v4l2controls
> + * \brief Array of V4L2ControlList
> + *
> + * The interpretation and position of different values in the array are left
> + * to the implementation of the pipeline protocol.
> + */
> +
>  /**
>   * \class IPAInterface
>   * \brief Interface for IPA implementation
> + *
> + * Every pipeline implementation in libcamera may attach all or some of its

s/pipeline/pipeline handler/

> + * cameras to an Image Processing Algorithm (IPA) plugin. An IPA plugin is

We don't use IPA plugin anywhere else, the documentation talks about IPA
module. I would do the same here and below.

> + * developed for a specific pipeline and each pipeline might have multiple

s/pipeline/pipeline handler/g
s/might/may/ (we should fix other occurences of might)

> + * competing IPA implementations, both open and closed source.

I would say "compatible" instead of "competing".

> + *
> + * To allow for multiple and competing IPA plugins for the same pipeline, an

s/and competing /

> + * interface for the pipeline and IPA communication is needed; IPAInterface

s/an interface/a standard interface/
s/pipeline/pipeline handler/
s/;/./

> + * is this interface.

I'd start a new paragraph here.

> The interface defines how and what type of data can be
> + * exchanged, not how the data exchanged makes up a protocol. Each pipeline's
> + * design is unique and so is its IPA protocol. It is the pipeline's
> + * responsibility to define and document a protocol on top of the IPAInterface
> + * which can be used by multiple IPA plugins.

 * The interface defines base data types and methods to exchange data. On top of
 * this, each pipeline handler is responsible for defining specific operations
 * that make up its IPA protocol, shared by all IPA modules compatible with the
 * pipeline handler.

> + *
> + * The IPAInterface provides some methods to help the protocol design. As the
> + * requirements of the IPA plugins become more clear it is expected more
> + * common protocol operations will be identified and added.

I think this is a bit confusing. Do you mean more common methods in
IPAInterface, or common operations in the sense of
IPAOperationData::operation ? I think we should consistently use the
word operation for the latter and method (or interface method) for the
former.

If you refer to operations, the IPAInterface doesn't provide any. I
agree that some common operations may be added in the future, but it's
not "*more* common protocol operations" as we have none. I would then
write

 * No standard IPA operations are currently defined by the IPA API. As
 * the requirements of the IPA plugins become clearer, common protocol
 * operations are expected to be identified and added.

If you refer to methods, I'm not sure we'll add more, and I'm not sure
we can really say they help the protocol design. I would propose
dropping the paragraph in that case.

I would add a blank line here.

> + * \todo Add more common operations if possible.

s/more common operations/commong IPA operations/

> + *
> + * The pipeline shall use the IPAManager to locate an IPAInterface compatible

s/pipeline/pipeline handler/

> + * with the pipeline. The interface may then be used in the pipeline to interact

Maybe just "a compatible IPAInterface" ? And s/in the pipeline //

> + * with the IPA plugin to make up the protocol.

How about just "The interface is then used to interact with the IPA
module." ? We've already talked about the protocol above.

I wonder if we should talk about the IPAManager here. We currently
generate a single documentation for all the libcamera APIs (public, IPA
and internal), but down the line I think it would make sense to split
them. Applications developers should not be bothered with IPA and internal
APIs. We may want to avoid bothering IPA developers with internal APIs (for
the purpose of this discussion I include the currently internal APIs
that can be used as helpers by open-source IPAs in the category of IPA
API). This being said, given that IPAs are to be developed with pipeline
handlers and not in isolation, it may make sense to not split the IPA
documentation and keep all the internal documentation in one large
piece. The above paragraph would then make complete sense.

And I would add a blank line here too.

> + * \todo Add reference to how pipelines shall document their protocol.
> + *
> + * \sa IPAManager

Do we need a \sa when doxygen already generates a link in the text ? I
tend to use \sa only for references that are not present in the text,
but maybe my usage is not correct.

>   */
>  
>  /**
> @@ -24,4 +151,72 @@ namespace libcamera {
>   * \brief Initialise the IPAInterface
>   */
>  
> +/**
> + * \fn IPAInterface::configure()
> + * \brief Configure the IPA stream and sensor settings
> + * \param[in] streamConfig List of stream configuration descriptions

"Configuration of all active streams" ?

> + * \param[in] entityControls List of controls provided by the pipeline entities

s/List of controls/Controls/ ?

> + *
> + * This operation shall be called when the camera is started to inform the IPA

s/This operation/This method/

> + * of the camera's streams and the sensor settings.

When the camera is started, or when the camera is configured ? I think
the former is fine for now (no need to update anything), but we may want
to move it to configure time later if we want to give the IPA a chance
to reject a configuration. I'm not sure there would be a use case for
that though, as it would be difficult to do so in a way that would
report meaningful error causes to applications, especially with
closed-source IPAs.

I would add the following sentence.

"The meaning of the numerical keys in the \a streamConfig and \a
entityControls maps is defined by the IPA protocol."

> + */
> +
> +/**
> + * \fn IPAInterface::mapBuffers()
> + * \brief Map the buffers shared by the pipeline to the IPA

s/the buffers/buffers/

s/by the pipeline to the IPA/by the pipeline handler with the IPA/

or

s/by the pipeline to the IPA/between the pipeline handler and the IPA/

In case I've missed other occurences, we should use "pipeline handler"
to refer to pipeline handlers. The word "pipeline" refers to the
device's pipeline (likely the hardware pipeline), as correctly done in
the description of the configure() operation entityControls parameter.

> + * \param[in] buffers List of buffers to map
> + *
> + * This operation is used to inform the IPA module of the memory buffers set up
> + * by the pipeline handler and associate to each of them a numerical \a type and
> + * \a id. All buffers that the pipeline wishes to share with an IPA must be
> + * mapped in the IPA with this operation.
> + *
> + * After the buffers have been initialized a specific buffer can be referenced
> + * using the numerical \a type and \a id provided when the buffers were mapped.
> + *
> + * The numerical values for type and id used to describe a buffer have no
> + * meaning in the core libcamera code and the interface is pipeline handler
> + * specific.

With the removal of the type field, I propose

 * This method informs the IPA module of memory buffers set up by the pipeline
 * handler that the IPA needs to access. It provides dmabuf file handles for
 * each buffer, and associates the buffers with unique numerical IDs.
 *
 * IPAs shall map the dmabuf file handles to their address space and keep a
 * cache of the mappings, indexed by the buffer numerical IDs. The IDs are used
 * in all other IPA interface methods to refer to buffers, including the
 * unmapBuffers() method.
 *
 * All buffers that the pipeline handler wishes to share with an IPA shall be
 * mapped with this method. Buffers may be mapped all at once with a single
 * call, or mapped and unmapped dynamically at runtime, depending on the IPA
 * protocol. Regardless of the protocol, all buffers mapped at a given time
 * shall have unique numerical IDs.
 *
 * The numerical IDs have no meaning defined by the IPA interface, and IPA
 * protocols shall not give them any specific meaning either. They should be
 * treated as opaque handles by IPAs, with the only exception that ID zero is
 * invalid.

> + *
> + * \sa unmapBuffers()
> + * \todo Can we make this a generic function?

Do you mean "Provide a generic implementation of mapBuffers and
unmapBuffers for IPAs" ?

> + */
> +
> +/**
> + * \fn IPAInterface::unmapBuffers()
> + * \brief Unmap the buffers shared by the pipeline to the IPA
> + * \param[in] buffers List of buffers to unmap

Should this take a list of IDs instead of IPABuffer ? Passing both IDs
and BufferMemory opens the door to passing incorrect information. The
IPA needs to cache the virtual mapping information in any case, so I
think it could as well cache the size (if it doesn't already) and not
need the BufferMemory to be passed by the pipeline handler.

> + *
> + * This operation removes the mapping set up with mapBuffers().

 * This method removes mappings set up with mapBuffers(). Buffers may be
 * unmapped all at once with a single call, or selectively at runtime, depending
 * on the IPA protocol. Numerical IDs of unmapped buffers may be reused when
 * mapping new buffers.

> + *
> + * \sa mapBuffers()
> + * \todo Can we make this a generic function?

You can probably drop this todo item, the previous one should be enough.

> + */
> +
> +/**
> + * \fn IPAInterface::processEvent()
> + * \brief Process an event from the pipeline handler
> + * \param[in] event Event to process

Should the parameter be named data and documented as "IPA operation
data" ?

> + *
> + * This operation is used by pipeline handlers to inform the IPA module of
> + * events that occurred during the on-going capture operation.
> + *
> + * Each \a event notified by the pipeline handler with this method is handled
> + * by the IPA which interprets the operation parameters in an implementation
> + * specific way, which needs to be separately documented.

 * The \a event notified by the pipeline handler with this method is handled by
 * the IPA, which interprets the operation parameters according to the
 * separately documented IPA protocol.

> + */
> +
> +/**
> + * \fn IPAInterface::queueFrameAction()

This isn't a function, but a signal. It should be documented as such,
but we'll then lose the arguments :-S It seems however that Doxygen
doesn't complain about the use of \fn. queueFrameAction is still listed
as a member data, not member function. Actually, using \var with \param
seems to be accepted, which seems weird. Maybe we could just replace \fn
with \var and keep the parameters ?

> + * \brief Queue an action associated with a frame to the pipeline handler
> + * \param[in] frame The frame number for the action
> + * \param[in] controls List of controls associated with the action

This isn't correct anymore, the signal has a single parameter. I think
the frame number makes sense though, should we modify the signal to take
a frame number and an IPAOperationData ? In any case I think the second
field should be documented as "IPA operation data"

> + *
> + * This signal is emitted when the IPA wishes to queue a FrameAction on the
> + * pipeline. The pipeline is still responsible for the scheduling of the action
> + * on its timeline.
> + *
> + * The IPA operation describing the frame action is passed as a parameter.

FrameAction isn't part of the IPA core API anymore. How about the
following ?

 * This signal is emitted by the IPA to queue an action to be executed by the
 * pipeline handler on a frame. The type of action is identified by the
 * \a data.operation field, as defined by the IPA protocol, and the rest of the
 * \a data is interpreted accordingly. The pipeline handler shall queue the
 * action and execute it as appropriate.

I'm not very happy about this description as it hints the usage of a
timeline without explicitly describing it, but I think that's the best
we can do for now until we generalise the timeline implementation.

> + */
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index 62fcb529e1c7e4ff..14e8bb6067623fc6 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -26,7 +26,12 @@ public:
>  	IPAProxyLinux(IPAModule *ipam);
>  	~IPAProxyLinux();
>  
> -	int init();
> +	int init() override { return 0; }
> +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> +	void processEvent(const IPAOperationData &event) override {}
>  
>  private:
>  	void readyRead(IPCUnixSocket *ipc);
> @@ -36,13 +41,6 @@ private:
>  	IPCUnixSocket *socket_;
>  };
>  
> -int IPAProxyLinux::init()
> -{
> -	LOG(IPAProxy, Debug) << "initializing IPA via dummy proxy!";
> -
> -	return 0;
> -}
> -
>  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)
>  	: proc_(nullptr), socket_(nullptr)
>  {

Hi Jacopo,

Thanks for your feedback.

On 2019-10-08 11:46:20 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>    minor notes, mostly on comments.
> 
> Feel free to collect what you consider more opportune.
> 
> On Tue, Oct 08, 2019 at 02:45:31AM +0200, 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.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/ipa/ipa_interface.h             |  38 +++++
> >  src/ipa/ipa_dummy.cpp                   |   7 +-
> >  src/libcamera/ipa_interface.cpp         | 195 ++++++++++++++++++++++++
> >  src/libcamera/proxy/ipa_proxy_linux.cpp |  14 +-
> >  4 files changed, 245 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> > index 2c5eb1fd524311cb..499340dc3a8c67c2 100644
> > --- a/include/ipa/ipa_interface.h
> > +++ b/include/ipa/ipa_interface.h
> > @@ -7,14 +7,52 @@
> >  #ifndef __LIBCAMERA_IPA_INTERFACE_H__
> >  #define __LIBCAMERA_IPA_INTERFACE_H__
> >
> > +#include <map>
> > +#include <vector>
> > +
> > +#include <libcamera/buffer.h>
> > +#include <libcamera/controls.h>
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/signal.h>
> > +
> > +#include "v4l2_controls.h"
> > +
> >  namespace libcamera {
> >
> > +struct IPAStream {
> > +	unsigned int pixelFormat;
> > +	Size size;
> > +};
> > +
> > +struct IPABuffer {
> > +	unsigned int type;
> > +	unsigned int id;
> > +	BufferMemory buffer;
> > +};
> > +
> > +struct IPAOperationData {
> > +	unsigned int operation;
> > +	std::vector<uint32_t> data;
> > +	std::vector<ControlList> controls;
> > +	std::vector<V4L2ControlList> v4l2controls;
> 
> Why vectors of control lists? Do you envision IPAs to set controls for
> multiple entities in a single operation ? you need a field to identify
> to which entity the controls apply to, or is it something pipe/IPA
> should handle somehow ?

Yes, the insertion order in the vector(s) are a property of the 
particular IPA protocol.

> 
> > +	/* \todo: Add more vectors for data types used in pipa<->IPA interactions. */
> > +};
> > +
> >  class IPAInterface
> >  {
> >  public:
> >  	virtual ~IPAInterface() {}
> >
> >  	virtual int init() = 0;
> > +
> > +	virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +			       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) = 0;
> > +
> > +	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > +	virtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > +
> > +	virtual void processEvent(const IPAOperationData &event) = 0;
> > +	Signal<const IPAOperationData &> queueFrameAction;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
> > index 9d0cbdc8b1ad5565..08f74c4952f3923a 100644
> > --- a/src/ipa/ipa_dummy.cpp
> > +++ b/src/ipa/ipa_dummy.cpp
> > @@ -15,7 +15,12 @@ namespace libcamera {
> >  class IPADummy : public IPAInterface
> >  {
> >  public:
> > -	int init();
> > +	int init() override;
> > +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> > +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void processEvent(const IPAOperationData &event) override {}
> >  };
> >
> >  int IPADummy::init()
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index d7d8ca8881efcf66..c365d14c53669c21 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -14,9 +14,136 @@
> >
> >  namespace libcamera {
> >
> > +/**
> > + * \struct IPAStream
> > + * \brief Stream configuration for the IPA protocol
> 
> Here and in the other briefs, I would not make "IPA protocol" the
> object of the statement. I would just reuse something like what we
> have for StreamConfiguration
> 
>         \brief Configuration parameters for a stream
> 
> 
> > + *
> > + * The StreamConfiguration class contains too much information to be suitable
> > + * for IPA protocols. The IPAStream structure mirrors the parameters from
> > + * StreamConfiguration which are useful for the IPA.
> 
> I'm not sure mentioning that StreamConfiguration is too much is
> something that applies to public documentation.

This is for pipeline/IPA developers so I think it's fine.

> 
> > + */
> > +
> > +/**
> > + * \var IPAStream::pixelFormat
> 
> I might be playing in the 'too verbose' field, but I would expand
> these with a few words to make them a little nicer to read. In
> example:
> 
> > + * \brief The pixel format
> 
>         \brief The requested stream pixel format
> 
>         The pixel format encoding is specific to the pipeline/IPA
>         implementation and might differ from the one configured from
>         applications on the Camera.

Discussed in separate thread, I think we don't want this. At least not 
now.

> 
> > + */
> > +
> > +/**
> > + * \var IPAStream::size
> > + * \brief The size
> > + */
> 
>         \brief The requested stream size
> > +
> > +/**
> > + * \struct IPABuffer
> > + * \brief Buffer information for the IPA protocol
> > + *
> > + * Buffers allocated by a pipeline handler might be needed inside the IPA. This
> > + * structure allows the pipeline handler to associate a buffer with numerical
> > + * ids that uniquely identify the buffer that can be used in the IPA protocol.
> 
> A suggestion, feel free to scratch or integrate what's better:
> "IPA modules need to access buffers allocated by pipeline handlers.
> This structure provides fields to establish a mapping between a buffer
> descriptor and a numerical id, that uniquely identifies the buffer in
> the following IPA operations calls. The mapping is established by calling
> IPAInterface::mapBuffers() and stays valid until it is removed with a
> call to IPAInterface::unmapBuffers().
> 
> > + *
> > + * The numerical identifiers are \a type and \a id. Type describes which buffer
> > + * pool \a buffer belongs to and id unique identifies the buffer within that
> > + * pool.
> 
> I would leave type out from here and expand it below
> 
> > + *
> > + * \sa IPAInterface::mapBuffers()
> > + * \sa IPAInterface::unmapBuffers()
> > + */
> > +
> > +/**
> > + * \var IPABuffer::type
> > + * \brief Type of the buffer
> 
>         \brief The buffer type
> 
>         Buffer type is a numerical identifier made available for IPA
>         protocol implementations to store the intended buffer usage.
>         The semantic associated to the values that can be assigned
>         to a type is implementation specific and not enforced by any
>         libcamera core component.
> 
> > + */
> > +
> > +/**
> > + * \var IPABuffer::id
> > + * \brief ID of the buffer
> 
> Here too I will spend a few more words on the accepted values, the
> validity of the mapping etc.. Here or in the IPABuffer class
> documentation as I've suggested
> 
> > + */
> > +
> > +/**
> > + * \var IPABuffer::buffer
> > + * \brief Hardware description of the buffer
> 
> Now that I think about this I'm a bit skeptical on passing a
> BufferMemory while we should really provide an array of file descriptors.
> But I know you and Laurent discussed this already and I'm fine
> whatever.
> 
> Anyway, 'Hardware description' does not play well imo. BufferMemory
> contains an array of planes exported as either fds and memory pointers,
> so it actually abstracts a buffer by providing information on how to
> access its content. I would just drop 'Hardware' and specify this is
> intended for IPA to be able to access the buffer content as you have
> done here below.
> 
> I wonder if the memory mapped pointer *mem_ should not be marked as
> not-accessible by IPA modules running in a different process space
> somehow..

As we will redesign the buffers I think this can be a part of it. The 
idea I had when designing this was to not let mem_ cross over IPC_.

> 
> > + *
> > + * The description allows the IPA to map and unmap the buffer memory to access
> > + * its content.
> 
> I would have detailed that a number of planes is exported as file
> descriptors and the IPA modules can map them in their memory space,
> but I would be actually describing BufferMemory. Not sure if it's
> appropriate.
> 
> > + */
> > +
> > +/**
> > + * \struct IPAOperationData
> > + * \brief Parameters for IPA operations
> > + *
> > + * A pipeline defines its IPA protocol by describing how an  IPAOperationData
> 
> Double space between "an  IPAOperationData"
> 
> > + * structure shall be filled out for each of its operations. When the structure
> > + * is populated it can be sent to the other part of the protocol and decoded.
> 
> This describes the IPA protocol definition, while the struct actually
> contains a number of fields that can be used for that purpose, at
> least that's my understanding of what you're trying to convey.
> 
> I would
> "The IPAOperationData structure provides fields that are meant to be
> used in the implementation of the IPA operations between pipeline
> handlers and IPA modules.
> 
> Specific implementations realize their protocol by using the
> IPAOperationData members. Some members, such as ControlList, have a well
> intended meaning while others, such as data[] provide space for
> protocol-specific implementations, to allow pipelines and IPAs to
> freely exchange data by implementing a binary protocol on top of it.
> 
> > + *
> > + * The \a operation describes which operation the receiver shall perform
> > + * and implicitly describes which of the other fields in the structure are
> > + * populated and how they shall be interpreted. All this is the responsibility
> > + * of the pipeline implementation.
> 
> I would mix this with pieces of the above text. Up to you.
> 
> > + *
> > + * \sa IPAInterface::processEvent()
> > + * \sa IPAInterface::queueFrameAction
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::operation
> > + * \brief Operation in the pipeline-to-IPA protocol
> 
> "Operation identifier code in the ... " ?
> 
> > + *
> > + * The allocation of numerical values to operations are left to the
> > + * implementation of the pipeline protocol.
> 
> Not allocation but I would say "the definition of operation identifier
> codes... "
> 
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::data
> > + * \brief Array of unsigned int values
> 
> Array of binary data. The type we use to manage it is not relevant.
> 
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::controls
> > + * \brief Array of ControlList
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::v4l2controls
> > + * \brief Array of V4L2ControlList
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> > +
> >  /**
> >   * \class IPAInterface
> >   * \brief Interface for IPA implementation
> > + *
> > + * Every pipeline implementation in libcamera may attach all or some of its
> > + * cameras to an Image Processing Algorithm (IPA) plugin. An IPA plugin is
> > + * developed for a specific pipeline and each pipeline might have multiple
> > + * competing IPA implementations, both open and closed source.
> 
> Should this be part of the interface description ? Isn't more a
> general architecture comment, probably better located in the file
> description ?

No strong feelings, but I like it here.

> 
> > + *
> > + * To allow for multiple and competing IPA plugins for the same pipeline, an
> > + * interface for the pipeline and IPA communication is needed; IPAInterface
> > + * is this interface. The interface defines how and what type of data can be
> > + * exchanged, not how the data exchanged makes up a protocol. Each pipeline's
> > + * design is unique and so is its IPA protocol. It is the pipeline's
> > + * responsibility to define and document a protocol on top of the IPAInterface
> > + * which can be used by multiple IPA plugins.
> > + *
> > + * The IPAInterface provides some methods to help the protocol design. As the
> > + * requirements of the IPA plugins become more clear it is expected more
> > + * common protocol operations will be identified and added.
> > + * \todo Add more common operations if possible.
> > + *
> > + * The pipeline shall use the IPAManager to locate an IPAInterface compatible
> > + * with the pipeline. The interface may then be used in the pipeline to interact
> > + * with the IPA plugin to make up the protocol.
> > + * \todo Add reference to how pipelines shall document their protocol.
> > + *
> > + * \sa IPAManager
> >   */
> >
> >  /**
> > @@ -24,4 +151,72 @@ namespace libcamera {
> >   * \brief Initialise the IPAInterface
> >   */
> >
> > +/**
> > + * \fn IPAInterface::configure()
> > + * \brief Configure the IPA stream and sensor settings
> > + * \param[in] streamConfig List of stream configuration descriptions
> > + * \param[in] entityControls List of controls provided by the pipeline entities
> > + *
> > + * This operation shall be called when the camera is started to inform the IPA
> > + * of the camera's streams and the sensor settings.
> 
> The entityControls are a map, how should it be interpreted? Is this up
> to each implementation to define that?

Yes.

> 
> In any case you are referring to 'the sensor' while the parameters are
> settings-per-entity. I would make this clear in the documentation,
> even if I assume you would like to make the interpretation of the
> entity identifier (which is a numerical ID at the moment) up to each
> implementation.
> 
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::mapBuffers()
> > + * \brief Map the buffers shared by the pipeline to the IPA
> 
> Map buffers reserved by pipelines in the IPA address space ?
> 
> > + * \param[in] buffers List of buffers to map
> > + *
> > + * This operation is used to inform the IPA module of the memory buffers set up
> > + * by the pipeline handler and associate to each of them a numerical \a type and
> > + * \a id. All buffers that the pipeline wishes to share with an IPA must be
> > + * mapped in the IPA with this operation.
> > + *
> > + * After the buffers have been initialized a specific buffer can be referenced
> > + * using the numerical \a type and \a id provided when the buffers were mapped.
> > + *
> > + * The numerical values for type and id used to describe a buffer have no
> > + * meaning in the core libcamera code and the interface is pipeline handler
> > + * specific.
> 
> This captures part of my comments above here. Again I will stress how
> long a mapping is valid for.
> 
> > + *
> > + * \sa unmapBuffers()
> > + * \todo Can we make this a generic function?
> 
> Isn't this 'generic' ? What do you mean here ?
> 
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::unmapBuffers()
> > + * \brief Unmap the buffers shared by the pipeline to the IPA
> > + * \param[in] buffers List of buffers to unmap
> > + *
> > + * This operation removes the mapping set up with mapBuffers().
> 
> When should this be called by pipeline handlers ? Will other mappings
> not part of 'buffers' stay valid?
> 
> > + *
> > + * \sa mapBuffers()
> > + * \todo Can we make this a generic function?
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::processEvent()
> > + * \brief Process an event from the pipeline handler
> > + * \param[in] event Event to process
> > + *
> > + * This operation is used by pipeline handlers to inform the IPA module of
> > + * events that occurred during the on-going capture operation.
> > + *
> > + * Each \a event notified by the pipeline handler with this method is handled
> > + * by the IPA which interprets the operation parameters in an implementation
> > + * specific way, which needs to be separately documented.
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::queueFrameAction()
> > + * \brief Queue an action associated with a frame to the pipeline handler
> > + * \param[in] frame The frame number for the action
> > + * \param[in] controls List of controls associated with the action
> > + *
> > + * This signal is emitted when the IPA wishes to queue a FrameAction on the
> > + * pipeline. The pipeline is still responsible for the scheduling of the action
> > + * on its timeline.
> 
> Timeline is now a rk1isp specific thing. Should it be removed from
> here and just mention a generic "responsible for handling the reported
> action opportunely" ?
> 
> > + *
> > + * The IPA operation describing the frame action is passed as a parameter.
> 
> Didn't get this to be honest :)
> 
> Thanks
>    j
> 
> > + */
> > +
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index 62fcb529e1c7e4ff..14e8bb6067623fc6 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -26,7 +26,12 @@ public:
> >  	IPAProxyLinux(IPAModule *ipam);
> >  	~IPAProxyLinux();
> >
> > -	int init();
> > +	int init() override { return 0; }
> > +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> > +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void processEvent(const IPAOperationData &event) override {}
> >
> >  private:
> >  	void readyRead(IPCUnixSocket *ipc);
> > @@ -36,13 +41,6 @@ private:
> >  	IPCUnixSocket *socket_;
> >  };
> >
> > -int IPAProxyLinux::init()
> > -{
> > -	LOG(IPAProxy, Debug) << "initializing IPA via dummy proxy!";
> > -
> > -	return 0;
> > -}
> > -
> >  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)
> >  	: proc_(nullptr), socket_(nullptr)
> >  {
> > --
> > 2.23.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent,

Thanks for your feedback.

On 2019-10-09 16:33:34 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tue, Oct 08, 2019 at 02:45:31AM +0200, 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.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/ipa/ipa_interface.h             |  38 +++++
> >  src/ipa/ipa_dummy.cpp                   |   7 +-
> >  src/libcamera/ipa_interface.cpp         | 195 ++++++++++++++++++++++++
> >  src/libcamera/proxy/ipa_proxy_linux.cpp |  14 +-
> >  4 files changed, 245 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> > index 2c5eb1fd524311cb..499340dc3a8c67c2 100644
> > --- a/include/ipa/ipa_interface.h
> > +++ b/include/ipa/ipa_interface.h
> > @@ -7,14 +7,52 @@
> >  #ifndef __LIBCAMERA_IPA_INTERFACE_H__
> >  #define __LIBCAMERA_IPA_INTERFACE_H__
> >  
> > +#include <map>
> > +#include <vector>
> > +
> > +#include <libcamera/buffer.h>
> > +#include <libcamera/controls.h>
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/signal.h>
> > +
> > +#include "v4l2_controls.h"
> 
> I had a quick look at how this could be rebased on top of my latest
> control patches, but it's not trivial and will likely require me to send
> a v2. Let's get this series merged first.
> 
> >  namespace libcamera {
> >  
> > +struct IPAStream {
> > +	unsigned int pixelFormat;
> > +	Size size;
> > +};
> > +
> > +struct IPABuffer {
> > +	unsigned int type;
> > +	unsigned int id;
> > +	BufferMemory buffer;
> > +};
> > +
> > +struct IPAOperationData {
> > +	unsigned int operation;
> > +	std::vector<uint32_t> data;
> > +	std::vector<ControlList> controls;
> > +	std::vector<V4L2ControlList> v4l2controls;
> > +	/* \todo: Add more vectors for data types used in pipa<->IPA interactions. */
> 
> s/todo:/todo/
> 
> Is this comment still valid ? What other data types do you foresee ?

I agree, I think we shall drop this todo.

> 
> > +};
> > +
> >  class IPAInterface
> >  {
> >  public:
> >  	virtual ~IPAInterface() {}
> >  
> >  	virtual int init() = 0;
> > +
> > +	virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +			       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) = 0;
> > +
> > +	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > +	virtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> 
> Just thinking out loud, I think we'll need some sort of synchronous
> status operation to verify that the initialisation went fine before
> starting the streams. It can wait and be implemented on top of this
> series.
> 
> > +	virtual void processEvent(const IPAOperationData &event) = 0;
> > +	Signal<const IPAOperationData &> queueFrameAction;
> >  };
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
> > index 9d0cbdc8b1ad5565..08f74c4952f3923a 100644
> > --- a/src/ipa/ipa_dummy.cpp
> > +++ b/src/ipa/ipa_dummy.cpp
> > @@ -15,7 +15,12 @@ namespace libcamera {
> >  class IPADummy : public IPAInterface
> >  {
> >  public:
> > -	int init();
> > +	int init() override;
> > +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> > +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void processEvent(const IPAOperationData &event) override {}
> >  };
> 
> I tried to rebase your series on top of Jacopo's IPA test series, and
> without surprise this changes conflicts as the file has been renamed.
> Conflict resolution is trivial though.
> 
> >  int IPADummy::init()
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index d7d8ca8881efcf66..c365d14c53669c21 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -14,9 +14,136 @@
> 
> I think we need some introduction here.
> 
> @@ -10,32 +10,50 @@
>  /**
>   * \file ipa_interface.h
>   * \brief Image Processing Algorithm interface
> + *
> + * Pipeline handlers communicate with IPAs through a C++ interface defined by
> + * the IPAInterface class. The class defines high-level methods and signals to
> + * configure the IPA, notify it of events, and receive actions back from the
> + * IPA.
> + *
> + * IPAs can be isolated in a separate process. This implies that all arguments
> + * to the IPA interface may need to be transferred over IPC. The IPA interface
> + * thus uses serialisable data types only. The IPA interface defines custom data
> + * structures that mirror core libcamera structures when the latter are not
> + * suitable, such as IPAStream to carry StreamConfiguration data.
> + *
> + * Synchronous communication between pipeline handlers and IPAs can thus be
> + * costly. For that reason, the interface operates asynchronously. This implies
> + * that methods don't return a status, and that both methods and signals may
> + * copy their arguments.
>   */
> 
> >  
> >  namespace libcamera {
> >  
> > +/**
> > + * \struct IPAStream
> > + * \brief Stream configuration for the IPA protocol
> 
> "IPA interface" instead of "IPA protocol" ?
> 
> > + *
> > + * The StreamConfiguration class contains too much information to be suitable
> > + * for IPA protocols. The IPAStream structure mirrors the parameters from
> > + * StreamConfiguration which are useful for the IPA.
> 
>  * The IPAStream structure stores stream configuration parameters needed by the
>  * IPAInterface::configure() method. It mirrors the StreamConfiguration class
>  * that is not suitable for this purpose due to not being serialisable.
> 
> > + */
> > +
> > +/**
> > + * \var IPAStream::pixelFormat
> > + * \brief The pixel format
> 
> "The stream pixel format" ?
> 
> > + */
> > +
> > +/**
> > + * \var IPAStream::size
> > + * \brief The size
> 
> "The stream size in pixels" (to match StreamConfiguration) ?
> 
> > + */
> > +
> > +/**
> > + * \struct IPABuffer
> > + * \brief Buffer information for the IPA protocol
> 
> "IPA interface" here too ?
> 
> > + *
> > + * Buffers allocated by a pipeline handler might be needed inside the IPA. This
> > + * structure allows the pipeline handler to associate a buffer with numerical
> > + * ids that uniquely identify the buffer that can be used in the IPA protocol.
> > + *
> > + * The numerical identifiers are \a type and \a id. Type describes which buffer
> > + * pool \a buffer belongs to and id unique identifies the buffer within that
> > + * pool.
> 
> Do we really need a type here ? Wouldn't it be better to just associate
> buffer memory with a unique ID in the map and unmap operations, and, if
> a type is needed, passing it through the IPAOperationData for
> processEvent or queueFrameAction ?
> 
> I had a quick look at what the implications would be on the rkisp1 IPA,
> and it seems fine. The only problem I see is that
> IPARkISP1::queueRequest() won't have a per-type free list anymore, but I
> think it's actually not the role of the IPA to keep such a free list. I
> think the pipeline handler should instead pass the ID of the params and
> stats buffers to RKISP1_IPA_EVENT_QUEUE_REQUEST (actually the stats
> buffer shouldn't be passed at queue request time, but that's a different
> issue, we can address it on top).
> 
> We may then however need to add mapping flags to IPABuffer, to indicate
> whether the IPA should map the buffer for read (mostly used for stats)
> or write (mostly used for params, although in that case read/write is
> probably the correct mapping type).

I will drop the type field for next version.

> 
> > + *
> > + * \sa IPAInterface::mapBuffers()
> > + * \sa IPAInterface::unmapBuffers()
> > + */
> > +
> > +/**
> > + * \var IPABuffer::type
> > + * \brief Type of the buffer
> > + */
> > +
> > +/**
> > + * \var IPABuffer::id
> > + * \brief ID of the buffer
> > + */
> > +
> > +/**
> > + * \var IPABuffer::buffer
> > + * \brief Hardware description of the buffer
> > + *
> > + * The description allows the IPA to map and unmap the buffer memory to access
> > + * its content.
> > + */
> 
> I'll review the rest in a little bit.
> 
> > +
> > +/**
> > + * \struct IPAOperationData
> > + * \brief Parameters for IPA operations
> > + *
> > + * A pipeline defines its IPA protocol by describing how an  IPAOperationData
> > + * structure shall be filled out for each of its operations. When the structure
> > + * is populated it can be sent to the other part of the protocol and decoded.
> > + *
> > + * The \a operation describes which operation the receiver shall perform
> > + * and implicitly describes which of the other fields in the structure are
> > + * populated and how they shall be interpreted. All this is the responsibility
> > + * of the pipeline implementation.
> > + *
> > + * \sa IPAInterface::processEvent()
> > + * \sa IPAInterface::queueFrameAction
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::operation
> > + * \brief Operation in the pipeline-to-IPA protocol
> > + *
> > + * The allocation of numerical values to operations are left to the
> > + * implementation of the pipeline protocol.
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::data
> > + * \brief Array of unsigned int values
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::controls
> > + * \brief Array of ControlList
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::v4l2controls
> > + * \brief Array of V4L2ControlList
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> > + */
> > +
> >  /**
> >   * \class IPAInterface
> >   * \brief Interface for IPA implementation
> > + *
> > + * Every pipeline implementation in libcamera may attach all or some of its
> > + * cameras to an Image Processing Algorithm (IPA) plugin. An IPA plugin is
> > + * developed for a specific pipeline and each pipeline might have multiple
> > + * competing IPA implementations, both open and closed source.
> > + *
> > + * To allow for multiple and competing IPA plugins for the same pipeline, an
> > + * interface for the pipeline and IPA communication is needed; IPAInterface
> > + * is this interface. The interface defines how and what type of data can be
> > + * exchanged, not how the data exchanged makes up a protocol. Each pipeline's
> > + * design is unique and so is its IPA protocol. It is the pipeline's
> > + * responsibility to define and document a protocol on top of the IPAInterface
> > + * which can be used by multiple IPA plugins.
> > + *
> > + * The IPAInterface provides some methods to help the protocol design. As the
> > + * requirements of the IPA plugins become more clear it is expected more
> > + * common protocol operations will be identified and added.
> > + * \todo Add more common operations if possible.
> > + *
> > + * The pipeline shall use the IPAManager to locate an IPAInterface compatible
> > + * with the pipeline. The interface may then be used in the pipeline to interact
> > + * with the IPA plugin to make up the protocol.
> > + * \todo Add reference to how pipelines shall document their protocol.
> > + *
> > + * \sa IPAManager
> >   */
> >  
> >  /**
> > @@ -24,4 +151,72 @@ namespace libcamera {
> >   * \brief Initialise the IPAInterface
> >   */
> >  
> > +/**
> > + * \fn IPAInterface::configure()
> > + * \brief Configure the IPA stream and sensor settings
> > + * \param[in] streamConfig List of stream configuration descriptions
> > + * \param[in] entityControls List of controls provided by the pipeline entities
> > + *
> > + * This operation shall be called when the camera is started to inform the IPA
> > + * of the camera's streams and the sensor settings.
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::mapBuffers()
> > + * \brief Map the buffers shared by the pipeline to the IPA
> > + * \param[in] buffers List of buffers to map
> > + *
> > + * This operation is used to inform the IPA module of the memory buffers set up
> > + * by the pipeline handler and associate to each of them a numerical \a type and
> > + * \a id. All buffers that the pipeline wishes to share with an IPA must be
> > + * mapped in the IPA with this operation.
> > + *
> > + * After the buffers have been initialized a specific buffer can be referenced
> > + * using the numerical \a type and \a id provided when the buffers were mapped.
> > + *
> > + * The numerical values for type and id used to describe a buffer have no
> > + * meaning in the core libcamera code and the interface is pipeline handler
> > + * specific.
> > + *
> > + * \sa unmapBuffers()
> > + * \todo Can we make this a generic function?
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::unmapBuffers()
> > + * \brief Unmap the buffers shared by the pipeline to the IPA
> > + * \param[in] buffers List of buffers to unmap
> > + *
> > + * This operation removes the mapping set up with mapBuffers().
> > + *
> > + * \sa mapBuffers()
> > + * \todo Can we make this a generic function?
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::processEvent()
> > + * \brief Process an event from the pipeline handler
> > + * \param[in] event Event to process
> > + *
> > + * This operation is used by pipeline handlers to inform the IPA module of
> > + * events that occurred during the on-going capture operation.
> > + *
> > + * Each \a event notified by the pipeline handler with this method is handled
> > + * by the IPA which interprets the operation parameters in an implementation
> > + * specific way, which needs to be separately documented.
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::queueFrameAction()
> > + * \brief Queue an action associated with a frame to the pipeline handler
> > + * \param[in] frame The frame number for the action
> > + * \param[in] controls List of controls associated with the action
> > + *
> > + * This signal is emitted when the IPA wishes to queue a FrameAction on the
> > + * pipeline. The pipeline is still responsible for the scheduling of the action
> > + * on its timeline.
> > + *
> > + * The IPA operation describing the frame action is passed as a parameter.
> > + */
> > +
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index 62fcb529e1c7e4ff..14e8bb6067623fc6 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -26,7 +26,12 @@ public:
> >  	IPAProxyLinux(IPAModule *ipam);
> >  	~IPAProxyLinux();
> >  
> > -	int init();
> > +	int init() override { return 0; }
> > +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> > +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void processEvent(const IPAOperationData &event) override {}
> >  
> >  private:
> >  	void readyRead(IPCUnixSocket *ipc);
> > @@ -36,13 +41,6 @@ private:
> >  	IPCUnixSocket *socket_;
> >  };
> >  
> > -int IPAProxyLinux::init()
> > -{
> > -	LOG(IPAProxy, Debug) << "initializing IPA via dummy proxy!";
> > -
> > -	return 0;
> > -}
> > -
> >  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)
> >  	: proc_(nullptr), socket_(nullptr)
> >  {
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Hi Laurent,

Thanks for your review.

On 2019-10-09 23:25:35 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Second part of the review :-)

:-)

> 
> On Tue, Oct 08, 2019 at 02:45:31AM +0200, 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.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/ipa/ipa_interface.h             |  38 +++++
> >  src/ipa/ipa_dummy.cpp                   |   7 +-
> >  src/libcamera/ipa_interface.cpp         | 195 ++++++++++++++++++++++++
> >  src/libcamera/proxy/ipa_proxy_linux.cpp |  14 +-
> >  4 files changed, 245 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> > index 2c5eb1fd524311cb..499340dc3a8c67c2 100644
> > --- a/include/ipa/ipa_interface.h
> > +++ b/include/ipa/ipa_interface.h
> > @@ -7,14 +7,52 @@
> >  #ifndef __LIBCAMERA_IPA_INTERFACE_H__
> >  #define __LIBCAMERA_IPA_INTERFACE_H__
> >  
> > +#include <map>
> > +#include <vector>
> > +
> > +#include <libcamera/buffer.h>
> > +#include <libcamera/controls.h>
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/signal.h>
> > +
> > +#include "v4l2_controls.h"
> > +
> >  namespace libcamera {
> >  
> > +struct IPAStream {
> > +	unsigned int pixelFormat;
> > +	Size size;
> > +};
> > +
> > +struct IPABuffer {
> > +	unsigned int type;
> > +	unsigned int id;
> > +	BufferMemory buffer;
> > +};
> > +
> > +struct IPAOperationData {
> > +	unsigned int operation;
> > +	std::vector<uint32_t> data;
> > +	std::vector<ControlList> controls;
> > +	std::vector<V4L2ControlList> v4l2controls;
> > +	/* \todo: Add more vectors for data types used in pipa<->IPA interactions. */
> > +};
> > +
> >  class IPAInterface
> >  {
> >  public:
> >  	virtual ~IPAInterface() {}
> >  
> >  	virtual int init() = 0;
> > +
> > +	virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +			       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) = 0;
> > +
> > +	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > +	virtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > +
> > +	virtual void processEvent(const IPAOperationData &event) = 0;
> > +	Signal<const IPAOperationData &> queueFrameAction;
> >  };
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp
> > index 9d0cbdc8b1ad5565..08f74c4952f3923a 100644
> > --- a/src/ipa/ipa_dummy.cpp
> > +++ b/src/ipa/ipa_dummy.cpp
> > @@ -15,7 +15,12 @@ namespace libcamera {
> >  class IPADummy : public IPAInterface
> >  {
> >  public:
> > -	int init();
> > +	int init() override;
> > +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> > +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void processEvent(const IPAOperationData &event) override {}
> >  };
> >  
> >  int IPADummy::init()
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index d7d8ca8881efcf66..c365d14c53669c21 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -14,9 +14,136 @@
> >  
> >  namespace libcamera {
> >  
> > +/**
> > + * \struct IPAStream
> > + * \brief Stream configuration for the IPA protocol
> > + *
> > + * The StreamConfiguration class contains too much information to be suitable
> > + * for IPA protocols. The IPAStream structure mirrors the parameters from
> > + * StreamConfiguration which are useful for the IPA.
> > + */
> > +
> > +/**
> > + * \var IPAStream::pixelFormat
> > + * \brief The pixel format
> > + */
> > +
> > +/**
> > + * \var IPAStream::size
> > + * \brief The size
> > + */
> > +
> > +/**
> > + * \struct IPABuffer
> > + * \brief Buffer information for the IPA protocol
> > + *
> > + * Buffers allocated by a pipeline handler might be needed inside the IPA. This
> > + * structure allows the pipeline handler to associate a buffer with numerical
> > + * ids that uniquely identify the buffer that can be used in the IPA protocol.
> > + *
> > + * The numerical identifiers are \a type and \a id. Type describes which buffer
> > + * pool \a buffer belongs to and id unique identifies the buffer within that
> > + * pool.
> > + *
> > + * \sa IPAInterface::mapBuffers()
> > + * \sa IPAInterface::unmapBuffers()
> 
> Following our discussion regarding removal of the type field, I would
> update the above documentation as follows.
> 
>  * \struct IPABuffer
>  * \brief Buffer information for the IPA interface
>  *
>  * The IPABuffer structure associates buffer memory with a unique ID. It is
>  * used to map buffers to the IPA with IPAInterface::mapBuffers(), after which
>  * buffers will be identified by their ID in the IPA interface.
> 
> (I've dropped the \sa because mapBuffers() is referenced in the text)
> 
> > + */
> > +
> > +/**
> > + * \var IPABuffer::type
> > + * \brief Type of the buffer
> > + */
> > +
> > +/**
> > + * \var IPABuffer::id
> > + * \brief ID of the buffer
> > + */
> 
> Let's be more precise.
> 
> /**
>  * \var IPABuffer::id
>  * \brief The buffer unique ID
>  *
>  * Buffers mapped to the IPA are identified by numerical unique IDs. The IDs
>  * are chosen by the pipeline handler to fulfil the following constraints:
>  *
>  * - IDs shall be positive integers different than zero
>  * - IDs shall be unique among all mapped buffers
>  *
>  * When buffers are unmapped with IPAInterface::unmapBuffers() their IDs are
>  * freed and may be reused for new buffer mappings.
>  */
> 
> > +
> > +/**
> > + * \var IPABuffer::buffer
> > + * \brief Hardware description of the buffer
> > + *
> > + * The description allows the IPA to map and unmap the buffer memory to access
> > + * its content.
> > + */
> 
> How about naming this field memory instead of buffer, as it describes
> buffer memory ?
> 
> /**
>  * \var IPABuffer::memory
>  * \brief The buffer memory description
>  *
>  * The memory field stores the dmabuf handle and size for each plane of the
>  * buffer.
>  */

Why not.

> 
> Now that I wrote that, I think BufferMemory isn't a very good fit as it
> also carries the mem field that the IPA shouldn't use. Let's rework this
> as part of the buffer API rework.

Yes.

> 
> > +
> > +/**
> > + * \struct IPAOperationData
> > + * \brief Parameters for IPA operations
> > + *
> > + * A pipeline defines its IPA protocol by describing how an  IPAOperationData
> 
> s/pipeline/pipeline handler/
> s/  / /
> 
> > + * structure shall be filled out for each of its operations. When the structure
> > + * is populated it can be sent to the other part of the protocol and decoded.
> > + *
> > + * The \a operation describes which operation the receiver shall perform
> > + * and implicitly describes which of the other fields in the structure are
> > + * populated and how they shall be interpreted. All this is the responsibility
> > + * of the pipeline implementation.
> > + *
> > + * \sa IPAInterface::processEvent()
> > + * \sa IPAInterface::queueFrameAction
> > + */
> 
> I realise now that my documentation proposal for the \file block is
> missing a description of IPA operations. I propose adding the following
> as the second paragraph:
> 
>  * Pipeline handlers define the set of events and actions used to communicate
>  * with their IPA. These are collectively referred to ias IPA operations and
>  * define the pipeline handler-specific IPA protocol. Each operation defines the
>  * data that it carries, and how the data is encoded in the IPAOperationData
>  * structure.
> 
> The IPAOperationData documentation could then be updated as
> 
> /**
>  * \struct IPAOperationData
>  * \brief Parameters for IPA operations
>  *
>  * The IPAOperationData structure carries parameters for the IPA operations
>  * performed through the IPAInterface::processEvent() method and the
>  * IPAInterface::queueFrameAction signal.
>  */
> 
> > +
> > +/**
> > + * \var IPAOperationData::operation
> > + * \brief Operation in the pipeline-to-IPA protocol
> > + *
> > + * The allocation of numerical values to operations are left to the
> > + * implementation of the pipeline protocol.
> > + */
> 
> And this block could be extended with the description of the operation
> field that was previously part of the \struct IPAOperationData block.
> 
> /**
>  * \var IPAOperationData::operation
>  * \brief IPA protocol operation
>  *
>  * The operation field describes which operation the receiver shall perform. It
>  * defines, through the IPA protocol, how the other fields of the structure are
>  * interpreted. The protocol freely assigns numerical values to operations.
>  */
> 
> > +
> > +/**
> > + * \var IPAOperationData::data
> > + * \brief Array of unsigned int values
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> 
> I would say "are defined by the IPA protocol" instead of "are left to
> ...", in order to consistently use the term IPA protocol to refer to the
> pipeline handler-specific protocol. Same for the two fields below.
> 
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::controls
> > + * \brief Array of ControlList
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> > + */
> > +
> > +/**
> > + * \var IPAOperationData::v4l2controls
> > + * \brief Array of V4L2ControlList
> > + *
> > + * The interpretation and position of different values in the array are left
> > + * to the implementation of the pipeline protocol.
> > + */
> > +
> >  /**
> >   * \class IPAInterface
> >   * \brief Interface for IPA implementation
> > + *
> > + * Every pipeline implementation in libcamera may attach all or some of its
> 
> s/pipeline/pipeline handler/
> 
> > + * cameras to an Image Processing Algorithm (IPA) plugin. An IPA plugin is
> 
> We don't use IPA plugin anywhere else, the documentation talks about IPA
> module. I would do the same here and below.
> 
> > + * developed for a specific pipeline and each pipeline might have multiple
> 
> s/pipeline/pipeline handler/g
> s/might/may/ (we should fix other occurences of might)
> 
> > + * competing IPA implementations, both open and closed source.
> 
> I would say "compatible" instead of "competing".
> 
> > + *
> > + * To allow for multiple and competing IPA plugins for the same pipeline, an
> 
> s/and competing /
> 
> > + * interface for the pipeline and IPA communication is needed; IPAInterface
> 
> s/an interface/a standard interface/
> s/pipeline/pipeline handler/
> s/;/./
> 
> > + * is this interface.
> 
> I'd start a new paragraph here.
> 
> > The interface defines how and what type of data can be
> > + * exchanged, not how the data exchanged makes up a protocol. Each pipeline's
> > + * design is unique and so is its IPA protocol. It is the pipeline's
> > + * responsibility to define and document a protocol on top of the IPAInterface
> > + * which can be used by multiple IPA plugins.
> 
>  * The interface defines base data types and methods to exchange data. On top of
>  * this, each pipeline handler is responsible for defining specific operations
>  * that make up its IPA protocol, shared by all IPA modules compatible with the
>  * pipeline handler.
> 
> > + *
> > + * The IPAInterface provides some methods to help the protocol design. As the
> > + * requirements of the IPA plugins become more clear it is expected more
> > + * common protocol operations will be identified and added.
> 
> I think this is a bit confusing. Do you mean more common methods in
> IPAInterface, or common operations in the sense of
> IPAOperationData::operation ? I think we should consistently use the
> word operation for the latter and method (or interface method) for the
> former.
> 
> If you refer to operations, the IPAInterface doesn't provide any. I
> agree that some common operations may be added in the future, but it's
> not "*more* common protocol operations" as we have none. I would then
> write
> 
>  * No standard IPA operations are currently defined by the IPA API. As
>  * the requirements of the IPA plugins become clearer, common protocol
>  * operations are expected to be identified and added.
> 
> If you refer to methods, I'm not sure we'll add more, and I'm not sure
> we can really say they help the protocol design. I would propose
> dropping the paragraph in that case.

I was referring to methods, so then this paragraph shall be dropped.

> 
> I would add a blank line here.
> 
> > + * \todo Add more common operations if possible.
> 
> s/more common operations/commong IPA operations/
> 
> > + *
> > + * The pipeline shall use the IPAManager to locate an IPAInterface compatible
> 
> s/pipeline/pipeline handler/
> 
> > + * with the pipeline. The interface may then be used in the pipeline to interact
> 
> Maybe just "a compatible IPAInterface" ? And s/in the pipeline //
> 
> > + * with the IPA plugin to make up the protocol.
> 
> How about just "The interface is then used to interact with the IPA
> module." ? We've already talked about the protocol above.
> 
> I wonder if we should talk about the IPAManager here. We currently
> generate a single documentation for all the libcamera APIs (public, IPA
> and internal), but down the line I think it would make sense to split
> them. Applications developers should not be bothered with IPA and internal
> APIs. We may want to avoid bothering IPA developers with internal APIs (for
> the purpose of this discussion I include the currently internal APIs
> that can be used as helpers by open-source IPAs in the category of IPA
> API). This being said, given that IPAs are to be developed with pipeline
> handlers and not in isolation, it may make sense to not split the IPA
> documentation and keep all the internal documentation in one large
> piece. The above paragraph would then make complete sense.

I agree that we might wish to split the docs. But I think pipe and IPA 
documentation go together.

> 
> And I would add a blank line here too.
> 
> > + * \todo Add reference to how pipelines shall document their protocol.
> > + *
> > + * \sa IPAManager
> 
> Do we need a \sa when doxygen already generates a link in the text ? I
> tend to use \sa only for references that are not present in the text,
> but maybe my usage is not correct.

I did not consider that, I think we shall drop the \sa here.

> 
> >   */
> >  
> >  /**
> > @@ -24,4 +151,72 @@ namespace libcamera {
> >   * \brief Initialise the IPAInterface
> >   */
> >  
> > +/**
> > + * \fn IPAInterface::configure()
> > + * \brief Configure the IPA stream and sensor settings
> > + * \param[in] streamConfig List of stream configuration descriptions
> 
> "Configuration of all active streams" ?
> 
> > + * \param[in] entityControls List of controls provided by the pipeline entities
> 
> s/List of controls/Controls/ ?
> 
> > + *
> > + * This operation shall be called when the camera is started to inform the IPA
> 
> s/This operation/This method/
> 
> > + * of the camera's streams and the sensor settings.
> 
> When the camera is started, or when the camera is configured ? I think
> the former is fine for now (no need to update anything), but we may want
> to move it to configure time later if we want to give the IPA a chance
> to reject a configuration. I'm not sure there would be a use case for
> that though, as it would be difficult to do so in a way that would
> report meaningful error causes to applications, especially with
> closed-source IPAs.
> 
> I would add the following sentence.
> 
> "The meaning of the numerical keys in the \a streamConfig and \a
> entityControls maps is defined by the IPA protocol."
> 
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::mapBuffers()
> > + * \brief Map the buffers shared by the pipeline to the IPA
> 
> s/the buffers/buffers/
> 
> s/by the pipeline to the IPA/by the pipeline handler with the IPA/
> 
> or
> 
> s/by the pipeline to the IPA/between the pipeline handler and the IPA/
> 
> In case I've missed other occurences, we should use "pipeline handler"
> to refer to pipeline handlers. The word "pipeline" refers to the
> device's pipeline (likely the hardware pipeline), as correctly done in
> the description of the configure() operation entityControls parameter.
> 
> > + * \param[in] buffers List of buffers to map
> > + *
> > + * This operation is used to inform the IPA module of the memory buffers set up
> > + * by the pipeline handler and associate to each of them a numerical \a type and
> > + * \a id. All buffers that the pipeline wishes to share with an IPA must be
> > + * mapped in the IPA with this operation.
> > + *
> > + * After the buffers have been initialized a specific buffer can be referenced
> > + * using the numerical \a type and \a id provided when the buffers were mapped.
> > + *
> > + * The numerical values for type and id used to describe a buffer have no
> > + * meaning in the core libcamera code and the interface is pipeline handler
> > + * specific.
> 
> With the removal of the type field, I propose
> 
>  * This method informs the IPA module of memory buffers set up by the pipeline
>  * handler that the IPA needs to access. It provides dmabuf file handles for
>  * each buffer, and associates the buffers with unique numerical IDs.
>  *
>  * IPAs shall map the dmabuf file handles to their address space and keep a
>  * cache of the mappings, indexed by the buffer numerical IDs. The IDs are used
>  * in all other IPA interface methods to refer to buffers, including the
>  * unmapBuffers() method.
>  *
>  * All buffers that the pipeline handler wishes to share with an IPA shall be
>  * mapped with this method. Buffers may be mapped all at once with a single
>  * call, or mapped and unmapped dynamically at runtime, depending on the IPA
>  * protocol. Regardless of the protocol, all buffers mapped at a given time
>  * shall have unique numerical IDs.
>  *
>  * The numerical IDs have no meaning defined by the IPA interface, and IPA
>  * protocols shall not give them any specific meaning either. They should be
>  * treated as opaque handles by IPAs, with the only exception that ID zero is
>  * invalid.
> 
> > + *
> > + * \sa unmapBuffers()
> > + * \todo Can we make this a generic function?
> 
> Do you mean "Provide a generic implementation of mapBuffers and
> unmapBuffers for IPAs" ?

Yes.

> 
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::unmapBuffers()
> > + * \brief Unmap the buffers shared by the pipeline to the IPA
> > + * \param[in] buffers List of buffers to unmap
> 
> Should this take a list of IDs instead of IPABuffer ? Passing both IDs
> and BufferMemory opens the door to passing incorrect information. The
> IPA needs to cache the virtual mapping information in any case, so I
> think it could as well cache the size (if it doesn't already) and not
> need the BufferMemory to be passed by the pipeline handler.

This is a really god point, yes it should only take the ID.

> 
> > + *
> > + * This operation removes the mapping set up with mapBuffers().
> 
>  * This method removes mappings set up with mapBuffers(). Buffers may be
>  * unmapped all at once with a single call, or selectively at runtime, depending
>  * on the IPA protocol. Numerical IDs of unmapped buffers may be reused when
>  * mapping new buffers.
> 
> > + *
> > + * \sa mapBuffers()
> > + * \todo Can we make this a generic function?
> 
> You can probably drop this todo item, the previous one should be enough.
> 
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::processEvent()
> > + * \brief Process an event from the pipeline handler
> > + * \param[in] event Event to process
> 
> Should the parameter be named data and documented as "IPA operation
> data" ?
> 
> > + *
> > + * This operation is used by pipeline handlers to inform the IPA module of
> > + * events that occurred during the on-going capture operation.
> > + *
> > + * Each \a event notified by the pipeline handler with this method is handled
> > + * by the IPA which interprets the operation parameters in an implementation
> > + * specific way, which needs to be separately documented.
> 
>  * The \a event notified by the pipeline handler with this method is handled by
>  * the IPA, which interprets the operation parameters according to the
>  * separately documented IPA protocol.
> 
> > + */
> > +
> > +/**
> > + * \fn IPAInterface::queueFrameAction()
> 
> This isn't a function, but a signal. It should be documented as such,
> but we'll then lose the arguments :-S It seems however that Doxygen
> doesn't complain about the use of \fn. queueFrameAction is still listed
> as a member data, not member function. Actually, using \var with \param
> seems to be accepted, which seems weird. Maybe we could just replace \fn
> with \var and keep the parameters ?
> 
> > + * \brief Queue an action associated with a frame to the pipeline handler
> > + * \param[in] frame The frame number for the action
> > + * \param[in] controls List of controls associated with the action
> 
> This isn't correct anymore, the signal has a single parameter. I think
> the frame number makes sense though, should we modify the signal to take
> a frame number and an IPAOperationData ? In any case I think the second
> field should be documented as "IPA operation data"
> 
> > + *
> > + * This signal is emitted when the IPA wishes to queue a FrameAction on the
> > + * pipeline. The pipeline is still responsible for the scheduling of the action
> > + * on its timeline.
> > + *
> > + * The IPA operation describing the frame action is passed as a parameter.
> 
> FrameAction isn't part of the IPA core API anymore. How about the
> following ?
> 
>  * This signal is emitted by the IPA to queue an action to be executed by the
>  * pipeline handler on a frame. The type of action is identified by the
>  * \a data.operation field, as defined by the IPA protocol, and the rest of the
>  * \a data is interpreted accordingly. The pipeline handler shall queue the
>  * action and execute it as appropriate.
> 
> I'm not very happy about this description as it hints the usage of a
> timeline without explicitly describing it, but I think that's the best
> we can do for now until we generalise the timeline implementation.
> 
> > + */
> > +
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index 62fcb529e1c7e4ff..14e8bb6067623fc6 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -26,7 +26,12 @@ public:
> >  	IPAProxyLinux(IPAModule *ipam);
> >  	~IPAProxyLinux();
> >  
> > -	int init();
> > +	int init() override { return 0; }
> > +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +		       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override {}
> > +	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void unmapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > +	void processEvent(const IPAOperationData &event) override {}
> >  
> >  private:
> >  	void readyRead(IPCUnixSocket *ipc);
> > @@ -36,13 +41,6 @@ private:
> >  	IPCUnixSocket *socket_;
> >  };
> >  
> > -int IPAProxyLinux::init()
> > -{
> > -	LOG(IPAProxy, Debug) << "initializing IPA via dummy proxy!";
> > -
> > -	return 0;
> > -}
> > -
> >  IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)
> >  	: proc_(nullptr), socket_(nullptr)
> >  {
> 
> -- 
> Regards,
> 
> Laurent Pinchart