Re: [PATCH i-g-t v2] tests/chamelium: Add common suspend and hibernate tests for DP and HDMI

[Lyude Paul <lyude@redhat.com>]

On Fri, 2017-06-16 at 17:17 +0300, Paul Kocialkowski wrote:
> This adds two new tests: common-hpd-after-suspend and
> common-hpd-after-hibernate that are aimed at testing HPD change during
> suspend/hibernate for both DP and HDMI, at the same time.
> 
> The interest in bringing this test up is to reduce the time spent in
> testing, with the downside of less precision regarding the test's
> outcome. The per-connector tests are still available to get a more
> precise idea of which connector causes a failure, when that happens.
> 
> VGA is explicitly excluded from this test as there is currently no
> way of doing delayed hpd pulses for it.
> 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> ---
>  tests/chamelium.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/tests/chamelium.c b/tests/chamelium.c
> index 76b14e95..d569c8ee 100644
> --- a/tests/chamelium.c
> +++ b/tests/chamelium.c
> @@ -241,6 +241,80 @@ test_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
>  }
>  
>  static void
> +test_suspend_resume_hpd_common(data_t *data, enum igt_suspend_state state,
> +			       enum igt_suspend_test test)
> +{
> +	struct udev_monitor *mon = igt_watch_hotplug();
> +	struct chamelium_port *port;
> +	int p;
> +
> +	for (p = 0; p < data->port_count; p++) {
> +		port = data->ports[p];
> +		if (chamelium_port_get_type(port) == DRM_MODE_CONNECTOR_VGA)
> +			continue;
> +
> +		reset_state(data, port);
> +
> +		igt_debug("Testing port %s\n", chamelium_port_get_name(port));
> +	}
> +
> +	igt_set_autoresume_delay(SUSPEND_RESUME_DELAY);
> +	igt_flush_hotplugs(mon);
> +
> +	/* Make sure we notice new connectors after resuming */
> +	for (p = 0; p < data->port_count; p++) {
> +		port = data->ports[p];
> +		if (chamelium_port_get_type(port) == DRM_MODE_CONNECTOR_VGA)
> +			continue;
> +
> +		chamelium_async_hpd_pulse_start(data->chamelium, port, false,
> +						SUSPEND_RESUME_DELAY / 2);
> +	}
> +
> +	igt_system_suspend_autoresume(state, test);
> +	chamelium_async_hpd_pulse_finish(data->chamelium);
> +
> +	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> +
> +	for (p = 0; p < data->port_count; p++) {
> +		port = data->ports[p];
> +		if (chamelium_port_get_type(port) == DRM_MODE_CONNECTOR_VGA)
> +			continue;
> +
> +		igt_assert_eq(reprobe_connector(data, port),
> +			      DRM_MODE_CONNECTED);
> +	}
> +
> +	igt_flush_hotplugs(mon);
> +
> +	/* Now make sure we notice disconnected connectors after resuming */
> +	for (p = 0; p < data->port_count; p++) {
> +		port = data->ports[p];
> +		if (chamelium_port_get_type(port) == DRM_MODE_CONNECTOR_VGA)
> +			continue;
> +
> +		chamelium_async_hpd_pulse_start(data->chamelium, port, true,
> +						SUSPEND_RESUME_DELAY / 2);
> +	}
> +
> +	igt_system_suspend_autoresume(state, test);
> +	chamelium_async_hpd_pulse_finish(data->chamelium);
> +
> +	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> +
> +	for (p = 0; p < data->port_count; p++) {
> +		port = data->ports[p];
> +		if (chamelium_port_get_type(port) == DRM_MODE_CONNECTOR_VGA)
> +			continue;
> +
> +		igt_assert_eq(reprobe_connector(data, port),
> +			      DRM_MODE_DISCONNECTED);
> +	}
> +
> +	igt_cleanup_hotplug(mon);
> +}
> +
> +static void

For smaller bits of code this isn't a big deal since we're talking
about tests here, but this is a lot of code to have for a single
function like this. Mind splitting this into something like (feel free
to take some liberty with the function naming)

test_suspend_resume_hpd(data_t *data, enum igt_suspend_state state, enum igt_suspend_test test)
  → try_suspend_resume_hpd(data_t *data, enum igt_suspend_state state, enum igt_suspend_test test, bool connected)

Other then that, looks good. With the changes mentioned:

Reviewed-by: Lyude Paul <lyude@redhat.com>

>  test_suspend_resume_edid_change(data_t *data, struct chamelium_port *port,
>  				enum igt_suspend_state state,
>  				enum igt_suspend_test test,
> @@ -750,6 +824,18 @@ igt_main
>  			test_hpd_without_ddc(&data, port);
>  	}
>  
> +	igt_subtest_group {
> +		igt_subtest("common-hpd-after-suspend")
> +			test_suspend_resume_hpd_common(&data,
> +						       SUSPEND_STATE_MEM,
> +						       SUSPEND_TEST_NONE);
> +
> +		igt_subtest("common-hpd-after-hibernate")
> +			test_suspend_resume_hpd_common(&data,
> +						       SUSPEND_STATE_DISK,
> +						       SUSPEND_TEST_DEVICES);
> +	}
> +
>  	igt_fixture {
>  		close(data.drm_fd);
>  	}
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx