[{"id":374,"web_url":"https://patchwork.libcamera.org/comment/374/","msgid":"<20190116154728.GI6484@bigcity.dyn.berto.se>","date":"2019-01-16T15:47:29","subject":"Re: [libcamera-devel] [PATCH v2 5/5] libcamera: pipeline: Add Intel\n\tIPU3 pipeline","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for the updated version.\n\nOn 2019-01-16 14:59:49 +0100, Jacopo Mondi wrote:\n> Add a pipeline handler for the Intel IPU3 device.\n> \n> The pipeline handler creates a Camera for each image sensor it finds to be\n> connected to an IPU3 CSI-2 receiver, and enables the link between the two.\n> \n> Tested on Soraka, listing detected cameras on the system, verifying the\n> pipeline handler gets matched and links properly enabled.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp    | 263 ++++++++++++++++++++++++\n>  src/libcamera/pipeline/ipu3/meson.build |   3 +\n>  src/libcamera/pipeline/meson.build      |   2 +\n>  3 files changed, 268 insertions(+)\n>  create mode 100644 src/libcamera/pipeline/ipu3/ipu3.cpp\n>  create mode 100644 src/libcamera/pipeline/ipu3/meson.build\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> new file mode 100644\n> index 0000000..066e9da\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -0,0 +1,263 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipu3.cpp - Pipeline handler for Intel IPU3\n> + */\n> +\n> +#include <map>\n> +#include <vector>\n> +\n> +#include <libcamera/camera.h>\n> +\n> +#include \"device_enumerator.h\"\n> +#include \"log.h\"\n> +#include \"media_device.h\"\n> +#include \"pipeline_handler.h\"\n> +\n> +namespace libcamera {\n> +\n> +class PipelineHandlerIPU3 : public PipelineHandler\n> +{\n> +public:\n> +\tPipelineHandlerIPU3();\n> +\t~PipelineHandlerIPU3();\n> +\n> +\tbool match(DeviceEnumerator *enumerator);\n> +\n> +\tunsigned int count();\n> +\tCamera *camera(unsigned int id) final;\n> +\n> +private:\n> +\tMediaDevice *cio2_;\n> +\tMediaDevice *imgu_;\n> +\n> +\tunsigned int numCameras_;\n\nIs this member needed? Can't you retrieve the number of items in \ncamerasMap_ container?\n\n> +\tstd::map<unsigned int, Camera *> camerasMap_;\n\nDoes this need to be a map? Can't it be a vector instead? Looking at the \nusage you loop over the elements anyhow.\n\nThis is not a big deal as the camera registration is to be reworked and \nthe count() and camera() functions will go away and the camera will \nregister it self with the Camera manager.\n\n> +\n> +\tunsigned int registerCameras();\n> +};\n> +\n> +PipelineHandlerIPU3::PipelineHandlerIPU3()\n> +\t: cio2_(nullptr), imgu_(nullptr), numCameras_(0)\n> +{\n> +}\n> +\n> +PipelineHandlerIPU3::~PipelineHandlerIPU3()\n> +{\n> +\tif (cio2_)\n> +\t\tcio2_->release();\n> +\n> +\tif (imgu_)\n> +\t\timgu_->release();\n> +\n> +\tfor (auto map : camerasMap_) {\n> +\t\tCamera *camera = map.second;\n> +\t\tif (camera)\n> +\t\t\tcamera->put();\n> +\t}\n> +\n> +\tcio2_ = nullptr;\n> +\timgu_ = nullptr;\n> +\tcamerasMap_.clear();\n> +}\n> +\n> +unsigned int PipelineHandlerIPU3::count()\n> +{\n> +\treturn numCameras_;\n> +}\n> +\n> +Camera *PipelineHandlerIPU3::camera(unsigned int id)\n> +{\n> +\tif (id >= numCameras_)\n> +\t\treturn nullptr;\n> +\n> +\t/*\n> +\t * The requested camera id does not match the key index used to store\n> +\t * Camera instances in the 'camerasMap_' map.\n> +\t *\n> +\t * The 'id' argument represent the n-th valid cameras registered\n> +\t * in the system, while the indexing key is the CSI-2 receiver index\n> +\t * the camera sensor is associated to, and some receiver might have no\n> +\t * camera sensor connected.\n> +\t */\n> +\tfor (auto it = camerasMap_.begin(); it != camerasMap_.end(); ++it, --id) {\n> +\t\tif (id == 0)\n> +\t\t\treturn (*it).second;\n> +\t}\n\nIf you keep camerasMap_ as a map I think you should use any of the \nstd::map helpers to resolve the id (which is the key in the map) to the \ncamera (the value).\n\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> +{\n> +\tDeviceMatch cio2_dm(\"ipu3-cio2\");\n> +\tcio2_dm.add(\"ipu3-csi2 0\");\n> +\tcio2_dm.add(\"ipu3-cio2 0\");\n> +\tcio2_dm.add(\"ipu3-csi2 1\");\n> +\tcio2_dm.add(\"ipu3-cio2 1\");\n> +\tcio2_dm.add(\"ipu3-csi2 2\");\n> +\tcio2_dm.add(\"ipu3-cio2 2\");\n> +\tcio2_dm.add(\"ipu3-csi2 3\");\n> +\tcio2_dm.add(\"ipu3-cio2 3\");\n> +\n> +\tDeviceMatch imgu_dm(\"ipu3-imgu\");\n> +\timgu_dm.add(\"ipu3-imgu 0\");\n> +\timgu_dm.add(\"ipu3-imgu 0 input\");\n> +\timgu_dm.add(\"ipu3-imgu 0 parameters\");\n> +\timgu_dm.add(\"ipu3-imgu 0 output\");\n> +\timgu_dm.add(\"ipu3-imgu 0 viewfinder\");\n> +\timgu_dm.add(\"ipu3-imgu 0 3a stat\");\n> +\timgu_dm.add(\"ipu3-imgu 1\");\n> +\timgu_dm.add(\"ipu3-imgu 1 input\");\n> +\timgu_dm.add(\"ipu3-imgu 1 parameters\");\n> +\timgu_dm.add(\"ipu3-imgu 1 output\");\n> +\timgu_dm.add(\"ipu3-imgu 1 viewfinder\");\n> +\timgu_dm.add(\"ipu3-imgu 1 3a stat\");\n> +\n> +\tcio2_ = enumerator->search(cio2_dm);\n> +\tif (!cio2_)\n> +\t\treturn false;\n> +\n> +\timgu_ = enumerator->search(imgu_dm);\n> +\tif (!imgu_)\n> +\t\treturn false;\n> +\n> +\t/*\n> +\t * It is safe to acquire both media devices at this point as\n> +\t * DeviceEnumerator::search() skips the busy ones for us.\n> +\t */\n> +\tcio2_->acquire();\n> +\timgu_->acquire();\n> +\n> +\t/*\n> +\t * Disable all links that are enabled by default on CIO2, as camera\n> +\t * creation enables all valid links it finds.\n> +\t *\n> +\t * Close the CIO2 media device after, as links are enabled and should\n> +\t * not need to be changed after.\n> +\t */\n> +\tif (cio2_->open())\n> +\t\tgoto error_release_mdev;\n> +\n> +\tif (cio2_->disableLinks())\n> +\t\tgoto error_close_cio2;\n> +\n> +\tnumCameras_ = registerCameras();\n> +\tLOG(Debug) << \"\\\"Intel IPU3\\\" pipeline handler initialized with \"\n> +\t\t   << numCameras_ << \" cameras registered\";\n> +\n> +\tcio2_->close();\n> +\n> +\treturn true;\n> +\n> +error_close_cio2:\n> +\tcio2_->close();\n> +\n> +error_release_mdev:\n> +\tcio2_->release();\n> +\timgu_->release();\n> +\n> +\treturn false;\n> +}\n> +\n> +/*\n> + * Cameras are created associating an image sensor (represented by a\n> + * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four\n> + * CIO2 CSI-2 receivers.\n> + *\n> + * Cameras are here created and stored in the member field 'camerasMap_' map,\n> + * indexed by the id of the CSI-2 receiver they are connected, to maintain\n> + * an ordering that does not depend on the device enumeration order.\n\nIs keeping order between the cameras a issue? As we will support \nhot-plug/unplug the over all order of cameras if one looks from the \napplications point of view can be different anyhow between two listings.\n\nMaybe we should sort all cameras alphabetically before handing over a \nlist to the application to provide some sort of consistency in list \norder?\n\n> + *\n> + * The function returns the number of cameras found in the system.\n> + */\n> +unsigned int PipelineHandlerIPU3::registerCameras()\n> +{\n> +\tconst std::vector<MediaEntity *> entities = cio2_->entities();\n> +\tunsigned int numCameras = 0;\n> +\tstruct {\n> +\t\tunsigned int id;\n> +\t\tMediaEntity *csi2;\n> +\t} csi2Receivers[] = {\n> +\t\t{ 0, cio2_->getEntityByName(\"ipu3-csi2 0\") },\n> +\t\t{ 1, cio2_->getEntityByName(\"ipu3-csi2 1\") },\n> +\t\t{ 2, cio2_->getEntityByName(\"ipu3-csi2 2\") },\n> +\t\t{ 3, cio2_->getEntityByName(\"ipu3-csi2 3\") },\n> +\t};\n> +\n> +\t/*\n> +\t * For each CSI-2 receiver on the IPU3, create a Camera if an\n> +\t * image sensor is connected to it.\n> +\t */\n> +\tfor (auto csi2Receiver : csi2Receivers) {\n> +\t\tMediaEntity *csi2 = csi2Receiver.csi2;\n> +\t\tunsigned int id = csi2Receiver.id;\n> +\n> +\t\t/*\n> +\t\t * This shall not happen, as the device enumerator matched\n> +\t\t * all entities described in the cio2_dm DeviceMatch.\n> +\t\t *\n> +\t\t * As this check is basically free, better stay safe than sorry.\n> +\t\t */\n> +\t\tif (!csi2)\n> +\t\t\tcontinue;\n> +\n> +\t\tstd::vector<MediaPad *> pads = csi2->pads();\n> +\t\tMediaPad *sink;\n> +\t\tfor (MediaPad *pad : pads) {\n> +\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SINK))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/* IPU3 CSI-2 receivers have a single sink pad. */\n> +\t\t\tsink = pad;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tstd::vector<MediaLink *> links = sink->links();\n> +\t\tif (links.empty())\n> +\t\t\tcontinue;\n> +\n> +\t\t/*\n> +\t\t * Verify that the receiver is connected to a sensor, enable\n> +\t\t * the media link between the two, and create a Camera with\n> +\t\t * a unique name.\n> +\t\t *\n> +\t\t * FIXME: This supports creating a single camera per CSI-2 receiver.\n> +\t\t */\n> +\t\tfor (MediaLink *link : links) {\n> +\t\t\t/* Again, this shall not happen, but better stay safe. */\n> +\t\t\tif (!link->source())\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tMediaEntity *sensor = link->source()->entity();\n> +\t\t\tif (!sensor)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tif (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tif (link->setEnabled(true))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tstd::size_t pos = sensor->name().find(\" \");\n> +\t\t\tstd::string cameraName = sensor->name().substr(0, pos);\n> +\t\t\tcameraName += \" \" + std::to_string(id);\n> +\n> +\t\t\tcamerasMap_[id] = new Camera(cameraName);\n> +\n> +\t\t\tLOG(Info) << \"Registered Camera[\" << numCameras\n> +\t\t\t\t  << \"] \\\"\" << cameraName << \"\\\"\"\n> +\t\t\t\t  << \" connected to CSI-2 receiver \" << id;\n> +\n> +\t\t\tnumCameras++;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\treturn numCameras;\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3, \"Intel IPU3 pipeline handler\");\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build\n> new file mode 100644\n> index 0000000..0ab766a\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/ipu3/meson.build\n> @@ -0,0 +1,3 @@\n> +libcamera_sources += files([\n> +    'ipu3.cpp',\n> +])\n> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> index 615ecd2..811c075 100644\n> --- a/src/libcamera/pipeline/meson.build\n> +++ b/src/libcamera/pipeline/meson.build\n> @@ -1,3 +1,5 @@\n>  libcamera_sources += files([\n>      'vimc.cpp',\n>  ])\n> +\n> +subdir('ipu3')\n> -- \n> 2.20.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n\t[IPv6:2a00:1450:4864:20::12f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65D3760B2D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 16 Jan 2019 16:47:31 +0100 (CET)","by mail-lf1-x12f.google.com with SMTP id b20so5236541lfa.12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 16 Jan 2019 07:47:31 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tg12-v6sm1061144lja.74.2019.01.16.07.47.29\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 16 Jan 2019 07:47:29 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=GnBJwluRBUK/MnQ+wjk5duE2/vf7E8RsS8VaCQqPZLg=;\n\tb=PsJVyyR9ke5cvwHCG8i3s5paqASluyFUpSpbtSQC/kQ4e36xks4IUmlHJpkrCbFmSd\n\tL2blDKLKZ39uLzTE+gcgfZcOC0dsXKJOFiDBI3k+QBGQiHCmPSNl02UkfY79jzP18WVw\n\tnkhbfwSGhK4Iab6S4rFfIuEIqBq8Vx/1EX/p32xGjijLsez7ZmFPyGnehE9xKixI7fui\n\t5uFCZNsC0930WpwLOMK0IwgjFWrE5Xnj+epqaJ0KSlXPXthBOYEyatdC427j2ZNyrq1O\n\tc8Vs+b3ZxppWj3J+QaxMD1e74Ju0CECMMGlH5kU5mK+0v+gkmra4Szb7ptpZ7/mMZUnm\n\tNqzQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=GnBJwluRBUK/MnQ+wjk5duE2/vf7E8RsS8VaCQqPZLg=;\n\tb=NbEFPgxZYU2MJz7+/mxcNdSd/bXXDtJuX7Y1YMX9Rs1Ft2XIt09S4fKJ4LXIrl1zKM\n\tX/N3fqhoFNpWp5RLCwLj4L4S8A1cI4D/IegKld+QXsy3iIy60KLJGv7r0YkmaXzXiPGo\n\thKmB+HaCv39qrzTma/zyXkUlugan30d5P8qcJUHsgR1hqBE89JhWvLmGyfrC2Q0PrphG\n\tUoSAjU4WtAa6KCZr7IeFKKGVDX24NZIhv8/eID5Z8i2YTQ8d4A54mQv9uxoAl3+ictEg\n\tiQNWPoDmcAJSRKuzuD50fc5v8iCqO+TTHdtNVVAPFfkaAnp42keFYpGfP59w+F9ZItTC\n\tvIzQ==","X-Gm-Message-State":"AJcUukczk+04Pc7T1n7l7DxNLdcwevuA6f+l4Wu77Qzlh07Vg0eIDMq3\n\t9ctyqT1xpROyQhrTnP2ipxrVnQ==","X-Google-Smtp-Source":"ALg8bN5SvdeX0+DCVesh6xq13MWpdLMZdsHV5cVgcGwbdR1TxaYnAXEQRp6dNAh8y+Be1VjLGTMOOQ==","X-Received":"by 2002:a19:24c6:: with SMTP id\n\tk189mr7480934lfk.77.1547653650463; \n\tWed, 16 Jan 2019 07:47:30 -0800 (PST)","Date":"Wed, 16 Jan 2019 16:47:29 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190116154728.GI6484@bigcity.dyn.berto.se>","References":"<20190116135949.2097-1-jacopo@jmondi.org>\n\t<20190116135949.2097-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190116135949.2097-6-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] libcamera: pipeline: Add Intel\n\tIPU3 pipeline","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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, 16 Jan 2019 15:47:31 -0000"}},{"id":377,"web_url":"https://patchwork.libcamera.org/comment/377/","msgid":"<20190116163940.xutjkcsjywbmuhyw@uno.localdomain>","date":"2019-01-16T16:39:40","subject":"Re: [libcamera-devel] [PATCH v2 5/5] libcamera: pipeline: Add Intel\n\tIPU3 pipeline","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Jan 16, 2019 at 04:47:29PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for the updated version.\n>\n> On 2019-01-16 14:59:49 +0100, Jacopo Mondi wrote:\n> > Add a pipeline handler for the Intel IPU3 device.\n> >\n> > The pipeline handler creates a Camera for each image sensor it finds to be\n> > connected to an IPU3 CSI-2 receiver, and enables the link between the two.\n> >\n> > Tested on Soraka, listing detected cameras on the system, verifying the\n> > pipeline handler gets matched and links properly enabled.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp    | 263 ++++++++++++++++++++++++\n> >  src/libcamera/pipeline/ipu3/meson.build |   3 +\n> >  src/libcamera/pipeline/meson.build      |   2 +\n> >  3 files changed, 268 insertions(+)\n> >  create mode 100644 src/libcamera/pipeline/ipu3/ipu3.cpp\n> >  create mode 100644 src/libcamera/pipeline/ipu3/meson.build\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > new file mode 100644\n> > index 0000000..066e9da\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -0,0 +1,263 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * ipu3.cpp - Pipeline handler for Intel IPU3\n> > + */\n> > +\n> > +#include <map>\n> > +#include <vector>\n> > +\n> > +#include <libcamera/camera.h>\n> > +\n> > +#include \"device_enumerator.h\"\n> > +#include \"log.h\"\n> > +#include \"media_device.h\"\n> > +#include \"pipeline_handler.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +class PipelineHandlerIPU3 : public PipelineHandler\n> > +{\n> > +public:\n> > +\tPipelineHandlerIPU3();\n> > +\t~PipelineHandlerIPU3();\n> > +\n> > +\tbool match(DeviceEnumerator *enumerator);\n> > +\n> > +\tunsigned int count();\n> > +\tCamera *camera(unsigned int id) final;\n> > +\n> > +private:\n> > +\tMediaDevice *cio2_;\n> > +\tMediaDevice *imgu_;\n> > +\n> > +\tunsigned int numCameras_;\n>\n> Is this member needed? Can't you retrieve the number of items in\n> camerasMap_ container?\n\nI could, I don't see any benefit if not optimizing away a integer\nvariable.\n\n>\n> > +\tstd::map<unsigned int, Camera *> camerasMap_;\n>\n> Does this need to be a map? Can't it be a vector instead? Looking at the\n> usage you loop over the elements anyhow.\n>\n\nAs the comments suggest, if this is a vector, the registration order\ndepends on the entity enumeration order, which might change between\ndriver versions, the probing order, and I suspect even through reboots\nif there are timeouts involved in probing sensors.\n\nUsing a map makes sure the element are sorted according to their keys\nvalue, and in this case I'm keying them on the CSI-2 receiver index\nthey are connected to.\n\n> This is not a big deal as the camera registration is to be reworked and\n> the count() and camera() functions will go away and the camera will\n> register it self with the Camera manager.\n>\n\nI think the same problems of ordering cameras we have here will be\npresent again even if the issue is moved to the camera manager.\n\n> > +\n> > +\tunsigned int registerCameras();\n> > +};\n> > +\n> > +PipelineHandlerIPU3::PipelineHandlerIPU3()\n> > +\t: cio2_(nullptr), imgu_(nullptr), numCameras_(0)\n> > +{\n> > +}\n> > +\n> > +PipelineHandlerIPU3::~PipelineHandlerIPU3()\n> > +{\n> > +\tif (cio2_)\n> > +\t\tcio2_->release();\n> > +\n> > +\tif (imgu_)\n> > +\t\timgu_->release();\n> > +\n> > +\tfor (auto map : camerasMap_) {\n> > +\t\tCamera *camera = map.second;\n> > +\t\tif (camera)\n> > +\t\t\tcamera->put();\n> > +\t}\n> > +\n> > +\tcio2_ = nullptr;\n> > +\timgu_ = nullptr;\n> > +\tcamerasMap_.clear();\n> > +}\n> > +\n> > +unsigned int PipelineHandlerIPU3::count()\n> > +{\n> > +\treturn numCameras_;\n> > +}\n> > +\n> > +Camera *PipelineHandlerIPU3::camera(unsigned int id)\n> > +{\n> > +\tif (id >= numCameras_)\n> > +\t\treturn nullptr;\n> > +\n> > +\t/*\n> > +\t * The requested camera id does not match the key index used to store\n> > +\t * Camera instances in the 'camerasMap_' map.\n> > +\t *\n> > +\t * The 'id' argument represent the n-th valid cameras registered\n> > +\t * in the system, while the indexing key is the CSI-2 receiver index\n> > +\t * the camera sensor is associated to, and some receiver might have no\n> > +\t * camera sensor connected.\n> > +\t */\n> > +\tfor (auto it = camerasMap_.begin(); it != camerasMap_.end(); ++it, --id) {\n> > +\t\tif (id == 0)\n> > +\t\t\treturn (*it).second;\n> > +\t}\n>\n> If you keep camerasMap_ as a map I think you should use any of the\n> std::map helpers to resolve the id (which is the key in the map) to the\n> camera (the value).\n>\n\nNot sure. As the comment explains, the n-th camera in the system is\nnot always in position map[n].\n\nThat's why I have to iterate on map entries unregardles of theit key\nvalue.\n\nEg.\n\nCSI-2 index       Camera#\n        0               0\n        1               -\n        2               1\n        3               -\n\nWhen asked for camera(1) the pipeline manager will give the camera\nconnected to CSI-2 receiver 2.\n\nAgain, that's meaningful order, consistent through driver versions\n(and reboots, if that's an issue) that only depends on the device.\n\nQuestion: hot-plug/hot-remove\nSuppose we can hot-plug a camera to CSI-2 receiver #1. What now? What\nwas camera(1) is now camera(2)... So this approach as surely issues\nwith that.\n\nI also think the big question here is: how would cameras be identified\nby applications? By name? By index?\n\n> > +\n> > +\treturn nullptr;\n> > +}\n> > +\n> > +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > +{\n> > +\tDeviceMatch cio2_dm(\"ipu3-cio2\");\n> > +\tcio2_dm.add(\"ipu3-csi2 0\");\n> > +\tcio2_dm.add(\"ipu3-cio2 0\");\n> > +\tcio2_dm.add(\"ipu3-csi2 1\");\n> > +\tcio2_dm.add(\"ipu3-cio2 1\");\n> > +\tcio2_dm.add(\"ipu3-csi2 2\");\n> > +\tcio2_dm.add(\"ipu3-cio2 2\");\n> > +\tcio2_dm.add(\"ipu3-csi2 3\");\n> > +\tcio2_dm.add(\"ipu3-cio2 3\");\n> > +\n> > +\tDeviceMatch imgu_dm(\"ipu3-imgu\");\n> > +\timgu_dm.add(\"ipu3-imgu 0\");\n> > +\timgu_dm.add(\"ipu3-imgu 0 input\");\n> > +\timgu_dm.add(\"ipu3-imgu 0 parameters\");\n> > +\timgu_dm.add(\"ipu3-imgu 0 output\");\n> > +\timgu_dm.add(\"ipu3-imgu 0 viewfinder\");\n> > +\timgu_dm.add(\"ipu3-imgu 0 3a stat\");\n> > +\timgu_dm.add(\"ipu3-imgu 1\");\n> > +\timgu_dm.add(\"ipu3-imgu 1 input\");\n> > +\timgu_dm.add(\"ipu3-imgu 1 parameters\");\n> > +\timgu_dm.add(\"ipu3-imgu 1 output\");\n> > +\timgu_dm.add(\"ipu3-imgu 1 viewfinder\");\n> > +\timgu_dm.add(\"ipu3-imgu 1 3a stat\");\n> > +\n> > +\tcio2_ = enumerator->search(cio2_dm);\n> > +\tif (!cio2_)\n> > +\t\treturn false;\n> > +\n> > +\timgu_ = enumerator->search(imgu_dm);\n> > +\tif (!imgu_)\n> > +\t\treturn false;\n> > +\n> > +\t/*\n> > +\t * It is safe to acquire both media devices at this point as\n> > +\t * DeviceEnumerator::search() skips the busy ones for us.\n> > +\t */\n> > +\tcio2_->acquire();\n> > +\timgu_->acquire();\n> > +\n> > +\t/*\n> > +\t * Disable all links that are enabled by default on CIO2, as camera\n> > +\t * creation enables all valid links it finds.\n> > +\t *\n> > +\t * Close the CIO2 media device after, as links are enabled and should\n> > +\t * not need to be changed after.\n> > +\t */\n> > +\tif (cio2_->open())\n> > +\t\tgoto error_release_mdev;\n> > +\n> > +\tif (cio2_->disableLinks())\n> > +\t\tgoto error_close_cio2;\n> > +\n> > +\tnumCameras_ = registerCameras();\n> > +\tLOG(Debug) << \"\\\"Intel IPU3\\\" pipeline handler initialized with \"\n> > +\t\t   << numCameras_ << \" cameras registered\";\n> > +\n> > +\tcio2_->close();\n> > +\n> > +\treturn true;\n> > +\n> > +error_close_cio2:\n> > +\tcio2_->close();\n> > +\n> > +error_release_mdev:\n> > +\tcio2_->release();\n> > +\timgu_->release();\n> > +\n> > +\treturn false;\n> > +}\n> > +\n> > +/*\n> > + * Cameras are created associating an image sensor (represented by a\n> > + * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four\n> > + * CIO2 CSI-2 receivers.\n> > + *\n> > + * Cameras are here created and stored in the member field 'camerasMap_' map,\n> > + * indexed by the id of the CSI-2 receiver they are connected, to maintain\n> > + * an ordering that does not depend on the device enumeration order.\n>\n> Is keeping order between the cameras a issue? As we will support\n> hot-plug/unplug the over all order of cameras if one looks from the\n> applications point of view can be different anyhow between two listings.\n>\n> Maybe we should sort all cameras alphabetically before handing over a\n> list to the application to provide some sort of consistency in list\n> order?\n>\n\nBack to the big question then: how will cameras be identified by\napplications?\n\n> > + *\n> > + * The function returns the number of cameras found in the system.\n> > + */\n> > +unsigned int PipelineHandlerIPU3::registerCameras()\n> > +{\n> > +\tconst std::vector<MediaEntity *> entities = cio2_->entities();\n> > +\tunsigned int numCameras = 0;\n> > +\tstruct {\n> > +\t\tunsigned int id;\n> > +\t\tMediaEntity *csi2;\n> > +\t} csi2Receivers[] = {\n> > +\t\t{ 0, cio2_->getEntityByName(\"ipu3-csi2 0\") },\n> > +\t\t{ 1, cio2_->getEntityByName(\"ipu3-csi2 1\") },\n> > +\t\t{ 2, cio2_->getEntityByName(\"ipu3-csi2 2\") },\n> > +\t\t{ 3, cio2_->getEntityByName(\"ipu3-csi2 3\") },\n> > +\t};\n> > +\n> > +\t/*\n> > +\t * For each CSI-2 receiver on the IPU3, create a Camera if an\n> > +\t * image sensor is connected to it.\n> > +\t */\n> > +\tfor (auto csi2Receiver : csi2Receivers) {\n> > +\t\tMediaEntity *csi2 = csi2Receiver.csi2;\n> > +\t\tunsigned int id = csi2Receiver.id;\n> > +\n> > +\t\t/*\n> > +\t\t * This shall not happen, as the device enumerator matched\n> > +\t\t * all entities described in the cio2_dm DeviceMatch.\n> > +\t\t *\n> > +\t\t * As this check is basically free, better stay safe than sorry.\n> > +\t\t */\n> > +\t\tif (!csi2)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tstd::vector<MediaPad *> pads = csi2->pads();\n> > +\t\tMediaPad *sink;\n> > +\t\tfor (MediaPad *pad : pads) {\n> > +\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SINK))\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\t/* IPU3 CSI-2 receivers have a single sink pad. */\n> > +\t\t\tsink = pad;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\tstd::vector<MediaLink *> links = sink->links();\n> > +\t\tif (links.empty())\n> > +\t\t\tcontinue;\n> > +\n> > +\t\t/*\n> > +\t\t * Verify that the receiver is connected to a sensor, enable\n> > +\t\t * the media link between the two, and create a Camera with\n> > +\t\t * a unique name.\n> > +\t\t *\n> > +\t\t * FIXME: This supports creating a single camera per CSI-2 receiver.\n> > +\t\t */\n> > +\t\tfor (MediaLink *link : links) {\n> > +\t\t\t/* Again, this shall not happen, but better stay safe. */\n> > +\t\t\tif (!link->source())\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tMediaEntity *sensor = link->source()->entity();\n> > +\t\t\tif (!sensor)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tif (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tif (link->setEnabled(true))\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tstd::size_t pos = sensor->name().find(\" \");\n> > +\t\t\tstd::string cameraName = sensor->name().substr(0, pos);\n> > +\t\t\tcameraName += \" \" + std::to_string(id);\n> > +\n> > +\t\t\tcamerasMap_[id] = new Camera(cameraName);\n> > +\n> > +\t\t\tLOG(Info) << \"Registered Camera[\" << numCameras\n> > +\t\t\t\t  << \"] \\\"\" << cameraName << \"\\\"\"\n> > +\t\t\t\t  << \" connected to CSI-2 receiver \" << id;\n> > +\n> > +\t\t\tnumCameras++;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn numCameras;\n> > +}\n> > +\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3, \"Intel IPU3 pipeline handler\");\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build\n> > new file mode 100644\n> > index 0000000..0ab766a\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/ipu3/meson.build\n> > @@ -0,0 +1,3 @@\n> > +libcamera_sources += files([\n> > +    'ipu3.cpp',\n> > +])\n> > diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> > index 615ecd2..811c075 100644\n> > --- a/src/libcamera/pipeline/meson.build\n> > +++ b/src/libcamera/pipeline/meson.build\n> > @@ -1,3 +1,5 @@\n> >  libcamera_sources += files([\n> >      'vimc.cpp',\n> >  ])\n> > +\n> > +subdir('ipu3')\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 0904860C83\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 16 Jan 2019 17:39:33 +0100 (CET)","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 79BC14000A;\n\tWed, 16 Jan 2019 16:39:31 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 16 Jan 2019 17:39:40 +0100","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":"<20190116163940.xutjkcsjywbmuhyw@uno.localdomain>","References":"<20190116135949.2097-1-jacopo@jmondi.org>\n\t<20190116135949.2097-6-jacopo@jmondi.org>\n\t<20190116154728.GI6484@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"fzierckstfvl34oa\"","Content-Disposition":"inline","In-Reply-To":"<20190116154728.GI6484@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] libcamera: pipeline: Add Intel\n\tIPU3 pipeline","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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, 16 Jan 2019 16:39:33 -0000"}}]