[{"id":204,"web_url":"https://patchwork.libcamera.org/comment/204/","msgid":"<20190104164954.GP22790@bigcity.dyn.berto.se>","date":"2019-01-04T16:49:54","subject":"Re: [libcamera-devel] [PATCH 5/5] test: MediaDevice: Add link\n\texercize test","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2019-01-03 18:38:59 +0100, Jacopo Mondi wrote:\n> Add test function to exercize the link handling abilities of the media\n> device.\n\nI have not reviewed the entire patch yet but this coughs my eye :-)\n\ns/exercize/exercise/\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> test/media_device/media_device_test.cpp | 101 ++++++++++++++++++++++++\n> 1 file changed, 101 insertions(+)\n> \n> diff --git a/test/media_device/media_device_test.cpp b/test/media_device/media_device_test.cpp\n> index c482b2e..99da3a6 100644\n> --- a/test/media_device/media_device_test.cpp\n> +++ b/test/media_device/media_device_test.cpp\n> @@ -42,8 +42,104 @@ private:\n> \tvoid printMediaGraph(const MediaDevice &media, ostream &os);\n> \tvoid printLinkFlags(const MediaLink *link, ostream &os);\n> \tvoid printNode(const MediaPad *pad, ostream &os);\n> +\n> +\tint exercizeLinks(const MediaDevice &media);\n> +\tint testLink(const MediaDevice &media, MediaLink *link);\n> };\n> \n> +int MediaDeviceTest::testLink(const MediaDevice &media, MediaLink *link)\n> +{\n> +\tMediaPad *sourcePad = link->source();\n> +\tMediaEntity *source = sourcePad->entity();\n> +\tMediaPad *sinkPad = link->sink();\n> +\tMediaEntity *sink = sinkPad->entity();\n> +\n> +\tcerr << \"Test link handling interface on link: \"\n> +\t << source->name() << \":\" << sourcePad->index()\n> +\t << \" -> \" << sink->name() << \":\" << sinkPad->index() << \"\\n\";\n> +\n> +\t/* Test the link() functions to be consistent. */\n> +\tMediaLink *alink = media.link(source->name(), sourcePad->index(),\n> +\t\t\t\t sink->name(), sinkPad->index());\n> +\tif (link != alink)\n> +\t\treturn -EINVAL;\n> +\n> +\talink = media.link(source, sourcePad->index(),\n> +\t\t\t sink, sinkPad->index());\n> +\tif (link != alink)\n> +\t\treturn -EINVAL;\n> +\n> +\talink = media.link(sourcePad, sinkPad);\n> +\tif (link != alink)\n> +\t\treturn -EINVAL;\n> +\n> +\t/* Fine, we get consisten results... now try to manipulate the link. */\n> +\tint ret = link->enable(true);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = link->enable(false);\n> +\tif (ret) {\n> +\t\tif (!(link->flags() & MEDIA_LNK_FL_IMMUTABLE))\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +\n> +}\n> +\n> +/*\n> + * Exercize the link handling interface.\n> + * Try to get existing and non-existing links, and try to enable\n> + * disable link.\n> + *\n> + * WARNING: this test will change the link between pads on the media\n> + * device it runs on, potentially modying the behavior of the system\n> + * where the test is run on.\n> + */\n> +int MediaDeviceTest::exercizeLinks(const MediaDevice &media)\n> +{\n> +\tauto entities = media.entities();\n> +\n> +\t/* First of all, reset all links in the media graph. */\n> +\tfor (MediaEntity *ent : entities) {\n> +\t\tcerr << \"Disable all links in entity: \" << ent->name() << \"\\n\";\n> +\n> +\t\tfor (MediaPad *pad : ent->pads()) {\n> +\t\t\tif (pad->flags() & MEDIA_PAD_FL_SINK)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tfor (MediaLink *link : pad->links()) {\n> +\t\t\t\tint ret = link->enable(false);\n> +\t\t\t\tif (ret) {\n> +\t\t\t\t\tif (!(link->flags() &\n> +\t\t\t\t\t MEDIA_LNK_FL_IMMUTABLE))\n> +\t\t\t\t\t\treturn ret;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\n> +\t/*\n> +\t * Exercize the link handling interface.\n> +\t */\n> +\tfor (MediaEntity *ent : entities) {\n> +\t\tfor (MediaPad *pad : ent->pads()) {\n> +\t\t\tif (pad->flags() & MEDIA_PAD_FL_SINK)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tfor (MediaLink *link : pad->links()) {\n> +\t\t\t\tint ret = testLink(media, link);\n> +\t\t\t\tif (ret)\n> +\t\t\t\t\treturn ret;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> void MediaDeviceTest::printNode(const MediaPad *pad, ostream &os)\n> {\n> \tconst MediaEntity *entity = pad->entity();\n> @@ -136,6 +232,11 @@ int MediaDeviceTest::testMediaDevice(const string devnode)\n> \n> \t/* Run tests in sequence. */\n> \tprintMediaGraph(dev, cerr);\n> +\n> +\tret = exercizeLinks(dev);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \t/* TODO: add more tests here. */\n> \n> \tdev.close();\n> -- \n> 2.20.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E30760B0B\n\tfor ;\n\tFri, 4 Jan 2019 17:49:56 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id g11-v6so32919312ljk.3\n\tfor ;\n\tFri, 04 Jan 2019 08:49:55 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tx11sm12574921lfd.81.2019.01.04.08.49.54\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 04 Jan 2019 08:49:54 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=DLWZ3x3qrf0oXlqs5LAeQshhmxa2dc1WEXeWpqyUWoM=;\n\tb=HQKiZ7kujeRJZdluCDEwtqeTWDrMcJrdqaG9UIE1y+0sSqV56q2J92FAJwKJAwVbYl\n\tJxaCM8633nSV/bq+Q3iwFnbdcNGnhVuodFMTnVTMVD1UvQALfho1rw6g+EW/F21jKOyv\n\tTSP2tyi8dYp4DiBxDNYws4Mv/+Tc1Z3w1hkjEXcuT6MO3bLHYCCBqSCDOSh71JosPzQ3\n\tDI0LbP4/2BHAJ5q/WBEOjEVUrpJb+hXrKHusGmNiRhZNKl3bNtuxsIvMVbHdkB0WHJY3\n\tly4MOa7AXf5vhhRWvrZBYKUMxvUj8LaONLAxN+P+a7eYsU42BBB+06E6irif7NaV95Ol\n\tKD+A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=DLWZ3x3qrf0oXlqs5LAeQshhmxa2dc1WEXeWpqyUWoM=;\n\tb=mav7+xzv+XW3HXygd+G9rZnMndvv0mRlppQGj3OlPPYMkY8m3oQkMaDwt6x5vCC6y3\n\tyao2HaD8X6oCS7xaJ+tujEWlPnPaov3lYt9lOUGG1dPszM6wsqau+Jx/0xeJkd21u7Ro\n\thw9HeLWiU6B4S147JxKJ+Rn9bhakpNCAx7wzOBpYF+JR2CmLJpLbc9QbpHU4xwGF0nM7\n\tjq9ay4Esq5Ixqj4ds6l/1RLqq6qKvTMo9n6MBGnuxYAQGPf4yxnJbUwiUCsZZBW40qjZ\n\tJ0oLhUa1HqdR4fP4yjcexL1AKkA6CI+1WEL3A7bgoUxhmTsQvvwmVqyO8RmGtnVyXG20\n\txtPw==","X-Gm-Message-State":"AJcUukftcZEi8rY6nmul01DazOHRqoWnZKY37F7MnufNvf3tkoAdUbhB\n\tSausj/KX5K74hxdcMSI/PMaEW753Q5A=","X-Google-Smtp-Source":"ALg8bN4e9wMN9qWUeoeJS5O2kgTsCdDw0bjuyJmcyiwmm6eXKnzVXGlSqZK07HkVjuMpGVUPkXWdKA==","X-Received":"by 2002:a2e:3012:: with SMTP id\n\tw18-v6mr28988258ljw.75.1546620595293; \n\tFri, 04 Jan 2019 08:49:55 -0800 (PST)","Date":"Fri, 4 Jan 2019 17:49:54 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190104164954.GP22790@bigcity.dyn.berto.se>","References":"<20190103173859.22624-1-jacopo@jmondi.org>\n\t<20190103173859.22624-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190103173859.22624-6-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 5/5] test: MediaDevice: Add link\n\texercize 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, 04 Jan 2019 16:49:56 -0000"}},{"id":240,"web_url":"https://patchwork.libcamera.org/comment/240/","msgid":"<5211623.yqJrlV88oN@avalon>","date":"2019-01-07T22:15:26","subject":"Re: [libcamera-devel] [PATCH 5/5] test: MediaDevice: Add link\n\texercize test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Friday, 4 January 2019 18:49:54 EET Niklas Söderlund wrote:\n> On 2019-01-03 18:38:59 +0100, Jacopo Mondi wrote:\n> > Add test function to exercize the link handling abilities of the media\n> > device.\n> \n> I have not reviewed the entire patch yet but this coughs my eye :-)\n> \n> s/exercize/exercise/\n> \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > \n> > test/media_device/media_device_test.cpp | 101 ++++++++++++++++++++++++\n> > 1 file changed, 101 insertions(+)\n> > \n> > diff --git a/test/media_device/media_device_test.cpp\n> > b/test/media_device/media_device_test.cpp index c482b2e..99da3a6 100644\n> > --- a/test/media_device/media_device_test.cpp\n> > +++ b/test/media_device/media_device_test.cpp\n> > @@ -42,8 +42,104 @@ private:\n> > \tvoid printMediaGraph(const MediaDevice &media, ostream &os);\n> > \tvoid printLinkFlags(const MediaLink *link, ostream &os);\n> > \tvoid printNode(const MediaPad *pad, ostream &os);\n> > +\n> > +\tint exercizeLinks(const MediaDevice &media);\n> > +\tint testLink(const MediaDevice &media, MediaLink *link);\n\nI think this would be a candidate for a separate test class.\n\n> > };\n> > \n> > +int MediaDeviceTest::testLink(const MediaDevice &media, MediaLink *link)\n> > +{\n> > +\tMediaPad *sourcePad = link->source();\n> > +\tMediaEntity *source = sourcePad->entity();\n> > +\tMediaPad *sinkPad = link->sink();\n> > +\tMediaEntity *sink = sinkPad->entity();\n> > +\n> > +\tcerr << \"Test link handling interface on link: \"\n> > +\t << source->name() << \":\" << sourcePad->index()\n> > +\t << \" -> \" << sink->name() << \":\" << sinkPad->index() << \"\\n\";\n\ncerr or cout ? This should be standarized as we have different tests using a \ndifferent output.\n\nendl instead of \\n ?\n\n> > +\t/* Test the link() functions to be consistent. */\n\n\t/* Test the link() lookup functions for consistency. */\n\n> > +\tMediaLink *alink = media.link(source->name(), sourcePad->index(),\n> > +\t\t\t\t sink->name(), sinkPad->index());\n> > +\tif (link != alink)\n> > +\t\treturn -EINVAL;\n\nWhen performing multiple test please log the exact cause of error.\n\n\t\tcerr << \"Link lookup by entity name and pad index failed\";\n\n(or cout)\n\nSame for all the other cases below.\n\n> > +\n> > +\talink = media.link(source, sourcePad->index(),\n> > +\t\t\t sink, sinkPad->index());\n> > +\tif (link != alink)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\talink = media.link(sourcePad, sinkPad);\n> > +\tif (link != alink)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\t/* Fine, we get consisten results... now try to manipulate the link. */\n\n\t/* Test link state modification. */\n\n> > +\tint ret = link->enable(true);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = link->enable(false);\n> > +\tif (ret) {\n> > +\t\tif (!(link->flags() & MEDIA_LNK_FL_IMMUTABLE))\n\nHow about skipping the call when the immutable flag is set ?\n\n> > +\t\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +\n\nExtra blank linke.\n\n> > +}\n> > +\n> > +/*\n> > + * Exercize the link handling interface.\n> > + * Try to get existing and non-existing links, and try to enable\n> > + * disable link.\n> > + *\n> > + * WARNING: this test will change the link between pads on the media\n> > + * device it runs on, potentially modying the behavior of the system\n> > + * where the test is run on.\n\nI think you should make this test depend on vimc, in order to have a device \nwith a known topology, and both mutable and immutable links to test.\n\n> > + */\n> > +int MediaDeviceTest::exercizeLinks(const MediaDevice &media)\n> > +{\n> > +\tauto entities = media.entities();\n> > +\n> > +\t/* First of all, reset all links in the media graph. */\n> > +\tfor (MediaEntity *ent : entities) {\n> > +\t\tcerr << \"Disable all links in entity: \" << ent->name() << \"\\n\";\n\nendl instead of \\n ?\n\n> > +\n> > +\t\tfor (MediaPad *pad : ent->pads()) {\n> > +\t\t\tif (pad->flags() & MEDIA_PAD_FL_SINK)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tfor (MediaLink *link : pad->links()) {\n> > +\t\t\t\tint ret = link->enable(false);\n> > +\t\t\t\tif (ret) {\n> > +\t\t\t\t\tif (!(link->flags() &\n> > +\t\t\t\t\t MEDIA_LNK_FL_IMMUTABLE))\n> > +\t\t\t\t\t\treturn ret;\n> > +\t\t\t\t}\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n\nTriple nested loops... Would it make sense to add a \nMediaDevice::disableLinks() method to do this, that could internally iterate \nover a single list of links ? Even if it had to do a triple loop it would \nstill be useful to offer this as a helper to pipeline managers instead of \nforcing them all to reimplement link reset (in slightly differently buggy ways \n:-))\n\n> > +\n> > +\n\nOne blank line is enough.\n\n> > +\t/*\n> > +\t * Exercize the link handling interface.\n> > +\t */\n> > +\tfor (MediaEntity *ent : entities) {\n> > +\t\tfor (MediaPad *pad : ent->pads()) {\n> > +\t\t\tif (pad->flags() & MEDIA_PAD_FL_SINK)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tfor (MediaLink *link : pad->links()) {\n> > +\t\t\t\tint ret = testLink(media, link);\n> > +\t\t\t\tif (ret)\n> > +\t\t\t\t\treturn ret;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n\nRelying on vimc, we could use specific links here to test specific features \n(error when disabling immutable links, ability to enable and disable mutable \nlinks, ...).\n\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > void MediaDeviceTest::printNode(const MediaPad *pad, ostream &os)\n> > {\n> > \tconst MediaEntity *entity = pad->entity();\n> > @@ -136,6 +232,11 @@ int MediaDeviceTest::testMediaDevice(const string\n> > devnode)> \n> > \t/* Run tests in sequence. */\n> > \tprintMediaGraph(dev, cerr);\n> > +\n> > +\tret = exercizeLinks(dev);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > \t/* TODO: add more tests here. */\n> > \tdev.close();","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 26924600CC\n\tfor ;\n\tMon, 7 Jan 2019 23:14:21 +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 7540AE4E;\n\tMon, 7 Jan 2019 23:14:19 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546899259;\n\tbh=hEW43mgzEvEgA54mNi/neoaW12Fa2R+r9HuGt6wkbhc=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=W7ojVSL0nGYlocDOgsboQmTLWJKo6WgviWvIlK2YgUrP5iA6mLzynBPM9OcNfqaum\n\t+IFnYdIBmsFIdeTADdEwxSyvloyGLpeDZE7bLAm8pCjSeuOaKxGaDb0U9HEodOPhm6\n\tca6smpI45fWrwiKXMGWYQ/yo/qteKHFm79UmKsRk=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Tue, 08 Jan 2019 00:15:26 +0200","Message-ID":"<5211623.yqJrlV88oN@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190104164954.GP22790@bigcity.dyn.berto.se>","References":"<20190103173859.22624-1-jacopo@jmondi.org>\n\t<20190103173859.22624-6-jacopo@jmondi.org>\n\t<20190104164954.GP22790@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH 5/5] test: MediaDevice: Add link\n\texercize 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":"Mon, 07 Jan 2019 22:14:21 -0000"}}]