[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:30:32 CEST 2019
Hi Laurent,
Thanks for your review.
On 2019-10-09 23:25:35 +0300, Laurent Pinchart wrote:
> 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.
> */
Why not.
>
> 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.
Yes.
>
> > +
> > +/**
> > + * \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 was referring to methods, so then this paragraph shall be dropped.
>
> 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.
I agree that we might wish to split the docs. But I think pipe and IPA
documentation go together.
>
> 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.
I did not consider that, I think we shall drop the \sa here.
>
> > */
> >
> > /**
> > @@ -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" ?
Yes.
>
> > + */
> > +
> > +/**
> > + * \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 is a really god point, yes it should only take the ID.
>
> > + *
> > + * 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list