[{"id":2862,"web_url":"https://patchwork.libcamera.org/comment/2862/","msgid":"<20191011123705.GC4882@pendragon.ideasonboard.com>","date":"2019-10-11T12:37:05","subject":"Re: [libcamera-devel] [PATCH v6 8/9] libcamera: ipa: rkisp1: Add\n\tbasic control of auto exposure","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Oct 11, 2019 at 05:22:15AM +0200, Niklas Söderlund wrote:\n> Add an IPA which controls the exposure time and analog gain for a sensor\n> connected to the rkisp1 pipeline. The IPA supports turning AE on and off\n> and informing the camera of the status of the AE control loop.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/ipa/rkisp1.h | 18 +++\n> src/ipa/meson.build | 2 +\n> src/ipa/rkisp1/meson.build | 6 +\n> src/ipa/rkisp1/rkisp1.cpp | 264 +++++++++++++++++++++++++++++++++++++\n> 4 files changed, 290 insertions(+)\n> create mode 100644 include/ipa/rkisp1.h\n> create mode 100644 src/ipa/rkisp1/meson.build\n> create mode 100644 src/ipa/rkisp1/rkisp1.cpp\n> \n> diff --git a/include/ipa/rkisp1.h b/include/ipa/rkisp1.h\n> new file mode 100644\n> index 0000000000000000..4fe0482b8de51419\n> --- /dev/null\n> +++ b/include/ipa/rkisp1.h\n> @@ -0,0 +1,18 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * rkisp1.h - Image Processing Algorithm interface for RkISP1\n> + */\n> +#ifndef __LIBCAMERA_IPA_INTERFACE_RKISP1_H__\n> +#define __LIBCAMERA_IPA_INTERFACE_RKISP1_H__\n> +\n> +enum RkISP1Operations {\n> +\tRKISP1_IPA_ACTION_V4L2_SET = 1,\n> +\tRKISP1_IPA_ACTION_PARAM_FILLED = 2,\n> +\tRKISP1_IPA_ACTION_METADATA = 3,\n> +\tRKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER = 4,\n> +\tRKISP1_IPA_EVENT_QUEUE_REQUEST = 5,\n> +};\n> +\n> +#endif /* __LIBCAMERA_IPA_INTERFACE_RKISP1_H__ */\n> diff --git a/src/ipa/meson.build b/src/ipa/meson.build\n> index ac16e1da6126bb2a..4f2a457712016d53 100644\n> --- a/src/ipa/meson.build\n> +++ b/src/ipa/meson.build\n> @@ -22,3 +22,5 @@ endforeach\n> \n> config_h.set('IPA_MODULE_DIR',\n> '\"' + join_paths(get_option('prefix'), ipa_install_dir) + '\"')\n> +\n> +subdir('rkisp1')\n> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> new file mode 100644\n> index 0000000000000000..c2b1d228555f0636\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/meson.build\n> @@ -0,0 +1,6 @@\n> +rkisp1_ipa = shared_module('ipa_rkisp1',\n> + 'rkisp1.cpp',\n> + name_prefix : '',\n> + include_directories : ipa_includes,\n> + install : true,\n> + install_dir : ipa_install_dir)\n\nI think you need to link this to libcamera.so.\n\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> new file mode 100644\n> index 0000000000000000..6510ddf14353c554\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -0,0 +1,264 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * rkisp1.cpp - RkISP1 Image Processing Algorithms\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPARkISP1)\n> +\n> +class IPARkISP1 : public IPAInterface\n> +{\n> +public:\n> +\tint init() override { return 0; }\n> +\n> +\tvoid configure(const std::map &streamConfig,\n> +\t\t const std::map &entityControls) override;\n> +\tvoid mapBuffers(const std::vector &buffers) override;\n> +\tvoid unmapBuffers(const std::vector &ids) override;\n> +\tvoid processEvent(const IPAOperationData &event) override;\n> +\n> +private:\n> +\tvoid queueRequest(unsigned int frame, unsigned int paramid,\n> +\t\t\t rkisp1_isp_params_cfg *params,\n> +\t\t\t const ControlList &controls);\n> +\tvoid updateStatistics(unsigned int frame,\n> +\t\t\t const rkisp1_stat_buffer *stats);\n> +\n> +\tvoid setControls(unsigned int frame);\n> +\tvoid queueBuffer(unsigned int frame, unsigned int id);\n> +\tvoid metadataReady(unsigned int frame, unsigned int aeState);\n> +\n> +\tstd::map bufferInfo_;\n> +\n> +\t/* Camera sensor controls. */\n> +\tbool autoExposure_;\n> +\tuint32_t exposure_;\n> +\tuint32_t minExposure_;\n> +\tuint32_t maxExposure_;\n> +\tuint32_t gain_;\n> +\tuint32_t minGain_;\n> +\tuint32_t maxGain_;\n> +};\n> +\n> +void IPARkISP1::configure(const std::map &streamConfig,\n> +\t\t\t const std::map &entityControls)\n> +{\n> +\tif (entityControls.empty())\n> +\t\treturn;\n> +\n> +\tconst V4L2ControlInfoMap &ctrls = entityControls.at(0);\n> +\n> +\tconst auto itExp = ctrls.find(V4L2_CID_EXPOSURE);\n> +\tif (itExp == ctrls.end()) {\n> +\t\tLOG(IPARkISP1, Error) << \"Can't find exposure control\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tconst auto itGain = ctrls.find(V4L2_CID_ANALOGUE_GAIN);\n> +\tif (itGain == ctrls.end()) {\n> +\t\tLOG(IPARkISP1, Error) << \"Can't find gain control\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tautoExposure_ = true;\n> +\n> +\tminExposure_ = std::max(itExp->second.range().min().get(), 1);\n\ns/uint64_t/uint32_t/\n\n> +\tmaxExposure_ = itExp->second.range().max().get();\n> +\texposure_ = minExposure_;\n> +\n> +\tminGain_ = std::max(itGain->second.range().min().get(), 1);\n\nHere too.\n\n> +\tmaxGain_ = itGain->second.range().max().get();\n> +\tgain_ = minGain_;\n> +\n> +\tLOG(IPARkISP1, Info)\n> +\t\t<< \"Exposure: \" << minExposure_ << \"-\" << maxExposure_\n> +\t\t<< \" Gain: \" << minGain_ << \"-\" << maxGain_;\n> +\n> +\tsetControls(0);\n> +}\n> +\n> +void IPARkISP1::mapBuffers(const std::vector &buffers)\n> +{\n> +\tfor (IPABuffer buffer : buffers) {\n> +\t\tbufferInfo_[buffer.id] = buffer.memory;\n> +\t\tbufferInfo_[buffer.id].planes()[0].mem();\n> +\t}\n> +}\n> +\n> +void IPARkISP1::unmapBuffers(const std::vector &ids)\n> +{\n> +\tfor (unsigned int id : ids)\n> +\t\tbufferInfo_.erase(id);\n> +}\n> +\n> +void IPARkISP1::processEvent(const IPAOperationData &event)\n> +{\n> +\tswitch (event.operation) {\n> +\tcase RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER: {\n> +\t\tunsigned int frame = event.data[0];\n\nWould it make sense to move the frame number out of IPAOperationData to\nan explicit parameter of the processEvent method, as we did for the\nqueueFrameAction signal ? If you're unsure this could be done later, on\ntop of this series.\n\n> +\t\tunsigned int id = event.data[1];\n\nMaybe s/id/buffer/ (or bufferId) to be more explicit ?\n\n> +\n> +\t\tconst rkisp1_stat_buffer *stats =\n> +\t\t\tstatic_cast(bufferInfo_[id].planes()[0].mem());\n> +\n> +\t\tupdateStatistics(frame, stats);\n> +\t\tbreak;\n> +\t}\n> +\tcase RKISP1_IPA_EVENT_QUEUE_REQUEST: {\n> +\t\tunsigned int frame = event.data[0];\n> +\t\tunsigned int id = event.data[1];\n\nSame here.\n\n> +\n> +\t\trkisp1_isp_params_cfg *params =\n> +\t\t\tstatic_cast(bufferInfo_[id].planes()[0].mem());\n> +\n> +\t\tqueueRequest(frame, event.data[1], params, event.controls[0]);\n> +\t\tbreak;\n> +\t}\n> +\tdefault:\n> +\t\tLOG(IPARkISP1, Error) << \"Unkown event \" << event.operation;\n> +\t\tbreak;\n> +\t}\n> +}\n> +\n> +void IPARkISP1::queueRequest(unsigned int frame, unsigned int paramid,\n> +\t\t\t rkisp1_isp_params_cfg *params,\n> +\t\t\t const ControlList &controls)\n> +{\n> +\t/* Prepare parameters buffer. */\n> +\tmemset(params, 0, sizeof(*params));\n> +\n> +\t/* Auto Exposure on/off. */\n> +\tif (controls.contains(controls::AeEnable)) {\n> +\t\tautoExposure_ = controls.get(controls::AeEnable);\n> +\t\tif (autoExposure_)\n> +\t\t\tparams->module_ens = CIFISP_MODULE_AEC;\n> +\n> +\t\tparams->module_en_update = CIFISP_MODULE_AEC;\n> +\t}\n> +\n> +\t/* Queue buffers to pipeline. */\n> +\tqueueBuffer(frame, paramid);\n> +}\n> +\n> +void IPARkISP1::updateStatistics(unsigned int frame,\n> +\t\t\t\t const rkisp1_stat_buffer *stats)\n> +{\n> +\tconst cifisp_stat *params = &stats->params;\n> +\tunsigned int aeState = 0;\n> +\n> +\tif (stats->meas_type & CIFISP_STAT_AUTOEXP) {\n> +\t\tconst cifisp_ae_stat *ae = ¶ms->ae;\n> +\n> +\t\tconst unsigned int target = 60;\n> +\n> +\t\tunsigned int value = 0;\n> +\t\tunsigned int num = 0;\n> +\t\tfor (int i = 0; i < CIFISP_AE_MEAN_MAX; i++) {\n> +\t\t\tif (ae->exp_mean[i] <= 15)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tvalue += ae->exp_mean[i];\n> +\t\t\tnum++;\n> +\t\t}\n> +\t\tvalue /= num;\n> +\n> +\t\tdouble factor = (double)target / value;\n> +\n> +\t\tif (frame % 3 == 0) {\n> +\t\t\tdouble tmp;\n\ns/tmp/exposure/ ?\n\n> +\n> +\t\t\ttmp = factor * exposure_ * gain_ / minGain_;\n> +\t\t\texposure_ = utils::clamp((uint64_t)tmp,\n> +\t\t\t\t\t\t\t minExposure_,\n> +\t\t\t\t\t\t\t maxExposure_);\n> +\n> +\t\t\ttmp = tmp / exposure_ * minGain_;\n> +\t\t\tgain_ = utils::clamp((uint64_t)tmp, minGain_,\n> +\t\t\t\t\t\t maxGain_);\n> +\n> +\t\t\tsetControls(frame + 1);\n> +\t\t}\n> +\n> +\t\taeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;\n> +\t}\n> +\n> +\tmetadataReady(frame, aeState);\n> +}\n> +\n> +void IPARkISP1::setControls(unsigned int frame)\n> +{\n> +\tIPAOperationData op;\n> +\top.operation = RKISP1_IPA_ACTION_V4L2_SET;\n> +\n> +\tV4L2ControlList ctrls;\n> +\tctrls.add(V4L2_CID_EXPOSURE, exposure_);\n> +\tctrls.add(V4L2_CID_ANALOGUE_GAIN, gain_);\n> +\top.v4l2controls.push_back(ctrls);\n> +\n> +\tqueueFrameAction.emit(frame, op);\n> +}\n> +\n> +void IPARkISP1::queueBuffer(unsigned int frame, unsigned int id)\n\nThe id argument is not used, you can remove it.\n\nThe method name should also be updated, as it doesn't queue a buffer\nanymore. As it's called in a single place, maybe you could inline it ?\nDon't forgot to also update the comment in queueRequest() accordingly.\n\n> +{\n> +\tIPAOperationData op;\n> +\top.operation = RKISP1_IPA_ACTION_PARAM_FILLED;\n> +\n> +\tqueueFrameAction.emit(frame, op);\n> +}\n> +\n> +void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)\n> +{\n> +\tControlList ctrls(nullptr);\n> +\n> +\tIPAOperationData op;\n> +\top.operation = RKISP1_IPA_ACTION_METADATA;\n> +\n> +\tif (aeState)\n> +\t\tctrls.set(controls::AeLocked, aeState == 2);\n\nI would either move those two lines just after defining the ctrls\nvariable, or move the ctrls variable definition just before them.\n\nWith these small issues fixed,\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\top.controls.push_back(ctrls);\n> +\n> +\tqueueFrameAction.emit(frame, op);\n> +}\n> +\n> +/*\n> + * External IPA module interface\n> + */\n> +\n> +extern \"C\" {\n> +const struct IPAModuleInfo ipaModuleInfo = {\n> +\tIPA_MODULE_API_VERSION,\n> +\t1,\n> +\t\"PipelineHandlerRkISP1\",\n> +\t\"RkISP1 IPA\",\n> +\t\"LGPL-2.1-or-later\",\n> +};\n> +\n> +IPAInterface *ipaCreate()\n> +{\n> +\treturn new IPARkISP1();\n> +}\n> +};\n> +\n> +}; /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 77FB561564\n\tfor ;\n\tFri, 11 Oct 2019 14:37:08 +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 BD27633A;\n\tFri, 11 Oct 2019 14:37:06 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570797426;\n\tbh=rIHJn6MOnoTEGjUuDVZruBcwMH7uVmnH31/owd1TZnw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AksBeIWOLeeOByUBATFeVhoP5OLrWMXpUTHWCmNX7bBp2r4Pc7LW5uySMwBDT/353\n\tWGSe/xPaVDaD+vg3D+UvUkQu9yyzRvMqFGraqDBLRtg1Lm7XA73Dh+B6tieSzEER1x\n\tELGSHgW1+f3F135Y/rzcPhhuqy1TrFOT2leE/Q/M=","Date":"Fri, 11 Oct 2019 15:37:05 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191011123705.GC4882@pendragon.ideasonboard.com>","References":"<20191011032216.2175173-1-niklas.soderlund@ragnatech.se>\n\t<20191011032216.2175173-9-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191011032216.2175173-9-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v6 8/9] libcamera: ipa: rkisp1: Add\n\tbasic control of auto exposure","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 11 Oct 2019 12:37:08 -0000"}}]