[{"id":1607,"web_url":"https://patchwork.libcamera.org/comment/1607/","msgid":"<20190517092207.GB4960@pendragon.ideasonboard.com>","date":"2019-05-17T09:22:07","subject":"Re: [libcamera-devel] [PATCH v4 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 Fri, May 17, 2019 at 02:54:45AM +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> them in the base class. Add a helper that pipeline handlers shall use\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 they store locally as the\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> to implement exclusive access to pipelines across processes.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> ---\n>  src/libcamera/include/pipeline_handler.h |  4 +++\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 30 ++++++----------------\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++--------\n>  src/libcamera/pipeline/uvcvideo.cpp      | 25 ++++++------------\n>  src/libcamera/pipeline/vimc.cpp          | 20 +++------------\n>  src/libcamera/pipeline_handler.cpp       | 32 ++++++++++++++++++++++++\n>  6 files changed, 59 insertions(+), 68 deletions(-)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 5830e53108faa105..8e6a136dd4907d9f 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 *acquireMediaDevice(DeviceEnumerator *enumerator,\n> +\t\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..75a70e66eacc443a 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_ = acquireMediaDevice(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_ = acquireMediaDevice(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..b395405c9ddb7f17 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,21 @@ 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_ = acquireMediaDevice(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> -\t\t\t\t\t      \"rockchip-sy-mipi-dphy\");\n> +\tdphy_ = V4L2Subdevice::fromEntityName(media_, \"rockchip-sy-mipi-dphy\");\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..351712cfdc69594d 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> @@ -69,21 +68,13 @@ private:\n>  \t\treturn static_cast<UVCCameraData *>(\n>  \t\t\tPipelineHandler::cameraData(camera));\n>  \t}\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 +168,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 = acquireMediaDevice(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 +197,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..737d6df67def59c7 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> @@ -189,17 +180,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> +\tMediaDevice *media = acquireMediaDevice(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..c92ee782701f57bf 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,35 @@ PipelineHandler::~PipelineHandler()\n>   * created, or false otherwise\n>   */\n>  \n> +/**\n> + * \\brief Search and acquire a MediDevice matching a device pattern\n> + * \\param[in] enumerator Enumerator containing all media devices in the system\n> + * \\param[in] dm Device match pattern\n> + *\n> + * Search the device \\a enumerator for an available media device matching the\n> + * device match pattern \\a dm. Matching media device that have previously been\n> + * acquired by MediaDevice::acquire() are not considered. If a match is found,\n> + * the media device is acquired and returned. The caller shall not release the\n> + * device explicitly, it will be automatically released when the pipeline\n> + * handler is destroyed.\n> + *\n> + * \\return A pointer to the matching MediaDevice, or nullptr if no match is found\n> + */\n> +MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> +\t\t\t\t\t\t const DeviceMatch &dm)\n> +{\n> +\tstd::shared_ptr<MediaDevice> media = enumerator->search(dm);\n> +\tif (!media)\n> +\t\treturn nullptr;\n> +\n> +\tif (!media->acquire())\n> +\t\treturn nullptr;\n> +\n> +\tmediaDevices_.push_back(media);\n> +\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[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC82D60C02\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 17 May 2019 11:22:23 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 366CC2FD;\n\tFri, 17 May 2019 11:22:23 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558084943;\n\tbh=z6rWUB8AAnTmTDLnoAddJqftY07Iqa+GvTSKC0fAqXo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c0jy+Iyo/OhYukbdskMIOfMO7IqFGvuhXpaUAcxrqbF+QyDvXcCnNupYLPOrcDSyK\n\tTlu9vcahUKGNHoBftOvRmvOgECZhGSQXUxvzDLxTxqNH59Z2q5o0nelkH2CQMfKe7x\n\t3GYDEue+Z9whgZqAkeQZ+btKRYFbLjbsKxP+b/Uc=","Date":"Fri, 17 May 2019 12:22:07 +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":"<20190517092207.GB4960@pendragon.ideasonboard.com>","References":"<20190517005447.27171-1-niklas.soderlund@ragnatech.se>\n\t<20190517005447.27171-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":"<20190517005447.27171-10-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 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":"Fri, 17 May 2019 09:22:24 -0000"}}]