[libcamera-devel] [PATCH 2/8] libcamera: pipeline: add name, major version, and minor version

[Kieran Bingham]

Hi Paul,

On 27/05/2019 23:35, Paul Elder wrote:
> In order to match an IPA module with a pipeline handler, the pipeline
> handler must have a name and a major and minor version. Add these to
> PipelineHandler as functions, so that they can be automatically defined
> by REGISTER_PIPELINE_HANDLER. Also update documentation accordingly.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/libcamera/include/pipeline_handler.h | 20 ++++++++++++++++++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp     |  6 +++++-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  6 +++++-
>  src/libcamera/pipeline/uvcvideo.cpp      |  6 +++++-
>  src/libcamera/pipeline/vimc.cpp          |  6 +++++-
>  src/libcamera/pipeline_handler.cpp       | 20 ++++++++++++++++++++
>  6 files changed, 58 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 7da6df1..7963e7a 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -77,6 +77,10 @@ public:
>  	bool completeBuffer(Camera *camera, Request *request, Buffer *buffer);
>  	void completeRequest(Camera *camera, Request *request);
>  
> +	virtual const char *name() = 0;
> +	virtual int majorVersion() = 0;
> +	virtual int minorVersion() = 0;
> +
>  protected:
>  	void registerCamera(std::shared_ptr<Camera> camera,
>  			    std::unique_ptr<CameraData> data);
> @@ -112,7 +116,7 @@ private:
>  	std::string name_;
>  };
>  
> -#define REGISTER_PIPELINE_HANDLER(handler)				\
> +#define REGISTER_PIPELINE_HANDLER(handler, majorV, minorV)		\
>  class handler##Factory final : public PipelineHandlerFactory		\
>  {									\
>  public:									\
> @@ -122,7 +126,19 @@ public:									\
>  		return std::make_shared<handler>(manager);		\
>  	}								\
>  };									\
> -static handler##Factory global_##handler##Factory;
> +static handler##Factory global_##handler##Factory;			\
> +const char *handler::name()						\
> +{									\
> +	return #handler;						\
> +}									\
> +int handler::majorVersion()						\
> +{									\
> +	return majorV;							\
> +}									\
> +int handler::minorVersion()						\
> +{									\
> +	return minorV;							\
> +}

Instead of duplicating these in each class, can they be put into the
base class at construction time, and handled by member functions there?



>  } /* namespace libcamera */
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 05005c4..a1b06fe 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -212,6 +212,10 @@ public:
>  
>  	bool match(DeviceEnumerator *enumerator) override;
>  
> +	const char *name() override;
> +	int majorVersion() override;
> +	int minorVersion() override;

Then we wouldn't have to add these overrides to each handler...

> +
>  private:
>  	IPU3CameraData *cameraData(const Camera *camera)
>  	{
> @@ -1452,6 +1456,6 @@ int CIO2Device::mediaBusToFormat(unsigned int code)
>  	}
>  }
>  
> -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3, 0, 1);
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 9b3eea2..7042e7f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -92,6 +92,10 @@ public:
>  
>  	bool match(DeviceEnumerator *enumerator) override;
>  
> +	const char *name() override;
> +	int majorVersion() override;
> +	int minorVersion() override;
> +
>  private:
>  	RkISP1CameraData *cameraData(const Camera *camera)
>  	{
> @@ -499,6 +503,6 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
>  	completeRequest(activeCamera_, request);
>  }
>  
> -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, 0, 1);

Do we define the rules anywhere on when the version number should be
updated?

Could we potentially automate this in the future in anyway?
(checksum of the PipelineHandler? (probably too crazy, and not linear
for matching and tracking backwards compatibility)

>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 45260f3..27c731e 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -68,6 +68,10 @@ public:
>  
>  	bool match(DeviceEnumerator *enumerator) override;
>  
> +	const char *name() override;
> +	int majorVersion() override;
> +	int minorVersion() override;
> +
>  private:
>  	UVCCameraData *cameraData(const Camera *camera)
>  	{
> @@ -263,6 +267,6 @@ void UVCCameraData::bufferReady(Buffer *buffer)
>  	pipe_->completeRequest(camera_, request);
>  }
>  
> -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, 0, 1);
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 0e4eede..d3ff527 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -71,6 +71,10 @@ public:
>  
>  	bool match(DeviceEnumerator *enumerator) override;
>  
> +	const char *name() override;
> +	int majorVersion() override;
> +	int minorVersion() override;
> +
>  private:
>  	VimcCameraData *cameraData(const Camera *camera)
>  	{
> @@ -274,6 +278,6 @@ void VimcCameraData::bufferReady(Buffer *buffer)
>  	pipe_->completeRequest(camera_, request);
>  }
>  
> -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, 0, 1);
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index dd56907..8f83307 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -505,6 +505,24 @@ 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::majorVersion()
> + * \brief Retrieve the pipeline handler major version
> + * \return The pipeline handler major version
> + */
> +
> +/**
> + * \fn PipelineHandler::minorVersion()
> + * \brief Retrieve the pipeline handler minor version
> + * \return The pipeline handler minor version
> + */
> +
>  /**
>   * \class PipelineHandlerFactory
>   * \brief Registration of PipelineHandler classes and creation of instances
> @@ -586,6 +604,8 @@ 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] majorV Major version of the PipelineHandler
> + * \param[in] minorV Minor version of the PipelineHandler
>   *
>   * Register a PipelineHandler subclass with the factory and make it available to
>   * try and match devices.
> 

-- 
Regards
--
Kieran