[libcamera-devel,v2,02/10] libcamera: pipeline: add name, major version, and minor version

1339 diff mbox series

In order to match an IPA module with a pipeline handler, the pipeline
handler must have a name and a version. Add these to PipelineHandler as
getters and attributes so that they can be automatically defined by
REGISTER_PIPELINE_HANDLER. Also add a macro PIPELINE_HANDLER to create a
single version number given a major and minor version pair. It is put in
ipa_module_info.h because IPA modules will also need this macro.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
Changes in v2:
- make the name and version getters into methods of the base PipelineHandler
  class, so that the specialized pipeline handlers no longer have to
  specify overriding
- add PIPELINE_VERSION macro to create one version number from major and
  minor pair

 include/libcamera/ipa/ipa_module_info.h  |  2 ++
 src/libcamera/include/pipeline_handler.h | 14 ++++++++---
 src/libcamera/ipa_module.cpp             | 17 +++++++++++++
 src/libcamera/pipeline/ipu3/ipu3.cpp     | 13 +++++++---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 +++++++----
 src/libcamera/pipeline/uvcvideo.cpp      | 11 +++++---
 src/libcamera/pipeline/vimc.cpp          | 12 ++++++---
 src/libcamera/pipeline_handler.cpp       | 32 ++++++++++++++++++++++--
 8 files changed, 93 insertions(+), 22 deletions(-)

Hi Paul,

Thank you for the patch.

On Mon, Jun 03, 2019 at 07:16:29PM -0400, Paul Elder wrote:
> In order to match an IPA module with a pipeline handler, the pipeline
> handler must have a name and a version. Add these to PipelineHandler as
> getters and attributes so that they can be automatically defined by
> REGISTER_PIPELINE_HANDLER. Also add a macro PIPELINE_HANDLER to create a
> single version number given a major and minor version pair. It is put in
> ipa_module_info.h because IPA modules will also need this macro.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> Changes in v2:
> - make the name and version getters into methods of the base PipelineHandler
>   class, so that the specialized pipeline handlers no longer have to
>   specify overriding
> - add PIPELINE_VERSION macro to create one version number from major and
>   minor pair
> 
>  include/libcamera/ipa/ipa_module_info.h  |  2 ++
>  src/libcamera/include/pipeline_handler.h | 14 ++++++++---
>  src/libcamera/ipa_module.cpp             | 17 +++++++++++++
>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 13 +++++++---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 +++++++----
>  src/libcamera/pipeline/uvcvideo.cpp      | 11 +++++---
>  src/libcamera/pipeline/vimc.cpp          | 12 ++++++---
>  src/libcamera/pipeline_handler.cpp       | 32 ++++++++++++++++++++++--
>  8 files changed, 93 insertions(+), 22 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipa_module_info.h b/include/libcamera/ipa/ipa_module_info.h
> index 4e0d668..e5f2a7e 100644
> --- a/include/libcamera/ipa/ipa_module_info.h
> +++ b/include/libcamera/ipa/ipa_module_info.h
> @@ -7,6 +7,8 @@
>  #ifndef __LIBCAMERA_IPA_MODULE_INFO_H__
>  #define __LIBCAMERA_IPA_MODULE_INFO_H__
>  
> +#define PIPELINE_VERSION(majorV, minorV) (majorV * 1000 + minorV)
> +
>  #ifdef __cplusplus
>  namespace libcamera {
>  #endif
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 7da6df1..67971b3 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -50,7 +50,8 @@ private:
>  class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>
>  {
>  public:
> -	PipelineHandler(CameraManager *manager);
> +	PipelineHandler(CameraManager *manager, const char *name,
> +			uint32_t version);
>  	virtual ~PipelineHandler();
>  
>  	virtual bool match(DeviceEnumerator *enumerator) = 0;
> @@ -77,6 +78,9 @@ public:
>  	bool completeBuffer(Camera *camera, Request *request, Buffer *buffer);
>  	void completeRequest(Camera *camera, Request *request);
>  
> +	const char *name() const { return name_; }
> +	uint32_t version() const { return version_; }
> +
>  protected:
>  	void registerCamera(std::shared_ptr<Camera> camera,
>  			    std::unique_ptr<CameraData> data);
> @@ -86,6 +90,9 @@ protected:
>  
>  	CameraManager *manager_;
>  
> +	const char *name_;
> +	uint32_t version_;

I think these two can be made private.

> +
>  private:
>  	void mediaDeviceDisconnected(MediaDevice *media);
>  	virtual void disconnect();
> @@ -112,14 +119,15 @@ private:
>  	std::string name_;
>  };
>  
> -#define REGISTER_PIPELINE_HANDLER(handler)				\
> +#define REGISTER_PIPELINE_HANDLER(handler, pipelineVersion)		\

I would replace the pipelineVersion argument with two major and minor
arguments to make usage of the REGISTER_PIPELINE_HANDLER() macro easier,
and convert them with PIPELINE_VERSION() in the make_shared() call
below.

>  class handler##Factory final : public PipelineHandlerFactory		\
>  {									\
>  public:									\
>  	handler##Factory() : PipelineHandlerFactory(#handler) {}	\
>  	std::shared_ptr<PipelineHandler> create(CameraManager *manager)	\
>  	{								\
> -		return std::make_shared<handler>(manager);		\
> +		return std::make_shared<handler>(manager, #handler,	\
> +						 pipelineVersion);	\

What bothers me is that you have to modify all pipeline handlers to pass
the name and version number to the base PipelineHandler constructor. How
about the following ?

- Make PipelineHandlerFactory a friend of PipelineHandler
- Add a new method to PipelineHandlerFactory

void PipelineHandlerFactoy::setInfo(PipelineHandler *handler, const char *name,
				    unsigned int major, unsigned int minor)
{
	handler->name_ = name;
	handler->version_ = PIPELINE_VERSION(major, minor);
}

(feel free to propose a better name for setInfo)

- Modify the create function to call setInfo()

	std::shared_ptr<PipelineHandler> create(CameraManager *manager)	\
	{								\
		std::shared_ptr<handler> h =				\
			std::make_shared<handler>(manager);		\
		setInfo(h.get(), #handler, major, minor);		\
		return h;						\
	}

>  	}								\
>  };									\
>  static handler##Factory global_##handler##Factory;
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index 86cbe71..db56415 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -169,6 +169,23 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>  
>  } /* namespace */
>  
> +/**
> + * \def PIPELINE_VERSION
> + * \brief Convert a major and minor version pair to a single version number
> + * \param[in] majorV Major version
> + * \param[in] minorV Minor version

I think you can name the arguments just minor and major.

> + *
> + * This macro should be used by the pipeline handler and by the IPA module

s/should/shall/

and based on the other comments above and below, I think you can remove
"by the pipeline handler and ".

> + * to declare the version of the pipeline handler, for matching purposes.
> + *
> + * The major version of the pipeline handler should be updated whenever a

s/should be updated/shall be increased/

> + * change is made that causes currently compatible IPA modules to no longer
> + * be compatible, or if there is an API change between the pipeline handler
> + * and IPA modules.

The last part of the sentence is really vague.

> The minor version should be updated whenever a change is
> + * made that causes currently compatible IPA modules to no longer be compatible,
> + * but future IPA versions will still be compatible.

I'm not sure to understand what you mean here...

> + */
> +
>  /**
>   * \struct IPAModuleInfo
>   * \brief Information of an IPA module
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 05005c4..cfbf86a 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -13,6 +13,7 @@
>  #include <linux/media-bus-format.h>
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/ipa/ipa_module_info.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -194,7 +195,8 @@ private:
>  class PipelineHandlerIPU3 : public PipelineHandler
>  {
>  public:
> -	PipelineHandlerIPU3(CameraManager *manager);
> +	PipelineHandlerIPU3(CameraManager *manager, const char *name,
> +			    uint32_t version);
>  
>  	CameraConfiguration *generateConfiguration(Camera *camera,
>  		const StreamRoles &roles) override;
> @@ -372,8 +374,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	return status;
>  }
>  
> -PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> -	: PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)
> +PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager,
> +					 const char *name,
> +					 uint32_t version)
> +	: PipelineHandler(manager, name, version),
> +	  cio2MediaDev_(nullptr), imguMediaDev_(nullptr)
>  {
>  }
>  
> @@ -1452,6 +1457,6 @@ int CIO2Device::mediaBusToFormat(unsigned int code)
>  	}
>  }
>  
> -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3, PIPELINE_VERSION(0, 1));
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 9b3eea2..077d84e 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -14,6 +14,7 @@
>  #include <linux/media-bus-format.h>
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/ipa/ipa_module_info.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -73,7 +74,8 @@ private:
>  class PipelineHandlerRkISP1 : public PipelineHandler
>  {
>  public:
> -	PipelineHandlerRkISP1(CameraManager *manager);
> +	PipelineHandlerRkISP1(CameraManager *manager, const char *name,
> +			      uint32_t version);
>  	~PipelineHandlerRkISP1();
>  
>  	CameraConfiguration *generateConfiguration(Camera *camera,
> @@ -200,9 +202,11 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  	return status;
>  }
>  
> -PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> -	: PipelineHandler(manager), dphy_(nullptr), isp_(nullptr),
> -	  video_(nullptr)
> +PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager,
> +					     const char *name,
> +					     uint32_t version)
> +	: PipelineHandler(manager, name, version), dphy_(nullptr),
> +	  isp_(nullptr), video_(nullptr)
>  {
>  }
>  
> @@ -499,6 +503,6 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
>  	completeRequest(activeCamera_, request);
>  }
>  
> -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, PIPELINE_VERSION(0, 1));
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 45260f3..430f467 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/ipa/ipa_module_info.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -50,7 +51,8 @@ public:
>  class PipelineHandlerUVC : public PipelineHandler
>  {
>  public:
> -	PipelineHandlerUVC(CameraManager *manager);
> +	PipelineHandlerUVC(CameraManager *manager, const char *name,
> +			   uint32_t version);
>  
>  	CameraConfiguration *generateConfiguration(Camera *camera,
>  		const StreamRoles &roles) override;
> @@ -115,8 +117,9 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
>  	return status;
>  }
>  
> -PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> -	: PipelineHandler(manager)
> +PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager, const char *name,
> +				       uint32_t version)
> +	: PipelineHandler(manager, name, version)
>  {
>  }
>  
> @@ -263,6 +266,6 @@ void UVCCameraData::bufferReady(Buffer *buffer)
>  	pipe_->completeRequest(camera_, request);
>  }
>  
> -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, PIPELINE_VERSION(0, 1));
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 0e4eede..78feeb8 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -9,6 +9,7 @@
>  #include <array>
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/ipa/ipa_module_info.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -53,7 +54,8 @@ public:
>  class PipelineHandlerVimc : public PipelineHandler
>  {
>  public:
> -	PipelineHandlerVimc(CameraManager *manager);
> +	PipelineHandlerVimc(CameraManager *manager, const char *name,
> +			    uint32_t version);
>  
>  	CameraConfiguration *generateConfiguration(Camera *camera,
>  		const StreamRoles &roles) override;
> @@ -130,8 +132,10 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
>  	return status;
>  }
>  
> -PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> -	: PipelineHandler(manager)
> +PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager,
> +					 const char *name,
> +					 uint32_t version)
> +	: PipelineHandler(manager, name, version)
>  {
>  }
>  
> @@ -274,6 +278,6 @@ void VimcCameraData::bufferReady(Buffer *buffer)
>  	pipe_->completeRequest(camera_, request);
>  }
>  
> -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, PIPELINE_VERSION(0, 1));
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index dd56907..b18f14d 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -104,14 +104,17 @@ LOG_DEFINE_CATEGORY(Pipeline)
>  /**
>   * \brief Construct a PipelineHandler instance
>   * \param[in] manager The camera manager
> + * \param[in] name The name of the pipeline handler
> + * \param[in] version The version of the pipeline handler
>   *
>   * In order to honour the std::enable_shared_from_this<> contract,
>   * PipelineHandler instances shall never be constructed manually, but always
>   * through the PipelineHandlerFactory::create() method implemented by the
>   * respective factories.
>   */
> -PipelineHandler::PipelineHandler(CameraManager *manager)
> -	: manager_(manager)
> +PipelineHandler::PipelineHandler(CameraManager *manager, const char *name,
> +				 uint32_t version)
> +	: manager_(manager), name_(name), version_(version)
>  {
>  }
>  
> @@ -505,6 +508,28 @@ CameraData *PipelineHandler::cameraData(const Camera *camera)
>   * constant for the whole lifetime of the pipeline handler.
>   */
>  
> +/**
> + * \fn PipelineHandler::name()
> + * \brief Retrieve the pipeline handler name
> + * \return The pipeline handler name
> + */
> +
> +/**
> + * \fn PipelineHandler::version()
> + * \brief Retrieve the pipeline handler version
> + * \return The pipeline handler version
> + */
> +
> +/**
> + * \var PipelineHandler::name_
> + * \brief Pipeline handler name
> + */
> +
> +/**
> + * \var PipelineHandler::version_
> + * \brief Pipeline handler version
> + */

You can remove the documentation of those two members if you make them
private.

> +
>  /**
>   * \class PipelineHandlerFactory
>   * \brief Registration of PipelineHandler classes and creation of instances
> @@ -586,9 +611,12 @@ std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories()
>   * \def REGISTER_PIPELINE_HANDLER
>   * \brief Register a pipeline handler with the pipeline handler factory
>   * \param[in] handler Class name of PipelineHandler derived class to register
> + * \param[in] pipelineVersion Version of the PipelineHandler
>   *
>   * Register a PipelineHandler subclass with the factory and make it available to
>   * try and match devices.
> + *
> + * \sa PIPELINE_VERSION
>   */
>  
>  } /* namespace libcamera */