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

[Laurent Pinchart]

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 at 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)
>  {

-- 
Regards,

Laurent Pinchart