[Intel-gfx] [RFC i-g-t v3 5/5] Add support for hotplug testing with the Chamelium
Lyude Paul
lyude at redhat.com
Wed Dec 7 19:56:50 UTC 2016
On Wed, 2016-12-07 at 12:27 +0100, Tomeu Vizoso wrote:
> Hi Lyude,
>
> this looks very good. Some minor comments below.
>
> Regards,
>
> Tomeu
>
> On 1 December 2016 at 02:24, Lyude <lyude at redhat.com> wrote:
> > For the purpose of testing things such as hotplugging and bad
> > monitors,
> > the ChromeOS team ended up designing a neat little device known as
> > the
> > Chamelium. More information on this can be found here:
> >
> > https://www.chromium.org/chromium-os/testing/chamelium
> >
> > This adds support for a couple of things to intel-gpu-tools:
> > - igt library functions for connecting to udev and monitoring it
> > for
> > hotplug events, loosely based off of the unfinished hotplugging
> > implementation in testdisplay
> > - Library functions for controlling the chamelium in tests using
> > xmlrpc. A couple of RPC calls were ommitted here, mainly because
> > they
> > didn't seem very useful for our needs (yet)
> > - A set of basic tests using the chamelium.
> >
> > Signed-off-by: Lyude <lyude at redhat.com>
> >
> > Changes since v1:
> > - Don't try to guess connector mappings, have the user specify them
> > manually using a configuration file
> > - Open DRM fd using DRIVER_ANY, not DRIVER_INTEL
> > - Lower the hotplug timeout a little bit, since 30 seconds was
> > leftover
> > from debugging these tests anyway
> > - Don't try to keep track of the original state of the chamelium
> > ports,
> > and just leave them plugged in after each run. This makes more
> > sense
> > to me, since I'd imagine in automated testing setups using
> > chameliums
> > that all of the extra monitors will probably be provided by the
> > Chamelium to begin with, so keeping them plugged in would make
> > sure
> > tests running afterwards that require >1 monitor don't get
> > skipped.
> > - Add wait_for_connector() to the chamelium tests. After some more
> > testing, I found that depending on the system some tests would
> > throw
> > false negatives due to us not waiting long enough for the system
> > to
> > detect that we connected something to it. This mainly happened
> > with
> > VGA connectors, since their lack of HPD makes them take
> > significantly
> > longer for the hardware to notice. wait_for_connector() fixes
> > this by
> > continually reprobing the status of the desired connector
> > (without
> > relying on a hpd event happening, since that might never come)
> > until
> > we get what we want, or we time out and fail.
> > - Use kmstest_get_property() for retrieving EDIDs instead of doing
> > it by
> > hand
> > - Don't hardcode PIPE_A for bringing up the display, use kmstest to
> > find
> > an appropriate CRTC to use.
> > Changes since v2:
> > - Fix incorrect usage of the list helpers when recording new EDIDs
> > - Add missing documentation
> > - Make sure documentation actually appears
> > - Since we finally got video capture working, add CRC functions and
> > fix
> > the ones we couldn't actually test before
> > - In the exit handler, reset the xmlrpc env so we can properly
> > reset the
> > Chamelium even after an RPC error
> > - Make sure compiling without Chamelium support still works
> > ---
> > configure.ac | 13 +
> > .../intel-gpu-tools/intel-gpu-tools-docs.xml | 1 +
> > lib/Makefile.am | 4 +-
> > lib/Makefile.sources | 10 +-
> > lib/igt.h | 4 +
> > lib/igt_chamelium.c | 849
> > +++++++++++++++++++++
> > lib/igt_chamelium.h | 73 ++
> > scripts/run-tests.sh | 4 +-
> > tests/Makefile.am | 5 +-
> > tests/Makefile.sources | 9 +-
> > tests/chamelium.c | 407
> > ++++++++++
> > 11 files changed, 1371 insertions(+), 8 deletions(-)
> > create mode 100644 lib/igt_chamelium.c
> > create mode 100644 lib/igt_chamelium.h
> > create mode 100644 tests/chamelium.c
> >
> > diff --git a/configure.ac b/configure.ac
> > index b62b8fc..1920609 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -263,6 +263,18 @@ if test "x$with_libunwind" = xyes; then
> > AC_MSG_ERROR([libunwind not found. Use --
> > without-libunwind to disable libunwind support.]))
> > fi
> >
> > +# enable support for using the chamelium
> > +AC_ARG_ENABLE(chamelium,
> > + AS_HELP_STRING([--disable-chamelium],
> > + [Build tests without chamelium
> > support]),
> > + [], [enable_chamelium=yes])
> > +
> > +AM_CONDITIONAL(HAVE_CHAMELIUM, [test "x$enable_chamelium" = xyes])
> > +if test "x$enable_chamelium" = xyes; then
> > + AC_DEFINE(HAVE_CHAMELIUM, 1, [chamelium suport])
> > + PKG_CHECK_MODULES(XMLRPC, xmlrpc_client)
> > +fi
> > +
>
> IMO libxmlrpc is a very common dependency and thus adding the
> possibility of disabling Chamelium support isn't worth the cruft we
> have to add later. Plus, alternative build configurations tend to
> break and remain broken as most people hacking on the project won't
> be
> using them.
>
> > # enable debug symbols
> > AC_ARG_ENABLE(debug,
> > AS_HELP_STRING([--disable-debug],
> > @@ -360,6 +372,7 @@ echo " Assembler :
> > ${enable_assembler}"
> > echo " Debugger : ${enable_debugger}"
> > echo " Overlay : X: ${enable_overlay_xlib}, Xv:
> > ${enable_overlay_xvlib}"
> > echo " x86-specific tools : ${build_x86}"
> > +echo " Chamelium support : ${enable_chamelium}"
> > echo ""
> > echo " • API-Documentation : ${enable_gtk_doc}"
> > echo " • Fail on warnings : ${enable_werror}"
> > diff --git a/docs/reference/intel-gpu-tools/intel-gpu-tools-
> > docs.xml b/docs/reference/intel-gpu-tools/intel-gpu-tools-docs.xml
> > index 55902ab..f515f33 100644
> > --- a/docs/reference/intel-gpu-tools/intel-gpu-tools-docs.xml
> > +++ b/docs/reference/intel-gpu-tools/intel-gpu-tools-docs.xml
> > @@ -33,6 +33,7 @@
> > <xi:include href="xml/igt_vc4.xml"/>
> > <xi:include href="xml/igt_vgem.xml"/>
> > <xi:include href="xml/igt_dummyload.xml"/>
> > + <xi:include href="xml/igt_chamelium.xml"/>
> > </chapter>
> > <xi:include href="xml/igt_test_programs.xml"/>
> >
> > diff --git a/lib/Makefile.am b/lib/Makefile.am
> > index b7c7020..0136663 100644
> > --- a/lib/Makefile.am
> > +++ b/lib/Makefile.am
> > @@ -23,7 +23,7 @@ if !HAVE_LIBDRM_INTEL
> > endif
> >
> > AM_CPPFLAGS = -I$(top_srcdir)
> > -AM_CFLAGS = $(CWARNFLAGS) $(DRM_CFLAGS) $(PCIACCESS_CFLAGS)
> > $(KMOD_CFLAGS) $(LIBUNWIND_CFLAGS) $(DEBUG_CFLAGS) \
> > +AM_CFLAGS = $(CWARNFLAGS) $(DRM_CFLAGS) $(PCIACCESS_CFLAGS)
> > $(KMOD_CFLAGS) $(LIBUNWIND_CFLAGS) $(DEBUG_CFLAGS) $(XMLRPC_CFLAGS)
> > $(UDEV_CFLAGS) \
> > -DIGT_SRCDIR=\""$(abs_top_srcdir)/tests"\" \
> > -DIGT_DATADIR=\""$(pkgdatadir)"\" \
> > -DIGT_LOG_DOMAIN=\""$(subst _,-,$*)"\" \
> > @@ -39,5 +39,7 @@ libintel_tools_la_LIBADD = \
> > $(LIBUDEV_LIBS) \
> > $(LIBUNWIND_LIBS) \
> > $(TIMER_LIBS) \
> > + $(XMLRPC_LIBS) \
> > + $(UDEV_LIBS) \
>
> The prefix we passed to PKG_CHECK_MODULES is LIBUDEV.
>
> > -lm
> >
> > diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> > index 7fc5ec2..7132e32 100644
> > --- a/lib/Makefile.sources
> > +++ b/lib/Makefile.sources
> > @@ -78,8 +78,14 @@ lib_source_list = \
> > igt_dummyload.c \
> > igt_dummyload.h \
> > uwildmat/uwildmat.h \
> > - uwildmat/uwildmat.c \
> > - $(NULL)
> > + uwildmat/uwildmat.c
> > +
> > +if HAVE_CHAMELIUM
> > +lib_source_list += igt_chamelium.h \
> > + igt_chamelium.c
> > +endif
> > +
> > +lib_source_list += $(NULL)
>
> The $(NULL) was there just so the last line in lib_source_list could
> end with \ and thus adding more files to the end wouldn't have to
> change more lines than strictly needed.
>
> >
> > .PHONY: version.h.tmp
> >
> > diff --git a/lib/igt.h b/lib/igt.h
> > index a0028d5..d0ded79 100644
> > --- a/lib/igt.h
> > +++ b/lib/igt.h
> > @@ -47,4 +47,8 @@
> > #include "media_spin.h"
> > #include "rendercopy.h"
> >
> > +#ifdef HAVE_CHAMELIUM
> > +#include "igt_chamelium.h"
> > +#endif
> > +
> > #endif /* IGT_H */
> > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > new file mode 100644
> > index 0000000..ee94e4a
> > --- /dev/null
> > +++ b/lib/igt_chamelium.c
> > @@ -0,0 +1,849 @@
> > +/*
> > + * Copyright © 2016 Red Hat Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to
> > whom the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice
> > (including the next
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + * Lyude Paul <lyude at redhat.com>
> > + */
> > +
> > +#include "config.h"
> > +
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <xmlrpc-c/base.h>
> > +#include <xmlrpc-c/client.h>
> > +#include <glib.h>
> > +
> > +#include "igt.h"
> > +
> > +#define check_rpc() \
> > + igt_assert_f(!env.fault_occurred, "Chamelium RPC call
> > failed: %s\n", \
> > + env.fault_string);
> > +
> > +/**
> > + * SECTION:igt_chamelium
> > + * @short_description: Library for encorporating the Chamelium
> > into igt tests
>
> incorporating (though I would personally just say "using")
>
> > + * @title: Chamelium
> > + * @include: igt_chamelium.h
> > + *
> > + * This library contains helpers for using Chameliums in IGT
> > tests. This allows
> > + * for tests to simulate more difficult tasks to automate such as
> > display
> > + * hotplugging, faulty display behaviors, etc.
> > + *
> > + * More information on the Chamelium can be found
> > + * [on the ChromeOS project page](https://www.chromium.org/chromiu
> > m-os/testing/chamelium).
> > + *
> > + * In order to run tests using the Chamelium, a valid
> > configuration file must be
> > + * present. The configuration file is a normal Glib keyfile
> > (similar to Windows
> > + * INI) structured like so:
> > + *
> > + * |[<!-- language="plain" -->
> > + * [Chamelium]
> > + * URL=http://chameleon:9992 # The URL used for connecting to
> > the Chamelium's RPC server
> > + *
> > + * # The rest of the sections are used for defining connector
> > mappings.
> > + * # This is required so any tests using the Chamelium know
> > which connector
> > + * # on the test machine should be connected to each Chamelium
> > port.
> > + * #
> > + * # In the event that any of these mappings are specified
> > incorrectly,
> > + * # any hotplugging tests for the incorrect connector mapping
> > will fail.
> > + *
> > + * [DP-1] # The name of the DRM connector
> > + * ChameliumPortID=1 # The ID of the port on the Chamelium
> > this connector is attached to
> > + *
> > + * [HDMI-A-1]
> > + * ChameliumPortID=3
> > + * ]|
> > + *
> > + * By default, this file is expected to exist in
> > ~/.igt_chamelium_rc . The
> > + * directory for this can be overriden by setting the environment
> > variable
> > + * %CHAMELIUM_CONFIG_PATH.
>
> It would be a pity if in time we need to add configuration parameters
> for other aspects of IGT and we have to choose between renaming this
> file, or having two configuration files for the same project. Why not
> call it just .igt_rc?
Nice catch. While I'm at it I'll add a patch to expose a function or
two to get a global GKeyFile that's shared across tests that need
configurations. As well I'll make sure connector groups are prefixed
with something such as "Chamelium:" since we'll be sharing things.
>
> > + */
> > +
> > +/**
> > + * chamelium_ports:
> > + *
> > + * Contains information on all of the ports that are configured
> > for use with the
> > + * Chamelium. This information is initialized when #chamelium_init
> > is called.
> > + */
> > +struct chamelium_port *chamelium_ports;
> > +
> > +/**
> > + * chamelium_port_count:
> > + *
> > + * How many ports are configured for use by the Chamelium.
> > + */
> > +int chamelium_port_count;
> > +
> > +static const char *chamelium_url;
> > +static xmlrpc_env env;
>
> I think it would be better to follow the same pattern as used in
> igt_pipe_crc_ calls and explicitly pass a pointer to a igt_pipe_crc_t
> around, instead of using globals.
>
> > +
> > +struct chamelium_edid {
> > + int id;
> > + struct igt_list link;
> > +};
> > +struct chamelium_edid *allocated_edids;
> > +
> > +/**
> > + * chamelium_plug:
> > + * @id: The ID of the port on the chamelium to plug
> > + *
> > + * Simulate a display connector being plugged into the system
> > using the
> > + * chamelium.
> > + */
> > +void chamelium_plug(int id)
> > +{
> > + xmlrpc_value *res;
> > +
> > + igt_debug("Plugging port %d\n", id);
>
> For this and other places below, I think it could be very useful when
> reading logs from CI if we could have the name of the connector along
> or instead the chamelium port id.
>
> > + res = xmlrpc_client_call(&env, chamelium_url, "Plug",
> > "(i)", id);
> > + check_rpc();
> > +
> > + xmlrpc_DECREF(res);
> > +}
> > +
> > +/**
> > + * chamelium_unplug:
> > + * @id: The ID of the port on the chamelium to unplug
> > + *
> > + * Simulate a display connector being unplugged from the system
> > using the
> > + * chamelium.
> > + */
> > +void chamelium_unplug(int id)
> > +{
> > + xmlrpc_value *res;
> > +
> > + igt_debug("Unplugging port %d\n", id);
> > + res = xmlrpc_client_call(&env, chamelium_url, "Unplug",
> > "(i)", id);
> > + check_rpc();
> > +
> > + xmlrpc_DECREF(res);
> > +}
> > +
> > +/**
> > + * chamelium_is_plugged:
> > + * @id: The ID of the port on the chamelium to check the status of
> > + *
> > + * Check whether or not the given port has been plugged into the
> > system using
> > + * #chamelium_plug.
> > + *
> > + * Returns: %true if the connector is set to plugged in, %false
> > otherwise.
> > + */
> > +bool chamelium_is_plugged(int id)
> > +{
> > + xmlrpc_value *res;
> > + xmlrpc_bool is_plugged;
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url, "IsPlugged",
> > "(i)", id);
> > + check_rpc();
> > +
> > + xmlrpc_read_bool(&env, res, &is_plugged);
> > + xmlrpc_DECREF(res);
> > +
> > + return is_plugged;
> > +}
> > +
> > +/**
> > + * chamelium_port_wait_video_input_stable:
> > + * @id: The ID of the port on the chamelium to check the status of
> > + * @timeout_secs: How long to wait for a video signal to appear
> > before timing
> > + * out
> > + *
> > + * Waits for a video signal to appear on the given port. This is
> > useful for
> > + * checking whether or not we've setup a monitor correctly.
> > + *
> > + * Returns: %true if a video signal was detected, %false if we
> > timed out
> > + */
> > +bool chamelium_port_wait_video_input_stable(int id, int
> > timeout_secs)
> > +{
> > + xmlrpc_value *res;
> > + xmlrpc_bool is_on;
> > +
> > + igt_debug("Waiting for video input to stabalize on port
> > %d\n", id);
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url,
> > "WaitVideoInputStable",
> > + "(ii)", id, timeout_secs);
> > + check_rpc();
> > +
> > + xmlrpc_read_bool(&env, res, &is_on);
> > + xmlrpc_DECREF(res);
> > +
> > + return is_on;
> > +}
> > +
> > +/**
> > + * chamelium_fire_hpd_pulses:
> > + * @id: The ID of the port to fire hotplug pulses on
> > + * @width_msec: How long each pulse should last
> > + * @count: The number of pulses to send
> > + *
> > + * A convienence function for sending multiple hotplug pulses to
> > the system.
> > + * The pulses start at low (e.g. connector is disconnected), and
> > then alternate
> > + * from high (e.g. connector is plugged in) to low. This is the
> > equivalent of
> > + * repeatedly calling #chamelium_plug and #chamelium_unplug,
> > waiting
> > + * @width_msec between each call.
> > + *
> > + * If @count is even, the last pulse sent will be high, and if
> > it's odd then it
> > + * will be low. Resetting the HPD line back to it's previous
> > state, if desired,
> > + * is the responsibility of the caller.
> > + */
> > +void chamelium_fire_hpd_pulses(int id, int width_msec, int count)
> > +{
> > + xmlrpc_value *pulse_widths = xmlrpc_array_new(&env),
> > + *width = xmlrpc_int_new(&env, width_msec),
> > *res;
>
> If it's going to be two lines anyway, we may just leave the type
> specifier in the second one.
>
> > + int i;
> > +
> > + igt_debug("Firing %d HPD pulses with width of %d msec on id
> > %d\n",
> > + count, width_msec, id);
> > +
> > + for (i = 0; i < count; i++)
> > + xmlrpc_array_append_item(&env, pulse_widths,
> > width);
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url,
> > "FireMixedHpdPulses",
> > + "(iA)", id, pulse_widths);
> > + check_rpc();
> > +
> > + xmlrpc_DECREF(res);
> > + xmlrpc_DECREF(width);
> > + xmlrpc_DECREF(pulse_widths);
> > +}
> > +
> > +/**
> > + * chamelium_fire_mixed_hpd_pulses:
> > + * @id: The ID of the port to fire hotplug pulses on
> > + * @...: The length of each pulse in milliseconds, terminated with
> > a %0
> > + *
> > + * Does the same thing as #chamelium_fire_hpd_pulses, but allows
> > the caller to
> > + * specify the length of each individual pulse.
> > + */
> > +void chamelium_fire_mixed_hpd_pulses(int id, ...)
> > +{
> > + va_list args;
> > + xmlrpc_value *pulse_widths = xmlrpc_array_new(&env),
> > *width, *res;
> > + int arg;
> > +
> > + igt_debug("Firing mixed HPD pulses on port %d\n", id);
> > +
> > + va_start(args, id);
> > + for (arg = va_arg(args, int); arg; arg = va_arg(args, int))
> > {
> > + width = xmlrpc_int_new(&env, arg);
> > + xmlrpc_array_append_item(&env, pulse_widths,
> > width);
> > + xmlrpc_DECREF(width);
> > + }
> > + va_end(args);
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url,
> > "FireMixedHpdPulses",
> > + "(iA)", id, pulse_widths);
> > + check_rpc();
> > + xmlrpc_DECREF(res);
> > +
> > + xmlrpc_DECREF(pulse_widths);
> > +}
> > +
> > +static void async_rpc_handler(const char *server_url, const char
> > *method_name,
> > + xmlrpc_value *param_array, void
> > *user_data,
> > + xmlrpc_env *fault, xmlrpc_value
> > *result)
> > +{
> > + /* We don't care about the responses */
>
> What about reporting any errors?
See below
>
> > +}
> > +
> > +/**
> > + * chamelium_async_hpd_pulse_start:
> > + * @id: The ID of the port to fire a hotplug pulse on
> > + * @high: Whether to fire a high pulse (e.g. simulate a connect),
> > or a low
> > + * pulse (e.g. simulate a disconnect)
> > + * @delay_secs: How long to wait before sending the HPD pulse.
> > + *
> > + * Instructs the chamelium to send an hpd pulse after @delay_secs
> > seconds have
> > + * passed, without waiting for the chamelium to finish. This is
> > useful for
> > + * testing things such as hpd after a suspend/resume cycle, since
> > we can't tell
> > + * the chamelium to send a hotplug at the same time that our
> > system is
> > + * suspended.
> > + *
> > + * It is required that the user eventually call
> > + * #chamelium_async_hpd_pulse_finish, to clean up the leftover
> > XML-RPC
> > + * responses from the chamelium.
> > + */
> > +void chamelium_async_hpd_pulse_start(int id, bool high, int
> > delay_secs)
> > +{
> > + xmlrpc_value *pulse_widths = xmlrpc_array_new(&env),
> > *width;
> > +
> > + /* TODO: Actually implement something in the chameleon
> > server to allow
> > + * for delayed actions such as hotplugs. This would work a
> > bit better
> > + * and allow us to test suspend/resume on ports without hpd
> > like VGA
> > + */
> > +
> > + igt_debug("Sending HPD pulse (%s) on port %d with %d second
> > delay\n",
> > + high ? "high->low" : "low->high", id,
> > delay_secs);
> > +
> > + /* If we're starting at high, make the first pulse width 0
> > so we keep
> > + * the port connected */
> > + if (high) {
> > + width = xmlrpc_int_new(&env, 0);
> > + xmlrpc_array_append_item(&env, pulse_widths,
> > width);
> > + xmlrpc_DECREF(width);
> > + }
> > +
> > + width = xmlrpc_int_new(&env, delay_secs * 1000);
> > + xmlrpc_array_append_item(&env, pulse_widths, width);
> > + xmlrpc_DECREF(width);
> > +
> > + xmlrpc_client_call_asynch(chamelium_url,
> > "FireMixedHpdPulses",
> > + async_rpc_handler, NULL, "(iA)",
> > + id, pulse_widths);
> > + xmlrpc_DECREF(pulse_widths);
> > +}
> > +
> > +/**
> > + * chamelium_async_hpd_pulse_finish:
> > + *
> > + * Waits for any asynchronous RPC started by
> > #chamelium_async_hpd_pulse_start
> > + * to complete, and then cleans up any leftover responses from the
> > chamelium.
> > + * If all of the RPC calls have already completed, this function
> > returns
> > + * immediately.
>
> This makes me wonder if it wouldn't have been better to go with
> libsoup, as it uses a real event loop that we may need to use in
> tests
> for other reasons.
Fwiw, I wouldn't worry about this part too much. The only reason we we
use async calls here is so that we can use the mixed pulses RPC to get
the chamelium to send delayed hotplug events by using the pulse width
as the delay. I wouldn't think there would be any situations where the
chamelium would fail to send a hotplug, since all that part requires is
just asserting the hpd line. It could fail if the IO board stopped
responding, but in that case everything else is going to fail too and
we won't get the hotplug event we're expecting anyway.
This is just my opinion though, if you can think of some use cases that
this could seriously cause some issues I'll be happy to change it.
>
> > + */
> > +void chamelium_async_hpd_pulse_finish(void)
> > +{
> > + xmlrpc_client_event_loop_finish_asynch();
> > +}
> > +
> > +/**
> > + * chamelium_new_edid:
> > + * @edid: The edid blob to upload to the chamelium
> > + *
> > + * Uploads and registers a new EDID with the chamelium. The EDID
> > will be
> > + * destroyed automatically when #chamelium_deinit is called.
> > + *
> > + * Returns: The ID of the EDID uploaded to the chamelium.
> > + */
> > +int chamelium_new_edid(const unsigned char *edid)
> > +{
> > + xmlrpc_value *res;
> > + struct chamelium_edid *allocated_edid;
> > + int edid_id;
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url, "CreateEdid",
> > + "(6)", edid, EDID_LENGTH);
> > + check_rpc();
> > +
> > + xmlrpc_read_int(&env, res, &edid_id);
> > + xmlrpc_DECREF(res);
> > +
> > + allocated_edid = malloc(sizeof(struct chamelium_edid));
>
> Should be zeroed?
>
> > + igt_assert(allocated_edid);
>
> Don't think it makes sense in userspace to check for malloc failures.
>
> > +
> > + allocated_edid->id = edid_id;
> > + igt_list_init(&allocated_edid->link);
> > +
> > + if (allocated_edids) {
> > + igt_list_add(&allocated_edids->link,
> > &allocated_edid->link);
> > + } else {
> > + allocated_edids = allocated_edid;
> > + }
> > +
> > + return edid_id;
> > +}
> > +
> > +static void chamelium_destroy_edid(int edid_id)
> > +{
> > + xmlrpc_value *res;
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url,
> > "DestroyEdid",
> > + "(i)", edid_id);
> > + check_rpc();
> > +
> > + xmlrpc_DECREF(res);
> > +}
> > +
> > +/**
> > + * chamelium_port_set_edid:
> > + * @id: The ID of the port to set the EDID on
> > + * @edid_id: The ID of an EDID on the chamelium created with
> > + * #chamelium_new_edid, or 0 to disable the EDID on the port
> > + *
> > + * Sets a port on the chamelium to use the specified EDID. This
> > does not fire a
> > + * hotplug pulse on it's own, and merely changes what EDID the
> > chamelium port
> > + * will report to us the next time we probe it. Users will need to
> > reprobe the
> > + * connectors themselves if they want to see the EDID reported by
> > the port
> > + * change.
> > + */
> > +void chamelium_port_set_edid(int id, int edid_id)
> > +{
> > + xmlrpc_value *res;
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url, "ApplyEdid",
> > + "(ii)", id, edid_id);
> > + check_rpc();
> > +
> > + xmlrpc_DECREF(res);
> > +}
> > +
> > +/**
> > + * chamelium_port_set_ddc_state:
> > + * @id: The ID of the port whose DDC bus we want to modify
> > + * @enabled: Whether or not to enable the DDC bus
> > + *
> > + * This disables the DDC bus (e.g. the i2c line on the connector
> > that gives us
> > + * an EDID) of the specified port on the chamelium. This is useful
> > for testing
> > + * behavior on legacy connectors such as VGA, where the presence
> > of a DDC bus
> > + * is not always guaranteed.
> > + */
> > +void chamelium_port_set_ddc_state(int port, bool enabled)
> > +{
> > + xmlrpc_value *res;
> > +
> > + igt_debug("%sabling DDC bus on port %d\n",
> > + enabled ? "En" : "Dis", port);
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url,
> > "SetDdcState",
> > + "(ib)", port, enabled);
> > + check_rpc();
> > +
> > + xmlrpc_DECREF(res);
> > +}
> > +
> > +/**
> > + * chamelium_port_get_ddc_state:
> > + * @id: The ID of the port whose DDC bus we want to check the
> > status of
> > + *
> > + * Check whether or not the DDC bus on the specified chamelium
> > port is enabled
> > + * or not.
> > + *
> > + * Returns: %true if the DDC bus is enabled, %false otherwise.
> > + */
> > +bool chamelium_port_get_ddc_state(int id)
> > +{
> > + xmlrpc_value *res;
> > + xmlrpc_bool enabled;
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url,
> > "IsDdcEnabled",
> > + "(i)", id);
> > + check_rpc();
> > +
> > + xmlrpc_read_bool(&env, res, &enabled);
> > +
> > + xmlrpc_DECREF(res);
> > + return enabled;
> > +}
> > +
> > +/**
> > + * chamelium_port_get_resolution:
> > + * @id: The ID of the port whose display resolution we want to
> > check
> > + * @x: Where to store the horizontal resolution of the port
> > + * @y: Where to store the verical resolution of the port
> > + *
> > + * Check the current reported display resolution of the specified
> > port on the
> > + * chamelium. This information is provided by the chamelium
> > itself, not DRM.
> > + * Useful for verifying that we really are scanning out at the
> > resolution we
> > + * think we are.
> > + */
> > +void chamelium_port_get_resolution(int id, int *x, int *y)
> > +{
> > + xmlrpc_value *res, *res_x, *res_y;
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url,
> > "DetectResolution",
> > + "(i)", id);
> > + check_rpc();
> > +
> > + xmlrpc_array_read_item(&env, res, 0, &res_x);
> > + xmlrpc_array_read_item(&env, res, 1, &res_y);
> > + xmlrpc_read_int(&env, res_x, x);
> > + xmlrpc_read_int(&env, res_y, y);
> > +
> > + xmlrpc_DECREF(res_x);
> > + xmlrpc_DECREF(res_y);
> > + xmlrpc_DECREF(res);
> > +}
> > +
> > +static void crc_from_xml(xmlrpc_value *xml_crc, igt_crc_t *out)
> > +{
> > + xmlrpc_value *res;
> > + int i;
> > +
> > + out->n_words = xmlrpc_array_size(&env, xml_crc);
> > + for (i = 0; i < out->n_words; i++) {
> > + xmlrpc_array_read_item(&env, xml_crc, i, &res);
> > + xmlrpc_read_int(&env, res, (int*)&out->crc[i]);
> > + xmlrpc_DECREF(res);
> > + }
> > +}
> > +
> > +/**
> > + * chamelium_get_crc_for_area:
> > + * @id: The ID of the port from which we want to retrieve the CRC
> > + * @x: The X coordinate on the emulated display to start
> > calculating the CRC
> > + * from
> > + * @y: The Y coordinate on the emulated display to start
> > calculating the CRC
> > + * from
> > + * @w: The width of the area to fetch the CRC from, or %0 for the
> > whole display
> > + * @h: The height of the area to fetch the CRC from, or %0 for the
> > whole display
> > + *
> > + * Reads back the pixel CRC for an area on the specified chamelium
> > port. This
> > + * is the same as using the CRC readback from a GPU, the main
> > difference being
> > + * the data is provided by the chamelium and also allows us to
> > specify a region
> > + * of the screen to use as opposed to the entire thing.
> > + *
> > + * Returns: The CRC read back from the chamelium
> > + */
> > +igt_crc_t *chamelium_get_crc_for_area(int id, int x, int y, int w,
> > int h)
> > +{
> > + xmlrpc_value *res;
> > + igt_crc_t *ret = malloc(sizeof(igt_crc_t));;
>
> Extra ;. Docs should mention that the caller has to free.
>
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url,
> > "ComputePixelChecksum",
> > + (w && h) ? "(iiiii)" : "(innnn)",
> > + id, x, y, w, h);
> > + check_rpc();
> > +
> > + crc_from_xml(res, ret);
> > + xmlrpc_DECREF(res);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * chamelium_start_capture:
> > + * @id: The ID of the port for which we want to start capturing
> > frames on
> > + * @x: The X coordinate to crop the video to
> > + * @y: The Y coordinate to crop the video to
> > + * @w: The width of the cropped video, or %0 for the whole display
> > + * @h: The height of the cropped video, or %0 for the whole
> > display
> > + *
> > + * Starts capturing video frames on the given Chamelium port. Once
> > the user is
> > + * finished capturing frames, they should call
> > #chamelium_stop_capture.
> > + *
> > + * For capturing a single frame, users can use the one-shot
> > + * @chamelium_get_crc_for_area
> > + */
> > +void chamelium_start_capture(int id, int x, int y, int w, int h)
> > +{
> > + xmlrpc_client_call(&env, chamelium_url,
> > "StartCapturingVideo",
> > + (w && h) ? "(iiiii)" : "(innnn)", id, x,
> > y, w, h);
> > + check_rpc();
> > +}
> > +
> > +/**
> > + * chamelium_stop_capture:
> > + * @frame_count: The number of frames to wait to capture, or %0 to
> > stop
> > + * immediately
> > + *
> > + * Finishes capturing video frames on the given Chamelium port. If
> > @frame_count
> > + * is specified, this call will block until the given number of
> > frames have been
> > + * captured.
> > + */
> > +void chamelium_stop_capture(int frame_count)
> > +{
> > + xmlrpc_client_call(&env, chamelium_url,
> > "StopCapturingVideo",
> > + "(i)", frame_count);
> > + check_rpc();
> > +}
> > +
> > +/**
> > + * chamelium_read_captured_crcs:
> > + * @frame_count: Where to store the number of CRCs we read in
> > + *
> > + * Reads all of the CRCs that have been captured thus far from the
> > Chamelium.
> > + *
> > + * Returns: An array of @frame_count length containing all of the
> > CRCs we read
> > + */
> > +igt_crc_t *chamelium_read_captured_crcs(int *frame_count)
> > +{
> > + igt_crc_t *ret;
> > + xmlrpc_value *res, *elem;
> > + int i;
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url,
> > "GetCapturedChecksums",
> > + "(in)", 0);
> > + check_rpc();
> > +
> > + *frame_count = xmlrpc_array_size(&env, res);
> > + ret = calloc(sizeof(igt_crc_t), *frame_count);
> > +
> > + for (i = 0; i < *frame_count; i++) {
> > + xmlrpc_array_read_item(&env, res, i, &elem);
> > +
> > + crc_from_xml(elem, &ret[i]);
> > + ret[i].frame = i;
> > +
> > + xmlrpc_DECREF(elem);
> > + }
> > +
> > + xmlrpc_DECREF(res);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * chamelium_get_frame_limit:
> > + * @id: The ID of the port to get the capture frame limit for
> > + * @w: The width of the area to get the capture frame limit for,
> > or %0 for the
> > + * whole display
> > + * @h: The height of the area to get the capture frame limit for,
> > or %0 for the
> > + * whole display
> > + *
> > + * Gets the max number of frames we can capture with the Chamelium
> > for the given
> > + * resolution.
> > + *
> > + * Returns: The number of the max number of frames we can capture
> > + */
> > +int chamelium_get_frame_limit(int id, int w, int h)
> > +{
> > + xmlrpc_value *res;
> > + int ret;
> > +
> > + if (!w && !h)
> > + chamelium_port_get_resolution(id, &w, &h);
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url,
> > "GetMaxFrameLimit",
> > + "(iii)", id, w, h);
> > + check_rpc();
> > +
> > + xmlrpc_read_int(&env, res, &ret);
> > + xmlrpc_DECREF(res);
> > +
> > + return ret;
> > +}
> > +
> > +static unsigned int chamelium_get_port_type(int port)
> > +{
> > + xmlrpc_value *res;
> > + const char *port_type_str;
> > + unsigned int port_type;
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url,
> > "GetConnectorType",
> > + "(i)", port);
> > + check_rpc();
> > +
> > + xmlrpc_read_string(&env, res, &port_type_str);
> > + igt_debug("Port %d is of type '%s'\n", port,
> > port_type_str);
> > +
> > + if (strcmp(port_type_str, "DP") == 0)
> > + port_type = DRM_MODE_CONNECTOR_DisplayPort;
> > + else if (strcmp(port_type_str, "HDMI") == 0)
> > + port_type = DRM_MODE_CONNECTOR_HDMIA;
> > + else if (strcmp(port_type_str, "VGA") == 0)
> > + port_type = DRM_MODE_CONNECTOR_VGA;
> > + else
> > + port_type = DRM_MODE_CONNECTOR_Unknown;
> > +
> > + free((void*)port_type_str);
> > + xmlrpc_DECREF(res);
> > +
> > + return port_type;
> > +}
> > +
> > +static void chamelium_read_port_mappings(int drm_fd, GKeyFile
> > *key_file)
> > +{
> > + drmModeRes *res;
> > + drmModeConnector *connector;
> > + struct chamelium_port *port;
> > + GError *error = NULL;
> > + char **group_list;
> > + char *group;
> > + size_t group_list_len;
> > + int port_i, i, j;
> > +
> > + group_list = g_key_file_get_groups(key_file,
> > &group_list_len);
> > + chamelium_port_count = group_list_len - 1;
> > + chamelium_ports = calloc(sizeof(struct chamelium_port),
> > + chamelium_port_count);
> > + memset(chamelium_ports, 0,
> > + sizeof(struct chamelium_port) *
> > chamelium_port_count);
>
> calloc already zeroes.
>
> > + port_i = 0;
> > + res = drmModeGetResources(drm_fd);
> > +
> > + for (i = 0; group_list[i] != NULL; i++) {
> > + group = group_list[i];
> > +
> > + if (strcmp(group, "Chamelium") == 0)
> > + continue;
> > +
> > + port = &chamelium_ports[port_i++];
> > + port->connector_name = strdup(group);
> > + port->id = g_key_file_get_integer(key_file, group,
> > + "ChameliumPortID"
> > ,
> > + &error);
> > + igt_require_f(port->id,
> > + "Failed to read chamelium port ID for
> > %s: %s\n",
> > + group, error->message);
> > +
> > + port->type = chamelium_get_port_type(port->id);
> > + igt_require_f(port->type !=
> > DRM_MODE_CONNECTOR_Unknown,
> > + "Unable to retrieve the physical port
> > type from the Chamelium for '%s'\n",
> > + group);
> > +
> > + for (j = 0;
> > + j < res->count_connectors && !port-
> > >connector_id;
> > + j++) {
> > + char connector_name[50];
> > +
> > + connector = drmModeGetConnectorCurrent(
> > + drm_fd, res->connectors[j]);
> > +
> > + /* We have to generate the connector name
> > on our own */
> > + snprintf(connector_name, 50, "%s-%u",
> > + kmstest_connector_type_str(connect
> > or->connector_type),
> > + connector->connector_type_id);
> > +
> > + if (strcmp(connector_name, group) == 0)
> > + port->connector_id = connector-
> > >connector_id;
> > +
> > + drmModeFreeConnector(connector);
> > + }
> > + igt_assert_f(port->connector_id,
> > + "No connector found with name '%s'\n",
> > group);
> > +
> > + igt_debug("Port '%s' with physical type '%s' mapped
> > to Chamelium port %d\n",
> > + group, kmstest_connector_type_str(port-
> > >type),
> > + port->id);
> > + }
> > +
> > + drmModeFreeResources(res);
> > + g_strfreev(group_list);
> > +}
> > +
> > +static void chamelium_read_config(int drm_fd)
> > +{
> > + GKeyFile *key_file = g_key_file_new();
> > + GError *error = NULL;
> > + char *key_file_loc;
> > + int rc;
> > +
> > + key_file_loc = getenv("CHAMELIUM_CONFIG_PATH");
> > + if (!key_file_loc) {
> > + igt_require(key_file_loc = alloca(100));
>
> Why skip the current test if alloca fails? Actually, why checking
> alloca's result? If we aren't 100% sure that alloca will succeed, we
> should be using malloc instead.
>
> > + snprintf(key_file_loc, 100, "%s/.igt_chamelium_rc",
> > + g_get_home_dir());
> > + }
> > +
> > + rc = g_key_file_load_from_file(key_file, key_file_loc,
> > + G_KEY_FILE_NONE, &error);
> > + igt_require_f(rc, "Failed to read chamelium configuration
> > file: %s\n",
> > + error->message);
>
> Not sure I agree with the usage of igt_require in this patch. How I
> envision this to work, is that we require that the functionality to
> be
> tested is there, otherwise we skip. But once we have determined that,
> if there is any problem when testing that functionality, it should
> cause a failure.
>
> I think it's also better if the test code is able to decide whether
> to
> skip or fail.
>
> > + chamelium_url = g_key_file_get_string(key_file,
> > "Chamelium", "URL",
> > + &error);
> > + igt_require_f(chamelium_url,
> > + "Couldn't read chamelium URL from config
> > file: %s\n",
> > + error->message);
> > +
> > + chamelium_read_port_mappings(drm_fd, key_file);
> > +
> > + g_key_file_free(key_file);
> > +}
> > +
> > +/**
> > + * chamelium_reset:
> > + *
> > + * Resets the chamelium's IO board. As well, this also has the
> > effect of
> > + * causing all of the chamelium ports to get set to unplugged
> > + */
> > +void chamelium_reset(void)
> > +{
> > + xmlrpc_value *res;
> > +
> > + igt_debug("Resetting the chamelium\n");
> > +
> > + res = xmlrpc_client_call(&env, chamelium_url, "Reset",
> > "()");
> > + check_rpc();
> > +
> > + xmlrpc_DECREF(res);
> > +}
> > +
> > +static void chamelium_exit_handler(int sig)
> > +{
> > + xmlrpc_env_clean(&env);
> > + xmlrpc_env_init(&env);
> > +
> > + chamelium_deinit();
> > +}
> > +
> > +/**
> > + * chamelium_init:
> > + * @drm_fd: drm file descriptor
> > + *
> > + * Sets up a connection with a chamelium, using the URL specified
> > in the
> > + * Chamelium configuration. This must be called first before
> > trying to use the
> > + * chamelium.
> > + *
> > + * If we fail to establish a connection with the chamelium, fail
> > to find a
> > + * configured connector, etc. we fail the current test.
> > + */
> > +void chamelium_init(int drm_fd)
> > +{
> > + xmlrpc_env_init(&env);
> > +
> > + xmlrpc_client_init2(&env, XMLRPC_CLIENT_NO_FLAGS, PACKAGE,
> > + PACKAGE_VERSION, NULL, 0);
> > + igt_fail_on_f(env.fault_occurred,
> > + "Failed to init xmlrpc: %s\n",
> > + env.fault_string);
> > +
> > + chamelium_read_config(drm_fd);
> > + chamelium_reset();
> > +
> > + igt_install_exit_handler(chamelium_exit_handler);
> > +}
> > +
> > +/**
> > + * chamelium_deinit:
> > + *
> > + * Frees the resources used by a connection to the chamelium that
> > was set up
> > + * with #chamelium_init. As well, this function restores the state
> > of the
> > + * chamelium like it was before calling #chamelium_init. This
> > function is also
> > + * called as an exit handler, so users only need to call manually
> > if they don't
> > + * want the chamelium interfering with other tests in the same
> > file.
> > + */
> > +void chamelium_deinit(void)
> > +{
> > + int i;
> > + struct chamelium_edid *pos, *tmp;
> > +
> > + if (!chamelium_url)
> > + return;
> > +
> > + /* We want to make sure we leave all of the ports plugged
> > in, since
> > + * testing setups requiring multiple monitors are probably
> > using the
> > + * chamelium to provide said monitors
> > + */
> > + chamelium_reset();
> > + for (i = 0; i < chamelium_port_count; i++)
> > + chamelium_plug(chamelium_ports[i].id);
> > +
> > + /* Destroy any EDIDs we created to make sure we don't leak
> > them */
> > + igt_list_for_each_safe(pos, tmp, &allocated_edids->link,
> > link) {
> > + chamelium_destroy_edid(pos->id);
> > + free(pos);
> > + }
> > +
> > + xmlrpc_client_cleanup();
> > + xmlrpc_env_clean(&env);
> > +
> > + for (i = 0; i < chamelium_port_count; i++)
> > + free(chamelium_ports[i].connector_name);
> > +
> > + free(chamelium_ports);
> > + allocated_edids = NULL;
> > + chamelium_url = NULL;
> > + chamelium_ports = NULL;
> > + chamelium_port_count = 0;
> > +}
> > +
> > diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
> > new file mode 100644
> > index 0000000..2f7aadf
> > --- /dev/null
> > +++ b/lib/igt_chamelium.h
> > @@ -0,0 +1,73 @@
> > +/*
> > + * Copyright © 2016 Red Hat Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to
> > whom the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice
> > (including the next
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors: Lyude Paul <lyude at redhat.com>
> > + */
> > +
> > +#ifndef IGT_CHAMELIUM_H
> > +#define IGT_CHAMELIUM_H
> > +
> > +#include "config.h"
> > +#include "igt.h"
> > +#include <stdbool.h>
> > +
> > +/**
> > + * chamelium_port:
> > + * @type: The DRM connector type of the chamelium port (not the
> > host's)
> > + * @id: The ID of the chamelium port
> > + * @connector_id: The ID of the DRM connector connected to this
> > port
> > + * @connector_name: The name of the DRM connector
> > + */
> > +struct chamelium_port {
> > + unsigned int type;
> > + int id;
> > + int connector_id;
> > + char *connector_name;
> > +};
> >
> > +extern int chamelium_port_count;
> > +extern struct chamelium_port *chamelium_ports;
>
> Not sure it's a good idea to expose all this to tests. Afterwards
> could be a lot of work to update tests if we decide to do changes.
>
> > +void chamelium_init(int drm_fd);
> > +void chamelium_deinit(void);
> > +void chamelium_reset(void);
> > +
> > +void chamelium_plug(int id);
> > +void chamelium_unplug(int id);
> > +bool chamelium_is_plugged(int id);
> > +bool chamelium_port_wait_video_input_stable(int id, int
> > timeout_secs);
> > +void chamelium_fire_mixed_hpd_pulses(int id, ...);
> > +void chamelium_fire_hpd_pulses(int id, int width_msec, int count);
> > +void chamelium_async_hpd_pulse_start(int id, bool high, int
> > delay_secs);
> > +void chamelium_async_hpd_pulse_finish(void);
> > +int chamelium_new_edid(const unsigned char *edid);
> > +void chamelium_port_set_edid(int id, int edid_id);
> > +bool chamelium_port_get_ddc_state(int id);
> > +void chamelium_port_set_ddc_state(int id, bool enabled);
> > +void chamelium_port_get_resolution(int id, int *x, int *y);
> > +igt_crc_t *chamelium_get_crc_for_area(int id, int x, int y, int w,
> > int h);
> > +void chamelium_start_capture(int id, int x, int y, int w, int h);
> > +void chamelium_stop_capture(int frame_count);
> > +igt_crc_t *chamelium_read_captured_crcs(int *frame_count);
> > +int chamelium_get_frame_limit(int id, int w, int h);
> > +
> > +#endif /* IGT_CHAMELIUM_H */
> > diff --git a/scripts/run-tests.sh b/scripts/run-tests.sh
> > index 97ba9e5..6539bf9 100755
> > --- a/scripts/run-tests.sh
> > +++ b/scripts/run-tests.sh
> > @@ -122,10 +122,10 @@ if [ ! -x "$PIGLIT" ]; then
> > fi
> >
> > if [ "x$RESUME" != "x" ]; then
> > - sudo IGT_TEST_ROOT="$IGT_TEST_ROOT" "$PIGLIT" resume
> > "$RESULTS" $NORETRY
> > + sudo IGT_TEST_ROOT="$IGT_TEST_ROOT"
> > CHAMELIUM_HOST="$CHAMELIUM_HOST" "$PIGLIT" resume "$RESULTS"
> > $NORETRY
> > else
> > mkdir -p "$RESULTS"
> > - sudo IGT_TEST_ROOT="$IGT_TEST_ROOT" "$PIGLIT" run igt -o
> > "$RESULTS" -s $VERBOSE $EXCLUDE $FILTER
> > + sudo IGT_TEST_ROOT="$IGT_TEST_ROOT"
> > CHAMELIUM_HOST="$CHAMELIUM_HOST" "$PIGLIT" run igt -o "$RESULTS" -s
> > $VERBOSE $EXCLUDE $FILTER
> > fi
> >
> > if [ "$SUMMARY" == "html" ]; then
>
> Guess this isn't needed any more?
>
> > diff --git a/tests/Makefile.am b/tests/Makefile.am
> > index a408126..06a8e6b 100644
> > --- a/tests/Makefile.am
> > +++ b/tests/Makefile.am
> > @@ -63,7 +63,7 @@ AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) -Wno-
> > unused-result $(DEBUG_CFLAGS)\
> > $(LIBUNWIND_CFLAGS) $(WERROR_CFLAGS) \
> > $(NULL)
> >
> > -LDADD = ../lib/libintel_tools.la $(GLIB_LIBS)
> > +LDADD = ../lib/libintel_tools.la $(GLIB_LIBS) $(XMLRPC_LIBS)
> >
> > AM_CFLAGS += $(CAIRO_CFLAGS) $(LIBUDEV_CFLAGS) $(GLIB_CFLAGS)
> > AM_LDFLAGS = -Wl,--as-needed
> > @@ -119,5 +119,8 @@ vc4_wait_bo_CFLAGS = $(AM_CFLAGS)
> > $(DRM_VC4_CFLAGS)
> > vc4_wait_bo_LDADD = $(LDADD) $(DRM_VC4_LIBS)
> > vc4_wait_seqno_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
> > vc4_wait_seqno_LDADD = $(LDADD) $(DRM_VC4_LIBS)
> > +
> > +chamelium_CFLAGS = $(AM_CFLAGS) $(XMLRPC_CFLAGS) $(UDEV_CFLAGS)
> > +chamelium_LDADD = $(LDADD) $(XMLRPC_LIBS) $(UDEV_LIBS)
>
> Same as before (the package prefix is LIBUDEV).
>
> > endif
> >
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 57a10c5..5eeae77 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -135,8 +135,13 @@ TESTS_progs_M = \
> > prime_vgem \
> > template \
> > vgem_basic \
> > - vgem_slow \
> > - $(NULL)
> > + vgem_slow
> > +
> > +if HAVE_CHAMELIUM
> > +TESTS_progs_M += chamelium
> > +endif
> > +
> > +TESTS_progs_M += $(NULL)
>
> Same as before.
>
> > TESTS_progs_XM = \
> > gem_concurrent_all \
> > diff --git a/tests/chamelium.c b/tests/chamelium.c
> > new file mode 100644
> > index 0000000..e0f645a
> > --- /dev/null
> > +++ b/tests/chamelium.c
> > @@ -0,0 +1,407 @@
> > +/*
> > + * Copyright © 2016 Red Hat Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to
> > whom the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice
> > (including the next
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + * Lyude Paul <lyude at redhat.com>
> > + */
> > +
> > +#include "config.h"
> > +#include "igt.h"
> > +
> > +#include <fcntl.h>
> > +#include <string.h>
> > +
> > +typedef struct {
> > + int drm_fd;
> > +} data_t;
> > +
> > +#define HOTPLUG_TIMEOUT 20 /* seconds */
> > +
> > +static void
> > +require_connector_present(data_t *data, unsigned int type)
> > +{
> > + int i;
> > + bool found = false;
> > +
> > + for (i = 0; i < chamelium_port_count && !found; i++) {
> > + if (chamelium_ports[i].type == type)
> > + found = true;
> > + }
> > +
> > + igt_require_f(found, "No port of type %s was found\n",
> > + kmstest_connector_type_str(type));
> > +}
> > +
> > +static drmModeConnection
> > +reprobe_connector(data_t *data, struct chamelium_port *port)
> > +{
> > + drmModeConnector *connector;
> > + drmModeConnection status;
> > +
> > + igt_debug("Reprobing %s...\n", port->connector_name);
> > + connector = drmModeGetConnector(data->drm_fd, port-
> > >connector_id);
> > + igt_assert(connector);
> > + status = connector->connection;
> > +
> > + drmModeFreeConnector(connector);
> > + return status;
> > +}
> > +
> > +static void
> > +wait_for_connector(data_t *data, struct chamelium_port *port,
> > + drmModeConnection status)
> > +{
> > + bool finished = false;
> > +
> > + igt_debug("Waiting for %s to %sconnect...\n", port-
> > >connector_name,
> > + status == DRM_MODE_DISCONNECTED ? "dis" : "");
> > +
> > + /*
> > + * Rely on simple reprobing so we don't fail tests that
> > don't require
> > + * that hpd events work in the event that hpd doesn't work
> > on the system
> > + */
> > + igt_until_timeout(HOTPLUG_TIMEOUT) {
> > + if (reprobe_connector(data, port) == status) {
> > + finished = true;
> > + return;
> > + }
> > +
> > + sleep(1);
> > + }
> > +
> > + igt_assert(finished);
> > +}
> > +
> > +static void
> > +reset_state(data_t *data, struct chamelium_port *port)
> > +{
> > + igt_reset_connectors();
> > + chamelium_reset();
> > + wait_for_connector(data, port, DRM_MODE_DISCONNECTED);
> > +}
> > +
> > +static void
> > +test_basic_hotplug(data_t *data, struct chamelium_port *port)
> > +{
> > + int i;
> > +
> > + reset_state(data, port);
> > +
> > + igt_watch_hotplug();
> > +
> > + for (i = 0; i < 15; i++) {
> > + igt_flush_hotplugs();
> > +
> > + /* Check if we get a sysfs hotplug event */
> > + chamelium_plug(port->id);
> > + igt_assert(igt_hotplug_detected(HOTPLUG_TIMEOUT));
> > + igt_assert(reprobe_connector(data, port) ==
> > DRM_MODE_CONNECTED);
>
> Better use igt_assert_eq in case we get DRM_MODE_UNKNOWNCONNECTION?
>
> > +
> > + igt_flush_hotplugs();
> > +
> > + /* Now check if we get a hotplug from disconnection
> > */
> > + chamelium_unplug(port->id);
> > + igt_assert(igt_hotplug_detected(HOTPLUG_TIMEOUT));
> > + igt_assert(reprobe_connector(data, port) ==
> > + DRM_MODE_DISCONNECTED);
>
> Ditto.
>
> > + /* Sleep so we don't accidentally cause an hpd
> > storm */
> > + sleep(1);
>
> Wouldn't such a long sleep unnecessarily slow test runs down?
Yes, but I don't see much of a way around it right now. Eventually I'd
like to add something to i915's debugfs to allow disabling HPD storm
detection (we'll need this as well if we want to eventually add HPD
storm detection tests).
>
> > + }
> > +}
> > +
> > +static void
> > +test_edid_read(data_t *data, struct chamelium_port *port,
> > + int edid_id, const unsigned char *edid)
> > +{
> > + drmModePropertyBlobPtr edid_blob = NULL;
> > + uint64_t edid_blob_id;
> > +
> > + reset_state(data, port);
> > +
> > + chamelium_port_set_edid(port->id, edid_id);
> > + chamelium_plug(port->id);
> > + wait_for_connector(data, port, DRM_MODE_CONNECTED);
> > +
> > + igt_assert(kmstest_get_property(data->drm_fd, port-
> > >connector_id,
> > + DRM_MODE_OBJECT_CONNECTOR,
> > "EDID", NULL,
> > + &edid_blob_id, NULL));
> > + igt_assert(edid_blob = drmModeGetPropertyBlob(data->drm_fd,
> > + edid_blob_id)
> > );
> > +
> > + /* Compare the EDID from the connector to what we expect */
> > + igt_assert(memcmp(edid, edid_blob->data, EDID_LENGTH) ==
> > 0);
>
> Would be cool if we had a igt_assert_mem_eq or such that would print
> the unexpected and expected values to the log (properly capped, of
> course).
I like this and this definitely wouldn't be too difficult to add.
>
> > + drmModeFreePropertyBlob(edid_blob);
> > +}
> > +
> > +static void
> > +test_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > + enum igt_suspend_state state,
> > + enum igt_suspend_test test)
> > +{
> > + int delay = 7;
>
> Would be better to have this and the 15 below as constants so it's
> easier to see what knobs are there at a glance?
>
> > + igt_skip_without_suspend_support(state, test);
> > + reset_state(data, port);
> > + igt_watch_hotplug();
> > +
> > + igt_set_autoresume_delay(15);
> > +
> > + /* Make sure we notice new connectors after resuming */
> > + chamelium_async_hpd_pulse_start(port->id, false, delay);
> > + igt_system_suspend_autoresume(state, test);
> > + chamelium_async_hpd_pulse_finish();
> > +
> > + igt_assert(igt_hotplug_detected(HOTPLUG_TIMEOUT));
> > + igt_assert(reprobe_connector(data, port) ==
> > DRM_MODE_CONNECTED);
> > +
> > + igt_flush_hotplugs();
> > +
> > + /* Now make sure we notice disconnected connectors after
> > resuming */
> > + chamelium_async_hpd_pulse_start(port->id, true, delay);
> > + igt_system_suspend_autoresume(state, test);
> > + chamelium_async_hpd_pulse_finish();
> > +
> > + igt_assert(igt_hotplug_detected(HOTPLUG_TIMEOUT));
> > + igt_assert(reprobe_connector(data, port) ==
> > DRM_MODE_DISCONNECTED);
> > +}
> > +
> > +static void
> > +test_suspend_resume_edid_change(data_t *data, struct
> > chamelium_port *port,
> > + enum igt_suspend_state state,
> > + enum igt_suspend_test test,
> > + int edid_id,
> > + int alt_edid_id)
> > +{
> > + igt_skip_without_suspend_support(state, test);
>
> Maybe igt_require_suspend?
>
> > + reset_state(data, port);
> > + igt_watch_hotplug();
> > +
> > + /* First plug in the port */
> > + chamelium_port_set_edid(port->id, edid_id);
> > + chamelium_plug(port->id);
> > + wait_for_connector(data, port, DRM_MODE_CONNECTED);
> > +
> > + igt_flush_hotplugs();
> > +
> > + /*
> > + * Change the edid before we suspend. On resume, the
> > machine should
> > + * notice the EDID change and fire a hotplug event.
> > + */
> > + chamelium_port_set_edid(port->id, alt_edid_id);
> > +
> > + igt_system_suspend_autoresume(state, test);
> > + igt_assert(igt_hotplug_detected(HOTPLUG_TIMEOUT));
> > +}
> > +
> > +static void
> > +test_display(data_t *data, struct chamelium_port *port)
> > +{
> > + igt_display_t display;
> > + igt_output_t *output;
> > + igt_plane_t *primary;
> > + struct igt_fb fb;
> > + drmModeRes *res;
> > + drmModeModeInfo *mode;
> > + drmModeConnector *connector;
> > + uint32_t crtc_id;
> > + int fb_id;
> > +
> > + reset_state(data, port);
> > +
> > + chamelium_plug(port->id);
> > + wait_for_connector(data, port, DRM_MODE_CONNECTED);
> > + igt_assert(res = drmModeGetResources(data->drm_fd));
> > + kmstest_unset_all_crtcs(data->drm_fd, res);
> > +
> > + igt_display_init(&display, data->drm_fd);
> > +
> > + /* Find the output struct for this connector */
> > + for_each_connected_output(&display, output) {
> > + if (output->config.connector->connector_id ==
> > + port->connector_id)
> > + break;
>
> Are we that sure that we'll always find such an output? Could be
> quite
> disconcerting if that stopped being true, even if by a bug.
Not sure I follow here: that's what the wait_for_connector() call is
there for. I'd expect that we could make the assumption we'll find a
connector if we've managed to make it that far without failing.
>
> > + }
> > +
> > + connector = drmModeGetConnectorCurrent(data->drm_fd,
> > + port->connector_id);
> > +
> > + /* Find a spare CRTC to use for the display */
> > + crtc_id = kmstest_find_crtc_for_connector(data->drm_fd,
> > res, connector,
> > + 0);
> > +
> > + /* Setup the display */
> > + igt_output_set_pipe(output,
> > kmstest_get_pipe_from_crtc_id(data->drm_fd,
> > + c
> > rtc_id));
> > + mode = igt_output_get_mode(output);
> > + primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> > + igt_assert(primary);
> > +
> > + fb_id = igt_create_pattern_fb(data->drm_fd,
> > + mode->hdisplay,
> > + mode->vdisplay,
> > + DRM_FORMAT_XRGB8888,
> > + LOCAL_DRM_FORMAT_MOD_NONE,
> > + &fb);
> > + igt_assert(fb_id > 0);
> > + igt_plane_set_fb(primary, &fb);
> > +
> > + igt_display_commit(&display);
> > +
> > + igt_assert(chamelium_port_wait_video_input_stable(port->id,
> > + HOTPLUG_T
> > IMEOUT));
>
> If we have all this already, wouldn't make sense to also test that
> the
> CRC functions work as expected?
I did add some debugging code in while writing those functions to make
sure all of them worked, but I suppose it would be a good idea to add
some as actual tests.
Anyway, I'll get to re-spinning a new version of this series with all
of these changes.
>
> > + drmModeFreeResources(res);
> > + drmModeFreeConnector(connector);
> > + igt_display_fini(&display);
> > +}
> > +
> > +static void
> > +test_hpd_without_ddc(data_t *data, struct chamelium_port *port)
> > +{
> > + reset_state(data, port);
> > + igt_watch_hotplug();
> > +
> > + /* Disable the DDC on the connector and make sure we still
> > get a
> > + * hotplug
> > + */
> > + chamelium_port_set_ddc_state(port->id, false);
> > + chamelium_plug(port->id);
> > +
> > + igt_assert(igt_hotplug_detected(HOTPLUG_TIMEOUT));
> > + igt_assert(reprobe_connector(data, port) ==
> > DRM_MODE_CONNECTED);
> > +}
> > +
> > +#define for_each_port(p, port) \
> > + for (p = 0, port = &chamelium_ports[p]; \
> > + p < chamelium_port_count; \
> > + p++, port = &chamelium_ports[p]) \
> > +
> > +#define connector_subtest(name__, type__) \
> > + igt_subtest(name__) \
> > + for_each_port(p, port) \
> > + if (port->type == DRM_MODE_CONNECTOR_ ##
> > type__)
> > +
> > +#define define_common_connector_tests(type_str__,
> > type__) \
> > + connector_subtest(type_str__ "-hpd",
> > type__) \
> > + test_basic_hotplug(&data,
> > port); \
> > +
> > \
> > + connector_subtest(type_str__ "-edid-read", type__)
> > { \
> > + test_edid_read(&data, port,
> > edid_id, \
> > + igt_kms_get_base_edid());
> > \
> > + test_edid_read(&data, port,
> > alt_edid_id, \
> > + igt_kms_get_alt_edid());
> > \
> > + }
> > \
> > +
> > \
> > + connector_subtest(type_str__ "-hpd-after-suspend",
> > type__) \
> > + test_suspend_resume_hpd(&data,
> > port, \
> > + SUSPEND_STATE_MEM,
> > \
> > + SUSPEND_TEST_NONE);
> > \
> > +
> > \
> > + connector_subtest(type_str__ "-hpd-after-hibernate",
> > type__) \
> > + test_suspend_resume_hpd(&data,
> > port, \
> > + SUSPEND_STATE_DISK,
> > \
> > + SUSPEND_TEST_DEVICES);
> > \
> > +
> > \
> > + connector_subtest(type_str__ "-edid-change-during-suspend",
> > type__) \
> > + test_suspend_resume_edid_change(&data,
> > port, \
> > + SUSPEND_STATE_MEM,
> > \
> > + SUSPEND_TEST_NONE,
> > \
> > + edid_id,
> > alt_edid_id); \
> > +
> > \
> > + connector_subtest(type_str__ "-edid-change-during-
> > hibernate", type__) \
> > + test_suspend_resume_edid_change(&data,
> > port, \
> > + SUSPEND_STATE_DISK,
> > \
> > + SUSPEND_TEST_DEVICE
> > S, \
> > + edid_id,
> > alt_edid_id); \
> > +
> > \
> > + connector_subtest(type_str__ "-display",
> > type__) \
> > + test_display(&data, port);
>
> Not sure about the others, but I think I would prefer if this was C
> code.
>
> > +static data_t data;
> > +
> > +igt_main
> > +{
> > + struct chamelium_port *port;
> > + int edid_id, alt_edid_id, p;
> > +
> > + igt_fixture {
> > + igt_skip_on_simulation();
> > +
> > + data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> > +
> > + chamelium_init(data.drm_fd);
> > +
> > + edid_id =
> > chamelium_new_edid(igt_kms_get_base_edid());
> > + alt_edid_id =
> > chamelium_new_edid(igt_kms_get_alt_edid());
> > +
> > + /* So fbcon doesn't try to reprobe things itself */
> > + kmstest_set_vt_graphics_mode();
> > + }
> > +
> > + igt_subtest_group {
> > + igt_fixture {
> > + require_connector_present(
> > + &data, DRM_MODE_CONNECTOR_DisplayPort);
> > + }
> > +
> > + define_common_connector_tests("dp", DisplayPort);
> > + }
> > +
> > + igt_subtest_group {
> > + igt_fixture {
> > + require_connector_present(
> > + &data, DRM_MODE_CONNECTOR_HDMIA);
> > + }
> > +
> > + define_common_connector_tests("hdmi", HDMIA);
> > + }
> > +
> > + igt_subtest_group {
> > + igt_fixture {
> > + require_connector_present(
> > + &data, DRM_MODE_CONNECTOR_VGA);
> > + }
> > +
> > + connector_subtest("vga-hpd", VGA)
> > + test_basic_hotplug(&data, port);
> > +
> > + connector_subtest("vga-edid-read", VGA) {
> > + test_edid_read(&data, port, edid_id,
> > + igt_kms_get_base_edid());
> > + test_edid_read(&data, port, alt_edid_id,
> > + igt_kms_get_alt_edid());
> > + }
> > +
> > + /* FIXME: Right now there isn't a way to do any
> > sort of delayed
> > + * psuedo-hotplug with VGA, so testing detection
> > after a
> > + * suspend/resume cycle isn't possible yet
> > + */
> > +
> > + connector_subtest("vga-hpd-without-ddc", VGA)
> > + test_hpd_without_ddc(&data, port);
> > +
> > + connector_subtest("vga-display", VGA)
> > + test_display(&data, port);
> > + }
> > +}
> > --
> > 2.9.3
> >
--
Cheers,
Lyude
More information about the Intel-gfx
mailing list