Re: [igt-dev] [PATCH i-g-t 9/9] lib/kms: Try to plug all Chamelium ports, abort if it fails

Petri Latvala
2020-02-14T12:27:24Z

On Wed, Feb 12, 2020 at 03:23:49PM +0200, Arkadiusz Hiler wrote:
> Using chamelium as a display for non-chamelium-aware test is
> challenging. The board can be left in multiple different states after
> kms_chamelium tests even though we have atexit() handlers and other
> measures which try to assure that all ports are plugged in. Sadly this
> is not 100% reliable. We also had a few boards hard hanging (happens
> very seldom) and requiring manual intervention.
>
> This leads to changes in the testing configuration - we may end up with
> any number of connectors plugged in which makes a lot of kms_ tests to
> flip between skip and pass depending on a run.
>
> In an attempt to make connectors state less random this patch makes
> igt_display_require() chamelium-aware. If chamelium is configured for
> given machine we try to reach it and make sure everything is plugged in.
> If we fail to do so we abort the execution because the testing
> configuration is an unknown.
>
> For machines without a configured chamelium this boils down to a nop.
>
> I have run a bunch of tests and measured how much time we spend in the
> Chamelium section of igt_display_require() (n = 1000) with chamelium
> configured:
>
> Min: 0.0030s Max: 0.0113s
> Avg: 0.0089s Median: 0.0089s
>
> With ~1000 of KMS subtests in a run it's only a mere 9s.
>
> Fixes: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/20
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
> lib/igt_chamelium.c | 55 +++++++++++++++++++++++++++++++++++--
> lib/igt_chamelium.h | 1 +
> lib/igt_kms.c | 18 ++++++++++++
> tests/kms_chamelium.c | 7 +++--
> tests/kms_color_chamelium.c | 7 +++--
> 5 files changed, 80 insertions(+), 8 deletions(-)
>
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index d5d8dc60..cdf0e3ad 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -1978,7 +1978,13 @@ static size_t chamelium_get_video_ports(struct chamelium *chamelium,
> int res_len, i, port_id;
> size_t port_ids_len = 0;
>
> - res = chamelium_rpc(chamelium, NULL, "GetSupportedInputs", "()");
> + res = __chamelium_rpc(chamelium, NULL, "GetSupportedInputs", "()");
> + if (chamelium->env.fault_occurred) {
> + igt_debug("Chamelium RPC call failed: %s\n",
> + chamelium->env.fault_string);
> +
> + return -1;
> + }
> res_len = xmlrpc_array_size(&chamelium->env, res);
> for (i = 0; i < res_len; i++) {
> xmlrpc_array_read_item(&chamelium->env, res, i, &res_port);
> @@ -2181,6 +2187,8 @@ static bool chamelium_autodiscover(struct chamelium *chamelium, int drm_fd)
> candidate_ports_len = chamelium_get_video_ports(chamelium,
> candidate_ports);
>
> + igt_assert(candidate_ports_len > 0);
> +
> igt_debug("Starting Chamelium port auto-discovery on %zu ports\n",
> candidate_ports_len);
> igt_gettime(&start);
> @@ -2299,14 +2307,14 @@ static bool chamelium_read_config(struct chamelium *chamelium)
> GError *error = NULL;
>
> if (!igt_key_file) {
> - igt_warn("No configuration file available for chamelium\n");
> + igt_debug("No configuration file available for chamelium\n");
> return false;
> }
>
> chamelium->url = g_key_file_get_string(igt_key_file, "Chamelium", "URL",
> &error);
> if (!chamelium->url) {
> - igt_warn("Couldn't read chamelium URL from config file: %s\n",
> + igt_debug("Couldn't read chamelium URL from config file: %s\n",
> error->message);
> return false;
> }
> @@ -2402,6 +2410,9 @@ error:
> * Chamelium configuration. This must be called first before trying to use the
> * chamelium.
> *
> + * Needs to happen *after* igt_display_require() as otherwise the board will
> + * get reset.
> + *
> * If we fail to establish a connection with the chamelium, fail to find a
> * configured connector, etc. we fail the current test.
> *
> @@ -2482,6 +2493,44 @@ void chamelium_deinit(struct chamelium *chamelium)
> free(chamelium);
> }
>
> +bool chamelium_plug_all(struct chamelium *chamelium)
> +{
> + size_t port_count;
> + int port_ids[CHAMELIUM_MAX_PORTS];
> + xmlrpc_value *v;
> + v = __chamelium_rpc(chamelium, NULL, "Reset", "()");
> +
> + if (v != NULL)
> + xmlrpc_DECREF(v);
> +
> + if (chamelium->env.fault_occurred) {
> + igt_debug("Chamelium RPC call failed: %s\n",
> + chamelium->env.fault_string);
> +
> + return false;
> + }
> +
> + port_count = chamelium_get_video_ports(chamelium, port_ids);
> + if (port_count <= 0)
> + return false;
> +
> + for (int i = 0; i < port_count; ++i) {
> + v = __chamelium_rpc(chamelium, NULL, "Plug", "(i)", port_ids[i]);
> +
> + if (v != NULL)
> + xmlrpc_DECREF(v);
> +
> + if (chamelium->env.fault_occurred) {
> + igt_debug("Chamelium RPC call failed: %s\n",
> + chamelium->env.fault_string);
> +
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> igt_constructor {
> /* Frame dumps can be large, so we need to be able to handle very large
> * responses
> diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
> index 3b9a1242..fb273b1a 100644
> --- a/lib/igt_chamelium.h
> +++ b/lib/igt_chamelium.h
> @@ -217,5 +217,6 @@ void chamelium_crop_analog_frame(struct chamelium_frame_dump *dump, int width,
> void chamelium_destroy_frame_dump(struct chamelium_frame_dump *dump);
> void chamelium_destroy_audio_file(struct chamelium_audio_file *audio_file);
> void chamelium_infoframe_destroy(struct chamelium_infoframe *infoframe);
> +bool chamelium_plug_all(struct chamelium *chamelium);
>
> #endif /* IGT_CHAMELIUM_H */
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 54de45e5..7f9fafb3 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -58,6 +58,9 @@
> #include "igt_device.h"
> #include "igt_sysfs.h"
> #include "sw_sync.h"
> +#ifdef HAVE_CHAMELIUM
> +#include "igt_chamelium.h"
> +#endif
>
> /**
> * SECTION:igt_kms
> @@ -1886,6 +1889,21 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> if (!resources)
> goto out;
>
> +#ifdef HAVE_CHAMELIUM
> + {
> + struct chamelium *chamelium;
> +
> + chamelium = chamelium_init_rpc_only();
> + if (chamelium) {
> + igt_abort_on_f(!chamelium_wait_reachable(chamelium, 20),
> + "cannot reach the configured chamelium!\n");
> + igt_abort_on_f(!chamelium_plug_all(chamelium),
> + "failed to plug all the chamelium ports!\n");
> + chamelium_deinit_rpc_only(chamelium);
> + }
> + }
> +#endif
> +
> /*
> * We cache the number of pipes, that number is a physical limit of the
> * hardware and cannot change of time (for now, at least).
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 8243d2ef..34e6dda9 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -2623,6 +2623,10 @@ igt_main
>
> igt_fixture {
> data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> + igt_display_require(&data.display, data.drm_fd);
> + igt_require(data.display.is_atomic);
> +
> + /* we need to initalize chamelium after igt_display_require */
> data.chamelium = chamelium_init(data.drm_fd);
> igt_require(data.chamelium);
>
> @@ -2636,9 +2640,6 @@ igt_main
>
> /* So fbcon doesn't try to reprobe things itself */
> kmstest_set_vt_graphics_mode();
> -
> - igt_display_require(&data.display, data.drm_fd);
> - igt_require(data.display.is_atomic);
> }

For this and kms_color_chamelium: Does it matter that
kmstest_set_vt_graphics_mode() is now after igt_display_require?

--
Petri Latvala