[libcamera-devel] [PATCH v4 06/11] libcamera: ipa: Extend to support IPA interactions
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Oct 6 00:40:57 CEST 2019
Hello,
On Sun, Oct 06, 2019 at 12:27:17AM +0200, Niklas Söderlund wrote:
> On 2019-10-04 12:03:20 +0200, Jacopo Mondi wrote:
> > On Fri, Oct 04, 2019 at 08:58:33AM +0300, Laurent Pinchart wrote:
> >> On Thu, Oct 03, 2019 at 07:49:36PM +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 | 37 +++++++
> >>> src/ipa/ipa_dummy.cpp | 7 +-
> >>> src/libcamera/ipa_interface.cpp | 132 ++++++++++++++++++++++++
> >>> src/libcamera/proxy/ipa_proxy_linux.cpp | 14 ++-
> >>> 4 files changed, 181 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> >>> index 2c5eb1fd524311cb..2c715314c7a418f0 100644
> >>> --- a/include/ipa/ipa_interface.h
> >>> +++ b/include/ipa/ipa_interface.h
> >>> @@ -7,14 +7,51 @@
> >>> #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 id;
> >>> + unsigned int type;
> >>> + BufferMemory buffer;
> >>> +};
> >>> +
> >>> +struct IPAOperationData {
> >>> + unsigned int operation;
> >>> + std::vector<uint32_t> data;
> >>> + std::vector<ControlList> controls;
> >>> + /* \todo: Add more vectors for data types used in pipa<->IPA interactions. */
> >>
> >> If I'm not mistaken you use this to set sensor parameters. What's the
> >> reason for not including a V4L2ControlList already ? Do you plan to add
> >> one later ? Or is the plan to find a way to pass V4L2 controls through a
> >> ControlList ?
> >
> > Just to add the you include the v4l2_controls.h already to pass the
> > V4L2ControlInfoMap at configure time, so I see no reasons why there
> > shouldn't be a V4L2CotrolList here.
>
> It was debated on irc while this series was written that V4L2CotrolList
> would be replaced by a ControlList. As the result of that was not clear
> I opted to keep what already was in this series. I will switch back to
> V4L2CotrolList in next version.
>
> >>> +};
> >>> +
> >>> 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;
> >>
> >> The data types are getting a bit long, an alias might be nice, but I
> >> think we'll end up refactoring this anyway, so it's fine for now in my
> >> opinion.
> >
> > How would you keep track of the entities in the entityControls map ?
> > Are pipelines and associated IPA supposed to know what entity 0,1,2..
> > are ?
>
> That is up to each pipeline handler to specify, which numerical ID will
> represent which media entity. This of course needs to be documented in a
> pipeline/IPA protocol description.
>
> > Should we provide a <unsigned int cookie, std::string entityName> at
> > init() time ?
>
> I think that is a bad idea. If we want to use the entity name instead of
> a numerical id we can make the map<std::string, V4L2ControlInfoMap>. I'm
> open for this.
>
> >>> +
> >>> + virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> >>> + virtual void unmapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> >>
> >> I like this :-)
> >>
> >>> +
> >>> + virtual void processEvent(const IPAOperationData &event) = 0;
> >>
> >> Do you think we should split queueRequest() to a dedicated method, as we
> >> know we'll always need it ? Or do you think we should wait ? If so, when
> >> do you think would be a good time ?
> >
> > Same questions actually.
>
> We had this in v3, then it was argued that there might be pipelines that
> needs more information than frame number and the request control list.
> And that queueRequest() should be merged with the new processEvent()
> operation.
>
> I have no strong feelings and can se pros and cons with both. But the
> killer for me as we are still debating this is that it's easier to go
> from processEvent() -> queueRequest() later then the other way around. I
> would prefer to keep this as is.
>
> > I know the argument against have an explicit queueRequest() is that by
> > using the IPAOperationData argument of processEvent() to transport its
> > parameters we have a more flexible interace, as pipe can use data[] to
> > transport arbitrary data.
> >
> > For this first version I would anyhow keep queueRequest() separate, as
> > it's a corner-stone operation of pipe/IPA interaction, with possibly a
> > data[] in its parameter with the canonical ControlList & so we get the
> > same amount of flexibility but we have the operation already broken
> > out.
> >
> > ----------------------------------------------------------------------
> > Probably for later, don't let this delay this work, but:
> > going forward, I still think we should find a way to define
> > operations signature of IPAInterface that use a pure virtual base
> > classes, and ask pipe/IPA implementations to use their own derived types
> > with an associated serialization procedure.
> >
> > Not to push for what I had, but the pure virtual base class should
> > inherit from a Serializable interface and derived classes should
> > define their own serialize() and deserialize() operations. When going
> > through IPC the operation's parameters would be serialized, when
> > short-circuiting and going through direct calls on the IPAInterface
> > the derived classes instances would be down-casted from the base class
> > pointer.
> >
> > Something like:
> >
> > class IPARequest : public Serializable
> > {
> > private:
> > unisgned int frame;
> > const ControlList &controls;
> > };
> >
> > class IPAInterface
> > {
> > virtual int queueRequest(IPARequest *request);
> > };
> >
> > --------------------------------------------------
> >
> > class RKISP1Request : public IPARequest
> > {
> > /* Augment the class with the required data/operations
> >
> > /* Provide a serialize/deserialize operations. */
> > int serialize() override;
> > int deserialize(int8_t *data) override;
> > };
> >
> > class RKISP1IPA
> > {
> > int queueRequest(IPARequest *request)
> > {
> > RKISP1Request *r = reinterpret_cast<RKISP1Request *>(request);
This should be a static_cast, not a reinterpret_cast.
> > ....
> > };
> > }
> > ---------------------------------------------------
> >
> > When going through IPC the IPAInterfaceWrapper would call
> > the virtual serialize/deserialize operations and reduce all to the
> > equivalent of data[] when going through the C interface.
> >
> > ---------------------------------------------------
It's not a bad idea on paper, let me sleep over it :-)
> >>> + 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..c0f2004f3ec993b2 100644
> >>> --- a/src/libcamera/ipa_interface.cpp
> >>> +++ b/src/libcamera/ipa_interface.cpp
> >>> @@ -14,6 +14,68 @@
> >>>
> >>> namespace libcamera {
> >>>
> >>> +
> >>
> >> One blank line is enough.
> >>
> >>> +/**
> >>> + * \struct IPAStream
> >>> + * \brief Hold IPA description of a stream.
> >>
> >> I think you should detail this a bit. Maybe something like the following
> >> ?
> >>
> >> * \brief Stream configuration for the IPA API
> >> *
> >> * This structure mirrors StreamConfiguration for the IPA API. It stores
> >> * the configuration of a stream.
> >>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var IPAStream::pixelFormat
> >>> + * \brief The streams pixel format
> >>
> >> s/streams/stream's/ or just "The pixel format"
> >>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var IPAStream::size
> >>> + * \brief The streams size
> >>
> >> Same here.
> >>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \struct IPABuffer
> >>> + * \brief Hold IPA description of a buffer
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var IPABuffer::id
> >>> + * \brief The pipeline to IPA id cookie for the buffer
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var IPABuffer::type
> >>> + * \brief The pipeline to IPA type for the buffer
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var IPABuffer::buffer
> >>> + * \brief The hardware description of the buffer shared between pipeline and IPA
> >>> + */
> >>
> >> All this is very terse. I know what this structure and its fields are
> >> used for because we've discussed it, but reading the documentation would
> >> leave the reader puzzled :-S
> >>
> >>> +
> >>> +/**
> >>> + * \struct IPAOperationData
> >>> + * \brief Hold data exchanged between pipeline and IPA
> >>
> >> Just drop the verb from the brief description. "Data container for
> >> parameters to IPA operations"
> >>
> >>> + *
> >>> + * This is the information carrier between pipeline and IPA. The is is used
> >>> + * to transport the pipeline specific protocol between the two. Core libcamera
> >>> + * components do not try to interpret the protocol and it's up to the pipeline
> >>> + * handler to uses the members of this struct in any way they see fit, and to
> >>> + * document it so multiple IPA implementations for the same pipeline may use it.
> >>
> >> Same here, I don't think this would make any sense to someone who hasn't
> >> followed the development closely.
> >>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var IPAOperationData::operation
> >>> + * \brief Intended as the operation code in the pipeline to IPA protocol
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var IPAOperationData::data
> >>> + * \brief Intended as the generic data carrier in the pipeline to IPA protocol
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var IPAOperationData::controls
> >>> + * \brief Intended as the ControlList carrier in the pipeline to IPA protocol
> >>> + */
> >>
> >> And same for the three fields too.
> >>
> >>> +
> >>> /**
> >>> * \class IPAInterface
> >>> * \brief Interface for IPA implementation
> >>> @@ -24,4 +86,74 @@ 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 function is called when a pipeline attaches to an IPA to inform the IPA
> >>> + * of the streams and controls limits the entities(s) in the video pipeline
> >>> + * supports.
> >>
> >> That's not correct, this is called when the streams are configured.
> >> Furthermore there's no "attach" operation defined anywhere. We really
> >> need to improve this documentation I'm afraid. I'll leave the
> >> documentation below unreviewed.
> >>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn IPAInterface::mapBuffers()
> >>> + * \brief Map the buffers shared by the pipeline to the IPA
> >>> + * \param[in] buffers List of buffers to map
> >>> + *
> >>> + * This function is called when a pipeline handler wants to inform the IPA of
> >>> + * which buffers it has mapped which the IPA can make use of. All buffers shared
> >
> > double which.
> >
> > I would
> > "This operation is used to inform the IPA module of the memory buffers
> > set up by pipeline handlers and associate to each of them a numerical
> > cookie id, that will be used to identify the buffer in all sequent
> > operations."
> >
> >>> + * between these two object's needs to be shared using this function prior to
> >>> + * use.
> >>> + *
> >>> + * After the buffers have been initialized a specific buffer can be referenced
> >>> + * using the numerical \a type and \a id provided when the buffers where 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 function is called when a pipeline handler wants to the IPA to unmap
> >>> + * all or some of the buffer it have mapped.
> >>> + *
> >
> > "remove 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
> >>> + *
> >>> + * The pipeline handler can send events to the IPA to notify it of what is
> >>> + * going on. This is the entry point on the IPA side for those messages.
> >
> > "what is going on" is pretty generic.
> >
> > "This operation is used by pipeline handlers to inform the IPA
> > module of events occurred during the on-going capture operation.
> >
> > Each \a event notified by the pipeline handler with this method is
> > handled by the IPAModule which interprets the operation parameters in
> > an implementation specific way, which needs to be separately
> > documented."
> >
> > Or something like that...
> >
> >>> + *
> >>> + * The protocol between pipeline and IPA are hilly specific for each pipeline
> >
> > hilly ?
> >
> >>> + * and needs to be documented separately. Libcamera reserves no operation
> >>> + * numbers and make no attempt to interpret the protocol.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn IPAInterface::queueFrameAction()
> >>> + * \brief Signal emitted when the IPA wish to queue a FrameAction
> >
> > "Queue an action associated with a frame to the pipeline handler"
> >
> >>> + * \param[in] frame The frame number for the request
> >
> > s/request/action
> >
> >>> + * \param[in] controls List of controls associated with the request
> >
> > s/request/action
> >
> >>> + *
> >>> + * This signal is emitted when the IPA wish 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 {}
> >>
> >> Very clearly a hack :-) That's fine, we'll fix it.
> >>
> >>>
> >>> 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