Hi Lyude, this looks very good. Some minor comments below. Regards, Tomeu On 1 December 2016 at 02:24, Lyude <lyude@xxxxxxxxxx> 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@xxxxxxxxxx> > > 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@xxxxxxxxxx> > + */ > + > +#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/chromium-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? > + */ > + > +/** > + * 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? > +} > + > +/** > + * 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. > + */ > +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(connector->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@xxxxxxxxxx> > + */ > + > +#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@xxxxxxxxxx> > + */ > + > +#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? > + } > +} > + > +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). > + 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. > + } > + > + 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, > + crtc_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_TIMEOUT)); If we have all this already, wouldn't make sense to also test that the CRC functions work as expected? > + 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_DEVICES, \ > + 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 > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
