[libcamera-devel] [PATCH v2 02/10] libcamera: pipeline: add name, major version, and minor version
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 4 12:28:35 CEST 2019
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 at 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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list