[{"id":4523,"web_url":"https://patchwork.libcamera.org/comment/4523/","msgid":"<20200425223247.GH10975@pendragon.ideasonboard.com>","date":"2020-04-25T22:32:47","subject":"Re: [libcamera-devel] [PATCH v3 13/13] 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 Fri, Apr 24, 2020 at 11:53:04PM +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                 | 21 +++++++-\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       | 24 +++++++--\n>  src/libcamera/ipa_interface.cpp             | 60 +++++++++++++++++++++\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp    |  9 +++-\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              | 21 +++++++-\n>  12 files changed, 173 insertions(+), 16 deletions(-)\n> \n> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> index e65844bc7b34..a7acc09c4715 100644\n> --- a/include/ipa/ipa_interface.h\n> +++ b/include/ipa/ipa_interface.h\n> @@ -18,6 +18,22 @@ struct ipa_context {\n>  \tconst struct ipa_context_ops *ops;\n>  };\n>  \n> +struct ipa_sensor_info {\n> +#define CAMERA_SENSOR_NAME_LEN 32\n> +\tchar name[CAMERA_SENSOR_NAME_LEN];\n\nFixed-size strings in APIs are not nice :-S Do you think we could use a\nconst char * ?\n\nThis should also be renamed to model if you agree with the previous\nrelated suggestion.\n\n> +\tuint8_t bits_per_pixel;\n> +\tuint32_t active_area_width;\n> +\tuint32_t active_area_height;\n\nHow about\n\n\tstruct {\n\t\tuint32_t width;\n\t\tuint32_t height;\n\t} active_area;\n\nSame for crop and output size.\n\n> +\tint32_t analog_crop_left;\n> +\tint32_t analog_crop_top;\n> +\tuint32_t analog_crop_width;\n> +\tuint32_t analog_crop_height;\n> +\tuint32_t output_width;\n> +\tuint32_t output_height;\n> +\tuint32_t pixel_clock;\n> +\tuint32_t line_length;\n> +};\n> +\n>  struct ipa_stream {\n>  \tunsigned int id;\n>  \tunsigned int pixel_format;\n> @@ -70,6 +86,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> @@ -96,6 +113,7 @@ struct ipa_context *ipaCreate();\n>  #include <libcamera/geometry.h>\n>  #include <libcamera/signal.h>\n>  \n> +#include \"camera_sensor.h\"\n\nYou can forward-declare struct CameraSensorInfo instead of includeing\ncamera_sensor.h. Same comment in many locations below.\n\n>  #include \"v4l2_controls.h\"\n>  \n>  namespace libcamera {\n> @@ -125,7 +143,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..049b9dd1eefc 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.name = sensor_info->name;\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\nAs this isn't an array but a braced initializer list for a class, should\nwe remove the trailing comma ? It shouldn't grow over time. Same for the\nother ones below.\n\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_width,\n> +\t\t\t\t  sensor_info->output_height, };\n> +\tsensorInfo.pixelClock = sensor_info->pixel_clock;\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..8b081359afff 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>  #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> + * befor accessing them.\n\ns/befor/before/\n\nI think we should make sure the corresponding kernel drivers implement\nthe features we need, and then turn this into a fatal error. Not part of\nthis series of course. What's your opinion on that ?\n\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>  #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..e11e7b6a894d 100644\n> --- a/src/libcamera/include/ipa_context_wrapper.h\n> +++ b/src/libcamera/include/ipa_context_wrapper.h\n> @@ -9,6 +9,7 @@\n>  \n>  #include <ipa/ipa_interface.h>\n>  \n> +#include \"camera_sensor.h\"\n>  #include \"control_serializer.h\"\n>  \n>  namespace libcamera {\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..b1ff83aa8be8 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,34 @@ 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> +\tstrncpy(sensor_info.name, sensorInfo.name.c_str(),\n> +\t\tsensorInfo.name.length());\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_width = sensorInfo.outputSize.width;\n> +\tsensor_info.output_height = sensorInfo.outputSize.height;\n> +\tsensor_info.pixel_clock = sensorInfo.pixelClock;\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 +172,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..22a3567bc1d0 100644\n> --- a/src/libcamera/ipa_interface.cpp\n> +++ b/src/libcamera/ipa_interface.cpp\n> @@ -92,6 +92,65 @@\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> + * \\def CAMERA_SENSOR_NAME_LEN\n> + * \\brief The camera sensor name maximum length\n> + *\n> + * \\var ipa_sensor_info::name\n> + * \\brief The camera sensor 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::analog_crop_left\n> + * \\brief The horizontal displacement from the active area top-left corner of\n> + * the cropped portion of the pixel array matrix\n\nI have a bit of a hard time parsing this late at night :-) Would the\nfollowing be more readable ?\n\n * \\brief The left coordinate of the analog crop rectangle, relative to\n * the pixel array active area\n\nSame for below.\n\n> + * \\sa libcamera::CameraSensorInfo::analogCrop\n> + *\n> + * \\var ipa_sensor_info::analog_crop_top\n> + * \\brief The vertical displacement from the active area top-left corner of\n> + * the cropped portion of the pixel array matrix\n> + * \\sa libcamera::CameraSensorInfo::analogCrop\n> + *\n> + * \\var ipa_sensor_info::analog_crop_width\n> + * \\brief The horizontal size of the cropped portion of the pixel array matrix\n> + * \\sa libcamera::CameraSensorInfo::analogCrop\n> + *\n> + * \\var ipa_sensor_info::analog_crop_height\n> + * \\brief The vertical size of the cropped portion of the pixel array matrix\n> + * \\sa libcamera::CameraSensorInfo::analogCrop\n> + *\n> + * \\var ipa_sensor_info::output_width\n> + * \\brief The horizontal size of the output image\n> + * \\sa libcamera::CameraSensorInfo::outputSize\n> + *\n> + * \\var ipa_sensor_info::output_height\n> + * \\brief The vertical size of the output image\n> + * \\sa libcamera::CameraSensorInfo::outputSize\n> + *\n> + * \\var ipa_sensor_info::pixel_clock\n\nI forgot to mention when reviewing the CameraSensorInfo class, should\nthis be named pixel rate instead of pixel clock, as it's effectively the\nnumber of pixels per second ? Some formats may require multiple clock\ncycles per pixel (or transmit multiple pixels per clock cycle), so it\ncould make a difference.\n\n> + * \\brief the pixel clock out frequency in Hz\n> + * \\sa libcamera::CameraSensorInfo::pixelClock\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 +506,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\nHow about adding another paragraph at the end of the documentation block\nto explain this in a bit more details ?\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> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index f42264361785..169492e8313f 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -822,6 +822,13 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \tactiveCamera_ = camera;\n>  \n>  \t/* Inform IPA of stream configuration and sensor controls. */\n> +\tCameraSensorInfo sensorInfo = {};\n> +\tret = data->sensor_->sensorInfo(&sensorInfo);\n> +\tif (ret) {\n> +\t\tLOG(RkISP1, Info) << \"Camera sensor information not available\";\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 +838,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>  #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>  #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..2cd10f981ecc 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,19 @@ 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> +\t\t\t     << sensorInfo.outputSize.width << \"x\"\n> +\t\t\t     << sensorInfo.outputSize.height << \")\" << endl;\n\nYou can use\n\n\t\t\tcerr << \"configure(): Invalid sensor info sizes: \"\n\t\t\t     << sensorInfo.outputSize.toString() <<\n\t\t\t     endl;\n\n> +\t\t\treturn report(Op_configure, TestFail);\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 +298,19 @@ protected:\n>  \t\tint ret;\n>  \n>  \t\t/* Test configure(). */\n> +\t\tCameraSensorInfo sensorInfo = { \"sensor\", 8,\n\ns/ = // would save a copy-construction (but I expect the compiler to\noptimize it out anyway).\n\n> +\t\t\t\t\t\t{ 2576, 1956 },\n> +\t\t\t\t\t\t{ 8, 8, 2560, 1940 },\n> +\t\t\t\t\t\t{ 2560, 1940 },\n> +\t\t\t\t\t\t96000000,\n> +\t\t\t\t\t\t2918, };\n\nWould\n\n\t\tCameraSensorInfo sensorInfo{\n\t\t\t\"sensor\",\n\t\t\t8,\n\t\t\t{ 2576, 1956 },\n\t\t\t{ 8, 8, 2560, 1940 },\n\t\t\t{ 2560, 1940 },\n\t\t\t96000000,\n\t\t\t2918,\n\t\t};\n\nbe more readable ?\n\nAs far as I can tell, clang++ supports the C99-extension designated\ninitializers in C++, and g++ also supports them provided the fields are\nexpressed in the same order in the initialization than in the\ndeclaration (this differs from the gcc implementation where the order\nisn't constrained). Should we use this to increase readability ?\n\n\t\tCameraSensorInfo sensorInfo{\n\t\t\t.name = \"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.outputCrop = { 2560, 1940 },\n\t\t\t.pixelClock = 96000000,\n\t\t\t.lineLength = 2918,\n\t\t};\n\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 0C983603FD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 26 Apr 2020 00:33:03 +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 6FFA44F7;\n\tSun, 26 Apr 2020 00:33:02 +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=\"aVf1QD/j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587853982;\n\tbh=tRd4nlm/05G4RZVY0Cse6qxzVtJVDkA6/DvU6bwAaZo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aVf1QD/j7lJg4OgORUZGRV1uN4Cw5P84fQHdvQaz+YDZbS08P0UZfXu7r/AbIkK3t\n\txgme05h+Cdah+FOsT584zLyewEGcwVB5Za+05r/xUc5s7cqYL9QeFWyY8SqN7hDak1\n\tBLBOpLYqFMBtDTknwaETKoaR46JF0BuqJmjqHULE=","Date":"Sun, 26 Apr 2020 01:32:47 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425223247.GH10975@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-14-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200424215304.558317-14-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 13/13] 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":"Sat, 25 Apr 2020 22:33:03 -0000"}},{"id":4585,"web_url":"https://patchwork.libcamera.org/comment/4585/","msgid":"<20200427194215.axsmjxaiimitrxq7@uno.localdomain>","date":"2020-04-27T19:42:15","subject":"Re: [libcamera-devel] [PATCH v3 13/13] 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 Sun, Apr 26, 2020 at 01:32:47AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 24, 2020 at 11:53:04PM +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                 | 21 +++++++-\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       | 24 +++++++--\n> >  src/libcamera/ipa_interface.cpp             | 60 +++++++++++++++++++++\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp    |  9 +++-\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              | 21 +++++++-\n> >  12 files changed, 173 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> > index e65844bc7b34..a7acc09c4715 100644\n> > --- a/include/ipa/ipa_interface.h\n> > +++ b/include/ipa/ipa_interface.h\n> > @@ -18,6 +18,22 @@ struct ipa_context {\n> >  \tconst struct ipa_context_ops *ops;\n> >  };\n> >\n> > +struct ipa_sensor_info {\n> > +#define CAMERA_SENSOR_NAME_LEN 32\n> > +\tchar name[CAMERA_SENSOR_NAME_LEN];\n>\n> Fixed-size strings in APIs are not nice :-S Do you think we could use a\n> const char * ?\n>\n> This should also be renamed to model if you agree with the previous\n> related suggestion.\n>\n\nAs clarified in the review of your recent patch on IPA configuration,\nI can change this, yes\n\n> > +\tuint8_t bits_per_pixel;\n> > +\tuint32_t active_area_width;\n> > +\tuint32_t active_area_height;\n>\n> How about\n>\n> \tstruct {\n> \t\tuint32_t width;\n> \t\tuint32_t height;\n> \t} active_area;\n\nmuch nicer!\n\n>\n> Same for crop and output size.\n\nI've named the two structures 'analog_crop' and 'output_size'\n\n>\n> > +\tint32_t analog_crop_left;\n> > +\tint32_t analog_crop_top;\n> > +\tuint32_t analog_crop_width;\n> > +\tuint32_t analog_crop_height;\n> > +\tuint32_t output_width;\n> > +\tuint32_t output_height;\n> > +\tuint32_t pixel_clock;\n> > +\tuint32_t line_length;\n> > +};\n> > +\n> >  struct ipa_stream {\n> >  \tunsigned int id;\n> >  \tunsigned int pixel_format;\n> > @@ -70,6 +86,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> > @@ -96,6 +113,7 @@ struct ipa_context *ipaCreate();\n> >  #include <libcamera/geometry.h>\n> >  #include <libcamera/signal.h>\n> >\n> > +#include \"camera_sensor.h\"\n>\n> You can forward-declare struct CameraSensorInfo instead of includeing\n> camera_sensor.h. Same comment in many locations below.\n>\n> >  #include \"v4l2_controls.h\"\n> >\n> >  namespace libcamera {\n> > @@ -125,7 +143,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..049b9dd1eefc 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.name = sensor_info->name;\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>\n> As this isn't an array but a braced initializer list for a class, should\n> we remove the trailing comma ? It shouldn't grow over time. Same for the\n> other ones below.\n\nAh yes, I usually end with a comma as it minimizes changes if it has\nto be extended, but this doesn't seem to be the case\n\n>\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_width,\n> > +\t\t\t\t  sensor_info->output_height, };\n> > +\tsensorInfo.pixelClock = sensor_info->pixel_clock;\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..8b081359afff 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> >  #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> > + * befor accessing them.\n>\n> s/befor/before/\n>\n> I think we should make sure the corresponding kernel drivers implement\n> the features we need, and then turn this into a fatal error. Not part of\n> this series of course. What's your opinion on that ?\n>\n\nI would say it's up to pipeline handlers/IPA. If they actually need\nsensor information, they won't be able to create one without propert\nsupport for the requested features in the sensor driver. For others,\nsay VIMC, this is fine. It has to be addressed in the pipeline handler\nthough...\n\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> >  #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..e11e7b6a894d 100644\n> > --- a/src/libcamera/include/ipa_context_wrapper.h\n> > +++ b/src/libcamera/include/ipa_context_wrapper.h\n> > @@ -9,6 +9,7 @@\n> >\n> >  #include <ipa/ipa_interface.h>\n> >\n> > +#include \"camera_sensor.h\"\n> >  #include \"control_serializer.h\"\n> >\n> >  namespace libcamera {\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..b1ff83aa8be8 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,34 @@ 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> > +\tstrncpy(sensor_info.name, sensorInfo.name.c_str(),\n> > +\t\tsensorInfo.name.length());\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_width = sensorInfo.outputSize.width;\n> > +\tsensor_info.output_height = sensorInfo.outputSize.height;\n> > +\tsensor_info.pixel_clock = sensorInfo.pixelClock;\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 +172,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..22a3567bc1d0 100644\n> > --- a/src/libcamera/ipa_interface.cpp\n> > +++ b/src/libcamera/ipa_interface.cpp\n> > @@ -92,6 +92,65 @@\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> > + * \\def CAMERA_SENSOR_NAME_LEN\n> > + * \\brief The camera sensor name maximum length\n> > + *\n> > + * \\var ipa_sensor_info::name\n> > + * \\brief The camera sensor 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::analog_crop_left\n> > + * \\brief The horizontal displacement from the active area top-left corner of\n> > + * the cropped portion of the pixel array matrix\n>\n> I have a bit of a hard time parsing this late at night :-) Would the\n> following be more readable ?\n>\n>  * \\brief The left coordinate of the analog crop rectangle, relative to\n>  * the pixel array active area\n>\n> Same for below.\n>\n> > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > + *\n> > + * \\var ipa_sensor_info::analog_crop_top\n> > + * \\brief The vertical displacement from the active area top-left corner of\n> > + * the cropped portion of the pixel array matrix\n> > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > + *\n> > + * \\var ipa_sensor_info::analog_crop_width\n> > + * \\brief The horizontal size of the cropped portion of the pixel array matrix\n> > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > + *\n> > + * \\var ipa_sensor_info::analog_crop_height\n> > + * \\brief The vertical size of the cropped portion of the pixel array matrix\n> > + * \\sa libcamera::CameraSensorInfo::analogCrop\n> > + *\n> > + * \\var ipa_sensor_info::output_width\n> > + * \\brief The horizontal size of the output image\n> > + * \\sa libcamera::CameraSensorInfo::outputSize\n> > + *\n> > + * \\var ipa_sensor_info::output_height\n> > + * \\brief The vertical size of the output image\n> > + * \\sa libcamera::CameraSensorInfo::outputSize\n> > + *\n> > + * \\var ipa_sensor_info::pixel_clock\n>\n> I forgot to mention when reviewing the CameraSensorInfo class, should\n> this be named pixel rate instead of pixel clock, as it's effectively the\n> number of pixels per second ? Some formats may require multiple clock\n> cycles per pixel (or transmit multiple pixels per clock cycle), so it\n> could make a difference.\n>\n\nI thought about it, it's the rate, but also the clock in Hz so it does\nnot actually make a difference there, but yes, as the v4l2 control\nrepresent the pixel rate, which depending on the format might be\ndifferent than the pixel clock (which is acutally the sample clock) we\nshould turn this into pixel rate.\n\n> > + * \\brief the pixel clock out frequency in Hz\n> > + * \\sa libcamera::CameraSensorInfo::pixelClock\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 +506,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>\n> How about adding another paragraph at the end of the documentation block\n> to explain this in a bit more details ?\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\nYes, I refrained to do so as streamConfig and entityControls are\nprotocol-specific, but yes, sensorInfo is generic, so it might fit\nthere\n\n>\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index f42264361785..169492e8313f 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -822,6 +822,13 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> >  \tactiveCamera_ = camera;\n> >\n> >  \t/* Inform IPA of stream configuration and sensor controls. */\n> > +\tCameraSensorInfo sensorInfo = {};\n> > +\tret = data->sensor_->sensorInfo(&sensorInfo);\n> > +\tif (ret) {\n> > +\t\tLOG(RkISP1, Info) << \"Camera sensor information not available\";\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 +838,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> >  #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> >  #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..2cd10f981ecc 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,19 @@ 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> > +\t\t\t     << sensorInfo.outputSize.width << \"x\"\n> > +\t\t\t     << sensorInfo.outputSize.height << \")\" << endl;\n>\n> You can use\n>\n> \t\t\tcerr << \"configure(): Invalid sensor info sizes: \"\n> \t\t\t     << sensorInfo.outputSize.toString() <<\n> \t\t\t     endl;\n>\n> > +\t\t\treturn report(Op_configure, TestFail);\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 +298,19 @@ protected:\n> >  \t\tint ret;\n> >\n> >  \t\t/* Test configure(). */\n> > +\t\tCameraSensorInfo sensorInfo = { \"sensor\", 8,\n>\n> s/ = // would save a copy-construction (but I expect the compiler to\n> optimize it out anyway).\n>\n\nI would expect so, I went for = as it's just nicer to me, so I can\nchange it back and use the list initializer.\n\n> > +\t\t\t\t\t\t{ 2576, 1956 },\n> > +\t\t\t\t\t\t{ 8, 8, 2560, 1940 },\n> > +\t\t\t\t\t\t{ 2560, 1940 },\n> > +\t\t\t\t\t\t96000000,\n> > +\t\t\t\t\t\t2918, };\n>\n> Would\n>\n> \t\tCameraSensorInfo sensorInfo{\n> \t\t\t\"sensor\",\n> \t\t\t8,\n> \t\t\t{ 2576, 1956 },\n> \t\t\t{ 8, 8, 2560, 1940 },\n> \t\t\t{ 2560, 1940 },\n> \t\t\t96000000,\n> \t\t\t2918,\n> \t\t};\n>\n> be more readable ?\n>\n> As far as I can tell, clang++ supports the C99-extension designated\n> initializers in C++, and g++ also supports them provided the fields are\n> expressed in the same order in the initialization than in the\n> declaration (this differs from the gcc implementation where the order\n> isn't constrained). Should we use this to increase readability ?\n\nI refrain from expressing my thoughts on the initializer list vs designated\ninitializer, standard versions vs compiler variations mess that C++\nbrought us.\n\n>\n> \t\tCameraSensorInfo sensorInfo{\n> \t\t\t.name = \"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.outputCrop = { 2560, 1940 },\n> \t\t\t.pixelClock = 96000000,\n> \t\t\t.lineLength = 2918,\n\nI'm happy to use this. It reminds me of C, the happy place my mind\ngoes to where I have C++ nightmares.\n\nThanks\n  j\n\n> \t\t};\n>\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 relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E85E7603FC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Apr 2020 21:39:07 +0200 (CEST)","from uno.localdomain (a-ur1-85.tin.it [212.216.150.148])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 3D3C540007;\n\tMon, 27 Apr 2020 19:39:05 +0000 (UTC)"],"X-Originating-IP":"212.216.150.148","Date":"Mon, 27 Apr 2020 21:42:15 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200427194215.axsmjxaiimitrxq7@uno.localdomain>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-14-jacopo@jmondi.org>\n\t<20200425223247.GH10975@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200425223247.GH10975@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 13/13] 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":"Mon, 27 Apr 2020 19:39:08 -0000"}}]