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

208 diff mbox series

Add a test unit that exercise link handling on the VIMC media graph.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo@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

--
2.20.1

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@ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo@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

Hi Laurent,

On Fri, Jan 11, 2019 at 07:20:30PM +0200, Laurent Pinchart wrote:
> 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@ragnatech.se>
> > Signed-off-by: Jacopo Mondi <jacopo@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.
>  */

Since you've written it I'll take it in :) I wouldn't have gone that
far in explaining what vimc is...

>
> > + */
> > +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<>.

Good point, it also does not need to be class member as it is used at
initialization time only.

>
> > +			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.
>

Indeed.

> > +
> > +		dev_->acquire();
> > +
> > +		if (dev_->open())
> > +			return TestFail;

The library already logs this.

> > +
> > +		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.
>

Those errors are logged by the library with proper printing of the
ioclt returned error.

> > +			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 ?
>

To me is equally readable, if you prefer to I can change this.

> > +			     << "\"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 ?
>

With the comment at file beginning it is not.

> > +			     << 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" ?
>

Better, gives more context indeed.

> > +			     << 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 ?
>

It is:

- entity 5: Debayer A (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev2
	pad0: Sink
		[fmt:RGB888_1X24/640x480 field:none]
		<- "Sensor A":0 [ENABLED,IMMUTABLE]
	pad1: Source
		[fmt:RGB888_1X24/640x480 field:none]
		-> "Scaler":0 [ENABLED]

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

Failed operations on the media device are logged by the library with
proper printout of the returned errno.

> > +			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.

I'll see how it looks like.

>
>
> > +			     << " 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" ?
>

Ok

> > +			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 :-)
>

I see. In case where the library fails vocally already, I don't think
it is necessary.

> > +			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.
>

Isn't this already verified by making sure a link enabled by default
is disabled after a disableLinks() call?

I can add it here though, it does not hurt.

> > +		return 0;
> > +	}
> > +
> > +	void cleanup()
> > +	{
> > +		dev_->close();
> > +		dev_->release();
> > +
> > +		delete dev_;
>
> The media device belongs to the enumerator, you should not delete it.
>

Correct, my bad. I wonder why the following delete didn't fail (or did
it silently maybe)

Thanks
  j

> > +		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
>
>
>

Hi Jacopo,

On Friday, 11 January 2019 20:06:09 EET Jacopo Mondi wrote:
> On Fri, Jan 11, 2019 at 07:20:30PM +0200, Laurent Pinchart wrote:
> > 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@ragnatech.se>
> >> Signed-off-by: Jacopo Mondi <jacopo@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

[snip]

> >> +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<>.
> 
> Good point, it also does not need to be class member as it is used at
> initialization time only.

Except that it owns the media device pointers, so it has to stay alive until 
the end of the test.

> >> +			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.
> 
> Indeed.
> 
> >> +
> >> +		dev_->acquire();
> >> +
> >> +		if (dev_->open())
> >> +			return TestFail;
> 
> The library already logs this.
> 
> >> +
> >> +		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.
> 
> Those errors are logged by the library with proper printing of the
> ioclt returned error.

Shouldn't a message still be printed here ? Errors in the library don't 
necessarily translate to test failures (think about Kieran's double open test 
that succeeds if the second open call fails), so it would make sense to keep 
the two separate. The library log could also be directed to a file, and I 
think it would then make sense for the test output to be self-contained.

> >> +			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 ?
> 
> To me is equally readable, if you prefer to I can change this.

Up to you. I think the most important part is to decide on a syntax and stick 
to it throughout the code.

> >> +			     << "\"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 ?
> 
> With the comment at file beginning it is not.
> 
> >> +			     << 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" ?
> 
> Better, gives more context indeed.
> 
> >> +			     << 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 ?
> 
> It is:
> 
> - entity 5: Debayer A (2 pads, 2 links)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev2
> 	pad0: Sink
> 		[fmt:RGB888_1X24/640x480 field:none]
> 		<- "Sensor A":0 [ENABLED,IMMUTABLE]
> 	pad1: Source
> 		[fmt:RGB888_1X24/640x480 field:none]
> 		-> "Scaler":0 [ENABLED]

Perfect :-)

> >> +			     << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		/* Enable the link and test if enabling was successful. */
> >> +		ret = link->setEnabled(true);
> >> +		if (ret)
> > 
> > Please log the cause of failure.
> 
> Failed operations on the media device are logged by the library with
> proper printout of the returned errno.
> 
> >> +			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.
> 
> I'll see how it looks like.
> 
> >> +			     << " 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" ?
> 
> Ok
> 
> >> +			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 :-)
> 
> I see. In case where the library fails vocally already, I don't think
> it is necessary.
> 
> >> +			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.
> 
> Isn't this already verified by making sure a link enabled by default
> is disabled after a disableLinks() call?

Partly only as the vimc device could have that link disabled already (and will 
if you run this test multiple times).

> I can add it here though, it does not hurt.
> 
> >> +		return 0;
> >> +	}
> >> +
> >> +	void cleanup()
> >> +	{
> >> +		dev_->close();
> >> +		dev_->release();
> >> +
> >> +		delete dev_;
> > 
> > The media device belongs to the enumerator, you should not delete it.
> 
> Correct, my bad. I wonder why the following delete didn't fail (or did
> it silently maybe)

Try running the test with valgrind :-)

> >> +		delete enumerator_;
> >> +	}
> >> +
> >> +private:
> >> +	DeviceEnumerator *enumerator_;
> >> +	MediaDevice *dev_;
> >> +};
> >> +
> >> +TEST_REGISTER(MediaDeviceLinkTest);

[snip]