[{"id":1752,"web_url":"https://patchwork.libcamera.org/comment/1752/","msgid":"<20190604102835.GE4771@pendragon.ideasonboard.com>","date":"2019-06-04T10:28:35","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: pipeline: add\n\tname, major version, and minor version","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, Jun 03, 2019 at 07:16:29PM -0400, Paul Elder wrote:\n> In order to match an IPA module with a pipeline handler, the pipeline\n> handler must have a name and a version. Add these to PipelineHandler as\n> getters and attributes so that they can be automatically defined by\n> REGISTER_PIPELINE_HANDLER. Also add a macro PIPELINE_HANDLER to create a\n> single version number given a major and minor version pair. It is put in\n> ipa_module_info.h because IPA modules will also need this macro.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n> Changes in v2:\n> - make the name and version getters into methods of the base PipelineHandler\n>   class, so that the specialized pipeline handlers no longer have to\n>   specify overriding\n> - add PIPELINE_VERSION macro to create one version number from major and\n>   minor pair\n> \n>  include/libcamera/ipa/ipa_module_info.h  |  2 ++\n>  src/libcamera/include/pipeline_handler.h | 14 ++++++++---\n>  src/libcamera/ipa_module.cpp             | 17 +++++++++++++\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 13 +++++++---\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 +++++++----\n>  src/libcamera/pipeline/uvcvideo.cpp      | 11 +++++---\n>  src/libcamera/pipeline/vimc.cpp          | 12 ++++++---\n>  src/libcamera/pipeline_handler.cpp       | 32 ++++++++++++++++++++++--\n>  8 files changed, 93 insertions(+), 22 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipa_module_info.h b/include/libcamera/ipa/ipa_module_info.h\n> index 4e0d668..e5f2a7e 100644\n> --- a/include/libcamera/ipa/ipa_module_info.h\n> +++ b/include/libcamera/ipa/ipa_module_info.h\n> @@ -7,6 +7,8 @@\n>  #ifndef __LIBCAMERA_IPA_MODULE_INFO_H__\n>  #define __LIBCAMERA_IPA_MODULE_INFO_H__\n>  \n> +#define PIPELINE_VERSION(majorV, minorV) (majorV * 1000 + minorV)\n> +\n>  #ifdef __cplusplus\n>  namespace libcamera {\n>  #endif\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 7da6df1..67971b3 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -50,7 +50,8 @@ private:\n>  class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>\n>  {\n>  public:\n> -\tPipelineHandler(CameraManager *manager);\n> +\tPipelineHandler(CameraManager *manager, const char *name,\n> +\t\t\tuint32_t version);\n>  \tvirtual ~PipelineHandler();\n>  \n>  \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> @@ -77,6 +78,9 @@ public:\n>  \tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n>  \tvoid completeRequest(Camera *camera, Request *request);\n>  \n> +\tconst char *name() const { return name_; }\n> +\tuint32_t version() const { return version_; }\n> +\n>  protected:\n>  \tvoid registerCamera(std::shared_ptr<Camera> camera,\n>  \t\t\t    std::unique_ptr<CameraData> data);\n> @@ -86,6 +90,9 @@ protected:\n>  \n>  \tCameraManager *manager_;\n>  \n> +\tconst char *name_;\n> +\tuint32_t version_;\n\nI think these two can be made private.\n\n> +\n>  private:\n>  \tvoid mediaDeviceDisconnected(MediaDevice *media);\n>  \tvirtual void disconnect();\n> @@ -112,14 +119,15 @@ private:\n>  \tstd::string name_;\n>  };\n>  \n> -#define REGISTER_PIPELINE_HANDLER(handler)\t\t\t\t\\\n> +#define REGISTER_PIPELINE_HANDLER(handler, pipelineVersion)\t\t\\\n\nI would replace the pipelineVersion argument with two major and minor\narguments to make usage of the REGISTER_PIPELINE_HANDLER() macro easier,\nand convert them with PIPELINE_VERSION() in the make_shared() call\nbelow.\n\n>  class handler##Factory final : public PipelineHandlerFactory\t\t\\\n>  {\t\t\t\t\t\t\t\t\t\\\n>  public:\t\t\t\t\t\t\t\t\t\\\n>  \thandler##Factory() : PipelineHandlerFactory(#handler) {}\t\\\n>  \tstd::shared_ptr<PipelineHandler> create(CameraManager *manager)\t\\\n>  \t{\t\t\t\t\t\t\t\t\\\n> -\t\treturn std::make_shared<handler>(manager);\t\t\\\n> +\t\treturn std::make_shared<handler>(manager, #handler,\t\\\n> +\t\t\t\t\t\t pipelineVersion);\t\\\n\nWhat bothers me is that you have to modify all pipeline handlers to pass\nthe name and version number to the base PipelineHandler constructor. How\nabout the following ?\n\n- Make PipelineHandlerFactory a friend of PipelineHandler\n- Add a new method to PipelineHandlerFactory\n\nvoid PipelineHandlerFactoy::setInfo(PipelineHandler *handler, const char *name,\n\t\t\t\t    unsigned int major, unsigned int minor)\n{\n\thandler->name_ = name;\n\thandler->version_ = PIPELINE_VERSION(major, minor);\n}\n\n(feel free to propose a better name for setInfo)\n\n- Modify the create function to call setInfo()\n\n\tstd::shared_ptr<PipelineHandler> create(CameraManager *manager)\t\\\n\t{\t\t\t\t\t\t\t\t\\\n\t\tstd::shared_ptr<handler> h =\t\t\t\t\\\n\t\t\tstd::make_shared<handler>(manager);\t\t\\\n\t\tsetInfo(h.get(), #handler, major, minor);\t\t\\\n\t\treturn h;\t\t\t\t\t\t\\\n\t}\n\n>  \t}\t\t\t\t\t\t\t\t\\\n>  };\t\t\t\t\t\t\t\t\t\\\n>  static handler##Factory global_##handler##Factory;\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index 86cbe71..db56415 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -169,6 +169,23 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\n>  \n>  } /* namespace */\n>  \n> +/**\n> + * \\def PIPELINE_VERSION\n> + * \\brief Convert a major and minor version pair to a single version number\n> + * \\param[in] majorV Major version\n> + * \\param[in] minorV Minor version\n\nI think you can name the arguments just minor and major.\n\n> + *\n> + * This macro should be used by the pipeline handler and by the IPA module\n\ns/should/shall/\n\nand based on the other comments above and below, I think you can remove\n\"by the pipeline handler and \".\n\n> + * to declare the version of the pipeline handler, for matching purposes.\n> + *\n> + * The major version of the pipeline handler should be updated whenever a\n\ns/should be updated/shall be increased/\n\n> + * change is made that causes currently compatible IPA modules to no longer\n> + * be compatible, or if there is an API change between the pipeline handler\n> + * and IPA modules.\n\nThe last part of the sentence is really vague.\n\n> The minor version should be updated whenever a change is\n> + * made that causes currently compatible IPA modules to no longer be compatible,\n> + * but future IPA versions will still be compatible.\n\nI'm not sure to understand what you mean here...\n\n> + */\n> +\n>  /**\n>   * \\struct IPAModuleInfo\n>   * \\brief Information of an IPA module\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 05005c4..cfbf86a 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -13,6 +13,7 @@\n>  #include <linux/media-bus-format.h>\n>  \n>  #include <libcamera/camera.h>\n> +#include <libcamera/ipa/ipa_module_info.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>  \n> @@ -194,7 +195,8 @@ private:\n>  class PipelineHandlerIPU3 : public PipelineHandler\n>  {\n>  public:\n> -\tPipelineHandlerIPU3(CameraManager *manager);\n> +\tPipelineHandlerIPU3(CameraManager *manager, const char *name,\n> +\t\t\t    uint32_t version);\n>  \n>  \tCameraConfiguration *generateConfiguration(Camera *camera,\n>  \t\tconst StreamRoles &roles) override;\n> @@ -372,8 +374,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \treturn status;\n>  }\n>  \n> -PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n> -\t: PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)\n> +PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager,\n> +\t\t\t\t\t const char *name,\n> +\t\t\t\t\t uint32_t version)\n> +\t: PipelineHandler(manager, name, version),\n> +\t  cio2MediaDev_(nullptr), imguMediaDev_(nullptr)\n>  {\n>  }\n>  \n> @@ -1452,6 +1457,6 @@ int CIO2Device::mediaBusToFormat(unsigned int code)\n>  \t}\n>  }\n>  \n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3, PIPELINE_VERSION(0, 1));\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 9b3eea2..077d84e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -14,6 +14,7 @@\n>  #include <linux/media-bus-format.h>\n>  \n>  #include <libcamera/camera.h>\n> +#include <libcamera/ipa/ipa_module_info.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>  \n> @@ -73,7 +74,8 @@ private:\n>  class PipelineHandlerRkISP1 : public PipelineHandler\n>  {\n>  public:\n> -\tPipelineHandlerRkISP1(CameraManager *manager);\n> +\tPipelineHandlerRkISP1(CameraManager *manager, const char *name,\n> +\t\t\t      uint32_t version);\n>  \t~PipelineHandlerRkISP1();\n>  \n>  \tCameraConfiguration *generateConfiguration(Camera *camera,\n> @@ -200,9 +202,11 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  \treturn status;\n>  }\n>  \n> -PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> -\t: PipelineHandler(manager), dphy_(nullptr), isp_(nullptr),\n> -\t  video_(nullptr)\n> +PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager,\n> +\t\t\t\t\t     const char *name,\n> +\t\t\t\t\t     uint32_t version)\n> +\t: PipelineHandler(manager, name, version), dphy_(nullptr),\n> +\t  isp_(nullptr), video_(nullptr)\n>  {\n>  }\n>  \n> @@ -499,6 +503,6 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n>  \tcompleteRequest(activeCamera_, request);\n>  }\n>  \n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, PIPELINE_VERSION(0, 1));\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 45260f3..430f467 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -6,6 +6,7 @@\n>   */\n>  \n>  #include <libcamera/camera.h>\n> +#include <libcamera/ipa/ipa_module_info.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>  \n> @@ -50,7 +51,8 @@ public:\n>  class PipelineHandlerUVC : public PipelineHandler\n>  {\n>  public:\n> -\tPipelineHandlerUVC(CameraManager *manager);\n> +\tPipelineHandlerUVC(CameraManager *manager, const char *name,\n> +\t\t\t   uint32_t version);\n>  \n>  \tCameraConfiguration *generateConfiguration(Camera *camera,\n>  \t\tconst StreamRoles &roles) override;\n> @@ -115,8 +117,9 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()\n>  \treturn status;\n>  }\n>  \n> -PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)\n> -\t: PipelineHandler(manager)\n> +PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager, const char *name,\n> +\t\t\t\t       uint32_t version)\n> +\t: PipelineHandler(manager, name, version)\n>  {\n>  }\n>  \n> @@ -263,6 +266,6 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n>  \tpipe_->completeRequest(camera_, request);\n>  }\n>  \n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, PIPELINE_VERSION(0, 1));\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 0e4eede..78feeb8 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -9,6 +9,7 @@\n>  #include <array>\n>  \n>  #include <libcamera/camera.h>\n> +#include <libcamera/ipa/ipa_module_info.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>  \n> @@ -53,7 +54,8 @@ public:\n>  class PipelineHandlerVimc : public PipelineHandler\n>  {\n>  public:\n> -\tPipelineHandlerVimc(CameraManager *manager);\n> +\tPipelineHandlerVimc(CameraManager *manager, const char *name,\n> +\t\t\t    uint32_t version);\n>  \n>  \tCameraConfiguration *generateConfiguration(Camera *camera,\n>  \t\tconst StreamRoles &roles) override;\n> @@ -130,8 +132,10 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n>  \treturn status;\n>  }\n>  \n> -PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)\n> -\t: PipelineHandler(manager)\n> +PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager,\n> +\t\t\t\t\t const char *name,\n> +\t\t\t\t\t uint32_t version)\n> +\t: PipelineHandler(manager, name, version)\n>  {\n>  }\n>  \n> @@ -274,6 +278,6 @@ void VimcCameraData::bufferReady(Buffer *buffer)\n>  \tpipe_->completeRequest(camera_, request);\n>  }\n>  \n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, PIPELINE_VERSION(0, 1));\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index dd56907..b18f14d 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -104,14 +104,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n>  /**\n>   * \\brief Construct a PipelineHandler instance\n>   * \\param[in] manager The camera manager\n> + * \\param[in] name The name of the pipeline handler\n> + * \\param[in] version The version of the pipeline handler\n>   *\n>   * In order to honour the std::enable_shared_from_this<> contract,\n>   * PipelineHandler instances shall never be constructed manually, but always\n>   * through the PipelineHandlerFactory::create() method implemented by the\n>   * respective factories.\n>   */\n> -PipelineHandler::PipelineHandler(CameraManager *manager)\n> -\t: manager_(manager)\n> +PipelineHandler::PipelineHandler(CameraManager *manager, const char *name,\n> +\t\t\t\t uint32_t version)\n> +\t: manager_(manager), name_(name), version_(version)\n>  {\n>  }\n>  \n> @@ -505,6 +508,28 @@ CameraData *PipelineHandler::cameraData(const Camera *camera)\n>   * constant for the whole lifetime of the pipeline handler.\n>   */\n>  \n> +/**\n> + * \\fn PipelineHandler::name()\n> + * \\brief Retrieve the pipeline handler name\n> + * \\return The pipeline handler name\n> + */\n> +\n> +/**\n> + * \\fn PipelineHandler::version()\n> + * \\brief Retrieve the pipeline handler version\n> + * \\return The pipeline handler version\n> + */\n> +\n> +/**\n> + * \\var PipelineHandler::name_\n> + * \\brief Pipeline handler name\n> + */\n> +\n> +/**\n> + * \\var PipelineHandler::version_\n> + * \\brief Pipeline handler version\n> + */\n\nYou can remove the documentation of those two members if you make them\nprivate.\n\n> +\n>  /**\n>   * \\class PipelineHandlerFactory\n>   * \\brief Registration of PipelineHandler classes and creation of instances\n> @@ -586,9 +611,12 @@ std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories()\n>   * \\def REGISTER_PIPELINE_HANDLER\n>   * \\brief Register a pipeline handler with the pipeline handler factory\n>   * \\param[in] handler Class name of PipelineHandler derived class to register\n> + * \\param[in] pipelineVersion Version of the PipelineHandler\n>   *\n>   * Register a PipelineHandler subclass with the factory and make it available to\n>   * try and match devices.\n> + *\n> + * \\sa PIPELINE_VERSION\n>   */\n>  \n>  } /* namespace libcamera */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 898D461E09\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  4 Jun 2019 12:28:48 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:2788:668:163:5bb7:9f6c:564c:d55e])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1544C2D2;\n\tTue,  4 Jun 2019 12:28:48 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559644128;\n\tbh=ZZV7NBGpw+cZGoMbQOMHdMc2a+FDOQWrOb1WO9eAeQE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TY1H5r9rJVVUPMd5SAXP1BKyUQHtuAaYLtBHN3UDw6y4mWW+uhQCtLkROTJWZY4rd\n\tZZSjS3Bv42QrV9QSmFJbmgYQvGPVUoer7LOBWzvtfgLAx+YZx5UegSlj1LGsw9lWTn\n\tItYZP5wrDvKlShFxBpX4gogd4s5TeGvFVUVZrB3o=","Date":"Tue, 4 Jun 2019 13:28:35 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190604102835.GE4771@pendragon.ideasonboard.com>","References":"<20190603231637.28554-1-paul.elder@ideasonboard.com>\n\t<20190603231637.28554-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190603231637.28554-3-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: pipeline: add\n\tname, major version, and minor version","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 04 Jun 2019 10:28:48 -0000"}}]