[libcamera-devel] [PATCH v2 3/3] test: media_device: Add link handling test
Jacopo Mondi
jacopo at jmondi.org
Fri Jan 11 19:06:09 CET 2019
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 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.
> */
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
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190111/73a45bcc/attachment-0001.sig>
More information about the libcamera-devel
mailing list