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

[Jacopo Mondi]

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 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;

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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191008/d5a96ab1/attachment.sig>