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

149 diff mbox series

Add test function to exercize the link handling abilities of the media
device.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 test/media_device/media_device_test.cpp | 101 ++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

Hi Jacopo,

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@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);
>  };
>  
> +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";
> +
> +	/* Test the link() functions to be consistent. */
> +	MediaLink *alink = media.link(source->name(), sourcePad->index(),
> +				      sink->name(), sinkPad->index());
> +	if (link != alink)
> +		return -EINVAL;
> +
> +	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. */
> +	int ret = link->enable(true);
> +	if (ret)
> +		return ret;
> +
> +	ret = link->enable(false);
> +	if (ret) {
> +		if (!(link->flags() & MEDIA_LNK_FL_IMMUTABLE))
> +			return ret;
> +	}
> +
> +	return 0;
> +
> +}
> +
> +/*
> + * 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.
> + */
> +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";
> +
> +		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;
> +				}
> +			}
> +		}
> +	}
> +
> +
> +	/*
> +	 * 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;
> +			}
> +		}
> +	}
> +
> +	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();
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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();