[libcamera-devel] [PATCH v4 06/11] libcamera: ipa: Extend to support IPA interactions

[Laurent Pinchart]

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 ?

> +};
> +
>  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)
>  {

-- 
Regards,

Laurent Pinchart