[libcamera-devel] [PATCH v5 07/10] libcamera: ipa: Extend to support IPA interactions
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Oct 10 11:22:32 CEST 2019
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 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 ?
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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list