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@xxxxxxxxxxxxxxx>
> >
> > ---
> > 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@xxxxxxxxxx>
>
> > 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@xxxxxxxxxxxxxxx>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
