[libcamera-devel] [PATCH 5/5] test: MediaDevice: Add link exercize test

[Laurent Pinchart]

Hello,

On Friday, 4 January 2019 18:49:54 EET Niklas Söderlund wrote:
> On 2019-01-03 18:38:59 +0100, Jacopo Mondi wrote:
> > Add test function to exercize the link handling abilities of the media
> > device.
> 
> I have not reviewed the entire patch yet but this coughs my eye :-)
> 
> s/exercize/exercise/
> 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > 
> >  test/media_device/media_device_test.cpp | 101 ++++++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> > 
> > diff --git a/test/media_device/media_device_test.cpp
> > b/test/media_device/media_device_test.cpp index c482b2e..99da3a6 100644
> > --- a/test/media_device/media_device_test.cpp
> > +++ b/test/media_device/media_device_test.cpp
> > @@ -42,8 +42,104 @@ private:
> >  	void printMediaGraph(const MediaDevice &media, ostream &os);
> >  	void printLinkFlags(const MediaLink *link, ostream &os);
> >  	void printNode(const MediaPad *pad, ostream &os);
> > +
> > +	int exercizeLinks(const MediaDevice &media);
> > +	int testLink(const MediaDevice &media, MediaLink *link);

I think this would be a candidate for a separate test class.

> >  };
> > 
> > +int MediaDeviceTest::testLink(const MediaDevice &media, MediaLink *link)
> > +{
> > +	MediaPad *sourcePad = link->source();
> > +	MediaEntity *source = sourcePad->entity();
> > +	MediaPad *sinkPad = link->sink();
> > +	MediaEntity *sink = sinkPad->entity();
> > +
> > +	cerr << "Test link handling interface on link: "
> > +	     << source->name() << ":" << sourcePad->index()
> > +	     << " -> " << sink->name() << ":" << sinkPad->index() << "\n";

cerr or cout ? This should be standarized as we have different tests using a 
different output.

endl instead of \n ?

> > +	/* Test the link() functions to be consistent. */

	/* Test the link() lookup functions for consistency. */

> > +	MediaLink *alink = media.link(source->name(), sourcePad->index(),
> > +				      sink->name(), sinkPad->index());
> > +	if (link != alink)
> > +		return -EINVAL;

When performing multiple test please log the exact cause of error.

		cerr << "Link lookup by entity name and pad index failed";

(or cout)

Same for all the other cases below.

> > +
> > +	alink = media.link(source, sourcePad->index(),
> > +			   sink, sinkPad->index());
> > +	if (link != alink)
> > +		return -EINVAL;
> > +
> > +	alink = media.link(sourcePad, sinkPad);
> > +	if (link != alink)
> > +		return -EINVAL;
> > +
> > +	/* Fine, we get consisten results... now try to manipulate the link. */

	/* Test link state modification. */

> > +	int ret = link->enable(true);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = link->enable(false);
> > +	if (ret) {
> > +		if (!(link->flags() & MEDIA_LNK_FL_IMMUTABLE))

How about skipping the call when the immutable flag is set ?

> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +

Extra blank linke.

> > +}
> > +
> > +/*
> > + * Exercize the link handling interface.
> > + * Try to get existing and non-existing links, and try to enable
> > + * disable link.
> > + *
> > + * WARNING: this test will change the link between pads on the media
> > + * device it runs on, potentially modying the behavior of the system
> > + * where the test is run on.

I think you should make this test depend on vimc, in order to have a device 
with a known topology, and both mutable and immutable links to test.

> > + */
> > +int MediaDeviceTest::exercizeLinks(const MediaDevice &media)
> > +{
> > +	auto entities = media.entities();
> > +
> > +	/* First of all, reset all links in the media graph. */
> > +	for (MediaEntity *ent : entities) {
> > +		cerr << "Disable all links in entity: " << ent->name() << "\n";

endl instead of \n ?

> > +
> > +		for (MediaPad *pad : ent->pads()) {
> > +			if (pad->flags() & MEDIA_PAD_FL_SINK)
> > +				continue;
> > +
> > +			for (MediaLink *link : pad->links()) {
> > +				int ret = link->enable(false);
> > +				if (ret) {
> > +					if (!(link->flags() &
> > +					      MEDIA_LNK_FL_IMMUTABLE))
> > +						return ret;
> > +				}
> > +			}
> > +		}
> > +	}

Triple nested loops... Would it make sense to add a 
MediaDevice::disableLinks() method to do this, that could internally iterate 
over a single list of links ? Even if it had to do a triple loop it would 
still be useful to offer this as a helper to pipeline managers instead of 
forcing them all to reimplement link reset (in slightly differently buggy ways 
:-))

> > +
> > +

One blank line is enough.

> > +	/*
> > +	 * Exercize the link handling interface.
> > +	 */
> > +	for (MediaEntity *ent : entities) {
> > +		for (MediaPad *pad : ent->pads()) {
> > +			if (pad->flags() & MEDIA_PAD_FL_SINK)
> > +				continue;
> > +
> > +			for (MediaLink *link : pad->links()) {
> > +				int ret = testLink(media, link);
> > +				if (ret)
> > +					return ret;
> > +			}
> > +		}
> > +	}

Relying on vimc, we could use specific links here to test specific features 
(error when disabling immutable links, ability to enable and disable mutable 
links, ...).

> > +
> > +	return 0;
> > +}
> > +
> >  void MediaDeviceTest::printNode(const MediaPad *pad, ostream &os)
> >  {
> >  	const MediaEntity *entity = pad->entity();
> > @@ -136,6 +232,11 @@ int MediaDeviceTest::testMediaDevice(const string
> > devnode)> 
> >  	/* Run tests in sequence. */
> >  	printMediaGraph(dev, cerr);
> > +
> > +	ret = exercizeLinks(dev);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* TODO: add more tests here. */
> >  	dev.close();

-- 
Regards,

Laurent Pinchart