[libcamera-devel] [PATCH v2 3/3] test: media_device: Add link handling test

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday, 11 January 2019 15:27:05 EET Jacopo Mondi wrote:
> Add a test unit that exercise link handling on the VIMC media graph.
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  test/media_device/media_device_link_test.cpp | 188 +++++++++++++++++++
>  test/media_device/meson.build                |   1 +
>  2 files changed, 189 insertions(+)
>  create mode 100644 test/media_device/media_device_link_test.cpp
> 
> diff --git a/test/media_device/media_device_link_test.cpp
> b/test/media_device/media_device_link_test.cpp new file mode 100644
> index 0000000..a335a1b
> --- /dev/null
> +++ b/test/media_device/media_device_link_test.cpp
> @@ -0,0 +1,188 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * media_device_link_test.cpp - Tests link handling on media devices
> + */
> +#include <iostream>
> +
> +#include "device_enumerator.h"
> +#include "media_device.h"
> +
> +#include "test.h"
> +
> +using namespace libcamera;
> +using namespace std;
> +
> +/*
> + * This test only runs on VIMC: exercising a known media graph makes
> possible
> + * to make assumptions on the expected results.
> + *
> + * If the VIMC module is not loaded, the test is skipped.

I'd say "If not VIMC device is found the test is skipped" as it's not just 
about loading the module (which can also be built in). How about expanding 
this a bit to give some more information about vimc ?

/*
 * This link test requires a vimc device in order to exercise the MediaObject
 * link handling API on a graph with a predetermined topology.
 *
 * vimc is a Media Controller kernel driver that creates virtual devices. From
 * a userspace point of view they appear as normal media controller devices,
 * but are not backed by any particular piece of hardware. They can thus be
 * used for testing purpose without depending on a particular hardware
 * platform.
 *
 * If no vimc device is found (most likely because the vimc driver is not
 * loaded) the test is skipped.
 */

> + */
> +class MediaDeviceLinkTest : public Test
> +{
> +	int init()
> +	{
> +		enumerator_ = DeviceEnumerator::create();

Should you check for !enumerator_ ?

> +		if (enumerator_->enumerate())

You may want to delete enumerator_ here, or possibly better use 
std::unique_ptr<>.

> +			return TestFail;
> +
> +		DeviceMatch dm("vimc");
> +		dev_ = enumerator_->search(dm);
> +		if (!dev_)
> +			return TestSkip;

How about printing a message to explain the reason ? Same for the two failure 
cases in this function.

> +
> +		dev_->acquire();
> +
> +		if (dev_->open())
> +			return TestFail;
> +
> +		return 0;
> +	}
> +
> +	int run()
> +	{
> +		/* First of all reset all links in the media graph. */

How about "First of all disable all links in the graph to ensure we start from 
a known state." ?

> +		int ret = dev_->disableLinks();
> +		if (ret)

Please print a message explaining the failure.

> +			return TestFail;
> +
> +		/*
> +		 * Test if link can be consistently retrieved through the
> +		 * different methods the media device offers.
> +		 */
> +		MediaLink *link = dev_->link("Debayer A", 1, "Scaler", 0);
> +		if (!link) {
> +			cerr << "Unable to find link \"Debayer A\"[1] ->"

Would it be more readable to use single quotes instead of double quotes, to 
avoid the escape sequences ?

> +			     << "\"Scaler\"[0]" << endl

Maybe adding a "using lookup by name" ? (and "lookup by entity" and "lookup by 
pad" below)

> +			     << "This link exists in VIMC media graph"

Do you think the second sentence is needed ?

> +			     << endl;
> +			return TestFail;
> +		}
> +
> +		MediaEntity *source = dev_->getEntityByName("Debayer A");
> +		if (!source) {
> +			cerr << "Unable to find entity \"Debayer A\"" << endl;
> +			return TestFail;
> +		}
> +
> +		MediaEntity *sink = dev_->getEntityByName("Scaler");
> +		if (!sink) {
> +			cerr << "Unable to find entity \"Scaler\"" << endl;
> +			return TestFail;
> +		}
> +
> +		MediaLink *link2 = dev_->link(source, 1, sink, 0);
> +		if (!link2) {
> +			cerr << "Unable to find link \"Debayer A\"[1] ->"
> +			     << "\"Scaler\"[0]" << endl
> +			     << "This link exists in VIMC media graph"
> +			     << endl;
> +			return TestFail;
> +		}
> +
> +		if (link != link2) {
> +			cerr << "The returned link does not match what expected"

"Link lookup by name and by entity don't match" ?

> +			     << endl;
> +			return TestFail;
> +		}
> +
> +		link2 = dev_->link(source->getPadByIndex(1),
> +				   sink->getPadByIndex(0));
> +		if (!link2) {
> +			cerr << "Unable to find link \"Debayer A\"[1] ->"
> +			     << "\"Scaler\"[0]" << endl
> +			     << "This link exists in VIMC media graph"
> +			     << endl;
> +			return TestFail;
> +		}
> +
> +		if (link != link2) {
> +			cerr << "The returned link does not match what expected"

"Link lookup by name and by pad don't match" ?

> +			     << endl;
> +			return TestFail;
> +		}
> +
> +		/* After reset the link shall not be enabled. */
> +		if (link->flags() & MEDIA_LNK_FL_ENABLED) {
> +			cerr << "Link \"Debayer A\"[1] -> \"Scaler\"[0]"
> +			     << " should not be enabled after a device reset"

Is this a link that was enabled by default by the driver ? If not, is there a 
link enabled by default that you could use instead of this one ?

> +			     << endl;
> +			return TestFail;
> +		}
> +
> +		/* Enable the link and test if enabling was successful. */
> +		ret = link->setEnabled(true);
> +		if (ret)

Please log the cause of failure.

> +			return TestFail;
> +
> +		if (!(link->flags() & MEDIA_LNK_FL_ENABLED)) {
> +			cerr << "Link \"Debayer A\"[1] -> \"Scaler\"[0]"

Maybe you want a const std::string linkName("'Debayer A'[1] -> 'Scaler'[0]") 
declared at the top to use it through the code, to avoid repeating this over 
and over ? On the other hand you lookup other links below, so you'd have to 
change the linkName. Starting test blocks with

	linkName = "'Debayer A'[1] -> 'Scaler'[0]";

maybe clarify what the code below does. Up to you.


> +			     << " should now be enabled" << endl;

This sound more like a recommendation than an error to me. Maybe "Link ... was 
enabled but is reported as disabled" ?

> +			return TestFail;
> +		}
> +
> +		/* Disable the link and test if disabling was successful. */
> +		ret = link->setEnabled(false);
> +		if (ret)

Missing error message here too. I like Kieran's idea of creating a TestFail 
object that would require a message :-)

> +			return TestFail;
> +
> +		if (link->flags() & MEDIA_LNK_FL_ENABLED) {
> +			cerr << "Link \"Debayer A\"[1] -> \"Scaler\"[0]"
> +			     << " should now be disabled" << endl;

Same as above.

> +			return TestFail;
> +		}
> +
> +		/* Try to get a non existing link. */
> +		link = dev_->link("Sensor A", 1, "Scaler", 0);
> +		if (link) {
> +			cerr << "Link \"Sensor A\"[1] -> \"Scaler\"[0]"
> +			     << " does not exist but something was returned"

"Lookup succeeded for link ... that does not exist in the graph" ?

> +			     << endl;
> +			return TestFail;
> +		}
> +
> +		/* Now get an immutable link and try to disable it. */
> +		link = dev_->link("Sensor A", 0, "Raw Capture 0", 0);
> +		if (!link) {
> +			cerr << "Unable to find link \"Sensor A\"[0] -> "
> +			     << "\"Raw Capture 0\"[0]" << endl
> +			     << "This link exists in VIMC media graph"
> +			     << endl;
> +			return TestFail;
> +		}
> +
> +		if (!(link->flags() & MEDIA_LNK_FL_IMMUTABLE)) {
> +			cerr << "Link \"Sensor A\"[0] -> \"Raw Capture 0\"[0]"
> +			     << " should have been 'IMMUTABLE'" << endl;

"should be" ?

> +			return TestFail;
> +		}
> +
> +		/* Disabling an immutable link shall fail. */
> +		ret = link->setEnabled(false);
> +		if (!ret) {
> +			cerr << "Link \"Sensor A\"[0] -> \"Raw Capture 0\"[0]"
> +			     << " is 'IMMUTABLE', it shouldn't be disabled"

"Disabling immutable ... link incorrectly succeeded" ?

> +			     << endl;
> +			return TestFail;
> +		}

I think one last test that enables a link, calls disableLinks() and verifies 
that the link is disabled would be useful.

> +		return 0;
> +	}
> +
> +	void cleanup()
> +	{
> +		dev_->close();
> +		dev_->release();
> +
> +		delete dev_;

The media device belongs to the enumerator, you should not delete it.

> +		delete enumerator_;
> +	}
> +
> +private:
> +	DeviceEnumerator *enumerator_;
> +	MediaDevice *dev_;
> +};
> +
> +TEST_REGISTER(MediaDeviceLinkTest);
> diff --git a/test/media_device/meson.build b/test/media_device/meson.build
> index e4bedb7..d91a022 100644
> --- a/test/media_device/meson.build
> +++ b/test/media_device/meson.build
> @@ -1,5 +1,6 @@
>  media_device_tests = [
>      ['media_device_print_test',         'media_device_print_test.cpp'],
> +    ['media_device_link_test',          'media_device_link_test.cpp'],
>  ]
> 
>  foreach t : media_device_tests

-- 
Regards,

Laurent Pinchart