[{"id":1584,"web_url":"https://patchwork.libcamera.org/comment/1584/","msgid":"<20190511125350.GC4946@pendragon.ideasonboard.com>","date":"2019-05-11T12:53:50","subject":"Re: [libcamera-devel] [PATCH v3 02/11] libcamera: media_device:\n\tOpen and close media device inside populate()","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:18:58AM +0200, Niklas Söderlund wrote:\n> Remove the need for the caller to open and close the media device when\n> populating the MediaDevice. This is done as an effort to make the usage\n> of the MediaDevice less error prone and the interface stricter.\n> \n> The rework also revealed and fixes a potential memory leak in\n> MediaDevice::populate() where resources would not be deleted if the\n> second MEDIA_IOC_G_TOPOLOGY would fail.\n> \n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Kieran Bingham \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/libcamera/device_enumerator.cpp | 8 +-------\n> src/libcamera/media_device.cpp | 12 ++++++++++--\n> test/media_device/media_device_print_test.cpp | 6 ------\n> test/pipeline/ipu3/ipu3_pipeline_test.cpp | 5 -----\n> test/v4l2_subdevice/v4l2_subdevice_test.cpp | 10 +---------\n> 5 files changed, 12 insertions(+), 29 deletions(-)\n> \n> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> index 259eec41776e2ec4..6fcbae76b64e3774 100644\n> --- a/src/libcamera/device_enumerator.cpp\n> +++ b/src/libcamera/device_enumerator.cpp\n> @@ -207,11 +207,7 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)\n> {\n> \tstd::shared_ptr media = std::make_shared(deviceNode);\n> \n> -\tint ret = media->open();\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n> -\tret = media->populate();\n> +\tint ret = media->populate();\n> \tif (ret < 0) {\n> \t\tLOG(DeviceEnumerator, Info)\n> \t\t\t<< \"Unable to populate media device \" << deviceNode\n> @@ -238,8 +234,6 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)\n> \t\t\treturn ret;\n> \t}\n> \n> -\tmedia->close();\n> -\n> \tLOG(DeviceEnumerator, Debug)\n> \t\t<< \"Added device \" << deviceNode << \": \" << media->driver();\n> \n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 449571fb4b78fb94..c39775164fab7487 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -221,6 +221,10 @@ int MediaDevice::populate()\n> \n> \tclear();\n> \n> +\tret = open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \t/*\n> \t * Keep calling G_TOPOLOGY until the version number stays stable.\n> \t */\n> @@ -237,7 +241,7 @@ int MediaDevice::populate()\n> \t\t\tLOG(MediaDevice, Error)\n> \t\t\t\t<< \"Failed to enumerate topology: \"\n> \t\t\t\t<< strerror(-ret);\n> -\t\t\treturn ret;\n> +\t\t\tgoto done;\n> \t\t}\n> \n> \t\tif (version == topology.topology_version)\n> @@ -262,6 +266,10 @@ int MediaDevice::populate()\n> \t populateLinks(topology))\n> \t\tvalid_ = true;\n> \n> +\tret = 0;\n> +done:\n> +\tclose();\n> +\n> \tdelete[] ents;\n> \tdelete[] interfaces;\n> \tdelete[] pads;\n> @@ -272,7 +280,7 @@ int MediaDevice::populate()\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\treturn 0;\n> +\treturn ret;\n> }\n> \n> /**\n> diff --git a/test/media_device/media_device_print_test.cpp b/test/media_device/media_device_print_test.cpp\n> index 3eef973939201b27..ceffd538e13fca73 100644\n> --- a/test/media_device/media_device_print_test.cpp\n> +++ b/test/media_device/media_device_print_test.cpp\n> @@ -124,10 +124,6 @@ int MediaDevicePrintTest::testMediaDevice(const string deviceNode)\n> \n> \tdev.close();\n> \n> -\tret = dev.open();\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> \tret = dev.populate();\n> \tif (ret)\n> \t\treturn ret;\n> @@ -135,8 +131,6 @@ int MediaDevicePrintTest::testMediaDevice(const string deviceNode)\n> \t/* Print out the media graph. */\n> \tprintMediaGraph(dev, cerr);\n> \n> -\tdev.close();\n> -\n> \treturn 0;\n> }\n> \n> diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> index 953f0233cde485cb..1d4cc4d4950b8391 100644\n> --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> @@ -71,11 +71,6 @@ int IPU3PipelineTest::init()\n> \t\treturn TestSkip;\n> \t}\n> \n> -\tif (cio2->open()) {\n> -\t\tcerr << \"Failed to open media device \" << cio2->deviceNode() << endl;\n> -\t\treturn TestFail;\n> -\t}\n> -\n> \t/*\n> \t * Camera sensor are connected to the CIO2 unit.\n> \t * Count how many sensors are connected in the system\n> diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> index 5810ef3c962fab8e..562a638cb28ebfb2 100644\n> --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> @@ -45,13 +45,6 @@ int V4L2SubdeviceTest::init()\n> \t\treturn TestSkip;\n> \t}\n> \n> -\tint ret = media_->open();\n> -\tif (ret) {\n> -\t\tcerr << \"Unable to open media device: \" << media_->deviceNode()\n> -\t\t << \": \" << strerror(ret) << endl;\n> -\t\treturn TestSkip;\n> -\t}\n> -\n> \tMediaEntity *videoEntity = media_->getEntityByName(\"Scaler\");\n> \tif (!videoEntity) {\n> \t\tcerr << \"Unable to find media entity 'Scaler'\" << endl;\n> @@ -59,8 +52,7 @@ int V4L2SubdeviceTest::init()\n> \t}\n> \n> \tscaler_ = new V4L2Subdevice(videoEntity);\n> -\tret = scaler_->open();\n> -\tif (ret) {\n> +\tif (scaler_->open()) {\n> \t\tcerr << \"Unable to open video subdevice \"\n> \t\t << scaler_->entity()->deviceNode() << endl;\n> \t\treturn TestSkip;","headers":{"Return-Path":"","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 BB4A760E4F\n\tfor ;\n\tSat, 11 May 2019 14:54:06 +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 55458D5;\n\tSat, 11 May 2019 14:54:06 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557579246;\n\tbh=XXTOlWILZTQmIRGMeyAHJiHcRu5pwFp2Li/nkuaHAg8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SdSpthIQxIbPA82J4srtrrztFZ5cOI/uux8X2mWcZbTYOb+O+kVSqaGtpYGwnURbb\n\tNPsGdt3M384Z3fTRT8kLVKlTr2Ib5UuDSbNGsCeDo6YtLf4ZuG/WdcJczm3T9DTjKx\n\t0wvaB7rM0GlL8n+muJ9/fWX579oPkx9ApQFnRlB4=","Date":"Sat, 11 May 2019 15:53:50 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190511125350.GC4946@pendragon.ideasonboard.com>","References":"<20190511091907.10050-1-niklas.soderlund@ragnatech.se>\n\t<20190511091907.10050-3-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-3-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 02/11] libcamera: media_device:\n\tOpen and close media device inside populate()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 11 May 2019 12:54:06 -0000"}}]