[{"id":2830,"web_url":"https://patchwork.libcamera.org/comment/2830/","msgid":"<20191008112152.ihvkconurg5lr2yj@uno.localdomain>","date":"2019-10-08T11:21:52","subject":"Re: [libcamera-devel] [PATCH v5 09/10] 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\nOn Tue, Oct 08, 2019 at 02:45:33AM +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>  include/ipa/rkisp1.h   |  23 ++++\n>  src/ipa/ipa_rkisp1.cpp | 281 +++++++++++++++++++++++++++++++++++++++++\n>  src/ipa/meson.build    |  13 ++\n>  3 files changed, 317 insertions(+)\n>  create mode 100644 include/ipa/rkisp1.h\n>  create mode 100644 src/ipa/ipa_rkisp1.cpp\n>\n> diff --git a/include/ipa/rkisp1.h b/include/ipa/rkisp1.h\n> new file mode 100644\n> index 0000000000000000..7db50afa6bef6e61\n> --- /dev/null\n> +++ b/include/ipa/rkisp1.h\n> @@ -0,0 +1,23 @@\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 RkISP1BufferType {\n> +\tRKISP1_BUFFER_PARAM = 1,\n> +\tRKISP1_BUFFER_STAT = 2,\n> +};\n> +\n> +enum RkISP1Operations {\n> +\tRKISP1_IPA_ACTION_V4L2_SET = 1,\n> +\tRKISP1_IPA_ACTION_QUEUE_BUFFER = 2,\n> +\tRKISP1_IPA_ACTION_METADATA = 3,\n> +\tRKISP1_IPA_EVENT_SIGNAL_BUFFER = 4,\n> +\tRKISP1_IPA_EVENT_QUEUE_REQUEST = 5,\n> +};\n> +\n> +#endif /* __LIBCAMERA_IPA_INTERFACE_RKISP1_H__ */\n> diff --git a/src/ipa/ipa_rkisp1.cpp b/src/ipa/ipa_rkisp1.cpp\n> new file mode 100644\n> index 0000000000000000..0cc0772422e1ea51\n> --- /dev/null\n> +++ b/src/ipa/ipa_rkisp1.cpp\n> @@ -0,0 +1,281 @@\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> + */\n> +\n> +#include <algorithm>\n> +#include <cstdint>\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 <ipa/rkisp1.h>\n> +#include <libcamera/buffer.h>\n> +#include <libcamera/control_ids.h>\n> +#include <libcamera/request.h>\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<unsigned int, IPAStream> &streamConfig,\n> +\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override;\n> +\tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n> +\tvoid unmapBuffers(const std::vector<IPABuffer> &buffers) override;\n> +\tvoid processEvent(const IPAOperationData &event) override;\n> +\n> +private:\n> +\tvoid signalBuffer(unsigned int type, unsigned int id);\n> +\tvoid queueRequest(unsigned int frame, const ControlList &controls);\n> +\tvoid updateStatistics(unsigned int frame, BufferMemory &statistics);\n> +\n> +\tvoid setControls(unsigned int frame);\n> +\tvoid queueBuffer(unsigned int frame, unsigned int type,\n> +\t\t\t unsigned int id);\n> +\tvoid metadataReady(unsigned int frame, unsigned int aeState);\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\nI really think this structures to keep track of free/available buffers\nis very fragile. But for now we'll have to live with that.\n\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::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> +\t\t\t  const std::map<unsigned int, V4L2ControlInfoMap> &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<uint64_t>(itExp->second.range().min().get<int32_t>(), 1);\n> +\tmaxExposure_ = itExp->second.range().max().get<int32_t>();\n> +\texposure_ = minExposure_;\n> +\n> +\tminGain_ = std::max<uint64_t>(itGain->second.range().min().get<int32_t>(), 1);\n> +\tmaxGain_ = itGain->second.range().max().get<int32_t>();\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<IPABuffer> &buffers)\n> +{\n> +\tfor (IPABuffer buffer : buffers) {\n> +\t\tbufferInfo_[buffer.type][buffer.id] = buffer.buffer;\n> +\t\tbufferInfo_[buffer.type][buffer.id].planes()[0].mem();\n\nYou don't need this unless you want to store the result of the memory\nmapping somewhere at this point.\n\n> +\t\tbufferFree_[buffer.type].push(buffer.id);\n> +\t}\n> +}\n> +\n> +void IPARkISP1::unmapBuffers(const std::vector<IPABuffer> &buffers)\n> +{\n> +\tfor (IPABuffer buffer : buffers)\n> +\t\tbufferInfo_[buffer.type].erase(buffer.id);\n\nShould buffers be removed from bufferFree_ too ?\n\n> +}\n> +\n> +void IPARkISP1::processEvent(const IPAOperationData &event)\n> +{\n> +\tswitch (event.operation) {\n> +\tcase RKISP1_IPA_EVENT_SIGNAL_BUFFER:\n> +\t\tsignalBuffer(event.data[0], event.data[1]);\n> +\t\tbreak;\n> +\tcase RKISP1_IPA_EVENT_QUEUE_REQUEST:\n> +\t\tqueueRequest(event.data[0], event.controls[0]);\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tLOG(IPARkISP1, Error) << \"Unkown event \" << event.operation;\n> +\t\tbreak;\n> +\t}\n> +}\n> +\n> +void IPARkISP1::signalBuffer(unsigned int type, unsigned int id)\n> +{\n> +\tif (type == RKISP1_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_[RKISP1_BUFFER_PARAM].empty()) {\n> +\t\tLOG(IPARkISP1, Error) << \"Param buffer underrun\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (bufferFree_[RKISP1_BUFFER_STAT].empty()) {\n> +\t\tLOG(IPARkISP1, Error) << \"Statistics buffer underrun\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tunsigned int paramid = bufferFree_[RKISP1_BUFFER_PARAM].front();\n> +\tbufferFree_[RKISP1_BUFFER_PARAM].pop();\n> +\tunsigned int statid = bufferFree_[RKISP1_BUFFER_STAT].front();\n> +\tbufferFree_[RKISP1_BUFFER_STAT].pop();\n> +\n> +\tbufferFrame_[RKISP1_BUFFER_PARAM][paramid] = frame;\n> +\tbufferFrame_[RKISP1_BUFFER_STAT][statid] = frame;\n> +\n> +\t/* Prepare parameters buffer. */\n> +\tBufferMemory &mem = bufferInfo_[RKISP1_BUFFER_PARAM][paramid];\n> +\trkisp1_isp_params_cfg *params =\n> +\t\tstatic_cast<rkisp1_isp_params_cfg *>(mem.planes()[0].mem());\n\nCould you store in the buffers the mapped memory location instead of\ngoing through BufferMemory\n\n> +\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, RKISP1_BUFFER_PARAM, paramid);\n> +\tqueueBuffer(frame, RKISP1_BUFFER_STAT, statid);\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> +\tunsigned int aeState = 0;\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\nWhy this empty lines ?\n\n> +\t\tfor (int i = 0; i < CIFISP_AE_MEAN_MAX; i++) {\n> +\t\t\tif (ae->exp_mean[i] > 15) {\n\n                        if (ae->exp_mean[i] < 15)\n                                continue\n?\n\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\nWhy every 3 frames ?\n\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\nthis could be made to fit in 80cols\n\n> +\n> +\t\t\ttmp = tmp / exposure_ * minGain_;\n> +\t\t\tgain_ = utils::clamp<uint64_t>((uint64_t)tmp, minGain_, maxGain_);\n\nsame here\n\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\nAs a general comment, this goes:\n\nPIPE                    IPA\ndqbuf(Meta)\nsignalbuffer     --->\n                        Update Stats\n                        metadataReady\n                        queueFrameAction\n                <----\nCompleteRequest\n\nI wonder if the request could be completed while the metadata are\nprocessed? Is it enough to make the metadata ControlList read only ?\n\n> +}\n> +\n> +void IPARkISP1::setControls(unsigned int frame)\n> +{\n> +\tIPAOperationData op;\n> +\top.operation = RKISP1_IPA_ACTION_V4L2_SET;\n> +\top.data = { frame };\n> +\n> +\tV4L2ControlList ctrls;\n> +\tctrls.add(V4L2_CID_EXPOSURE);\n> +\tctrls.add(V4L2_CID_ANALOGUE_GAIN);\n> +\tctrls[V4L2_CID_EXPOSURE]->value().set<int32_t>(exposure_);\n> +\tctrls[V4L2_CID_ANALOGUE_GAIN]->value().set<int32_t>(gain_);\n\nI think you could do\n        ctrl.add(V4L2_CID_.., value)\n\n> +\top.v4l2controls.push_back(ctrls);\n> +\n> +\tqueueFrameAction.emit(op);\n> +}\n> +\n> +void IPARkISP1::queueBuffer(unsigned int frame, unsigned int type,\n> +\t\t\t    unsigned int id)\n> +{\n> +\tIPAOperationData op;\n> +\top.operation = RKISP1_IPA_ACTION_QUEUE_BUFFER;\n> +\top.data = { frame, type, id };\n> +\n> +\tqueueFrameAction.emit(op);\n> +}\n> +\n> +void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)\n> +{\n> +\tIPAOperationData op;\n> +\top.operation = RKISP1_IPA_ACTION_METADATA;\n> +\top.data = { frame, aeState };\n> +\n> +\tqueueFrameAction.emit(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 */\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\nDo you need this or could you just create the rkisp1 shared_module\noutside of the for loop ?\n\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 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 C05C261565\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  8 Oct 2019 13:20:07 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 4AC2340018;\n\tTue,  8 Oct 2019 11:20:06 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 8 Oct 2019 13:21:52 +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":"<20191008112152.ihvkconurg5lr2yj@uno.localdomain>","References":"<20191008004534.1585142-1-niklas.soderlund@ragnatech.se>\n\t<20191008004534.1585142-10-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"eyx4psgyebpjyg6f\"","Content-Disposition":"inline","In-Reply-To":"<20191008004534.1585142-10-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 09/10] 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":"Tue, 08 Oct 2019 11:20:07 -0000"}},{"id":2856,"web_url":"https://patchwork.libcamera.org/comment/2856/","msgid":"<20191009203726.GQ22998@pendragon.ideasonboard.com>","date":"2019-10-09T20:37:26","subject":"Re: [libcamera-devel] [PATCH v5 09/10] 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":"Hello,\n\nOn Tue, Oct 08, 2019 at 01:21:52PM +0200, Jacopo Mondi wrote:\n> On Tue, Oct 08, 2019 at 02:45:33AM +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> >  include/ipa/rkisp1.h   |  23 ++++\n> >  src/ipa/ipa_rkisp1.cpp | 281 +++++++++++++++++++++++++++++++++++++++++\n> >  src/ipa/meson.build    |  13 ++\n> >  3 files changed, 317 insertions(+)\n> >  create mode 100644 include/ipa/rkisp1.h\n> >  create mode 100644 src/ipa/ipa_rkisp1.cpp\n> >\n> > diff --git a/include/ipa/rkisp1.h b/include/ipa/rkisp1.h\n> > new file mode 100644\n> > index 0000000000000000..7db50afa6bef6e61\n> > --- /dev/null\n> > +++ b/include/ipa/rkisp1.h\n> > @@ -0,0 +1,23 @@\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 RkISP1BufferType {\n> > +\tRKISP1_BUFFER_PARAM = 1,\n> > +\tRKISP1_BUFFER_STAT = 2,\n> > +};\n> > +\n> > +enum RkISP1Operations {\n> > +\tRKISP1_IPA_ACTION_V4L2_SET = 1,\n> > +\tRKISP1_IPA_ACTION_QUEUE_BUFFER = 2,\n> > +\tRKISP1_IPA_ACTION_METADATA = 3,\n> > +\tRKISP1_IPA_EVENT_SIGNAL_BUFFER = 4,\n> > +\tRKISP1_IPA_EVENT_QUEUE_REQUEST = 5,\n> > +};\n> > +\n> > +#endif /* __LIBCAMERA_IPA_INTERFACE_RKISP1_H__ */\n> > diff --git a/src/ipa/ipa_rkisp1.cpp b/src/ipa/ipa_rkisp1.cpp\n> > new file mode 100644\n> > index 0000000000000000..0cc0772422e1ea51\n> > --- /dev/null\n> > +++ b/src/ipa/ipa_rkisp1.cpp\n> > @@ -0,0 +1,281 @@\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> > + */\n> > +\n> > +#include <algorithm>\n> > +#include <cstdint>\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 <ipa/rkisp1.h>\n> > +#include <libcamera/buffer.h>\n> > +#include <libcamera/control_ids.h>\n> > +#include <libcamera/request.h>\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<unsigned int, IPAStream> &streamConfig,\n> > +\t\t       const std::map<unsigned int, V4L2ControlInfoMap> &entityControls) override;\n> > +\tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n> > +\tvoid unmapBuffers(const std::vector<IPABuffer> &buffers) override;\n> > +\tvoid processEvent(const IPAOperationData &event) override;\n> > +\n> > +private:\n> > +\tvoid signalBuffer(unsigned int type, unsigned int id);\n> > +\tvoid queueRequest(unsigned int frame, const ControlList &controls);\n> > +\tvoid updateStatistics(unsigned int frame, BufferMemory &statistics);\n> > +\n> > +\tvoid setControls(unsigned int frame);\n> > +\tvoid queueBuffer(unsigned int frame, unsigned int type,\n> > +\t\t\t unsigned int id);\n> > +\tvoid metadataReady(unsigned int frame, unsigned int aeState);\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> I really think this structures to keep track of free/available buffers\n> is very fragile. But for now we'll have to live with that.\n\nThere's certainly room for improvement, but for now I think the main\nfocus should be on the IPAInterface. We'll refine the IPA\nimplementation.\n\nThis being said, buffer tracking will need to be moved to the pipeline\nhandler as we remove the IPABuffer::type field.\n\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\nShouldn't these be 32-bit integers as the controls are defined as 32-bit\nin V4L2 ?\n\n> > +};\n> > +\n> > +void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > +\t\t\t  const std::map<unsigned int, V4L2ControlInfoMap> &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<uint64_t>(itExp->second.range().min().get<int32_t>(), 1);\n> > +\tmaxExposure_ = itExp->second.range().max().get<int32_t>();\n> > +\texposure_ = minExposure_;\n> > +\n> > +\tminGain_ = std::max<uint64_t>(itGain->second.range().min().get<int32_t>(), 1);\n> > +\tmaxGain_ = itGain->second.range().max().get<int32_t>();\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<IPABuffer> &buffers)\n> > +{\n> > +\tfor (IPABuffer buffer : buffers) {\n> > +\t\tbufferInfo_[buffer.type][buffer.id] = buffer.buffer;\n> > +\t\tbufferInfo_[buffer.type][buffer.id].planes()[0].mem();\n> \n> You don't need this unless you want to store the result of the memory\n> mapping somewhere at this point.\n> \n> > +\t\tbufferFree_[buffer.type].push(buffer.id);\n> > +\t}\n> > +}\n> > +\n> > +void IPARkISP1::unmapBuffers(const std::vector<IPABuffer> &buffers)\n> > +{\n> > +\tfor (IPABuffer buffer : buffers)\n> > +\t\tbufferInfo_[buffer.type].erase(buffer.id);\n> \n> Should buffers be removed from bufferFree_ too ?\n> \n> > +}\n> > +\n> > +void IPARkISP1::processEvent(const IPAOperationData &event)\n> > +{\n> > +\tswitch (event.operation) {\n> > +\tcase RKISP1_IPA_EVENT_SIGNAL_BUFFER:\n> > +\t\tsignalBuffer(event.data[0], event.data[1]);\n> > +\t\tbreak;\n> > +\tcase RKISP1_IPA_EVENT_QUEUE_REQUEST:\n> > +\t\tqueueRequest(event.data[0], event.controls[0]);\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tLOG(IPARkISP1, Error) << \"Unkown event \" << event.operation;\n> > +\t\tbreak;\n> > +\t}\n> > +}\n> > +\n> > +void IPARkISP1::signalBuffer(unsigned int type, unsigned int id)\n> > +{\n> > +\tif (type == RKISP1_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_[RKISP1_BUFFER_PARAM].empty()) {\n> > +\t\tLOG(IPARkISP1, Error) << \"Param buffer underrun\";\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tif (bufferFree_[RKISP1_BUFFER_STAT].empty()) {\n> > +\t\tLOG(IPARkISP1, Error) << \"Statistics buffer underrun\";\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tunsigned int paramid = bufferFree_[RKISP1_BUFFER_PARAM].front();\n> > +\tbufferFree_[RKISP1_BUFFER_PARAM].pop();\n> > +\tunsigned int statid = bufferFree_[RKISP1_BUFFER_STAT].front();\n> > +\tbufferFree_[RKISP1_BUFFER_STAT].pop();\n> > +\n> > +\tbufferFrame_[RKISP1_BUFFER_PARAM][paramid] = frame;\n> > +\tbufferFrame_[RKISP1_BUFFER_STAT][statid] = frame;\n> > +\n> > +\t/* Prepare parameters buffer. */\n> > +\tBufferMemory &mem = bufferInfo_[RKISP1_BUFFER_PARAM][paramid];\n> > +\trkisp1_isp_params_cfg *params =\n> > +\t\tstatic_cast<rkisp1_isp_params_cfg *>(mem.planes()[0].mem());\n> \n> Could you store in the buffers the mapped memory location instead of\n> going through BufferMemory\n> \n> > +\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, RKISP1_BUFFER_PARAM, paramid);\n> > +\tqueueBuffer(frame, RKISP1_BUFFER_STAT, statid);\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> > +\tunsigned int aeState = 0;\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> \n> Why this empty lines ?\n> \n> > +\t\tfor (int i = 0; i < CIFISP_AE_MEAN_MAX; i++) {\n> > +\t\t\tif (ae->exp_mean[i] > 15) {\n> \n>                         if (ae->exp_mean[i] < 15)\n>                                 continue\n> ?\n> \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> \n> Why every 3 frames ?\n> \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> this could be made to fit in 80cols\n> \n> > +\n> > +\t\t\ttmp = tmp / exposure_ * minGain_;\n> > +\t\t\tgain_ = utils::clamp<uint64_t>((uint64_t)tmp, minGain_, maxGain_);\n> \n> same here\n> \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> As a general comment, this goes:\n> \n> PIPE                    IPA\n> dqbuf(Meta)\n> signalbuffer     --->\n>                         Update Stats\n>                         metadataReady\n>                         queueFrameAction\n>                 <----\n> CompleteRequest\n> \n> I wonder if the request could be completed while the metadata are\n> processed? Is it enough to make the metadata ControlList read only ?\n> \n> > +}\n> > +\n> > +void IPARkISP1::setControls(unsigned int frame)\n> > +{\n> > +\tIPAOperationData op;\n> > +\top.operation = RKISP1_IPA_ACTION_V4L2_SET;\n> > +\top.data = { frame };\n> > +\n> > +\tV4L2ControlList ctrls;\n> > +\tctrls.add(V4L2_CID_EXPOSURE);\n> > +\tctrls.add(V4L2_CID_ANALOGUE_GAIN);\n> > +\tctrls[V4L2_CID_EXPOSURE]->value().set<int32_t>(exposure_);\n> > +\tctrls[V4L2_CID_ANALOGUE_GAIN]->value().set<int32_t>(gain_);\n> \n> I think you could do\n>         ctrl.add(V4L2_CID_.., value)\n> \n> > +\top.v4l2controls.push_back(ctrls);\n> > +\n> > +\tqueueFrameAction.emit(op);\n> > +}\n> > +\n> > +void IPARkISP1::queueBuffer(unsigned int frame, unsigned int type,\n> > +\t\t\t    unsigned int id)\n> > +{\n> > +\tIPAOperationData op;\n> > +\top.operation = RKISP1_IPA_ACTION_QUEUE_BUFFER;\n> > +\top.data = { frame, type, id };\n> > +\n> > +\tqueueFrameAction.emit(op);\n> > +}\n> > +\n> > +void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)\n> > +{\n> > +\tIPAOperationData op;\n> > +\top.operation = RKISP1_IPA_ACTION_METADATA;\n> > +\top.data = { frame, aeState };\n\nAs metadata are handled as controls, should we pass the aeState through\ncontrols instead of data ?\n\n> > +\n> > +\tqueueFrameAction.emit(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 */\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\nNo need for a license here.\n\n> > +endforeach\n> > +\n> \n> Do you need this or could you just create the rkisp1 shared_module\n> outside of the for loop ?\n\nWe could even already move the IPA to a subdirectory.\n\n> >  config_h.set('IPA_MODULE_DIR',\n> >               '\"' + join_paths(get_option('prefix'), ipa_install_dir) + '\"')","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 3D32D60BC6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  9 Oct 2019 22:37:29 +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 A0EB04FF;\n\tWed,  9 Oct 2019 22:37:28 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570653448;\n\tbh=0f6m1+Tl9qk3CTrgdhq0rMNU4Kgbm70jl8lsxm1qerk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oqTd5rgka7Y+U8qI+ffdIdxj+rosNUMMpnAeZXB+s2NFnPY3D9PIirH9O4OyDBJIs\n\tjzLuYtFAPueWVXK+yuRa58jUEPRwPVfXtTAnliuMCaJ0A+7K/UVZuPRj4oy0/l+5A/\n\th5weEMtoSGjsM9SNmntMkPE3dswZLNa8iKiIemM4=","Date":"Wed, 9 Oct 2019 23:37:26 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20191009203726.GQ22998@pendragon.ideasonboard.com>","References":"<20191008004534.1585142-1-niklas.soderlund@ragnatech.se>\n\t<20191008004534.1585142-10-niklas.soderlund@ragnatech.se>\n\t<20191008112152.ihvkconurg5lr2yj@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191008112152.ihvkconurg5lr2yj@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 09/10] 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":"Wed, 09 Oct 2019 20:37:29 -0000"}}]