[libcamera-devel] [PATCH v4 06/11] libcamera: ipa: Extend to support IPA interactions
Jacopo Mondi
jacopo at jmondi.org
Fri Oct 4 12:03:20 CEST 2019
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 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.
> > +};
> > +
> > 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191004/3156acee/attachment.sig>
More information about the libcamera-devel
mailing list