[{"id":4605,"web_url":"https://patchwork.libcamera.org/comment/4605/","msgid":"<20200428022131.GG3579@pendragon.ideasonboard.com>","date":"2020-04-28T02:21:31","subject":"Re: [libcamera-devel] [PATCH v4 7/7] libcamera: ipa: Add support\n\tfor CameraSensorInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Apr 27, 2020 at 11:32:36PM +0200, Jacopo Mondi wrote:\n> Add support for camera sensor information in the libcamera IPA protocol.\n> \n> Define a new 'struct ipa_sensor_info' structure in the IPA context and\n> use it to perform translation between the C and the C++ API.\n> \n> Update the IPAInterface::configure() operation to accept a new\n> CameraSensorInfo parameter and port all users of that function to\n> the new interface.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  include/ipa/ipa_interface.h                 | 26 +++++++-\n>  src/ipa/libipa/ipa_interface_wrapper.cpp    | 19 +++++-\n>  src/ipa/libipa/ipa_interface_wrapper.h      |  1 +\n>  src/ipa/rkisp1/rkisp1.cpp                   | 13 +++-\n>  src/ipa/vimc/vimc.cpp                       |  4 +-\n>  src/libcamera/include/ipa_context_wrapper.h |  4 +-\n>  src/libcamera/ipa_context_wrapper.cpp       | 23 ++++++-\n>  src/libcamera/ipa_interface.cpp             | 73 +++++++++++++++++++++\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp    | 14 +++-\n>  src/libcamera/proxy/ipa_proxy_linux.cpp     |  4 +-\n>  src/libcamera/proxy/ipa_proxy_thread.cpp    |  9 ++-\n>  test/ipa/ipa_wrappers_test.cpp              | 22 ++++++-\n>  12 files changed, 195 insertions(+), 17 deletions(-)\n> \n> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> index e65844bc7b34..0a6d849d8f89 100644\n> --- a/include/ipa/ipa_interface.h\n> +++ b/include/ipa/ipa_interface.h\n> @@ -18,6 +18,27 @@ struct ipa_context {\n>  \tconst struct ipa_context_ops *ops;\n>  };\n>  \n> +struct ipa_sensor_info {\n> +\tconst char *model;\n> +\tuint8_t bits_per_pixel;\n> +\tstruct {\n> +\t\tuint32_t width;\n> +\t\tuint32_t height;\n> +\t} active_area;\n> +\tstruct {\n> +\t\tint32_t left;\n> +\t\tint32_t top;\n> +\t\tuint32_t width;\n> +\t\tuint32_t height;\n> +\t} analog_crop;\n> +\tstruct {\n> +\t\tuint32_t width;\n> +\t\tuint32_t height;\n> +\t} output_size;\n> +\tuint32_t pixel_rate;\n\nIs 32 bits enough ? (Same question for CameraSensorInfo)\n\n> +\tuint32_t line_length;\n> +};\n> +\n>  struct ipa_stream {\n>  \tunsigned int id;\n>  \tunsigned int pixel_format;\n> @@ -70,6 +91,7 @@ struct ipa_context_ops {\n>  \t\t\t\t   const struct ipa_callback_ops *callbacks,\n>  \t\t\t\t   void *cb_ctx);\n>  \tvoid (*configure)(struct ipa_context *ctx,\n> +\t\t\t  const struct ipa_sensor_info *sensor_info,\n>  \t\t\t  const struct ipa_stream *streams,\n>  \t\t\t  unsigned int num_streams,\n>  \t\t\t  const struct ipa_control_info_map *maps,\n> @@ -116,6 +138,7 @@ struct IPAOperationData {\n>  \tstd::vector<ControlList> controls;\n>  };\n>  \n> +struct CameraSensorInfo;\n\nCould you add a blank line ?\n\nI've just realized that we'll need to move CameraSensorInfo out of\ncamera_sensor.h if we want to make it accessible to IPAs without\naccessing internal APIs :-( That can be fixed on top, but could you add\na \\todo somewhere ?\n\n>  class IPAInterface\n>  {\n>  public:\n> @@ -125,7 +148,8 @@ public:\n>  \tvirtual int start() = 0;\n>  \tvirtual void stop() = 0;\n>  \n> -\tvirtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +\tvirtual void configure(const CameraSensorInfo &sensorInfo,\n> +\t\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;\n>  \n>  \tvirtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> index f50f93a0185a..e65822c62315 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> @@ -15,6 +15,7 @@\n>  #include <ipa/ipa_interface.h>\n>  \n>  #include \"byte_stream_buffer.h\"\n> +#include \"camera_sensor.h\"\n>  \n>  /**\n>   * \\file ipa_interface_wrapper.h\n> @@ -111,6 +112,7 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,\n>  }\n>  \n>  void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> +\t\t\t\t    const struct ipa_sensor_info *sensor_info,\n>  \t\t\t\t    const struct ipa_stream *streams,\n>  \t\t\t\t    unsigned int num_streams,\n>  \t\t\t\t    const struct ipa_control_info_map *maps,\n> @@ -120,6 +122,21 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n>  \n>  \tctx->serializer_.reset();\n>  \n> +\t/* Translate the IPA sensor info. */\n> +\tCameraSensorInfo sensorInfo{};\n> +\tsensorInfo.model = sensor_info->model;\n> +\tsensorInfo.bitsPerPixel = sensor_info->bits_per_pixel;\n> +\tsensorInfo.activeAreaSize = { sensor_info->active_area.width,\n> +\t\t\t\t      sensor_info->active_area.height };\n> +\tsensorInfo.analogCrop = { sensor_info->analog_crop.left,\n> +\t\t\t\t  sensor_info->analog_crop.top,\n> +\t\t\t\t  sensor_info->analog_crop.width,\n> +\t\t\t\t  sensor_info->analog_crop.height };\n> +\tsensorInfo.outputSize = { sensor_info->output_size.width,\n> +\t\t\t\t  sensor_info->output_size.height };\n> +\tsensorInfo.pixelRate = sensor_info->pixel_rate;\n> +\tsensorInfo.lineLength = sensor_info->line_length;\n> +\n>  \t/* Translate the IPA stream configurations map. */\n>  \tstd::map<unsigned int, IPAStream> ipaStreams;\n>  \n> @@ -145,7 +162,7 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n>  \t\tentityControls.emplace(id, infoMaps[id]);\n>  \t}\n>  \n> -\tctx->ipa_->configure(ipaStreams, entityControls);\n> +\tctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);\n>  }\n>  \n>  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h\n> index e4bc6dd4535d..febd6e66d0c4 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.h\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> @@ -30,6 +30,7 @@ private:\n>  \t\t\t\t       const struct ipa_callback_ops *callbacks,\n>  \t\t\t\t       void *cb_ctx);\n>  \tstatic void configure(struct ipa_context *ctx,\n> +\t\t\t      const struct ipa_sensor_info *sensor_info,\n>  \t\t\t      const struct ipa_stream *streams,\n>  \t\t\t      unsigned int num_streams,\n>  \t\t\t      const struct ipa_control_info_map *maps,\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index acbbe58c7a2e..b6da672c1384 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -22,6 +22,7 @@\n>  #include <libcamera/request.h>\n>  #include <libipa/ipa_interface_wrapper.h>\n>  \n> +#include \"camera_sensor.h\"\n\nNo need to include this here, there's already at least a forward\ndeclaration available as the base class uses CameraSensorInfo in its\nAPI.\n\n>  #include \"ipa_module.h\"\n>  #include \"log.h\"\n>  #include \"utils.h\"\n>  \n> @@ -36,7 +37,8 @@ public:\n>  \tint start() override { return 0; }\n>  \tvoid stop() override {}\n>  \n> -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +\tvoid configure(const CameraSensorInfo &info,\n> +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;\n>  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n>  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override;\n> @@ -66,7 +68,14 @@ private:\n>  \tuint32_t maxGain_;\n>  };\n>  \n> -void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +/**\n> + * \\todo The RkISP1 pipeline currently provides an empty CameraSensorInfo\n> + * if the connected sensor does not provide enough information to properly\n> + * assemble one. Make sure the reported sensor information are relevant\n> + * before accessing them.\n> + */\n> +void IPARkISP1::configure(const CameraSensorInfo &info,\n> +\t\t\t  const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t\t  const std::map<unsigned int, const ControlInfoMap &> &entityControls)\n>  {\n>  \tif (entityControls.empty())\n> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> index d2267e11737f..b8aadd8588cb 100644\n> --- a/src/ipa/vimc/vimc.cpp\n> +++ b/src/ipa/vimc/vimc.cpp\n> @@ -19,6 +19,7 @@\n>  \n>  #include <libipa/ipa_interface_wrapper.h>\n>  \n> +#include \"camera_sensor.h\"\n\nSame here.\n\n>  #include \"log.h\"\n>  \n>  namespace libcamera {\n> @@ -36,7 +37,8 @@ public:\n>  \tint start() override;\n>  \tvoid stop() override;\n>  \n> -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}\n>  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n>  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override {}\n> diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h\n> index 0a48bfe732c8..cfb435ee1b91 100644\n> --- a/src/libcamera/include/ipa_context_wrapper.h\n> +++ b/src/libcamera/include/ipa_context_wrapper.h\n> @@ -13,6 +13,7 @@\n>  \n>  namespace libcamera {\n>  \n> +struct CameraSensorInfo;\n\nFor the same reason the forward declaration isn't needed here.\n\n>  class IPAContextWrapper final : public IPAInterface\n>  {\n>  public:\n> @@ -22,7 +23,8 @@ public:\n>  \tint init() override;\n>  \tint start() override;\n>  \tvoid stop() override;\n> -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;\n>  \n>  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> index ab6ce396b88a..d591c67f8791 100644\n> --- a/src/libcamera/ipa_context_wrapper.cpp\n> +++ b/src/libcamera/ipa_context_wrapper.cpp\n> @@ -12,6 +12,7 @@\n>  #include <libcamera/controls.h>\n>  \n>  #include \"byte_stream_buffer.h\"\n> +#include \"camera_sensor.h\"\n>  #include \"utils.h\"\n>  \n>  /**\n> @@ -104,17 +105,33 @@ void IPAContextWrapper::stop()\n>  \tctx_->ops->stop(ctx_);\n>  }\n>  \n> -void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> +\t\t\t\t  const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t\t\t  const std::map<unsigned int, const ControlInfoMap &> &entityControls)\n>  {\n>  \tif (intf_)\n> -\t\treturn intf_->configure(streamConfig, entityControls);\n> +\t\treturn intf_->configure(sensorInfo, streamConfig, entityControls);\n>  \n>  \tif (!ctx_)\n>  \t\treturn;\n>  \n>  \tserializer_.reset();\n>  \n> +\t/* Translate the camera sensor info. */\n> +\tstruct ipa_sensor_info sensor_info = {};\n> +\tsensor_info.model = sensorInfo.model.c_str();\n> +\tsensor_info.bits_per_pixel = sensorInfo.bitsPerPixel;\n> +\tsensor_info.active_area.width = sensorInfo.activeAreaSize.width;\n> +\tsensor_info.active_area.height = sensorInfo.activeAreaSize.height;\n> +\tsensor_info.analog_crop.left = sensorInfo.analogCrop.x;\n> +\tsensor_info.analog_crop.top = sensorInfo.analogCrop.y;\n> +\tsensor_info.analog_crop.width = sensorInfo.analogCrop.width;\n> +\tsensor_info.analog_crop.height = sensorInfo.analogCrop.height;\n> +\tsensor_info.output_size.width = sensorInfo.outputSize.width;\n> +\tsensor_info.output_size.height = sensorInfo.outputSize.height;\n> +\tsensor_info.pixel_rate = sensorInfo.pixelRate;\n> +\tsensor_info.line_length = sensorInfo.lineLength;\n> +\n>  \t/* Translate the IPA stream configurations map. */\n>  \tstruct ipa_stream c_streams[streamConfig.size()];\n>  \n> @@ -154,7 +171,7 @@ void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea\n>  \t\t++i;\n>  \t}\n>  \n> -\tctx_->ops->configure(ctx_, c_streams, streamConfig.size(),\n> +\tctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),\n>  \t\t\t     c_info_maps, entityControls.size());\n>  }\n>  \n> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> index 890d4340e4f3..13df090f3374 100644\n> --- a/src/libcamera/ipa_interface.cpp\n> +++ b/src/libcamera/ipa_interface.cpp\n> @@ -92,6 +92,74 @@\n>   * \\brief The IPA context operations\n>   */\n>  \n> +/**\n> + * \\struct ipa_sensor_info\n> + * \\brief Camera sensor information for the IPA context operations\n> + * \\sa libcamera::CameraSensorInfo\n> + *\n> + * \\var ipa_sensor_info::model\n> + * \\brief The camera sensor model name\n> + * \\todo Remove this field as soon as no IPA depends on it anymore\n> + *\n> + * \\var ipa_sensor_info::bits_per_pixel\n> + * \\brief The camera sensor image format bit depth\n> + * \\sa libcamera::CameraSensorInfo::bitsPerPixel\n> + *\n> + * \\var ipa_sensor_info::active_area.width\n> + * \\brief The camera sensor pixel array active area width\n> + * \\sa libcamera::CameraSensorInfo::activeAreaSize\n> + *\n> + * \\var ipa_sensor_info::active_area.height\n> + * \\brief The camera sensor pixel array active area height\n> + * \\sa libcamera::CameraSensorInfo::activeAreaSize\n> + *\n> + * \\var ipa_sensor_info::active_area\n> + * \\brief The camera sensor pixel array active size\n> + * \\sa libcamera::CameraSensorInfo::activeAreaSize\n> + *\n> + * \\var ipa_sensor_info::analog_crop.left\n> + * \\brief The left coordinate of the analog crop rectangle, relative to the\n> + * pixel array active area\n> + * \\sa libcamera::CameraSensorInfo::analogCrop\n> + *\n> + * \\var ipa_sensor_info::analog_crop.top\n> + * \\brief The top coordinate of the analog crop rectangle, relative to the pixel\n> + * array active area\n> + * \\sa libcamera::CameraSensorInfo::analogCrop\n> + *\n> + * \\var ipa_sensor_info::analog_crop.width\n> + * \\brief The horizontal size of the analog crop rectangle\n> + * \\sa libcamera::CameraSensorInfo::analogCrop\n> + *\n> + * \\var ipa_sensor_info::analog_crop.height\n> + * \\brief The vertical size of the analog crop rectangle\n> + * \\sa libcamera::CameraSensorInfo::analogCrop\n> + *\n> + * \\var ipa_sensor_info::analog_crop\n> + * \\brief The analog crop rectangle\n> + * \\sa libcamera::CameraSensorInfo::analogCrop\n> + *\n> + * \\var ipa_sensor_info::output_size.width\n> + * \\brief The horizontal size of the output image\n> + * \\sa libcamera::CameraSensorInfo::outputSize\n> + *\n> + * \\var ipa_sensor_info::output_size.height\n> + * \\brief The vertical size of the output image\n> + * \\sa libcamera::CameraSensorInfo::outputSize\n> + *\n> + * \\var ipa_sensor_info::output_size\n> + * \\brief The size of the output image\n> + * \\sa libcamera::CameraSensorInfo::outputSize\n> + *\n> + * \\var ipa_sensor_info::pixel_rate\n> + * \\brief The number of pixel produced in a second\n> + * \\sa libcamera::CameraSensorInfo::pixelClock\n\ns/pixelClock/pixelRate/\n\nNo warning from Doxygen ? Could you check the compiled doc to see if\neverything is fine ?\n\n> + *\n> + * \\var ipa_sensor_info::line_length\n> + * \\brief The full line length, including blanking, in pixel units\n> + * \\sa libcamera::CameraSensorInfo::lineLength\n> + */\n> +\n>  /**\n>   * \\struct ipa_stream\n>   * \\brief Stream information for the IPA context operations\n> @@ -447,6 +515,7 @@ namespace libcamera {\n>  /**\n>   * \\fn IPAInterface::configure()\n>   * \\brief Configure the IPA stream and sensor settings\n> + * \\param[in] sensorInfo Camera sensor information\n>   * \\param[in] streamConfig Configuration of all active streams\n>   * \\param[in] entityControls Controls provided by the pipeline entities\n>   *\n> @@ -454,6 +523,10 @@ namespace libcamera {\n>   * the camera's streams and the sensor settings. The meaning of the numerical\n>   * keys in the \\a streamConfig and \\a entityControls maps is defined by the IPA\n>   * protocol.\n> + *\n> + * The \\a sensorInfo conveys information about the camera sensor settings that\n> + * the pipeline handler has selected for the configuration. The IPA may use\n> + * that information to tune its algorithms.\n>   */\n>  \n>  /**\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index f42264361785..4058b9014fb9 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -821,7 +821,17 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \n>  \tactiveCamera_ = camera;\n>  \n> -\t/* Inform IPA of stream configuration and sensor controls. */\n> +\t/*\n> +\t * Inform IPA of stream configuration and sensor controls.\n> +\t */\n\nAny reason to switch to a multiline comment ? I don't mind much, just\ncurious.\n\n> +\tCameraSensorInfo sensorInfo = {};\n> +\tret = data->sensor_->sensorInfo(&sensorInfo);\n> +\tif (ret) {\n> +\t\t/* \\todo Turn this in an hard failure. */\n> +\t\tLOG(RkISP1, Info) << \"Camera sensor information not available\";\n\nLet's make it a Warning already then.\n\n> +\t\tsensorInfo = {};\n> +\t}\n> +\n>  \tstd::map<unsigned int, IPAStream> streamConfig;\n>  \tstreamConfig[0] = {\n>  \t\t.pixelFormat = data->stream_.configuration().pixelFormat,\n> @@ -831,7 +841,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \tstd::map<unsigned int, const ControlInfoMap &> entityControls;\n>  \tentityControls.emplace(0, data->sensor_->controls());\n>  \n> -\tdata->ipa_->configure(streamConfig, entityControls);\n> +\tdata->ipa_->configure(sensorInfo, streamConfig, entityControls);\n>  \n>  \treturn ret;\n>  }\n> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> index 2aa80b946704..54b1abc4727b 100644\n> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> @@ -10,6 +10,7 @@\n>  #include <ipa/ipa_interface.h>\n>  #include <ipa/ipa_module_info.h>\n>  \n> +#include \"camera_sensor.h\"\n\nNo need for this either.\n\n>  #include \"ipa_module.h\"\n>  #include \"ipa_proxy.h\"\n>  #include \"ipc_unixsocket.h\"\n> @@ -29,7 +30,8 @@ public:\n>  \tint init() override { return 0; }\n>  \tint start() override { return 0; }\n>  \tvoid stop() override {}\n> -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}\n>  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n>  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override {}\n> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> index 368ccca1cf60..b06734371b39 100644\n> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> @@ -10,6 +10,7 @@\n>  #include <ipa/ipa_interface.h>\n>  #include <ipa/ipa_module_info.h>\n>  \n> +#include \"camera_sensor.h\"\n\nSame here, this file doesn't access the contents of CameraSensorInfo,\nyou don't need to include the header file.\n\n>  #include \"ipa_context_wrapper.h\"\n>  #include \"ipa_module.h\"\n>  #include \"ipa_proxy.h\"\n> @@ -29,7 +30,8 @@ public:\n>  \tint start() override;\n>  \tvoid stop() override;\n>  \n> -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;\n>  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n>  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override;\n> @@ -126,10 +128,11 @@ void IPAProxyThread::stop()\n>  \tthread_.wait();\n>  }\n>  \n> -void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> +\t\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls)\n>  {\n> -\tipa_->configure(streamConfig, entityControls);\n> +\tipa_->configure(sensorInfo, streamConfig, entityControls);\n>  }\n>  \n>  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)\n> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\n> index fae1d56b67c7..a6bf0836ac4e 100644\n> --- a/test/ipa/ipa_wrappers_test.cpp\n> +++ b/test/ipa/ipa_wrappers_test.cpp\n> @@ -15,6 +15,7 @@\n>  #include <libcamera/controls.h>\n>  #include <libipa/ipa_interface_wrapper.h>\n>  \n> +#include \"camera_sensor.h\"\n>  #include \"device_enumerator.h\"\n>  #include \"ipa_context_wrapper.h\"\n>  #include \"media_device.h\"\n> @@ -60,9 +61,17 @@ public:\n>  \t\treport(Op_stop, TestPass);\n>  \t}\n>  \n> -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override\n>  \t{\n> +\t\t/* Verify sensorInfo. */\n> +\t\tif (sensorInfo.outputSize.width != 2560 ||\n> +\t\t    sensorInfo.outputSize.height != 1940) {\n> +\t\t\tcerr << \"configure(): Invalid sensor info sizes: (\"\n\nLeftover '('. I would remove the ':' too. And s/sizes/size/\n\n> +\t\t\t     << sensorInfo.outputSize.toString();\n> +\t\t}\n> +\n>  \t\t/* Verify streamConfig. */\n>  \t\tif (streamConfig.size() != 2) {\n>  \t\t\tcerr << \"configure(): Invalid number of streams \"\n> @@ -287,13 +296,22 @@ protected:\n>  \t\tint ret;\n>  \n>  \t\t/* Test configure(). */\n> +\t\tCameraSensorInfo sensorInfo{\n> +\t\t\t.model = \"sensor\",\n> +\t\t\t.bitsPerPixel = 8,\n> +\t\t\t.activeAreaSize = { 2576, 1956 },\n> +\t\t\t.analogCrop = { 8, 8, 2560, 1940 },\n> +\t\t\t.outputSize = { 2560, 1940 },\n> +\t\t\t.pixelRate = 96000000,\n> +\t\t\t.lineLength = 2918,\n> +\t\t};\n>  \t\tstd::map<unsigned int, IPAStream> config{\n>  \t\t\t{ 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } },\n>  \t\t\t{ 2, { V4L2_PIX_FMT_NV12, { 800, 600 } } },\n>  \t\t};\n>  \t\tstd::map<unsigned int, const ControlInfoMap &> controlInfo;\n>  \t\tcontrolInfo.emplace(42, subdev_->controls());\n> -\t\tret = INVOKE(configure, config, controlInfo);\n> +\t\tret = INVOKE(configure, sensorInfo, config, controlInfo);\n>  \t\tif (ret == TestFail)\n>  \t\t\treturn TestFail;\n>","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 594DE603F5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 28 Apr 2020 04:21:47 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C26D772C;\n\tTue, 28 Apr 2020 04:21:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"g6rj1XYU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588040507;\n\tbh=EaYUqo6082dA+7CubVj8fpknwd5aK3NZmq88344v/pg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=g6rj1XYU3BfKmBU6JM63sAWvaw/VnZrN5Ajw6LgO0BqthalvyL2NO+GVbQMwMPyD4\n\tgsij6pC/k93QfZbHwySp5+hGJW889ncIRaecfBGdTVoBKw2I4F5sXhCK6h/rX6wIO+\n\tUSl58UjOiE0FDH5a8HVTm4GGMKl9MUwnDmqna9fU=","Date":"Tue, 28 Apr 2020 05:21:31 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200428022131.GG3579@pendragon.ideasonboard.com>","References":"<20200427213236.333777-1-jacopo@jmondi.org>\n\t<20200427213236.333777-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200427213236.333777-8-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 7/7] libcamera: ipa: Add support\n\tfor CameraSensorInfo","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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, 28 Apr 2020 02:21:47 -0000"}},{"id":4612,"web_url":"https://patchwork.libcamera.org/comment/4612/","msgid":"<20200428073100.dofseafc5gi55czp@uno.localdomain>","date":"2020-04-28T07:31:00","subject":"Re: [libcamera-devel] [PATCH v4 7/7] libcamera: ipa: Add support\n\tfor CameraSensorInfo","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 28, 2020 at 05:21:31AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Apr 27, 2020 at 11:32:36PM +0200, Jacopo Mondi wrote:\n> > Add support for camera sensor information in the libcamera IPA protocol.\n> >\n> > Define a new 'struct ipa_sensor_info' structure in the IPA context and\n> > use it to perform translation between the C and the C++ API.\n> >\n> > Update the IPAInterface::configure() operation to accept a new\n> > CameraSensorInfo parameter and port all users of that function to\n> > the new interface.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  include/ipa/ipa_interface.h                 | 26 +++++++-\n> >  src/ipa/libipa/ipa_interface_wrapper.cpp    | 19 +++++-\n> >  src/ipa/libipa/ipa_interface_wrapper.h      |  1 +\n> >  src/ipa/rkisp1/rkisp1.cpp                   | 13 +++-\n> >  src/ipa/vimc/vimc.cpp                       |  4 +-\n> >  src/libcamera/include/ipa_context_wrapper.h |  4 +-\n> >  src/libcamera/ipa_context_wrapper.cpp       | 23 ++++++-\n> >  src/libcamera/ipa_interface.cpp             | 73 +++++++++++++++++++++\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp    | 14 +++-\n> >  src/libcamera/proxy/ipa_proxy_linux.cpp     |  4 +-\n> >  src/libcamera/proxy/ipa_proxy_thread.cpp    |  9 ++-\n> >  test/ipa/ipa_wrappers_test.cpp              | 22 ++++++-\n> >  12 files changed, 195 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> > index e65844bc7b34..0a6d849d8f89 100644\n> > --- a/include/ipa/ipa_interface.h\n> > +++ b/include/ipa/ipa_interface.h\n> > @@ -18,6 +18,27 @@ struct ipa_context {\n> >  \tconst struct ipa_context_ops *ops;\n> >  };\n> >\n> > +struct ipa_sensor_info {\n> > +\tconst char *model;\n> > +\tuint8_t bits_per_pixel;\n> > +\tstruct {\n> > +\t\tuint32_t width;\n> > +\t\tuint32_t height;\n> > +\t} active_area;\n> > +\tstruct {\n> > +\t\tint32_t left;\n> > +\t\tint32_t top;\n> > +\t\tuint32_t width;\n> > +\t\tuint32_t height;\n> > +\t} analog_crop;\n> > +\tstruct {\n> > +\t\tuint32_t width;\n> > +\t\tuint32_t height;\n> > +\t} output_size;\n> > +\tuint32_t pixel_rate;\n>\n> Is 32 bits enough ? (Same question for CameraSensorInfo)\n>\n\nThe V4L2 control is 64 bits, let's make this 64 too\n\n> > +\tuint32_t line_length;\n> > +};\n> > +\n> >  struct ipa_stream {\n> >  \tunsigned int id;\n> >  \tunsigned int pixel_format;\n> > @@ -70,6 +91,7 @@ struct ipa_context_ops {\n> >  \t\t\t\t   const struct ipa_callback_ops *callbacks,\n> >  \t\t\t\t   void *cb_ctx);\n> >  \tvoid (*configure)(struct ipa_context *ctx,\n> > +\t\t\t  const struct ipa_sensor_info *sensor_info,\n> >  \t\t\t  const struct ipa_stream *streams,\n> >  \t\t\t  unsigned int num_streams,\n> >  \t\t\t  const struct ipa_control_info_map *maps,\n> > @@ -116,6 +138,7 @@ struct IPAOperationData {\n> >  \tstd::vector<ControlList> controls;\n> >  };\n> >\n> > +struct CameraSensorInfo;\n>\n> Could you add a blank line ?\n>\n> I've just realized that we'll need to move CameraSensorInfo out of\n> camera_sensor.h if we want to make it accessible to IPAs without\n> accessing internal APIs :-( That can be fixed on top, but could you add\n> a \\todo somewhere ?\n>\n\nI had the same thought, but I then realized we already include\nv4l2_controls, and I considerd this to be fine.\n\nWe might want to remove both includes.\n\n> >  class IPAInterface\n> >  {\n> >  public:\n> > @@ -125,7 +148,8 @@ public:\n> >  \tvirtual int start() = 0;\n> >  \tvirtual void stop() = 0;\n> >\n> > -\tvirtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +\tvirtual void configure(const CameraSensorInfo &sensorInfo,\n> > +\t\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> >  \t\t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;\n> >\n> >  \tvirtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;\n> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > index f50f93a0185a..e65822c62315 100644\n> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > @@ -15,6 +15,7 @@\n> >  #include <ipa/ipa_interface.h>\n> >\n> >  #include \"byte_stream_buffer.h\"\n> > +#include \"camera_sensor.h\"\n> >\n> >  /**\n> >   * \\file ipa_interface_wrapper.h\n> > @@ -111,6 +112,7 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,\n> >  }\n> >\n> >  void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> > +\t\t\t\t    const struct ipa_sensor_info *sensor_info,\n> >  \t\t\t\t    const struct ipa_stream *streams,\n> >  \t\t\t\t    unsigned int num_streams,\n> >  \t\t\t\t    const struct ipa_control_info_map *maps,\n> > @@ -120,6 +122,21 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> >\n> >  \tctx->serializer_.reset();\n> >\n> > +\t/* Translate the IPA sensor info. */\n> > +\tCameraSensorInfo sensorInfo{};\n> > +\tsensorInfo.model = sensor_info->model;\n> > +\tsensorInfo.bitsPerPixel = sensor_info->bits_per_pixel;\n> > +\tsensorInfo.activeAreaSize = { sensor_info->active_area.width,\n> > +\t\t\t\t      sensor_info->active_area.height };\n> > +\tsensorInfo.analogCrop = { sensor_info->analog_crop.left,\n> > +\t\t\t\t  sensor_info->analog_crop.top,\n> > +\t\t\t\t  sensor_info->analog_crop.width,\n> > +\t\t\t\t  sensor_info->analog_crop.height };\n> > +\tsensorInfo.outputSize = { sensor_info->output_size.width,\n> > +\t\t\t\t  sensor_info->output_size.height };\n> > +\tsensorInfo.pixelRate = sensor_info->pixel_rate;\n> > +\tsensorInfo.lineLength = sensor_info->line_length;\n> > +\n> >  \t/* Translate the IPA stream configurations map. */\n> >  \tstd::map<unsigned int, IPAStream> ipaStreams;\n> >\n> > @@ -145,7 +162,7 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> >  \t\tentityControls.emplace(id, infoMaps[id]);\n> >  \t}\n> >\n> > -\tctx->ipa_->configure(ipaStreams, entityControls);\n> > +\tctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);\n> >  }\n> >\n> >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\n> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h\n> > index e4bc6dd4535d..febd6e66d0c4 100644\n> > --- a/src/ipa/libipa/ipa_interface_wrapper.h\n> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> > @@ -30,6 +30,7 @@ private:\n> >  \t\t\t\t       const struct ipa_callback_ops *callbacks,\n> >  \t\t\t\t       void *cb_ctx);\n> >  \tstatic void configure(struct ipa_context *ctx,\n> > +\t\t\t      const struct ipa_sensor_info *sensor_info,\n> >  \t\t\t      const struct ipa_stream *streams,\n> >  \t\t\t      unsigned int num_streams,\n> >  \t\t\t      const struct ipa_control_info_map *maps,\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index acbbe58c7a2e..b6da672c1384 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -22,6 +22,7 @@\n> >  #include <libcamera/request.h>\n> >  #include <libipa/ipa_interface_wrapper.h>\n> >\n> > +#include \"camera_sensor.h\"\n>\n> No need to include this here, there's already at least a forward\n> declaration available as the base class uses CameraSensorInfo in its\n> API.\n>\n\nThat's a bit fragile, as we rely on the forward declaration of\nipa_interface.h\n\nAlso, I tend to go for #include in cpp files as, compared to headers,\nwe probably are going to actually use the type, and this seems to be\neasier to maintain, for a very little price.\n\n> >  #include \"ipa_module.h\"\n> >  #include \"log.h\"\n> >  #include \"utils.h\"\n> >\n> > @@ -36,7 +37,8 @@ public:\n> >  \tint start() override { return 0; }\n> >  \tvoid stop() override {}\n> >\n> > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +\tvoid configure(const CameraSensorInfo &info,\n> > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;\n> >  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n> >  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override;\n> > @@ -66,7 +68,14 @@ private:\n> >  \tuint32_t maxGain_;\n> >  };\n> >\n> > -void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +/**\n> > + * \\todo The RkISP1 pipeline currently provides an empty CameraSensorInfo\n> > + * if the connected sensor does not provide enough information to properly\n> > + * assemble one. Make sure the reported sensor information are relevant\n> > + * before accessing them.\n> > + */\n> > +void IPARkISP1::configure(const CameraSensorInfo &info,\n> > +\t\t\t  const std::map<unsigned int, IPAStream> &streamConfig,\n> >  \t\t\t  const std::map<unsigned int, const ControlInfoMap &> &entityControls)\n> >  {\n> >  \tif (entityControls.empty())\n> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> > index d2267e11737f..b8aadd8588cb 100644\n> > --- a/src/ipa/vimc/vimc.cpp\n> > +++ b/src/ipa/vimc/vimc.cpp\n> > @@ -19,6 +19,7 @@\n> >\n> >  #include <libipa/ipa_interface_wrapper.h>\n> >\n> > +#include \"camera_sensor.h\"\n>\n> Same here.\n>\n\nSame commment, but for now, I'll drop.\n\n> >  #include \"log.h\"\n> >\n> >  namespace libcamera {\n> > @@ -36,7 +37,8 @@ public:\n> >  \tint start() override;\n> >  \tvoid stop() override;\n> >\n> > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}\n> >  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> >  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override {}\n> > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h\n> > index 0a48bfe732c8..cfb435ee1b91 100644\n> > --- a/src/libcamera/include/ipa_context_wrapper.h\n> > +++ b/src/libcamera/include/ipa_context_wrapper.h\n> > @@ -13,6 +13,7 @@\n> >\n> >  namespace libcamera {\n> >\n> > +struct CameraSensorInfo;\n>\n> For the same reason the forward declaration isn't needed here.\n>\n\nThis seems very fragile and also a bit obscure. To me it's like not\nincluding an header as it is already included in an header we include.\n\n> >  class IPAContextWrapper final : public IPAInterface\n> >  {\n> >  public:\n> > @@ -22,7 +23,8 @@ public:\n> >  \tint init() override;\n> >  \tint start() override;\n> >  \tvoid stop() override;\n> > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;\n> >\n> >  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n> > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> > index ab6ce396b88a..d591c67f8791 100644\n> > --- a/src/libcamera/ipa_context_wrapper.cpp\n> > +++ b/src/libcamera/ipa_context_wrapper.cpp\n> > @@ -12,6 +12,7 @@\n> >  #include <libcamera/controls.h>\n> >\n> >  #include \"byte_stream_buffer.h\"\n> > +#include \"camera_sensor.h\"\n> >  #include \"utils.h\"\n> >\n> >  /**\n> > @@ -104,17 +105,33 @@ void IPAContextWrapper::stop()\n> >  \tctx_->ops->stop(ctx_);\n> >  }\n> >\n> > -void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> > +\t\t\t\t  const std::map<unsigned int, IPAStream> &streamConfig,\n> >  \t\t\t\t  const std::map<unsigned int, const ControlInfoMap &> &entityControls)\n> >  {\n> >  \tif (intf_)\n> > -\t\treturn intf_->configure(streamConfig, entityControls);\n> > +\t\treturn intf_->configure(sensorInfo, streamConfig, entityControls);\n> >\n> >  \tif (!ctx_)\n> >  \t\treturn;\n> >\n> >  \tserializer_.reset();\n> >\n> > +\t/* Translate the camera sensor info. */\n> > +\tstruct ipa_sensor_info sensor_info = {};\n> > +\tsensor_info.model = sensorInfo.model.c_str();\n> > +\tsensor_info.bits_per_pixel = sensorInfo.bitsPerPixel;\n> > +\tsensor_info.active_area.width = sensorInfo.activeAreaSize.width;\n> > +\tsensor_info.active_area.height = sensorInfo.activeAreaSize.height;\n> > +\tsensor_info.analog_crop.left = sensorInfo.analogCrop.x;\n> > +\tsensor_info.analog_crop.top = sensorInfo.analogCrop.y;\n> > +\tsensor_info.analog_crop.width = sensorInfo.analogCrop.width;\n> > +\tsensor_info.analog_crop.height = sensorInfo.analogCrop.height;\n> > +\tsensor_info.output_size.width = sensorInfo.outputSize.width;\n> > +\tsensor_info.output_size.height = sensorInfo.outputSize.height;\n> > +\tsensor_info.pixel_rate = sensorInfo.pixelRate;\n> > +\tsensor_info.line_length = sensorInfo.lineLength;\n> > +\n> >  \t/* Translate the IPA stream configurations map. */\n> >  \tstruct ipa_stream c_streams[streamConfig.size()];\n> >\n> > @@ -154,7 +171,7 @@ void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea\n> >  \t\t++i;\n> >  \t}\n> >\n> > -\tctx_->ops->configure(ctx_, c_streams, streamConfig.size(),\n> > +\tctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),\n> >  \t\t\t     c_info_maps, entityControls.size());\n> >  }\n> >\n> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> > index 890d4340e4f3..13df090f3374 100644\n> > --- a/src/libcamera/ipa_interface.cpp\n> > +++ b/src/libcamera/ipa_interface.cpp\n> > @@ -92,6 +92,74 @@\n> >   * \\brief The IPA context operations\n> >   */\n> >\n> > +/**\n> > + * \\struct ipa_sensor_info\n> > + * \\brief Camera sensor information for the IPA context operations\n> > + * \\sa libcamera::CameraSensorInfo\n> > + *\n> > + * \\var ipa_sensor_info::model\n> > + * \\brief The camera sensor model name\n> > + * \\todo Remove this field as soon as no IPA depends on it anymore\n> > + *\n> > + * \\var ipa_sensor_info::bits_per_pixel\n> > + * \\brief The camera sensor image format bit depth\n> > + * \\sa libcamera::CameraSensorInfo::bitsPerPixel\n> > + *\n> > + * \\var ipa_sensor_info::active_area.width\n> > + * \\brief The camera sensor pixel array active area width\n> > + * \\sa libcamera::CameraSensorInfo::activeAreaSize\n> > + *\n> > + * \\var ipa_sensor_info::active_area.height\n> > + * \\brief The camera sensor pixel array active area height\n> > + * \\sa libcamera::CameraSensorInfo::activeAreaSize\n> > + *\n> > + * \\var ipa_sensor_info::active_area\n> > + * \\brief The camera sensor pixel array active size\n> > + * \\sa libcamera::CameraSensorInfo::activeAreaSize\n> > + *\n> > + * \\var ipa_sensor_info::analog_crop.left\n> > + * \\brief The left coordinate of the analog crop rectangle, relative to the\n> > + * pixel array active area\n> > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > + *\n> > + * \\var ipa_sensor_info::analog_crop.top\n> > + * \\brief The top coordinate of the analog crop rectangle, relative to the pixel\n> > + * array active area\n> > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > + *\n> > + * \\var ipa_sensor_info::analog_crop.width\n> > + * \\brief The horizontal size of the analog crop rectangle\n> > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > + *\n> > + * \\var ipa_sensor_info::analog_crop.height\n> > + * \\brief The vertical size of the analog crop rectangle\n> > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > + *\n> > + * \\var ipa_sensor_info::analog_crop\n> > + * \\brief The analog crop rectangle\n> > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > + *\n> > + * \\var ipa_sensor_info::output_size.width\n> > + * \\brief The horizontal size of the output image\n> > + * \\sa libcamera::CameraSensorInfo::outputSize\n> > + *\n> > + * \\var ipa_sensor_info::output_size.height\n> > + * \\brief The vertical size of the output image\n> > + * \\sa libcamera::CameraSensorInfo::outputSize\n> > + *\n> > + * \\var ipa_sensor_info::output_size\n> > + * \\brief The size of the output image\n> > + * \\sa libcamera::CameraSensorInfo::outputSize\n> > + *\n> > + * \\var ipa_sensor_info::pixel_rate\n> > + * \\brief The number of pixel produced in a second\n> > + * \\sa libcamera::CameraSensorInfo::pixelClock\n>\n> s/pixelClock/pixelRate/\n>\n> No warning from Doxygen ? Could you check the compiled doc to see if\n> everything is fine ?\n\nNo warning here, weird. I'll fix this.\n\n>\n> > + *\n> > + * \\var ipa_sensor_info::line_length\n> > + * \\brief The full line length, including blanking, in pixel units\n> > + * \\sa libcamera::CameraSensorInfo::lineLength\n> > + */\n> > +\n> >  /**\n> >   * \\struct ipa_stream\n> >   * \\brief Stream information for the IPA context operations\n> > @@ -447,6 +515,7 @@ namespace libcamera {\n> >  /**\n> >   * \\fn IPAInterface::configure()\n> >   * \\brief Configure the IPA stream and sensor settings\n> > + * \\param[in] sensorInfo Camera sensor information\n> >   * \\param[in] streamConfig Configuration of all active streams\n> >   * \\param[in] entityControls Controls provided by the pipeline entities\n> >   *\n> > @@ -454,6 +523,10 @@ namespace libcamera {\n> >   * the camera's streams and the sensor settings. The meaning of the numerical\n> >   * keys in the \\a streamConfig and \\a entityControls maps is defined by the IPA\n> >   * protocol.\n> > + *\n> > + * The \\a sensorInfo conveys information about the camera sensor settings that\n> > + * the pipeline handler has selected for the configuration. The IPA may use\n> > + * that information to tune its algorithms.\n> >   */\n> >\n> >  /**\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index f42264361785..4058b9014fb9 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -821,7 +821,17 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> >\n> >  \tactiveCamera_ = camera;\n> >\n> > -\t/* Inform IPA of stream configuration and sensor controls. */\n> > +\t/*\n> > +\t * Inform IPA of stream configuration and sensor controls.\n> > +\t */\n>\n> Any reason to switch to a multiline comment ? I don't mind much, just\n> curious.\n>\n\nI probably added a line of comment and then removed, then missed the\nchange during the rebase. I'll restore it.\n\n> > +\tCameraSensorInfo sensorInfo = {};\n> > +\tret = data->sensor_->sensorInfo(&sensorInfo);\n> > +\tif (ret) {\n> > +\t\t/* \\todo Turn this in an hard failure. */\n\nAh, that's the comment :)\n\n> > +\t\tLOG(RkISP1, Info) << \"Camera sensor information not available\";\n>\n> Let's make it a Warning already then.\n>\n\nAck\n\n> > +\t\tsensorInfo = {};\n> > +\t}\n> > +\n> >  \tstd::map<unsigned int, IPAStream> streamConfig;\n> >  \tstreamConfig[0] = {\n> >  \t\t.pixelFormat = data->stream_.configuration().pixelFormat,\n> > @@ -831,7 +841,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> >  \tstd::map<unsigned int, const ControlInfoMap &> entityControls;\n> >  \tentityControls.emplace(0, data->sensor_->controls());\n> >\n> > -\tdata->ipa_->configure(streamConfig, entityControls);\n> > +\tdata->ipa_->configure(sensorInfo, streamConfig, entityControls);\n> >\n> >  \treturn ret;\n> >  }\n> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > index 2aa80b946704..54b1abc4727b 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > @@ -10,6 +10,7 @@\n> >  #include <ipa/ipa_interface.h>\n> >  #include <ipa/ipa_module_info.h>\n> >\n> > +#include \"camera_sensor.h\"\n>\n> No need for this either.\n>\n> >  #include \"ipa_module.h\"\n> >  #include \"ipa_proxy.h\"\n> >  #include \"ipc_unixsocket.h\"\n> > @@ -29,7 +30,8 @@ public:\n> >  \tint init() override { return 0; }\n> >  \tint start() override { return 0; }\n> >  \tvoid stop() override {}\n> > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}\n> >  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> >  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override {}\n> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > index 368ccca1cf60..b06734371b39 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > @@ -10,6 +10,7 @@\n> >  #include <ipa/ipa_interface.h>\n> >  #include <ipa/ipa_module_info.h>\n> >\n> > +#include \"camera_sensor.h\"\n>\n> Same here, this file doesn't access the contents of CameraSensorInfo,\n> you don't need to include the header file.\n>\n> >  #include \"ipa_context_wrapper.h\"\n> >  #include \"ipa_module.h\"\n> >  #include \"ipa_proxy.h\"\n> > @@ -29,7 +30,8 @@ public:\n> >  \tint start() override;\n> >  \tvoid stop() override;\n> >\n> > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;\n> >  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n> >  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override;\n> > @@ -126,10 +128,11 @@ void IPAProxyThread::stop()\n> >  \tthread_.wait();\n> >  }\n> >\n> > -void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> > +\t\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> >  \t\t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls)\n> >  {\n> > -\tipa_->configure(streamConfig, entityControls);\n> > +\tipa_->configure(sensorInfo, streamConfig, entityControls);\n> >  }\n> >\n> >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)\n> > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\n> > index fae1d56b67c7..a6bf0836ac4e 100644\n> > --- a/test/ipa/ipa_wrappers_test.cpp\n> > +++ b/test/ipa/ipa_wrappers_test.cpp\n> > @@ -15,6 +15,7 @@\n> >  #include <libcamera/controls.h>\n> >  #include <libipa/ipa_interface_wrapper.h>\n> >\n> > +#include \"camera_sensor.h\"\n> >  #include \"device_enumerator.h\"\n> >  #include \"ipa_context_wrapper.h\"\n> >  #include \"media_device.h\"\n> > @@ -60,9 +61,17 @@ public:\n> >  \t\treport(Op_stop, TestPass);\n> >  \t}\n> >\n> > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override\n> >  \t{\n> > +\t\t/* Verify sensorInfo. */\n> > +\t\tif (sensorInfo.outputSize.width != 2560 ||\n> > +\t\t    sensorInfo.outputSize.height != 1940) {\n> > +\t\t\tcerr << \"configure(): Invalid sensor info sizes: (\"\n>\n> Leftover '('. I would remove the ':' too. And s/sizes/size/\n>\n\nah ups\n\n> > +\t\t\t     << sensorInfo.outputSize.toString();\n> > +\t\t}\n> > +\n> >  \t\t/* Verify streamConfig. */\n> >  \t\tif (streamConfig.size() != 2) {\n> >  \t\t\tcerr << \"configure(): Invalid number of streams \"\n> > @@ -287,13 +296,22 @@ protected:\n> >  \t\tint ret;\n> >\n> >  \t\t/* Test configure(). */\n> > +\t\tCameraSensorInfo sensorInfo{\n> > +\t\t\t.model = \"sensor\",\n> > +\t\t\t.bitsPerPixel = 8,\n> > +\t\t\t.activeAreaSize = { 2576, 1956 },\n> > +\t\t\t.analogCrop = { 8, 8, 2560, 1940 },\n> > +\t\t\t.outputSize = { 2560, 1940 },\n> > +\t\t\t.pixelRate = 96000000,\n> > +\t\t\t.lineLength = 2918,\n> > +\t\t};\n> >  \t\tstd::map<unsigned int, IPAStream> config{\n> >  \t\t\t{ 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } },\n> >  \t\t\t{ 2, { V4L2_PIX_FMT_NV12, { 800, 600 } } },\n> >  \t\t};\n> >  \t\tstd::map<unsigned int, const ControlInfoMap &> controlInfo;\n> >  \t\tcontrolInfo.emplace(42, subdev_->controls());\n> > -\t\tret = INVOKE(configure, config, controlInfo);\n> > +\t\tret = INVOKE(configure, sensorInfo, config, controlInfo);\n> >  \t\tif (ret == TestFail)\n> >  \t\t\treturn TestFail;\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1169603F7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 28 Apr 2020 09:27:50 +0200 (CEST)","from uno.localdomain (a-ur1-85.tin.it [212.216.150.148])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 654DA60003;\n\tTue, 28 Apr 2020 07:27:49 +0000 (UTC)"],"X-Originating-IP":"212.216.150.148","Date":"Tue, 28 Apr 2020 09:31:00 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200428073100.dofseafc5gi55czp@uno.localdomain>","References":"<20200427213236.333777-1-jacopo@jmondi.org>\n\t<20200427213236.333777-8-jacopo@jmondi.org>\n\t<20200428022131.GG3579@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200428022131.GG3579@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 7/7] libcamera: ipa: Add support\n\tfor CameraSensorInfo","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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, 28 Apr 2020 07:27:51 -0000"}},{"id":4618,"web_url":"https://patchwork.libcamera.org/comment/4618/","msgid":"<20200428121512.GE5859@pendragon.ideasonboard.com>","date":"2020-04-28T12:15:12","subject":"Re: [libcamera-devel] [PATCH v4 7/7] libcamera: ipa: Add support\n\tfor CameraSensorInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Apr 28, 2020 at 09:31:00AM +0200, Jacopo Mondi wrote:\n> On Tue, Apr 28, 2020 at 05:21:31AM +0300, Laurent Pinchart wrote:\n> > On Mon, Apr 27, 2020 at 11:32:36PM +0200, Jacopo Mondi wrote:\n> > > Add support for camera sensor information in the libcamera IPA protocol.\n> > >\n> > > Define a new 'struct ipa_sensor_info' structure in the IPA context and\n> > > use it to perform translation between the C and the C++ API.\n> > >\n> > > Update the IPAInterface::configure() operation to accept a new\n> > > CameraSensorInfo parameter and port all users of that function to\n> > > the new interface.\n> > >\n> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > >  include/ipa/ipa_interface.h                 | 26 +++++++-\n> > >  src/ipa/libipa/ipa_interface_wrapper.cpp    | 19 +++++-\n> > >  src/ipa/libipa/ipa_interface_wrapper.h      |  1 +\n> > >  src/ipa/rkisp1/rkisp1.cpp                   | 13 +++-\n> > >  src/ipa/vimc/vimc.cpp                       |  4 +-\n> > >  src/libcamera/include/ipa_context_wrapper.h |  4 +-\n> > >  src/libcamera/ipa_context_wrapper.cpp       | 23 ++++++-\n> > >  src/libcamera/ipa_interface.cpp             | 73 +++++++++++++++++++++\n> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp    | 14 +++-\n> > >  src/libcamera/proxy/ipa_proxy_linux.cpp     |  4 +-\n> > >  src/libcamera/proxy/ipa_proxy_thread.cpp    |  9 ++-\n> > >  test/ipa/ipa_wrappers_test.cpp              | 22 ++++++-\n> > >  12 files changed, 195 insertions(+), 17 deletions(-)\n> > >\n> > > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> > > index e65844bc7b34..0a6d849d8f89 100644\n> > > --- a/include/ipa/ipa_interface.h\n> > > +++ b/include/ipa/ipa_interface.h\n> > > @@ -18,6 +18,27 @@ struct ipa_context {\n> > >  \tconst struct ipa_context_ops *ops;\n> > >  };\n> > >\n> > > +struct ipa_sensor_info {\n> > > +\tconst char *model;\n> > > +\tuint8_t bits_per_pixel;\n> > > +\tstruct {\n> > > +\t\tuint32_t width;\n> > > +\t\tuint32_t height;\n> > > +\t} active_area;\n> > > +\tstruct {\n> > > +\t\tint32_t left;\n> > > +\t\tint32_t top;\n> > > +\t\tuint32_t width;\n> > > +\t\tuint32_t height;\n> > > +\t} analog_crop;\n> > > +\tstruct {\n> > > +\t\tuint32_t width;\n> > > +\t\tuint32_t height;\n> > > +\t} output_size;\n> > > +\tuint32_t pixel_rate;\n> >\n> > Is 32 bits enough ? (Same question for CameraSensorInfo)\n> \n> The V4L2 control is 64 bits, let's make this 64 too\n> \n> > > +\tuint32_t line_length;\n> > > +};\n> > > +\n> > >  struct ipa_stream {\n> > >  \tunsigned int id;\n> > >  \tunsigned int pixel_format;\n> > > @@ -70,6 +91,7 @@ struct ipa_context_ops {\n> > >  \t\t\t\t   const struct ipa_callback_ops *callbacks,\n> > >  \t\t\t\t   void *cb_ctx);\n> > >  \tvoid (*configure)(struct ipa_context *ctx,\n> > > +\t\t\t  const struct ipa_sensor_info *sensor_info,\n> > >  \t\t\t  const struct ipa_stream *streams,\n> > >  \t\t\t  unsigned int num_streams,\n> > >  \t\t\t  const struct ipa_control_info_map *maps,\n> > > @@ -116,6 +138,7 @@ struct IPAOperationData {\n> > >  \tstd::vector<ControlList> controls;\n> > >  };\n> > >\n> > > +struct CameraSensorInfo;\n> >\n> > Could you add a blank line ?\n> >\n> > I've just realized that we'll need to move CameraSensorInfo out of\n> > camera_sensor.h if we want to make it accessible to IPAs without\n> > accessing internal APIs :-( That can be fixed on top, but could you add\n> > a \\todo somewhere ?\n> \n> I had the same thought, but I then realized we already include\n> v4l2_controls, and I considerd this to be fine.\n> \n> We might want to remove both includes.\n\nOuch :-S Indeed. Let's do so on top.\n\n> > >  class IPAInterface\n> > >  {\n> > >  public:\n> > > @@ -125,7 +148,8 @@ public:\n> > >  \tvirtual int start() = 0;\n> > >  \tvirtual void stop() = 0;\n> > >\n> > > -\tvirtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +\tvirtual void configure(const CameraSensorInfo &sensorInfo,\n> > > +\t\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> > >  \t\t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;\n> > >\n> > >  \tvirtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;\n> > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > > index f50f93a0185a..e65822c62315 100644\n> > > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > > @@ -15,6 +15,7 @@\n> > >  #include <ipa/ipa_interface.h>\n> > >\n> > >  #include \"byte_stream_buffer.h\"\n> > > +#include \"camera_sensor.h\"\n> > >\n> > >  /**\n> > >   * \\file ipa_interface_wrapper.h\n> > > @@ -111,6 +112,7 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,\n> > >  }\n> > >\n> > >  void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> > > +\t\t\t\t    const struct ipa_sensor_info *sensor_info,\n> > >  \t\t\t\t    const struct ipa_stream *streams,\n> > >  \t\t\t\t    unsigned int num_streams,\n> > >  \t\t\t\t    const struct ipa_control_info_map *maps,\n> > > @@ -120,6 +122,21 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> > >\n> > >  \tctx->serializer_.reset();\n> > >\n> > > +\t/* Translate the IPA sensor info. */\n> > > +\tCameraSensorInfo sensorInfo{};\n> > > +\tsensorInfo.model = sensor_info->model;\n> > > +\tsensorInfo.bitsPerPixel = sensor_info->bits_per_pixel;\n> > > +\tsensorInfo.activeAreaSize = { sensor_info->active_area.width,\n> > > +\t\t\t\t      sensor_info->active_area.height };\n> > > +\tsensorInfo.analogCrop = { sensor_info->analog_crop.left,\n> > > +\t\t\t\t  sensor_info->analog_crop.top,\n> > > +\t\t\t\t  sensor_info->analog_crop.width,\n> > > +\t\t\t\t  sensor_info->analog_crop.height };\n> > > +\tsensorInfo.outputSize = { sensor_info->output_size.width,\n> > > +\t\t\t\t  sensor_info->output_size.height };\n> > > +\tsensorInfo.pixelRate = sensor_info->pixel_rate;\n> > > +\tsensorInfo.lineLength = sensor_info->line_length;\n> > > +\n> > >  \t/* Translate the IPA stream configurations map. */\n> > >  \tstd::map<unsigned int, IPAStream> ipaStreams;\n> > >\n> > > @@ -145,7 +162,7 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> > >  \t\tentityControls.emplace(id, infoMaps[id]);\n> > >  \t}\n> > >\n> > > -\tctx->ipa_->configure(ipaStreams, entityControls);\n> > > +\tctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);\n> > >  }\n> > >\n> > >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\n> > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h\n> > > index e4bc6dd4535d..febd6e66d0c4 100644\n> > > --- a/src/ipa/libipa/ipa_interface_wrapper.h\n> > > +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> > > @@ -30,6 +30,7 @@ private:\n> > >  \t\t\t\t       const struct ipa_callback_ops *callbacks,\n> > >  \t\t\t\t       void *cb_ctx);\n> > >  \tstatic void configure(struct ipa_context *ctx,\n> > > +\t\t\t      const struct ipa_sensor_info *sensor_info,\n> > >  \t\t\t      const struct ipa_stream *streams,\n> > >  \t\t\t      unsigned int num_streams,\n> > >  \t\t\t      const struct ipa_control_info_map *maps,\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index acbbe58c7a2e..b6da672c1384 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -22,6 +22,7 @@\n> > >  #include <libcamera/request.h>\n> > >  #include <libipa/ipa_interface_wrapper.h>\n> > >\n> > > +#include \"camera_sensor.h\"\n> >\n> > No need to include this here, there's already at least a forward\n> > declaration available as the base class uses CameraSensorInfo in its\n> > API.\n> \n> That's a bit fragile, as we rely on the forward declaration of\n> ipa_interface.h\n\nThere's a guaranteed forward declaration in ipa_interface.h. I tend to\nrely on that when I don't need the full definition, as in here. Up to\nyou.\n\n> Also, I tend to go for #include in cpp files as, compared to headers,\n> we probably are going to actually use the type, and this seems to be\n> easier to maintain, for a very little price.\n> \n> > >  #include \"ipa_module.h\"\n> > >  #include \"log.h\"\n> > >  #include \"utils.h\"\n> > >\n> > > @@ -36,7 +37,8 @@ public:\n> > >  \tint start() override { return 0; }\n> > >  \tvoid stop() override {}\n> > >\n> > > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +\tvoid configure(const CameraSensorInfo &info,\n> > > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> > >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;\n> > >  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n> > >  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override;\n> > > @@ -66,7 +68,14 @@ private:\n> > >  \tuint32_t maxGain_;\n> > >  };\n> > >\n> > > -void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +/**\n> > > + * \\todo The RkISP1 pipeline currently provides an empty CameraSensorInfo\n> > > + * if the connected sensor does not provide enough information to properly\n> > > + * assemble one. Make sure the reported sensor information are relevant\n> > > + * before accessing them.\n> > > + */\n> > > +void IPARkISP1::configure(const CameraSensorInfo &info,\n> > > +\t\t\t  const std::map<unsigned int, IPAStream> &streamConfig,\n> > >  \t\t\t  const std::map<unsigned int, const ControlInfoMap &> &entityControls)\n> > >  {\n> > >  \tif (entityControls.empty())\n> > > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> > > index d2267e11737f..b8aadd8588cb 100644\n> > > --- a/src/ipa/vimc/vimc.cpp\n> > > +++ b/src/ipa/vimc/vimc.cpp\n> > > @@ -19,6 +19,7 @@\n> > >\n> > >  #include <libipa/ipa_interface_wrapper.h>\n> > >\n> > > +#include \"camera_sensor.h\"\n> >\n> > Same here.\n> >\n> \n> Same commment, but for now, I'll drop.\n> \n> > >  #include \"log.h\"\n> > >\n> > >  namespace libcamera {\n> > > @@ -36,7 +37,8 @@ public:\n> > >  \tint start() override;\n> > >  \tvoid stop() override;\n> > >\n> > > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> > > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> > >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}\n> > >  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> > >  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override {}\n> > > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h\n> > > index 0a48bfe732c8..cfb435ee1b91 100644\n> > > --- a/src/libcamera/include/ipa_context_wrapper.h\n> > > +++ b/src/libcamera/include/ipa_context_wrapper.h\n> > > @@ -13,6 +13,7 @@\n> > >\n> > >  namespace libcamera {\n> > >\n> > > +struct CameraSensorInfo;\n> >\n> > For the same reason the forward declaration isn't needed here.\n> \n> This seems very fragile and also a bit obscure. To me it's like not\n> including an header as it is already included in an header we include.\n\nThe usage of CameraSensorInfo here is to implement IPAInterface. We\ninclude ipa_interface.h to do so. IPAInterface uses CameraSensorInfo, so\nipa_interface.h is guaranteed to have at least a forward declaration. We\ndon't rely on an internal detail of ipa_interface.h, but on something it\nguarantees.\n\n> > >  class IPAContextWrapper final : public IPAInterface\n> > >  {\n> > >  public:\n> > > @@ -22,7 +23,8 @@ public:\n> > >  \tint init() override;\n> > >  \tint start() override;\n> > >  \tvoid stop() override;\n> > > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> > > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> > >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;\n> > >\n> > >  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n> > > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> > > index ab6ce396b88a..d591c67f8791 100644\n> > > --- a/src/libcamera/ipa_context_wrapper.cpp\n> > > +++ b/src/libcamera/ipa_context_wrapper.cpp\n> > > @@ -12,6 +12,7 @@\n> > >  #include <libcamera/controls.h>\n> > >\n> > >  #include \"byte_stream_buffer.h\"\n> > > +#include \"camera_sensor.h\"\n> > >  #include \"utils.h\"\n> > >\n> > >  /**\n> > > @@ -104,17 +105,33 @@ void IPAContextWrapper::stop()\n> > >  \tctx_->ops->stop(ctx_);\n> > >  }\n> > >\n> > > -void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> > > +\t\t\t\t  const std::map<unsigned int, IPAStream> &streamConfig,\n> > >  \t\t\t\t  const std::map<unsigned int, const ControlInfoMap &> &entityControls)\n> > >  {\n> > >  \tif (intf_)\n> > > -\t\treturn intf_->configure(streamConfig, entityControls);\n> > > +\t\treturn intf_->configure(sensorInfo, streamConfig, entityControls);\n> > >\n> > >  \tif (!ctx_)\n> > >  \t\treturn;\n> > >\n> > >  \tserializer_.reset();\n> > >\n> > > +\t/* Translate the camera sensor info. */\n> > > +\tstruct ipa_sensor_info sensor_info = {};\n> > > +\tsensor_info.model = sensorInfo.model.c_str();\n> > > +\tsensor_info.bits_per_pixel = sensorInfo.bitsPerPixel;\n> > > +\tsensor_info.active_area.width = sensorInfo.activeAreaSize.width;\n> > > +\tsensor_info.active_area.height = sensorInfo.activeAreaSize.height;\n> > > +\tsensor_info.analog_crop.left = sensorInfo.analogCrop.x;\n> > > +\tsensor_info.analog_crop.top = sensorInfo.analogCrop.y;\n> > > +\tsensor_info.analog_crop.width = sensorInfo.analogCrop.width;\n> > > +\tsensor_info.analog_crop.height = sensorInfo.analogCrop.height;\n> > > +\tsensor_info.output_size.width = sensorInfo.outputSize.width;\n> > > +\tsensor_info.output_size.height = sensorInfo.outputSize.height;\n> > > +\tsensor_info.pixel_rate = sensorInfo.pixelRate;\n> > > +\tsensor_info.line_length = sensorInfo.lineLength;\n> > > +\n> > >  \t/* Translate the IPA stream configurations map. */\n> > >  \tstruct ipa_stream c_streams[streamConfig.size()];\n> > >\n> > > @@ -154,7 +171,7 @@ void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea\n> > >  \t\t++i;\n> > >  \t}\n> > >\n> > > -\tctx_->ops->configure(ctx_, c_streams, streamConfig.size(),\n> > > +\tctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),\n> > >  \t\t\t     c_info_maps, entityControls.size());\n> > >  }\n> > >\n> > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> > > index 890d4340e4f3..13df090f3374 100644\n> > > --- a/src/libcamera/ipa_interface.cpp\n> > > +++ b/src/libcamera/ipa_interface.cpp\n> > > @@ -92,6 +92,74 @@\n> > >   * \\brief The IPA context operations\n> > >   */\n> > >\n> > > +/**\n> > > + * \\struct ipa_sensor_info\n> > > + * \\brief Camera sensor information for the IPA context operations\n> > > + * \\sa libcamera::CameraSensorInfo\n> > > + *\n> > > + * \\var ipa_sensor_info::model\n> > > + * \\brief The camera sensor model name\n> > > + * \\todo Remove this field as soon as no IPA depends on it anymore\n> > > + *\n> > > + * \\var ipa_sensor_info::bits_per_pixel\n> > > + * \\brief The camera sensor image format bit depth\n> > > + * \\sa libcamera::CameraSensorInfo::bitsPerPixel\n> > > + *\n> > > + * \\var ipa_sensor_info::active_area.width\n> > > + * \\brief The camera sensor pixel array active area width\n> > > + * \\sa libcamera::CameraSensorInfo::activeAreaSize\n> > > + *\n> > > + * \\var ipa_sensor_info::active_area.height\n> > > + * \\brief The camera sensor pixel array active area height\n> > > + * \\sa libcamera::CameraSensorInfo::activeAreaSize\n> > > + *\n> > > + * \\var ipa_sensor_info::active_area\n> > > + * \\brief The camera sensor pixel array active size\n> > > + * \\sa libcamera::CameraSensorInfo::activeAreaSize\n> > > + *\n> > > + * \\var ipa_sensor_info::analog_crop.left\n> > > + * \\brief The left coordinate of the analog crop rectangle, relative to the\n> > > + * pixel array active area\n> > > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > > + *\n> > > + * \\var ipa_sensor_info::analog_crop.top\n> > > + * \\brief The top coordinate of the analog crop rectangle, relative to the pixel\n> > > + * array active area\n> > > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > > + *\n> > > + * \\var ipa_sensor_info::analog_crop.width\n> > > + * \\brief The horizontal size of the analog crop rectangle\n> > > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > > + *\n> > > + * \\var ipa_sensor_info::analog_crop.height\n> > > + * \\brief The vertical size of the analog crop rectangle\n> > > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > > + *\n> > > + * \\var ipa_sensor_info::analog_crop\n> > > + * \\brief The analog crop rectangle\n> > > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > > + *\n> > > + * \\var ipa_sensor_info::output_size.width\n> > > + * \\brief The horizontal size of the output image\n> > > + * \\sa libcamera::CameraSensorInfo::outputSize\n> > > + *\n> > > + * \\var ipa_sensor_info::output_size.height\n> > > + * \\brief The vertical size of the output image\n> > > + * \\sa libcamera::CameraSensorInfo::outputSize\n> > > + *\n> > > + * \\var ipa_sensor_info::output_size\n> > > + * \\brief The size of the output image\n> > > + * \\sa libcamera::CameraSensorInfo::outputSize\n> > > + *\n> > > + * \\var ipa_sensor_info::pixel_rate\n> > > + * \\brief The number of pixel produced in a second\n> > > + * \\sa libcamera::CameraSensorInfo::pixelClock\n> >\n> > s/pixelClock/pixelRate/\n> >\n> > No warning from Doxygen ? Could you check the compiled doc to see if\n> > everything is fine ?\n> \n> No warning here, weird. I'll fix this.\n> \n> > > + *\n> > > + * \\var ipa_sensor_info::line_length\n> > > + * \\brief The full line length, including blanking, in pixel units\n> > > + * \\sa libcamera::CameraSensorInfo::lineLength\n> > > + */\n> > > +\n> > >  /**\n> > >   * \\struct ipa_stream\n> > >   * \\brief Stream information for the IPA context operations\n> > > @@ -447,6 +515,7 @@ namespace libcamera {\n> > >  /**\n> > >   * \\fn IPAInterface::configure()\n> > >   * \\brief Configure the IPA stream and sensor settings\n> > > + * \\param[in] sensorInfo Camera sensor information\n> > >   * \\param[in] streamConfig Configuration of all active streams\n> > >   * \\param[in] entityControls Controls provided by the pipeline entities\n> > >   *\n> > > @@ -454,6 +523,10 @@ namespace libcamera {\n> > >   * the camera's streams and the sensor settings. The meaning of the numerical\n> > >   * keys in the \\a streamConfig and \\a entityControls maps is defined by the IPA\n> > >   * protocol.\n> > > + *\n> > > + * The \\a sensorInfo conveys information about the camera sensor settings that\n> > > + * the pipeline handler has selected for the configuration. The IPA may use\n> > > + * that information to tune its algorithms.\n> > >   */\n> > >\n> > >  /**\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index f42264361785..4058b9014fb9 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -821,7 +821,17 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> > >\n> > >  \tactiveCamera_ = camera;\n> > >\n> > > -\t/* Inform IPA of stream configuration and sensor controls. */\n> > > +\t/*\n> > > +\t * Inform IPA of stream configuration and sensor controls.\n> > > +\t */\n> >\n> > Any reason to switch to a multiline comment ? I don't mind much, just\n> > curious.\n> \n> I probably added a line of comment and then removed, then missed the\n> change during the rebase. I'll restore it.\n> \n> > > +\tCameraSensorInfo sensorInfo = {};\n> > > +\tret = data->sensor_->sensorInfo(&sensorInfo);\n> > > +\tif (ret) {\n> > > +\t\t/* \\todo Turn this in an hard failure. */\n> \n> Ah, that's the comment :)\n> \n> > > +\t\tLOG(RkISP1, Info) << \"Camera sensor information not available\";\n> >\n> > Let's make it a Warning already then.\n> >\n> \n> Ack\n> \n> > > +\t\tsensorInfo = {};\n> > > +\t}\n> > > +\n> > >  \tstd::map<unsigned int, IPAStream> streamConfig;\n> > >  \tstreamConfig[0] = {\n> > >  \t\t.pixelFormat = data->stream_.configuration().pixelFormat,\n> > > @@ -831,7 +841,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> > >  \tstd::map<unsigned int, const ControlInfoMap &> entityControls;\n> > >  \tentityControls.emplace(0, data->sensor_->controls());\n> > >\n> > > -\tdata->ipa_->configure(streamConfig, entityControls);\n> > > +\tdata->ipa_->configure(sensorInfo, streamConfig, entityControls);\n> > >\n> > >  \treturn ret;\n> > >  }\n> > > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > > index 2aa80b946704..54b1abc4727b 100644\n> > > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > > @@ -10,6 +10,7 @@\n> > >  #include <ipa/ipa_interface.h>\n> > >  #include <ipa/ipa_module_info.h>\n> > >\n> > > +#include \"camera_sensor.h\"\n> >\n> > No need for this either.\n> >\n> > >  #include \"ipa_module.h\"\n> > >  #include \"ipa_proxy.h\"\n> > >  #include \"ipc_unixsocket.h\"\n> > > @@ -29,7 +30,8 @@ public:\n> > >  \tint init() override { return 0; }\n> > >  \tint start() override { return 0; }\n> > >  \tvoid stop() override {}\n> > > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> > > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> > >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}\n> > >  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> > >  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override {}\n> > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > > index 368ccca1cf60..b06734371b39 100644\n> > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > > @@ -10,6 +10,7 @@\n> > >  #include <ipa/ipa_interface.h>\n> > >  #include <ipa/ipa_module_info.h>\n> > >\n> > > +#include \"camera_sensor.h\"\n> >\n> > Same here, this file doesn't access the contents of CameraSensorInfo,\n> > you don't need to include the header file.\n> >\n> > >  #include \"ipa_context_wrapper.h\"\n> > >  #include \"ipa_module.h\"\n> > >  #include \"ipa_proxy.h\"\n> > > @@ -29,7 +30,8 @@ public:\n> > >  \tint start() override;\n> > >  \tvoid stop() override;\n> > >\n> > > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> > > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> > >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;\n> > >  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n> > >  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override;\n> > > @@ -126,10 +128,11 @@ void IPAProxyThread::stop()\n> > >  \tthread_.wait();\n> > >  }\n> > >\n> > > -void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> > > +\t\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> > >  \t\t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls)\n> > >  {\n> > > -\tipa_->configure(streamConfig, entityControls);\n> > > +\tipa_->configure(sensorInfo, streamConfig, entityControls);\n> > >  }\n> > >\n> > >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)\n> > > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\n> > > index fae1d56b67c7..a6bf0836ac4e 100644\n> > > --- a/test/ipa/ipa_wrappers_test.cpp\n> > > +++ b/test/ipa/ipa_wrappers_test.cpp\n> > > @@ -15,6 +15,7 @@\n> > >  #include <libcamera/controls.h>\n> > >  #include <libipa/ipa_interface_wrapper.h>\n> > >\n> > > +#include \"camera_sensor.h\"\n> > >  #include \"device_enumerator.h\"\n> > >  #include \"ipa_context_wrapper.h\"\n> > >  #include \"media_device.h\"\n> > > @@ -60,9 +61,17 @@ public:\n> > >  \t\treport(Op_stop, TestPass);\n> > >  \t}\n> > >\n> > > -\tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > > +\tvoid configure(const CameraSensorInfo &sensorInfo,\n> > > +\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> > >  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override\n> > >  \t{\n> > > +\t\t/* Verify sensorInfo. */\n> > > +\t\tif (sensorInfo.outputSize.width != 2560 ||\n> > > +\t\t    sensorInfo.outputSize.height != 1940) {\n> > > +\t\t\tcerr << \"configure(): Invalid sensor info sizes: (\"\n> >\n> > Leftover '('. I would remove the ':' too. And s/sizes/size/\n> >\n> \n> ah ups\n> \n> > > +\t\t\t     << sensorInfo.outputSize.toString();\n> > > +\t\t}\n> > > +\n> > >  \t\t/* Verify streamConfig. */\n> > >  \t\tif (streamConfig.size() != 2) {\n> > >  \t\t\tcerr << \"configure(): Invalid number of streams \"\n> > > @@ -287,13 +296,22 @@ protected:\n> > >  \t\tint ret;\n> > >\n> > >  \t\t/* Test configure(). */\n> > > +\t\tCameraSensorInfo sensorInfo{\n> > > +\t\t\t.model = \"sensor\",\n> > > +\t\t\t.bitsPerPixel = 8,\n> > > +\t\t\t.activeAreaSize = { 2576, 1956 },\n> > > +\t\t\t.analogCrop = { 8, 8, 2560, 1940 },\n> > > +\t\t\t.outputSize = { 2560, 1940 },\n> > > +\t\t\t.pixelRate = 96000000,\n> > > +\t\t\t.lineLength = 2918,\n> > > +\t\t};\n> > >  \t\tstd::map<unsigned int, IPAStream> config{\n> > >  \t\t\t{ 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } },\n> > >  \t\t\t{ 2, { V4L2_PIX_FMT_NV12, { 800, 600 } } },\n> > >  \t\t};\n> > >  \t\tstd::map<unsigned int, const ControlInfoMap &> controlInfo;\n> > >  \t\tcontrolInfo.emplace(42, subdev_->controls());\n> > > -\t\tret = INVOKE(configure, config, controlInfo);\n> > > +\t\tret = INVOKE(configure, sensorInfo, config, controlInfo);\n> > >  \t\tif (ret == TestFail)\n> > >  \t\t\treturn TestFail;\n> > >","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 18A8C60AF5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 28 Apr 2020 14:15:28 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 76CBF72C;\n\tTue, 28 Apr 2020 14:15:27 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"c5dMPEOu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588076127;\n\tbh=Fd2F8+6ZrZaBgdTw1WDvSC/wwY//4SHnBdxH92IQbaY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c5dMPEOuDjpu/mY/JELrMw3ZVKnGzlWnzyREQmmhLLjfUnlKgG3CQlQquPscEuG2M\n\tJerVzcYu4A0FxDx8m4WgL6/7qM/5Z+P/Z/PvXA8JgzobWAt98gOTkaJbgf5Ihyyw5H\n\tIQw4lQJwPBVHzMiMO5SFWtlmQffXRuF9B5kCI4DQ=","Date":"Tue, 28 Apr 2020 15:15:12 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200428121512.GE5859@pendragon.ideasonboard.com>","References":"<20200427213236.333777-1-jacopo@jmondi.org>\n\t<20200427213236.333777-8-jacopo@jmondi.org>\n\t<20200428022131.GG3579@pendragon.ideasonboard.com>\n\t<20200428073100.dofseafc5gi55czp@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200428073100.dofseafc5gi55czp@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 7/7] libcamera: ipa: Add support\n\tfor CameraSensorInfo","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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, 28 Apr 2020 12:15:28 -0000"}}]