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

[Paul Kocialkowski <paul.kocialkowski@linux.intel.com>]

Hi,

On Fri, 2017-06-16 at 19:28 -0400, Lyude Paul wrote:
> 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)

That is quite right, this is indeed starting to look too big. Thanks for
pointing it out. I'll send v2 with a dedicated function as suggested.

> 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);
> >  	}
-- 
Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx