[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:18:39 CEST 2019
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 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 ?
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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list