[libcamera-devel] [PATCH v5 07/10] libcamera: ipa: Extend to support IPA interactions
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 9 15:33:34 CEST 2019
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 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"
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)
> {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list