[{"id":289,"web_url":"https://patchwork.libcamera.org/comment/289/","msgid":"<1839957.J3ASyjkBG1@avalon>","date":"2019-01-11T17:20:30","subject":"Re: [libcamera-devel] [PATCH v2 3/3] test: media_device: Add link\n\thandling test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Friday, 11 January 2019 15:27:05 EET Jacopo Mondi wrote:\n> Add a test unit that exercise link handling on the VIMC media graph.\n> \n> Reviewed-by: Niklas Söderlund \n> Signed-off-by: Jacopo Mondi \n> ---\n> test/media_device/media_device_link_test.cpp | 188 +++++++++++++++++++\n> test/media_device/meson.build | 1 +\n> 2 files changed, 189 insertions(+)\n> create mode 100644 test/media_device/media_device_link_test.cpp\n> \n> diff --git a/test/media_device/media_device_link_test.cpp\n> b/test/media_device/media_device_link_test.cpp new file mode 100644\n> index 0000000..a335a1b\n> --- /dev/null\n> +++ b/test/media_device/media_device_link_test.cpp\n> @@ -0,0 +1,188 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * media_device_link_test.cpp - Tests link handling on media devices\n> + */\n> +#include \n> +\n> +#include \"device_enumerator.h\"\n> +#include \"media_device.h\"\n> +\n> +#include \"test.h\"\n> +\n> +using namespace libcamera;\n> +using namespace std;\n> +\n> +/*\n> + * This test only runs on VIMC: exercising a known media graph makes\n> possible\n> + * to make assumptions on the expected results.\n> + *\n> + * If the VIMC module is not loaded, the test is skipped.\n\nI'd say \"If not VIMC device is found the test is skipped\" as it's not just \nabout loading the module (which can also be built in). How about expanding \nthis a bit to give some more information about vimc ?\n\n/*\n * This link test requires a vimc device in order to exercise the MediaObject\n * link handling API on a graph with a predetermined topology.\n *\n * vimc is a Media Controller kernel driver that creates virtual devices. From\n * a userspace point of view they appear as normal media controller devices,\n * but are not backed by any particular piece of hardware. They can thus be\n * used for testing purpose without depending on a particular hardware\n * platform.\n *\n * If no vimc device is found (most likely because the vimc driver is not\n * loaded) the test is skipped.\n */\n\n> + */\n> +class MediaDeviceLinkTest : public Test\n> +{\n> +\tint init()\n> +\t{\n> +\t\tenumerator_ = DeviceEnumerator::create();\n\nShould you check for !enumerator_ ?\n\n> +\t\tif (enumerator_->enumerate())\n\nYou may want to delete enumerator_ here, or possibly better use \nstd::unique_ptr<>.\n\n> +\t\t\treturn TestFail;\n> +\n> +\t\tDeviceMatch dm(\"vimc\");\n> +\t\tdev_ = enumerator_->search(dm);\n> +\t\tif (!dev_)\n> +\t\t\treturn TestSkip;\n\nHow about printing a message to explain the reason ? Same for the two failure \ncases in this function.\n\n> +\n> +\t\tdev_->acquire();\n> +\n> +\t\tif (dev_->open())\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint run()\n> +\t{\n> +\t\t/* First of all reset all links in the media graph. */\n\nHow about \"First of all disable all links in the graph to ensure we start from \na known state.\" ?\n\n> +\t\tint ret = dev_->disableLinks();\n> +\t\tif (ret)\n\nPlease print a message explaining the failure.\n\n> +\t\t\treturn TestFail;\n> +\n> +\t\t/*\n> +\t\t * Test if link can be consistently retrieved through the\n> +\t\t * different methods the media device offers.\n> +\t\t */\n> +\t\tMediaLink *link = dev_->link(\"Debayer A\", 1, \"Scaler\", 0);\n> +\t\tif (!link) {\n> +\t\t\tcerr << \"Unable to find link \\\"Debayer A\\\"[1] ->\"\n\nWould it be more readable to use single quotes instead of double quotes, to \navoid the escape sequences ?\n\n> +\t\t\t << \"\\\"Scaler\\\"[0]\" << endl\n\nMaybe adding a \"using lookup by name\" ? (and \"lookup by entity\" and \"lookup by \npad\" below)\n\n> +\t\t\t << \"This link exists in VIMC media graph\"\n\nDo you think the second sentence is needed ?\n\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tMediaEntity *source = dev_->getEntityByName(\"Debayer A\");\n> +\t\tif (!source) {\n> +\t\t\tcerr << \"Unable to find entity \\\"Debayer A\\\"\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tMediaEntity *sink = dev_->getEntityByName(\"Scaler\");\n> +\t\tif (!sink) {\n> +\t\t\tcerr << \"Unable to find entity \\\"Scaler\\\"\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tMediaLink *link2 = dev_->link(source, 1, sink, 0);\n> +\t\tif (!link2) {\n> +\t\t\tcerr << \"Unable to find link \\\"Debayer A\\\"[1] ->\"\n> +\t\t\t << \"\\\"Scaler\\\"[0]\" << endl\n> +\t\t\t << \"This link exists in VIMC media graph\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (link != link2) {\n> +\t\t\tcerr << \"The returned link does not match what expected\"\n\n\"Link lookup by name and by entity don't match\" ?\n\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tlink2 = dev_->link(source->getPadByIndex(1),\n> +\t\t\t\t sink->getPadByIndex(0));\n> +\t\tif (!link2) {\n> +\t\t\tcerr << \"Unable to find link \\\"Debayer A\\\"[1] ->\"\n> +\t\t\t << \"\\\"Scaler\\\"[0]\" << endl\n> +\t\t\t << \"This link exists in VIMC media graph\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (link != link2) {\n> +\t\t\tcerr << \"The returned link does not match what expected\"\n\n\"Link lookup by name and by pad don't match\" ?\n\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* After reset the link shall not be enabled. */\n> +\t\tif (link->flags() & MEDIA_LNK_FL_ENABLED) {\n> +\t\t\tcerr << \"Link \\\"Debayer A\\\"[1] -> \\\"Scaler\\\"[0]\"\n> +\t\t\t << \" should not be enabled after a device reset\"\n\nIs this a link that was enabled by default by the driver ? If not, is there a \nlink enabled by default that you could use instead of this one ?\n\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Enable the link and test if enabling was successful. */\n> +\t\tret = link->setEnabled(true);\n> +\t\tif (ret)\n\nPlease log the cause of failure.\n\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (!(link->flags() & MEDIA_LNK_FL_ENABLED)) {\n> +\t\t\tcerr << \"Link \\\"Debayer A\\\"[1] -> \\\"Scaler\\\"[0]\"\n\nMaybe you want a const std::string linkName(\"'Debayer A'[1] -> 'Scaler'[0]\") \ndeclared at the top to use it through the code, to avoid repeating this over \nand over ? On the other hand you lookup other links below, so you'd have to \nchange the linkName. Starting test blocks with\n\n\tlinkName = \"'Debayer A'[1] -> 'Scaler'[0]\";\n\nmaybe clarify what the code below does. Up to you.\n\n\n> +\t\t\t << \" should now be enabled\" << endl;\n\nThis sound more like a recommendation than an error to me. Maybe \"Link ... was \nenabled but is reported as disabled\" ?\n\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Disable the link and test if disabling was successful. */\n> +\t\tret = link->setEnabled(false);\n> +\t\tif (ret)\n\nMissing error message here too. I like Kieran's idea of creating a TestFail \nobject that would require a message :-)\n\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (link->flags() & MEDIA_LNK_FL_ENABLED) {\n> +\t\t\tcerr << \"Link \\\"Debayer A\\\"[1] -> \\\"Scaler\\\"[0]\"\n> +\t\t\t << \" should now be disabled\" << endl;\n\nSame as above.\n\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Try to get a non existing link. */\n> +\t\tlink = dev_->link(\"Sensor A\", 1, \"Scaler\", 0);\n> +\t\tif (link) {\n> +\t\t\tcerr << \"Link \\\"Sensor A\\\"[1] -> \\\"Scaler\\\"[0]\"\n> +\t\t\t << \" does not exist but something was returned\"\n\n\"Lookup succeeded for link ... that does not exist in the graph\" ?\n\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Now get an immutable link and try to disable it. */\n> +\t\tlink = dev_->link(\"Sensor A\", 0, \"Raw Capture 0\", 0);\n> +\t\tif (!link) {\n> +\t\t\tcerr << \"Unable to find link \\\"Sensor A\\\"[0] -> \"\n> +\t\t\t << \"\\\"Raw Capture 0\\\"[0]\" << endl\n> +\t\t\t << \"This link exists in VIMC media graph\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (!(link->flags() & MEDIA_LNK_FL_IMMUTABLE)) {\n> +\t\t\tcerr << \"Link \\\"Sensor A\\\"[0] -> \\\"Raw Capture 0\\\"[0]\"\n> +\t\t\t << \" should have been 'IMMUTABLE'\" << endl;\n\n\"should be\" ?\n\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Disabling an immutable link shall fail. */\n> +\t\tret = link->setEnabled(false);\n> +\t\tif (!ret) {\n> +\t\t\tcerr << \"Link \\\"Sensor A\\\"[0] -> \\\"Raw Capture 0\\\"[0]\"\n> +\t\t\t << \" is 'IMMUTABLE', it shouldn't be disabled\"\n\n\"Disabling immutable ... link incorrectly succeeded\" ?\n\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n\nI think one last test that enables a link, calls disableLinks() and verifies \nthat the link is disabled would be useful.\n\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tvoid cleanup()\n> +\t{\n> +\t\tdev_->close();\n> +\t\tdev_->release();\n> +\n> +\t\tdelete dev_;\n\nThe media device belongs to the enumerator, you should not delete it.\n\n> +\t\tdelete enumerator_;\n> +\t}\n> +\n> +private:\n> +\tDeviceEnumerator *enumerator_;\n> +\tMediaDevice *dev_;\n> +};\n> +\n> +TEST_REGISTER(MediaDeviceLinkTest);\n> diff --git a/test/media_device/meson.build b/test/media_device/meson.build\n> index e4bedb7..d91a022 100644\n> --- a/test/media_device/meson.build\n> +++ b/test/media_device/meson.build\n> @@ -1,5 +1,6 @@\n> media_device_tests = [\n> ['media_device_print_test', 'media_device_print_test.cpp'],\n> + ['media_device_link_test', 'media_device_link_test.cpp'],\n> ]\n> \n> foreach t : media_device_tests","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 4166860B13\n\tfor ;\n\tFri, 11 Jan 2019 18:19:20 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A0F6553E;\n\tFri, 11 Jan 2019 18:19:18 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547227158;\n\tbh=x85D6doaiw3OZD9MSajBWPhFUYJeT8YAeFEjzNZcIBg=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=lt4gd6M2CH1qwHzBs3/CRQzCpFcJqU/kYTBpCGOewo5gdVlgn3dBzWTUujmRd0Cc/\n\tSbX6znr5vDcySQEqS4JTTV5mC22HzTKZ+5UYy2LB2PmGbBfYCVSmB7plyg5b86SnQn\n\tTfiPpgsnybncAzYiYF6qy5KPVEunwggbisqB15dY=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Fri, 11 Jan 2019 19:20:30 +0200","Message-ID":"<1839957.J3ASyjkBG1@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190111132705.19329-4-jacopo@jmondi.org>","References":"<20190111132705.19329-1-jacopo@jmondi.org>\n\t<20190111132705.19329-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] test: media_device: Add link\n\thandling test","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":"Fri, 11 Jan 2019 17:19:20 -0000"}},{"id":291,"web_url":"https://patchwork.libcamera.org/comment/291/","msgid":"<20190111180609.ro4qkr74vhoma7hr@uno.localdomain>","date":"2019-01-11T18:06:09","subject":"Re: [libcamera-devel] [PATCH v2 3/3] test: media_device: Add link\n\thandling test","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Jan 11, 2019 at 07:20:30PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Friday, 11 January 2019 15:27:05 EET Jacopo Mondi wrote:\n> > Add a test unit that exercise link handling on the VIMC media graph.\n> >\n> > Reviewed-by: Niklas Söderlund \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > test/media_device/media_device_link_test.cpp | 188 +++++++++++++++++++\n> > test/media_device/meson.build | 1 +\n> > 2 files changed, 189 insertions(+)\n> > create mode 100644 test/media_device/media_device_link_test.cpp\n> >\n> > diff --git a/test/media_device/media_device_link_test.cpp\n> > b/test/media_device/media_device_link_test.cpp new file mode 100644\n> > index 0000000..a335a1b\n> > --- /dev/null\n> > +++ b/test/media_device/media_device_link_test.cpp\n> > @@ -0,0 +1,188 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * media_device_link_test.cpp - Tests link handling on media devices\n> > + */\n> > +#include \n> > +\n> > +#include \"device_enumerator.h\"\n> > +#include \"media_device.h\"\n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace libcamera;\n> > +using namespace std;\n> > +\n> > +/*\n> > + * This test only runs on VIMC: exercising a known media graph makes\n> > possible\n> > + * to make assumptions on the expected results.\n> > + *\n> > + * If the VIMC module is not loaded, the test is skipped.\n>\n> I'd say \"If not VIMC device is found the test is skipped\" as it's not just\n> about loading the module (which can also be built in). How about expanding\n> this a bit to give some more information about vimc ?\n>\n> /*\n> * This link test requires a vimc device in order to exercise the MediaObject\n> * link handling API on a graph with a predetermined topology.\n> *\n> * vimc is a Media Controller kernel driver that creates virtual devices. From\n> * a userspace point of view they appear as normal media controller devices,\n> * but are not backed by any particular piece of hardware. They can thus be\n> * used for testing purpose without depending on a particular hardware\n> * platform.\n> *\n> * If no vimc device is found (most likely because the vimc driver is not\n> * loaded) the test is skipped.\n> */\n\nSince you've written it I'll take it in :) I wouldn't have gone that\nfar in explaining what vimc is...\n\n>\n> > + */\n> > +class MediaDeviceLinkTest : public Test\n> > +{\n> > +\tint init()\n> > +\t{\n> > +\t\tenumerator_ = DeviceEnumerator::create();\n>\n> Should you check for !enumerator_ ?\n>\n> > +\t\tif (enumerator_->enumerate())\n>\n> You may want to delete enumerator_ here, or possibly better use\n> std::unique_ptr<>.\n\nGood point, it also does not need to be class member as it is used at\ninitialization time only.\n\n>\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tDeviceMatch dm(\"vimc\");\n> > +\t\tdev_ = enumerator_->search(dm);\n> > +\t\tif (!dev_)\n> > +\t\t\treturn TestSkip;\n>\n> How about printing a message to explain the reason ? Same for the two failure\n> cases in this function.\n>\n\nIndeed.\n\n> > +\n> > +\t\tdev_->acquire();\n> > +\n> > +\t\tif (dev_->open())\n> > +\t\t\treturn TestFail;\n\nThe library already logs this.\n\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tint run()\n> > +\t{\n> > +\t\t/* First of all reset all links in the media graph. */\n>\n> How about \"First of all disable all links in the graph to ensure we start from\n> a known state.\" ?\n>\n> > +\t\tint ret = dev_->disableLinks();\n> > +\t\tif (ret)\n>\n> Please print a message explaining the failure.\n>\n\nThose errors are logged by the library with proper printing of the\nioclt returned error.\n\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\t/*\n> > +\t\t * Test if link can be consistently retrieved through the\n> > +\t\t * different methods the media device offers.\n> > +\t\t */\n> > +\t\tMediaLink *link = dev_->link(\"Debayer A\", 1, \"Scaler\", 0);\n> > +\t\tif (!link) {\n> > +\t\t\tcerr << \"Unable to find link \\\"Debayer A\\\"[1] ->\"\n>\n> Would it be more readable to use single quotes instead of double quotes, to\n> avoid the escape sequences ?\n>\n\nTo me is equally readable, if you prefer to I can change this.\n\n> > +\t\t\t << \"\\\"Scaler\\\"[0]\" << endl\n>\n> Maybe adding a \"using lookup by name\" ? (and \"lookup by entity\" and \"lookup by\n> pad\" below)\n>\n> > +\t\t\t << \"This link exists in VIMC media graph\"\n>\n> Do you think the second sentence is needed ?\n>\n\nWith the comment at file beginning it is not.\n\n> > +\t\t\t << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tMediaEntity *source = dev_->getEntityByName(\"Debayer A\");\n> > +\t\tif (!source) {\n> > +\t\t\tcerr << \"Unable to find entity \\\"Debayer A\\\"\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tMediaEntity *sink = dev_->getEntityByName(\"Scaler\");\n> > +\t\tif (!sink) {\n> > +\t\t\tcerr << \"Unable to find entity \\\"Scaler\\\"\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tMediaLink *link2 = dev_->link(source, 1, sink, 0);\n> > +\t\tif (!link2) {\n> > +\t\t\tcerr << \"Unable to find link \\\"Debayer A\\\"[1] ->\"\n> > +\t\t\t << \"\\\"Scaler\\\"[0]\" << endl\n> > +\t\t\t << \"This link exists in VIMC media graph\"\n> > +\t\t\t << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (link != link2) {\n> > +\t\t\tcerr << \"The returned link does not match what expected\"\n>\n> \"Link lookup by name and by entity don't match\" ?\n>\n\nBetter, gives more context indeed.\n\n> > +\t\t\t << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tlink2 = dev_->link(source->getPadByIndex(1),\n> > +\t\t\t\t sink->getPadByIndex(0));\n> > +\t\tif (!link2) {\n> > +\t\t\tcerr << \"Unable to find link \\\"Debayer A\\\"[1] ->\"\n> > +\t\t\t << \"\\\"Scaler\\\"[0]\" << endl\n> > +\t\t\t << \"This link exists in VIMC media graph\"\n> > +\t\t\t << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (link != link2) {\n> > +\t\t\tcerr << \"The returned link does not match what expected\"\n>\n> \"Link lookup by name and by pad don't match\" ?\n>\n> > +\t\t\t << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* After reset the link shall not be enabled. */\n> > +\t\tif (link->flags() & MEDIA_LNK_FL_ENABLED) {\n> > +\t\t\tcerr << \"Link \\\"Debayer A\\\"[1] -> \\\"Scaler\\\"[0]\"\n> > +\t\t\t << \" should not be enabled after a device reset\"\n>\n> Is this a link that was enabled by default by the driver ? If not, is there a\n> link enabled by default that you could use instead of this one ?\n>\n\nIt is:\n\n- entity 5: Debayer A (2 pads, 2 links)\n type V4L2 subdev subtype Unknown flags 0\n device node name /dev/v4l-subdev2\n\tpad0: Sink\n\t\t[fmt:RGB888_1X24/640x480 field:none]\n\t\t<- \"Sensor A\":0 [ENABLED,IMMUTABLE]\n\tpad1: Source\n\t\t[fmt:RGB888_1X24/640x480 field:none]\n\t\t-> \"Scaler\":0 [ENABLED]\n\n> > +\t\t\t << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Enable the link and test if enabling was successful. */\n> > +\t\tret = link->setEnabled(true);\n> > +\t\tif (ret)\n>\n> Please log the cause of failure.\n>\n\nFailed operations on the media device are logged by the library with\nproper printout of the returned errno.\n\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tif (!(link->flags() & MEDIA_LNK_FL_ENABLED)) {\n> > +\t\t\tcerr << \"Link \\\"Debayer A\\\"[1] -> \\\"Scaler\\\"[0]\"\n>\n> Maybe you want a const std::string linkName(\"'Debayer A'[1] -> 'Scaler'[0]\")\n> declared at the top to use it through the code, to avoid repeating this over\n> and over ? On the other hand you lookup other links below, so you'd have to\n> change the linkName. Starting test blocks with\n>\n> \tlinkName = \"'Debayer A'[1] -> 'Scaler'[0]\";\n>\n> maybe clarify what the code below does. Up to you.\n\nI'll see how it looks like.\n\n>\n>\n> > +\t\t\t << \" should now be enabled\" << endl;\n>\n> This sound more like a recommendation than an error to me. Maybe \"Link ... was\n> enabled but is reported as disabled\" ?\n>\n\nOk\n\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Disable the link and test if disabling was successful. */\n> > +\t\tret = link->setEnabled(false);\n> > +\t\tif (ret)\n>\n> Missing error message here too. I like Kieran's idea of creating a TestFail\n> object that would require a message :-)\n>\n\nI see. In case where the library fails vocally already, I don't think\nit is necessary.\n\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tif (link->flags() & MEDIA_LNK_FL_ENABLED) {\n> > +\t\t\tcerr << \"Link \\\"Debayer A\\\"[1] -> \\\"Scaler\\\"[0]\"\n> > +\t\t\t << \" should now be disabled\" << endl;\n>\n> Same as above.\n>\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Try to get a non existing link. */\n> > +\t\tlink = dev_->link(\"Sensor A\", 1, \"Scaler\", 0);\n> > +\t\tif (link) {\n> > +\t\t\tcerr << \"Link \\\"Sensor A\\\"[1] -> \\\"Scaler\\\"[0]\"\n> > +\t\t\t << \" does not exist but something was returned\"\n>\n> \"Lookup succeeded for link ... that does not exist in the graph\" ?\n>\n> > +\t\t\t << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Now get an immutable link and try to disable it. */\n> > +\t\tlink = dev_->link(\"Sensor A\", 0, \"Raw Capture 0\", 0);\n> > +\t\tif (!link) {\n> > +\t\t\tcerr << \"Unable to find link \\\"Sensor A\\\"[0] -> \"\n> > +\t\t\t << \"\\\"Raw Capture 0\\\"[0]\" << endl\n> > +\t\t\t << \"This link exists in VIMC media graph\"\n> > +\t\t\t << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (!(link->flags() & MEDIA_LNK_FL_IMMUTABLE)) {\n> > +\t\t\tcerr << \"Link \\\"Sensor A\\\"[0] -> \\\"Raw Capture 0\\\"[0]\"\n> > +\t\t\t << \" should have been 'IMMUTABLE'\" << endl;\n>\n> \"should be\" ?\n>\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Disabling an immutable link shall fail. */\n> > +\t\tret = link->setEnabled(false);\n> > +\t\tif (!ret) {\n> > +\t\t\tcerr << \"Link \\\"Sensor A\\\"[0] -> \\\"Raw Capture 0\\\"[0]\"\n> > +\t\t\t << \" is 'IMMUTABLE', it shouldn't be disabled\"\n>\n> \"Disabling immutable ... link incorrectly succeeded\" ?\n>\n> > +\t\t\t << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n>\n> I think one last test that enables a link, calls disableLinks() and verifies\n> that the link is disabled would be useful.\n>\n\nIsn't this already verified by making sure a link enabled by default\nis disabled after a disableLinks() call?\n\nI can add it here though, it does not hurt.\n\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tvoid cleanup()\n> > +\t{\n> > +\t\tdev_->close();\n> > +\t\tdev_->release();\n> > +\n> > +\t\tdelete dev_;\n>\n> The media device belongs to the enumerator, you should not delete it.\n>\n\nCorrect, my bad. I wonder why the following delete didn't fail (or did\nit silently maybe)\n\nThanks\n j\n\n> > +\t\tdelete enumerator_;\n> > +\t}\n> > +\n> > +private:\n> > +\tDeviceEnumerator *enumerator_;\n> > +\tMediaDevice *dev_;\n> > +};\n> > +\n> > +TEST_REGISTER(MediaDeviceLinkTest);\n> > diff --git a/test/media_device/meson.build b/test/media_device/meson.build\n> > index e4bedb7..d91a022 100644\n> > --- a/test/media_device/meson.build\n> > +++ b/test/media_device/meson.build\n> > @@ -1,5 +1,6 @@\n> > media_device_tests = [\n> > ['media_device_print_test', 'media_device_print_test.cpp'],\n> > + ['media_device_link_test', 'media_device_link_test.cpp'],\n> > ]\n> >\n> > foreach t : media_device_tests\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n>\n>","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B42AE60B13\n\tfor ;\n\tFri, 11 Jan 2019 19:06:02 +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 relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 2295020004;\n\tFri, 11 Jan 2019 18:06:01 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 11 Jan 2019 19:06:09 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190111180609.ro4qkr74vhoma7hr@uno.localdomain>","References":"<20190111132705.19329-1-jacopo@jmondi.org>\n\t<20190111132705.19329-4-jacopo@jmondi.org>\n\t<1839957.J3ASyjkBG1@avalon>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"pqa2llrfftpdlbma\"","Content-Disposition":"inline","In-Reply-To":"<1839957.J3ASyjkBG1@avalon>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] test: media_device: Add link\n\thandling test","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":"Fri, 11 Jan 2019 18:06:02 -0000"}},{"id":292,"web_url":"https://patchwork.libcamera.org/comment/292/","msgid":"<3058487.Aa6PpMSfY4@avalon>","date":"2019-01-11T18:17:45","subject":"Re: [libcamera-devel] [PATCH v2 3/3] test: media_device: Add link\n\thandling test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Friday, 11 January 2019 20:06:09 EET Jacopo Mondi wrote:\n> On Fri, Jan 11, 2019 at 07:20:30PM +0200, Laurent Pinchart wrote:\n> > On Friday, 11 January 2019 15:27:05 EET Jacopo Mondi wrote:\n> >> Add a test unit that exercise link handling on the VIMC media graph.\n> >> \n> >> Reviewed-by: Niklas Söderlund \n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> \n> >> test/media_device/media_device_link_test.cpp | 188 +++++++++++++++++++\n> >> test/media_device/meson.build | 1 +\n> >> 2 files changed, 189 insertions(+)\n> >> create mode 100644 test/media_device/media_device_link_test.cpp\n> >> \n> >> diff --git a/test/media_device/media_device_link_test.cpp\n> >> b/test/media_device/media_device_link_test.cpp new file mode 100644\n> >> index 0000000..a335a1b\n> >> --- /dev/null\n> >> +++ b/test/media_device/media_device_link_test.cpp\n\n[snip]\n\n> >> +class MediaDeviceLinkTest : public Test\n> >> +{\n> >> +\tint init()\n> >> +\t{\n> >> +\t\tenumerator_ = DeviceEnumerator::create();\n> > \n> > Should you check for !enumerator_ ?\n> > \n> >> +\t\tif (enumerator_->enumerate())\n> > \n> > You may want to delete enumerator_ here, or possibly better use\n> > std::unique_ptr<>.\n> \n> Good point, it also does not need to be class member as it is used at\n> initialization time only.\n\nExcept that it owns the media device pointers, so it has to stay alive until \nthe end of the test.\n\n> >> +\t\t\treturn TestFail;\n> >> +\n> >> +\t\tDeviceMatch dm(\"vimc\");\n> >> +\t\tdev_ = enumerator_->search(dm);\n> >> +\t\tif (!dev_)\n> >> +\t\t\treturn TestSkip;\n> > \n> > How about printing a message to explain the reason ? Same for the two\n> > failure cases in this function.\n> \n> Indeed.\n> \n> >> +\n> >> +\t\tdev_->acquire();\n> >> +\n> >> +\t\tif (dev_->open())\n> >> +\t\t\treturn TestFail;\n> \n> The library already logs this.\n> \n> >> +\n> >> +\t\treturn 0;\n> >> +\t}\n> >> +\n> > > +\tint run()\n> >> +\t{\n> >> +\t\t/* First of all reset all links in the media graph. */\n> > \n> > How about \"First of all disable all links in the graph to ensure we start\n> > from a known state.\" ?\n> > \n> >> +\t\tint ret = dev_->disableLinks();\n> >> +\t\tif (ret)\n> > \n> > Please print a message explaining the failure.\n> \n> Those errors are logged by the library with proper printing of the\n> ioclt returned error.\n\nShouldn't a message still be printed here ? Errors in the library don't \nnecessarily translate to test failures (think about Kieran's double open test \nthat succeeds if the second open call fails), so it would make sense to keep \nthe two separate. The library log could also be directed to a file, and I \nthink it would then make sense for the test output to be self-contained.\n\n> >> +\t\t\treturn TestFail;\n> >> +\n> >> +\t\t/*\n> >> +\t\t * Test if link can be consistently retrieved through the\n> >> +\t\t * different methods the media device offers.\n> >> +\t\t */\n> >> +\t\tMediaLink *link = dev_->link(\"Debayer A\", 1, \"Scaler\", 0);\n> >> +\t\tif (!link) {\n> >> +\t\t\tcerr << \"Unable to find link \\\"Debayer A\\\"[1] ->\"\n> > \n> > Would it be more readable to use single quotes instead of double quotes,\n> > to avoid the escape sequences ?\n> \n> To me is equally readable, if you prefer to I can change this.\n\nUp to you. I think the most important part is to decide on a syntax and stick \nto it throughout the code.\n\n> >> +\t\t\t << \"\\\"Scaler\\\"[0]\" << endl\n> > \n> > Maybe adding a \"using lookup by name\" ? (and \"lookup by entity\" and\n> > \"lookup by pad\" below)\n> > \n> >> +\t\t\t << \"This link exists in VIMC media graph\"\n> > \n> > Do you think the second sentence is needed ?\n> \n> With the comment at file beginning it is not.\n> \n> >> +\t\t\t << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tMediaEntity *source = dev_->getEntityByName(\"Debayer A\");\n> >> +\t\tif (!source) {\n> >> +\t\t\tcerr << \"Unable to find entity \\\"Debayer A\\\"\" << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tMediaEntity *sink = dev_->getEntityByName(\"Scaler\");\n> >> +\t\tif (!sink) {\n> >> +\t\t\tcerr << \"Unable to find entity \\\"Scaler\\\"\" << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tMediaLink *link2 = dev_->link(source, 1, sink, 0);\n> >> +\t\tif (!link2) {\n> >> +\t\t\tcerr << \"Unable to find link \\\"Debayer A\\\"[1] ->\"\n> >> +\t\t\t << \"\\\"Scaler\\\"[0]\" << endl\n> >> +\t\t\t << \"This link exists in VIMC media graph\"\n> >> +\t\t\t << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tif (link != link2) {\n> >> +\t\t\tcerr << \"The returned link does not match what expected\"\n> > \n> > \"Link lookup by name and by entity don't match\" ?\n> \n> Better, gives more context indeed.\n> \n> >> +\t\t\t << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tlink2 = dev_->link(source->getPadByIndex(1),\n> >> +\t\t\t\t sink->getPadByIndex(0));\n> >> +\t\tif (!link2) {\n> >> +\t\t\tcerr << \"Unable to find link \\\"Debayer A\\\"[1] ->\"\n> >> +\t\t\t << \"\\\"Scaler\\\"[0]\" << endl\n> >> +\t\t\t << \"This link exists in VIMC media graph\"\n> >> +\t\t\t << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tif (link != link2) {\n> >> +\t\t\tcerr << \"The returned link does not match what expected\"\n> > \n> > \"Link lookup by name and by pad don't match\" ?\n> > \n> >> +\t\t\t << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\t/* After reset the link shall not be enabled. */\n> >> +\t\tif (link->flags() & MEDIA_LNK_FL_ENABLED) {\n> >> +\t\t\tcerr << \"Link \\\"Debayer A\\\"[1] -> \\\"Scaler\\\"[0]\"\n> >> +\t\t\t << \" should not be enabled after a device reset\"\n> > \n> > Is this a link that was enabled by default by the driver ? If not, is\n> > there a link enabled by default that you could use instead of this one ?\n> \n> It is:\n> \n> - entity 5: Debayer A (2 pads, 2 links)\n> type V4L2 subdev subtype Unknown flags 0\n> device node name /dev/v4l-subdev2\n> \tpad0: Sink\n> \t\t[fmt:RGB888_1X24/640x480 field:none]\n> \t\t<- \"Sensor A\":0 [ENABLED,IMMUTABLE]\n> \tpad1: Source\n> \t\t[fmt:RGB888_1X24/640x480 field:none]\n> \t\t-> \"Scaler\":0 [ENABLED]\n\nPerfect :-)\n\n> >> +\t\t\t << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\t/* Enable the link and test if enabling was successful. */\n> >> +\t\tret = link->setEnabled(true);\n> >> +\t\tif (ret)\n> > \n> > Please log the cause of failure.\n> \n> Failed operations on the media device are logged by the library with\n> proper printout of the returned errno.\n> \n> >> +\t\t\treturn TestFail;\n> >> +\n> >> +\t\tif (!(link->flags() & MEDIA_LNK_FL_ENABLED)) {\n> >> +\t\t\tcerr << \"Link \\\"Debayer A\\\"[1] -> \\\"Scaler\\\"[0]\"\n> > \n> > Maybe you want a const std::string linkName(\"'Debayer A'[1] ->\n> > 'Scaler'[0]\") declared at the top to use it through the code, to avoid\n> > repeating this over and over ? On the other hand you lookup other links\n> > below, so you'd have to change the linkName. Starting test blocks with\n> > \n> > \tlinkName = \"'Debayer A'[1] -> 'Scaler'[0]\";\n> > \n> > maybe clarify what the code below does. Up to you.\n> \n> I'll see how it looks like.\n> \n> >> +\t\t\t << \" should now be enabled\" << endl;\n> > \n> > This sound more like a recommendation than an error to me. Maybe \"Link ...\n> > was enabled but is reported as disabled\" ?\n> \n> Ok\n> \n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\t/* Disable the link and test if disabling was successful. */\n> >> +\t\tret = link->setEnabled(false);\n> >> +\t\tif (ret)\n> > \n> > Missing error message here too. I like Kieran's idea of creating a\n> > TestFail object that would require a message :-)\n> \n> I see. In case where the library fails vocally already, I don't think\n> it is necessary.\n> \n> >> +\t\t\treturn TestFail;\n> >> +\n> >> +\t\tif (link->flags() & MEDIA_LNK_FL_ENABLED) {\n> >> +\t\t\tcerr << \"Link \\\"Debayer A\\\"[1] -> \\\"Scaler\\\"[0]\"\n> >> +\t\t\t << \" should now be disabled\" << endl;\n> > \n> > Same as above.\n> > \n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\t/* Try to get a non existing link. */\n> >> +\t\tlink = dev_->link(\"Sensor A\", 1, \"Scaler\", 0);\n> >> +\t\tif (link) {\n> >> +\t\t\tcerr << \"Link \\\"Sensor A\\\"[1] -> \\\"Scaler\\\"[0]\"\n> >> +\t\t\t << \" does not exist but something was returned\"\n> > \n> > \"Lookup succeeded for link ... that does not exist in the graph\" ?\n> > \n> >> +\t\t\t << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\t/* Now get an immutable link and try to disable it. */\n> >> +\t\tlink = dev_->link(\"Sensor A\", 0, \"Raw Capture 0\", 0);\n> >> +\t\tif (!link) {\n> >> +\t\t\tcerr << \"Unable to find link \\\"Sensor A\\\"[0] -> \"\n> >> +\t\t\t << \"\\\"Raw Capture 0\\\"[0]\" << endl\n> >> +\t\t\t << \"This link exists in VIMC media graph\"\n> >> +\t\t\t << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tif (!(link->flags() & MEDIA_LNK_FL_IMMUTABLE)) {\n> >> +\t\t\tcerr << \"Link \\\"Sensor A\\\"[0] -> \\\"Raw Capture 0\\\"[0]\"\n> >> +\t\t\t << \" should have been 'IMMUTABLE'\" << endl;\n> > \n> > \"should be\" ?\n> > \n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\t/* Disabling an immutable link shall fail. */\n> >> +\t\tret = link->setEnabled(false);\n> >> +\t\tif (!ret) {\n> >> +\t\t\tcerr << \"Link \\\"Sensor A\\\"[0] -> \\\"Raw Capture 0\\\"[0]\"\n> >> +\t\t\t << \" is 'IMMUTABLE', it shouldn't be disabled\"\n> > \n> > \"Disabling immutable ... link incorrectly succeeded\" ?\n> > \n> >> +\t\t\t << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> > \n> > I think one last test that enables a link, calls disableLinks() and\n> > verifies that the link is disabled would be useful.\n> \n> Isn't this already verified by making sure a link enabled by default\n> is disabled after a disableLinks() call?\n\nPartly only as the vimc device could have that link disabled already (and will \nif you run this test multiple times).\n\n> I can add it here though, it does not hurt.\n> \n> >> +\t\treturn 0;\n> >> +\t}\n> >> +\n> >> +\tvoid cleanup()\n> >> +\t{\n> >> +\t\tdev_->close();\n> >> +\t\tdev_->release();\n> >> +\n> >> +\t\tdelete dev_;\n> > \n> > The media device belongs to the enumerator, you should not delete it.\n> \n> Correct, my bad. I wonder why the following delete didn't fail (or did\n> it silently maybe)\n\nTry running the test with valgrind :-)\n\n> >> +\t\tdelete enumerator_;\n> >> +\t}\n> >> +\n> >> +private:\n> >> +\tDeviceEnumerator *enumerator_;\n> >> +\tMediaDevice *dev_;\n> >> +};\n> >> +\n> >> +TEST_REGISTER(MediaDeviceLinkTest);\n\n[snip]","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 A786360C68\n\tfor ;\n\tFri, 11 Jan 2019 19:16:35 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 063C353E;\n\tFri, 11 Jan 2019 19:16:33 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547230594;\n\tbh=zkmuW6pwJcGq/ASFf05MLJSu+blGf72YNYoc25EKmO4=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=nzQz6jb50DBvVfjJoOyAjSwJ44Oh+n5Xd5lvr3Wfgor7dL3a+cUtCxR39SZZ52phI\n\tzSpD8cVaI80ylDmyGKCZSDtYjTp12rJQCivI9TtufY6lB5yz711XpOd/3hSy8kyiRR\n\thplcTcgoskvAeWRgRJG7gKLFrixcXS5Evglkil+k=","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Fri, 11 Jan 2019 20:17:45 +0200","Message-ID":"<3058487.Aa6PpMSfY4@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190111180609.ro4qkr74vhoma7hr@uno.localdomain>","References":"<20190111132705.19329-1-jacopo@jmondi.org>\n\t<1839957.J3ASyjkBG1@avalon>\n\t<20190111180609.ro4qkr74vhoma7hr@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] test: media_device: Add link\n\thandling test","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":"Fri, 11 Jan 2019 18:16:35 -0000"}}]