[{"id":1416,"web_url":"https://patchwork.libcamera.org/comment/1416/","msgid":"<20190416224215.GN4822@pendragon.ideasonboard.com>","date":"2019-04-16T22:42:15","subject":"Re: [libcamera-devel] [RFC 02/11] libcamera: media_device: Open and\n\tclose 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 Sun, Apr 14, 2019 at 03:34:57AM +0200, Niklas Söderlund wrote:\n> Remove the need for the caller to open and close the media device when\n> populating the MediaDeivce. This is done as an effort to make the usage\n\ns/MediaDeivce/MediaDevice/\n\n> of the MediaDevice less error prone and the interface stricter.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/device_enumerator.cpp | 8 +-------\n> src/libcamera/media_device.cpp | 10 ++++++++--\n> test/media_device/media_device_print_test.cpp | 6 ------\n> test/pipeline/ipu3/ipu3_pipeline_test.cpp | 5 -----\n> 4 files changed, 9 insertions(+), 20 deletions(-)\n> \n> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> index 58b81c354a706f03..6c08806d17dbfeec 100644\n> --- a/src/libcamera/device_enumerator.cpp\n> +++ b/src/libcamera/device_enumerator.cpp\n> @@ -212,11 +212,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> @@ -243,8 +239,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 1e9024bf97217bcf..ecb00a1d5abffe80 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -220,6 +220,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> @@ -236,7 +240,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 err_out;\n\nThis seems to fix a memory leak when the second call to\nMEDIA_IOC_G_TOPOLOGY fails, that's nice. Could you mention it in the\ncommit message ?\n\n> \t\t}\n> \n> \t\tif (version == topology.topology_version)\n> @@ -260,6 +264,8 @@ int MediaDevice::populate()\n> \t populatePads(topology) &&\n> \t populateLinks(topology))\n> \t\tvalid_ = true;\n\nPlease add a blank line.\n\n> +err_out:\n\nI'd name this label done as this isn't path restricted to errors.\n\n> +\tclose();\n> \n> \tdelete[] ents;\n> \tdelete[] interfaces;\n> @@ -268,7 +274,7 @@ int MediaDevice::populate()\n> \n> \tif (!valid_) {\n> \t\tclear();\n> -\t\treturn -EINVAL;\n> +\t\treturn ret ? ret : -EINVAL;\n\nThat's not very nice. How about adding instead the following before this\ncheck ?\n\n\tif (ret)\n\t\treturn ret;\n\n> \t}\n> \n> \treturn 0;\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","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 450FC60004\n\tfor ;\n\tWed, 17 Apr 2019 00:42:24 +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 B10BDE2;\n\tWed, 17 Apr 2019 00:42:23 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555454543;\n\tbh=CSm65qrXmOrNbYpVIO+SLq5W5gknbAMP+Dtti6oboCE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h5gX8YA73wWgKz5yBmVCRHIilum1r2gxvUMJPU31zPKfEt2xp7QU7rsTxhn7nZn7F\n\tmq/43ikDNQCUEOgstp4NxOYEEfzOtwr7Fnm7sH32fTuvAv/GP7gDGuOPxXVXi/nQKM\n\t81d/wc5TUw4yvUS5xFvo2gAaL6F98wxmojq4vdjo=","Date":"Wed, 17 Apr 2019 01:42:15 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190416224215.GN4822@pendragon.ideasonboard.com>","References":"<20190414013506.10515-1-niklas.soderlund@ragnatech.se>\n\t<20190414013506.10515-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":"<20190414013506.10515-3-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 02/11] libcamera: media_device: Open and\n\tclose 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":"Tue, 16 Apr 2019 22:42:24 -0000"}}]