| Message ID | 20191003174941.1296988-7-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. 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@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 ? > +}; > + > 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. > + > + 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 ? > + 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 > + * 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. > + * > + * \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. > + * > + * The protocol between pipeline and IPA are hilly specific for each pipeline > + * 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 > + * \param[in] frame The frame number for the request > + * \param[in] controls List of controls associated with the request > + * > + * 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) > {
Hi Niklas, Laurent, a few comments On Fri, Oct 04, 2019 at 08:58:33AM +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > 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@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. > > +}; > > + > > 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 ? Should we provide a <unsigned int cookie, std::string entityName> at init() time ? > > + > > + 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. 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); .... }; } --------------------------------------------------- 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. --------------------------------------------------- > > + 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 > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo and Laurent, Thanks for your feedback. On 2019-10-04 12:03:20 +0200, Jacopo Mondi wrote: > Hi Niklas, Laurent, > a few comments > > On Fri, Oct 04, 2019 at 08:58:33AM +0300, Laurent Pinchart wrote: > > Hi Niklas, > > > > Thank you for the patch. > > > > 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@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); > .... > }; > } > --------------------------------------------------- > > 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. > > --------------------------------------------------- > > > > + 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 > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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@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) > >>> {
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. */ +}; + 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..c0f2004f3ec993b2 100644 --- a/src/libcamera/ipa_interface.cpp +++ b/src/libcamera/ipa_interface.cpp @@ -14,6 +14,68 @@ namespace libcamera { + +/** + * \struct IPAStream + * \brief Hold IPA description of a stream. + */ + +/** + * \var IPAStream::pixelFormat + * \brief The streams pixel format + */ + +/** + * \var IPAStream::size + * \brief The streams size + */ + +/** + * \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 + */ + +/** + * \struct IPAOperationData + * \brief Hold data exchanged between pipeline and IPA + * + * 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. + */ + +/** + * \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 + */ + /** * \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. + */ + +/** + * \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 + * 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. + * + * \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. + * + * The protocol between pipeline and IPA are hilly specific for each pipeline + * 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 + * \param[in] frame The frame number for the request + * \param[in] controls List of controls associated with the request + * + * 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 {} 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) {
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@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(-)