[{"id":1590,"web_url":"https://patchwork.libcamera.org/comment/1590/","msgid":"<20190511132944.GH4946@pendragon.ideasonboard.com>","date":"2019-05-11T13:29:44","subject":"Re: [libcamera-devel] [PATCH v3 09/11] libcamera: pipeline_handler:\n\tKeep track of MediaDevice","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 Sat, May 11, 2019 at 11:19:05AM +0200, Niklas Söderlund wrote:\n> Instead of requiring each pipeline handle implementation to keep track\n> of calling release() on its media devices upon deletion keep track of\n\ns/deletion/deletion,/\n\n> them in the base class. Add a helper which pipeline handlers shall use\n\ns/which/tat/\n\n> to acquire a media device instead of directly interacting with the\n> DeviceEnumerator.\n> \n> This also means that pipeline handler implementations do no need to keep\n> a shared_ptr<> reference to the media devices it stores locally as the\n\ns/it stores/they store/\n\n> PipelineHandler base class will keep a shared_ptr<> reference to all\n> media devices consumed for the entire lifetime of the pipeline handler\n> implementation.\n> \n> Centrally keeping track of media devices will also be beneficial\n> implementing pipeline exclusive access across processes.\n\ns/implementing pipeline exclusive access/to implement exclusive access to pipelines/\n\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/include/pipeline_handler.h |  4 +++\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 30 +++++----------------\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 ++++-------\n>  src/libcamera/pipeline/uvcvideo.cpp      | 24 +++++------------\n>  src/libcamera/pipeline/vimc.cpp          | 22 +++++-----------\n>  src/libcamera/pipeline_handler.cpp       | 33 ++++++++++++++++++++++++\n>  6 files changed, 62 insertions(+), 66 deletions(-)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 5830e53108faa105..4c13496246c2251c 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -22,6 +22,7 @@ class Camera;\n>  class CameraConfiguration;\n>  class CameraManager;\n>  class DeviceEnumerator;\n> +class DeviceMatch;\n>  class MediaDevice;\n>  class PipelineHandler;\n>  class Request;\n> @@ -53,6 +54,8 @@ public:\n>  \tvirtual ~PipelineHandler();\n>  \n>  \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> +\tMediaDevice *tryAcquire(DeviceEnumerator *enumerator,\n> +\t\t\t\tconst DeviceMatch &dm);\n>  \n>  \tvirtual CameraConfiguration\n>  \tstreamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0;\n> @@ -84,6 +87,7 @@ private:\n>  \tvoid mediaDeviceDisconnected(MediaDevice *media);\n>  \tvirtual void disconnect();\n>  \n> +\tstd::vector<std::shared_ptr<MediaDevice>> mediaDevices_;\n>  \tstd::vector<std::weak_ptr<Camera>> cameras_;\n>  \tstd::map<const Camera *, std::unique_ptr<CameraData>> cameraData_;\n>  };\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 8a6a0e2721955d15..f1e04db1707a0335 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -149,7 +149,6 @@ class PipelineHandlerIPU3 : public PipelineHandler\n>  {\n>  public:\n>  \tPipelineHandlerIPU3(CameraManager *manager);\n> -\t~PipelineHandlerIPU3();\n>  \n>  \tCameraConfiguration\n>  \tstreamConfiguration(Camera *camera,\n> @@ -201,8 +200,8 @@ private:\n>  \n>  \tImgUDevice imgu0_;\n>  \tImgUDevice imgu1_;\n> -\tstd::shared_ptr<MediaDevice> cio2MediaDev_;\n> -\tstd::shared_ptr<MediaDevice> imguMediaDev_;\n> +\tMediaDevice *cio2MediaDev_;\n> +\tMediaDevice *imguMediaDev_;\n>  };\n>  \n>  PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n> @@ -210,15 +209,6 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n>  {\n>  }\n>  \n> -PipelineHandlerIPU3::~PipelineHandlerIPU3()\n> -{\n> -\tif (cio2MediaDev_)\n> -\t\tcio2MediaDev_->release();\n> -\n> -\tif (imguMediaDev_)\n> -\t\timguMediaDev_->release();\n> -}\n> -\n>  CameraConfiguration\n>  PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n>  \t\t\t\t\t const std::vector<StreamUsage> &usages)\n> @@ -614,20 +604,14 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>  \timgu_dm.add(\"ipu3-imgu 1 viewfinder\");\n>  \timgu_dm.add(\"ipu3-imgu 1 3a stat\");\n>  \n> -\tcio2MediaDev_ = enumerator->search(cio2_dm);\n> +\tcio2MediaDev_ = tryAcquire(enumerator, cio2_dm);\n>  \tif (!cio2MediaDev_)\n>  \t\treturn false;\n>  \n> -\tif (!cio2MediaDev_->acquire())\n> -\t\treturn false;\n> -\n> -\timguMediaDev_ = enumerator->search(imgu_dm);\n> +\timguMediaDev_ = tryAcquire(enumerator, imgu_dm);\n>  \tif (!imguMediaDev_)\n>  \t\treturn false;\n>  \n> -\tif (!imguMediaDev_->acquire())\n> -\t\treturn false;\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> @@ -682,11 +666,11 @@ int PipelineHandlerIPU3::registerCameras()\n>  {\n>  \tint ret;\n>  \n> -\tret = imgu0_.init(imguMediaDev_.get(), 0);\n> +\tret = imgu0_.init(imguMediaDev_, 0);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tret = imgu1_.init(imguMediaDev_.get(), 1);\n> +\tret = imgu1_.init(imguMediaDev_, 1);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> @@ -705,7 +689,7 @@ int PipelineHandlerIPU3::registerCameras()\n>  \t\t};\n>  \t\tCIO2Device *cio2 = &data->cio2_;\n>  \n> -\t\tret = cio2->init(cio2MediaDev_.get(), id);\n> +\t\tret = cio2->init(cio2MediaDev_, id);\n>  \t\tif (ret)\n>  \t\t\tcontinue;\n>  \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index b94d742dd6ec33a4..761d144f514e110f 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -81,7 +81,7 @@ private:\n>  \tint createCamera(MediaEntity *sensor);\n>  \tvoid bufferReady(Buffer *buffer);\n>  \n> -\tstd::shared_ptr<MediaDevice> media_;\n> +\tMediaDevice *media_;\n>  \tV4L2Subdevice *dphy_;\n>  \tV4L2Subdevice *isp_;\n>  \tV4L2Device *video_;\n> @@ -100,9 +100,6 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n>  \tdelete video_;\n>  \tdelete isp_;\n>  \tdelete dphy_;\n> -\n> -\tif (media_)\n> -\t\tmedia_->release();\n>  }\n>  \n>  /* -----------------------------------------------------------------------------\n> @@ -355,24 +352,22 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>  \tdm.add(\"rkisp1-input-params\");\n>  \tdm.add(\"rockchip-sy-mipi-dphy\");\n>  \n> -\tmedia_ = enumerator->search(dm);\n> +\tmedia_ = tryAcquire(enumerator, dm);\n>  \tif (!media_)\n>  \t\treturn false;\n>  \n> -\tmedia_->acquire();\n> -\n>  \t/* Create the V4L2 subdevices we will need. */\n> -\tdphy_ = V4L2Subdevice::fromEntityName(media_.get(),\n> +\tdphy_ = V4L2Subdevice::fromEntityName(media_,\n>  \t\t\t\t\t      \"rockchip-sy-mipi-dphy\");\n\nYou can now remove the line break.\n\n>  \tif (dphy_->open() < 0)\n>  \t\treturn false;\n>  \n> -\tisp_ = V4L2Subdevice::fromEntityName(media_.get(), \"rkisp1-isp-subdev\");\n> +\tisp_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1-isp-subdev\");\n>  \tif (isp_->open() < 0)\n>  \t\treturn false;\n>  \n>  \t/* Locate and open the capture video node. */\n> -\tvideo_ = V4L2Device::fromEntityName(media_.get(), \"rkisp1_mainpath\");\n> +\tvideo_ = V4L2Device::fromEntityName(media_, \"rkisp1_mainpath\");\n>  \tif (video_->open() < 0)\n>  \t\treturn false;\n>  \n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index e40b052f4d877d5d..bc4b4ec76a2d869c 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -24,7 +24,6 @@ class PipelineHandlerUVC : public PipelineHandler\n>  {\n>  public:\n>  \tPipelineHandlerUVC(CameraManager *manager);\n> -\t~PipelineHandlerUVC();\n>  \n>  \tCameraConfiguration\n>  \tstreamConfiguration(Camera *camera,\n> @@ -70,20 +69,13 @@ private:\n>  \t\t\tPipelineHandler::cameraData(camera));\n>  \t}\n>  \n\nYou can remove this blank line too.\n\n> -\tstd::shared_ptr<MediaDevice> media_;\n>  };\n>  \n>  PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)\n> -\t: PipelineHandler(manager), media_(nullptr)\n> +\t: PipelineHandler(manager)\n>  {\n>  }\n>  \n> -PipelineHandlerUVC::~PipelineHandlerUVC()\n> -{\n> -\tif (media_)\n> -\t\tmedia_->release();\n> -}\n> -\n>  CameraConfiguration\n>  PipelineHandlerUVC::streamConfiguration(Camera *camera,\n>  \t\t\t\t\tconst std::vector<StreamUsage> &usages)\n> @@ -177,19 +169,17 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n>  \n>  bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>  {\n> +\tMediaDevice *media;\n>  \tDeviceMatch dm(\"uvcvideo\");\n>  \n> -\tmedia_ = enumerator->search(dm);\n> -\tif (!media_)\n> -\t\treturn false;\n> -\n> -\tif (!media_->acquire())\n> +\tmedia = tryAcquire(enumerator, dm);\n> +\tif (!media)\n>  \t\treturn false;\n>  \n>  \tstd::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>(this);\n>  \n>  \t/* Locate and open the default video node. */\n> -\tfor (MediaEntity *entity : media_->entities()) {\n> +\tfor (MediaEntity *entity : media->entities()) {\n>  \t\tif (entity->flags() & MEDIA_ENT_FL_DEFAULT) {\n>  \t\t\tdata->video_ = new V4L2Device(entity);\n>  \t\t\tbreak;\n> @@ -208,11 +198,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>  \n>  \t/* Create and register the camera. */\n>  \tstd::set<Stream *> streams{ &data->stream_ };\n> -\tstd::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);\n> +\tstd::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);\n>  \tregisterCamera(std::move(camera), std::move(data));\n>  \n>  \t/* Enable hot-unplug notifications. */\n> -\thotplugMediaDevice(media_.get());\n> +\thotplugMediaDevice(media);\n>  \n>  \treturn true;\n>  }\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 7b6ebd4cc0a27e25..730a4c2829c10efe 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -24,7 +24,6 @@ class PipelineHandlerVimc : public PipelineHandler\n>  {\n>  public:\n>  \tPipelineHandlerVimc(CameraManager *manager);\n> -\t~PipelineHandlerVimc();\n>  \n>  \tCameraConfiguration\n>  \tstreamConfiguration(Camera *camera,\n> @@ -69,21 +68,13 @@ private:\n>  \t\treturn static_cast<VimcCameraData *>(\n>  \t\t\tPipelineHandler::cameraData(camera));\n>  \t}\n> -\n> -\tstd::shared_ptr<MediaDevice> media_;\n>  };\n>  \n>  PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)\n> -\t: PipelineHandler(manager), media_(nullptr)\n> +\t: PipelineHandler(manager)\n>  {\n>  }\n>  \n> -PipelineHandlerVimc::~PipelineHandlerVimc()\n> -{\n> -\tif (media_)\n> -\t\tmedia_->release();\n> -}\n> -\n>  CameraConfiguration\n>  PipelineHandlerVimc::streamConfiguration(Camera *camera,\n>  \t\t\t\t\t const std::vector<StreamUsage> &usages)\n> @@ -177,6 +168,8 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n>  \n>  bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n>  {\n> +\tMediaDevice *media;\n> +\n>  \tDeviceMatch dm(\"vimc\");\n>  \n>  \tdm.add(\"Raw Capture 0\");\n> @@ -189,17 +182,14 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n>  \tdm.add(\"RGB/YUV Input\");\n>  \tdm.add(\"Scaler\");\n>  \n> -\tmedia_ = enumerator->search(dm);\n> -\tif (!media_)\n> -\t\treturn false;\n> -\n> -\tif (!media_->acquire())\n\nI would declare the media variable right here.\n\n> +\tmedia = tryAcquire(enumerator, dm);\n> +\tif (!media)\n>  \t\treturn false;\n>  \n>  \tstd::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this);\n>  \n>  \t/* Locate and open the capture video node. */\n> -\tdata->video_ = new V4L2Device(media_->getEntityByName(\"Raw Capture 1\"));\n> +\tdata->video_ = new V4L2Device(media->getEntityByName(\"Raw Capture 1\"));\n>  \tif (data->video_->open())\n>  \t\treturn false;\n>  \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 4ecd6c49efd8ca89..8f50ef51f0c23301 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -11,6 +11,7 @@\n>  #include <libcamera/camera.h>\n>  #include <libcamera/camera_manager.h>\n>  \n> +#include \"device_enumerator.h\"\n>  #include \"log.h\"\n>  #include \"media_device.h\"\n>  #include \"utils.h\"\n> @@ -116,6 +117,8 @@ PipelineHandler::PipelineHandler(CameraManager *manager)\n>  \n>  PipelineHandler::~PipelineHandler()\n>  {\n> +\tfor (std::shared_ptr<MediaDevice> media : mediaDevices_)\n> +\t\tmedia->release();\n>  };\n>  \n>  /**\n> @@ -149,6 +152,36 @@ PipelineHandler::~PipelineHandler()\n>   * created, or false otherwise\n>   */\n>  \n> +/**\n> + * \\brief Try to acquire a MediaDevice from a device pattern\n\n\"Search and acquire a MediDevice matching a device pattern\" ?\n\n> + * \\param[in] enumerator Enumerator containing all media devices in the system\n> + * \\param[in] dm Device match pattern\n> + *\n> + * Search in the enumerated media devices that are not already in use for a\n> + * match described in \\a dm. If a match is found acquire it and store it for\n> + * release when the pipeline handler is destroyed.\n\n * Search the device \\a enumerator for an available media device\n * matching the device match pattern \\a dm. Matching media device that\n * have previously been acquired by MediaDevice::acquire() are not\n * considered. If a match is found, the media device is acquired and\n * returned. The caller shall not release the device explicitly, it will\n * be automatically released when the pipeline handler is destroyed.\n\n> + *\n> + * \\return pointer to the matching MediaDevice, or nullptr if no match is found\n\ns/pointer/A pointer/\n\n> + */\n> +MediaDevice *PipelineHandler::tryAcquire(DeviceEnumerator *enumerator,\n> +\t\t\t\t\t const DeviceMatch &dm)\n\nThere are two things that bother me here. The first one is the the\nenumerator needs to be passed explicitly to this function. We could\nstore it somewhere internally, but it doesn't feel very clean, so I\nthink I can live with this (but if you think of a nice way to do so,\nplease tell). The second is the name of the function, it should be\ncalled tryAcquireMediaDevice(). This is a bit long, so maybe\nacquireMediaDevice() would be better ? I'm open to alternative name\nsuggestions, but the function name should make the purpose more\nexplicit.\n\n> +{\n> +\tstd::shared_ptr<MediaDevice> media;\n> +\n\nThis blank line is optional.\n\n> +\tmedia = enumerator->search(dm);\n> +\tif (!media)\n> +\t\tgoto out;\n> +\n> +\tif (!media->acquire()) {\n> +\t\tmedia.reset();\n> +\t\tgoto out;\n\nMaybe just return nullptr here and aboe and remove the out label ?\n\n> +\t}\n> +\n> +\tmediaDevices_.push_back(media);\n> +out:\n> +\treturn media.get();\n> +}\n> +\n>  /**\n>   * \\fn PipelineHandler::streamConfiguration()\n>   * \\brief Retrieve a group of stream configurations for a specified camera","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 8CB8C60E4F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 11 May 2019 15:30:01 +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 0009BD5;\n\tSat, 11 May 2019 15:30:00 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557581401;\n\tbh=PVCehPXhId0CMuirz/0oX1Y3bZD7kk1Q2/XXWs2aBYs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pp/zY6pL4+sUjiI7EwPU+Ojs7djVzqVSrA8tvhIqE2f9mrBk7FiBT+KmUd08WA51n\n\t+aWSOT30QrXRh6T9wp0hVc2eohZ+K19OPfGJrGdo4OibQsxme2UbIVhy647my1hbaj\n\tvADK0ETCnhG+kKmpTFCdZTtTxCtT6P31pJRb7lfk=","Date":"Sat, 11 May 2019 16:29:44 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190511132944.GH4946@pendragon.ideasonboard.com>","References":"<20190511091907.10050-1-niklas.soderlund@ragnatech.se>\n\t<20190511091907.10050-10-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":"<20190511091907.10050-10-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 09/11] libcamera: pipeline_handler:\n\tKeep track of MediaDevice","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":"Sat, 11 May 2019 13:30:01 -0000"}}]