[{"id":2710,"web_url":"https://patchwork.libcamera.org/comment/2710/","msgid":"<20190928105326.3dhfnxrg5cvamnwf@uno.localdomain>","date":"2019-09-28T10:53:26","subject":"Re: [libcamera-devel] [PATCH v3 12/13] libcamera: ipa: rkisp1: Add\n\tbasic control of auto exposure","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\n I would change the subject to point out you're adding the IPA module\nand not extending it with one more control\n\nOn Fri, Sep 27, 2019 at 04:44:16AM +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 <niklas.soderlund@ragnatech.se>\n> ---\n>  src/ipa/ipa_rkisp1.cpp | 228 +++++++++++++++++++++++++++++++++++++++++\n>  src/ipa/meson.build    |  13 +++\n>  2 files changed, 241 insertions(+)\n>  create mode 100644 src/ipa/ipa_rkisp1.cpp\n>\n> diff --git a/src/ipa/ipa_rkisp1.cpp b/src/ipa/ipa_rkisp1.cpp\n> new file mode 100644\n> index 0000000000000000..f90465516c6aff87\n> --- /dev/null\n> +++ b/src/ipa/ipa_rkisp1.cpp\n> @@ -0,0 +1,228 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_rkisp1.cpp - RkISP1 Image Processing Algorithms\n\nThe RkISP name is very unfortunate. I miss-spell it to RKISP all the\ntimes. Nothing related to this patch though.\n\n> + */\n> +\n> +#include <algorithm>\n> +#include <math.h>\n> +#include <queue>\n> +#include <string.h>\n> +\n> +#include <linux/rkisp1-config.h>\n> +\n> +#include <ipa/ipa_interface.h>\n> +#include <ipa/ipa_module_info.h>\n> +#include <libcamera/buffer.h>\n> +#include <libcamera/request.h>\n> +\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +\n> +#define BUFFER_PARAM 1\n> +#define BUFFER_STAT 2\n\nI would prefix them with RKISP1\n\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 initSensor(const V4L2ControlInfoMap &controls) override;\n> +\tvoid initBuffers(unsigned int type,\n> +\t\t\t const std::vector<BufferMemory> &buffers) override;\n> +\tvoid signalBuffer(unsigned int type, unsigned int id) override;\n> +\tvoid queueRequest(unsigned int frame, const ControlList &controls) override;\n> +\n> +private:\n> +\tvoid setControls(unsigned int frame);\n> +\tvoid updateStatistics(unsigned int frame, BufferMemory &statistics);\n> +\n> +\tstd::map<unsigned int, std::map<unsigned int, BufferMemory>> bufferInfo_;\n> +\tstd::map<unsigned int, std::map<unsigned int, unsigned int>> bufferFrame_;\n> +\tstd::map<unsigned int, std::queue<unsigned int>> bufferFree_;\n> +\n> +\t/* Camera sensor controls. */\n> +\tbool autoExposure_;\n> +\tuint64_t exposure_;\n> +\tuint64_t minExposure_;\n> +\tuint64_t maxExposure_;\n> +\tuint64_t gain_;\n> +\tuint64_t minGain_;\n> +\tuint64_t maxGain_;\n> +};\n> +\n> +void IPARkISP1::initSensor(const V4L2ControlInfoMap &controls)\n> +{\n> +\tconst auto itExp = controls.find(V4L2_CID_EXPOSURE);\n> +\tif (itExp == controls.end()) {\n> +\t\tLOG(IPARkISP1, Error) << \"Can't find exposure control\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tconst auto itGain = controls.find(V4L2_CID_ANALOGUE_GAIN);\n> +\tif (itGain == controls.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<uint64_t>(itExp->second.min(), 1);\n> +\tmaxExposure_ = itExp->second.max();\n> +\texposure_ = minExposure_;\n> +\n> +\tminGain_ = std::max<uint64_t>(itGain->second.min(), 1);\n> +\tmaxGain_ = itGain->second.max();\n> +\tgain_ = minGain_;\n\nWe need a default for v4l2 controls otherwise you would initialize all\nto min. Or at least the current value reported by the kernel.\n\nI now wonder if the -supported- V4L2Controls and Controls should not\nbe passed at init() and here we should receive a V4L2ControlList which\ncontains default, gets processed by the IPA (if it wants to) and sent\nback with setControls()\n\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::initBuffers(unsigned int type,\n> +\t\t\t    const std::vector<BufferMemory> &buffers)\n> +{\n> +\tbufferInfo_[type].clear();\n> +\tfor (unsigned int i = 0; i < buffers.size(); i++) {\n> +\t\tbufferInfo_[type][i] = buffers[i];\n> +\t\tbufferFree_[type].push(i);\n> +\t}\n\nAs I said, I fear this will get soon unmaintainable. There a lot of\nbookeeping and the types used to do so are two/three level maps. I know,\nI had the same comment for IPU3 and I was like \"what?? I'm maintaining it,\nit's super easy and works\". It soon fel into pieces as soon as the use case\nchanged slightly.\n\nTo avoid going down the same path I would consider (not for this first\nversion, but something to work on after) to serialize Pools and add\nthere methods to pull a buffer (remove it from the list of usable\nones) and push it back to the pool (make it available again). Could we\nconsider this during the Buffer rework effert we've been planning for\nsome time?\n\n> +}\n> +\n> +void IPARkISP1::signalBuffer(unsigned int type, unsigned int id)\n> +{\n> +\tif (type == BUFFER_STAT) {\n> +\t\tunsigned int frame = bufferFrame_[type][id];\n> +\t\tBufferMemory &mem = bufferInfo_[type][id];\n> +\t\tupdateStatistics(frame, mem);\n> +\t}\n> +\n> +\tbufferFree_[type].push(id);\n> +}\n> +\n> +void IPARkISP1::queueRequest(unsigned int frame, const ControlList &controls)\n> +{\n> +\t/* Find buffers. */\n> +\tif (bufferFree_[BUFFER_PARAM].empty()) {\n> +\t\tLOG(IPARkISP1, Error) << \"Param buffer underrun\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (bufferFree_[BUFFER_STAT].empty()) {\n> +\t\tLOG(IPARkISP1, Error) << \"Statistics buffer underrun\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tunsigned int paramid = bufferFree_[BUFFER_PARAM].front();\n> +\tbufferFree_[BUFFER_PARAM].pop();\n> +\tunsigned int statid = bufferFree_[BUFFER_STAT].front();\n> +\tbufferFree_[BUFFER_STAT].pop();\n> +\n> +\tbufferFrame_[BUFFER_PARAM][paramid] = frame;\n> +\tbufferFrame_[BUFFER_STAT][statid] = frame;\n> +\n> +\t/* Prepare parameters buffer. */\n> +\tBufferMemory &mem = bufferInfo_[BUFFER_PARAM][paramid];\n> +\trkisp1_isp_params_cfg *params =\n> +\t\tstatic_cast<rkisp1_isp_params_cfg *>(mem.planes()[0].mem());\n> +\n> +\tmemset(params, 0, sizeof(*params));\n> +\n> +\t/* Auto Exposure on/off. */\n> +\tif (controls.contains(AeEnable)) {\n> +\t\tautoExposure_ = controls[AeEnable].getBool();\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.emit(frame, BUFFER_PARAM, paramid);\n> +\tqueueBuffer.emit(frame, BUFFER_STAT, statid);\n\nHere I am missing a piece. Not easy to comment as the 'issue' is in\nthe pipeline handler queueRequest implementation and in the timeline\nand here, but: shouldn't we have dependecy tracking somehow ? The\nqueueRequest in the pipeline handler schedules queueing of the buffer\nto the video capture node after this two signals have been emitted and\nthe two corresponding actions scheduled on the timeline. What\nguarantees the buffer that is used to capture images is queued -after-\nthe associated ISP parameters ?\n\n> +}\n> +\n> +void IPARkISP1::setControls(unsigned int frame)\n> +{\n> +\tV4L2ControlList ctrls;\n> +\tctrls.add(V4L2_CID_EXPOSURE);\n> +\tctrls.add(V4L2_CID_ANALOGUE_GAIN);\n> +\tctrls[V4L2_CID_EXPOSURE]->setValue(exposure_);\n> +\tctrls[V4L2_CID_ANALOGUE_GAIN]->setValue(gain_);\n\nyou could\n        ctrls.add(V4L2_CID..., value_)\n\n> +\n> +\tupdateSensor.emit(frame, ctrls);\n> +}\n> +\n> +void IPARkISP1::updateStatistics(unsigned int frame, BufferMemory &statistics)\n> +{\n> +\tconst rkisp1_stat_buffer *stats =\n> +\t\tstatic_cast<rkisp1_stat_buffer *>(statistics.planes()[0].mem());\n> +\tconst cifisp_stat *params = &stats->params;\n> +\tIPAMetaData metaData = {};\n> +\n> +\tif (stats->meas_type & CIFISP_STAT_AUTOEXP) {\n> +\t\tconst cifisp_ae_stat *ae = &params->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\tvalue += ae->exp_mean[i];\n> +\t\t\t\tnum++;\n> +\t\t\t}\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> +\n> +\t\t\ttmp = factor * exposure_ * gain_ / minGain_;\n> +\t\t\texposure_ = utils::clamp<uint64_t>((uint64_t)tmp, minExposure_, maxExposure_);\n> +\n> +\t\t\ttmp = tmp / exposure_ * minGain_;\n> +\t\t\tgain_ = utils::clamp<uint64_t>((uint64_t)tmp, minGain_, maxGain_);\n> +\n> +\t\t\tsetControls(frame + 1);\n> +\t\t}\n> +\n> +\t\tmetaData.aeState = fabs(factor - 1.0f) < 0.05f ?\n> +\t\t\tAeState::Converged : AeState::Searching;\n> +\t} else {\n> +\t\tmetaData.aeState = AeState::Inactive;\n> +\t}\n> +\n> +\tmetaDataReady.emit(frame, metaData);\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 */\n> diff --git a/src/ipa/meson.build b/src/ipa/meson.build\n> index 10448c2ffc76af4b..eb5b852eec282735 100644\n> --- a/src/ipa/meson.build\n> +++ b/src/ipa/meson.build\n> @@ -3,6 +3,10 @@ ipa_dummy_sources = [\n>      ['ipa_dummy_isolate', 'Proprietary'],\n>  ]\n>\n> +ipa_sources = [\n> +    ['ipa_rkisp1', 'ipa_rkisp1.cpp', 'LGPL-2.1-or-later'],\n> +]\n> +\n>  ipa_install_dir = join_paths(get_option('libdir'), 'libcamera')\n>\n>  foreach t : ipa_dummy_sources\n> @@ -14,5 +18,14 @@ foreach t : ipa_dummy_sources\n>                          cpp_args : '-DLICENSE=\"' + t[1] + '\"')\n>  endforeach\n>\n> +foreach t : ipa_sources\n> +    ipa = shared_module(t[0], t[1],\n> +                        name_prefix : '',\n> +                        include_directories : includes,\n> +                        install : true,\n> +                        install_dir : ipa_install_dir,\n> +                        cpp_args : '-DLICENSE=\"' + t[2] + '\"')\n> +endforeach\n> +\n>  config_h.set('IPA_MODULE_DIR',\n>               '\"' + join_paths(get_option('prefix'), ipa_install_dir) + '\"')\n> --\n> 2.23.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B93BD60BBA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 28 Sep 2019 12:51:45 +0200 (CEST)","from uno.localdomain\n\t(host71-63-dynamic.19-79-r.retail.telecomitalia.it [79.19.63.71])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id B42F7240003;\n\tSat, 28 Sep 2019 10:51:44 +0000 (UTC)"],"X-Originating-IP":"79.19.63.71","Date":"Sat, 28 Sep 2019 12:53:26 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190928105326.3dhfnxrg5cvamnwf@uno.localdomain>","References":"<20190927024417.725906-1-niklas.soderlund@ragnatech.se>\n\t<20190927024417.725906-13-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ewkgjupnquig2nsc\"","Content-Disposition":"inline","In-Reply-To":"<20190927024417.725906-13-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 12/13] 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":"<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, 28 Sep 2019 10:51:45 -0000"}}]