| Message ID | 20170626135906.32708-7-paul.kocialkowski@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2017-06-26 at 16:59 +0300, Paul Kocialkowski wrote: > This removes the reset call from the reset_state function and renames > it > to disconnect_connector. Since a call to reset is already done in > chamelium_init, there is no need to do an extra reset in each test. > > This allows reducing the execution time a bit. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> NAK It's annoying that we have to reset the connectors so much, but unfortunately there is a reason in this case. The problem is that chamelium_init() does not get called once per test. At first glance when running through ./scripts/run_tests.sh this might appear to be the case, however if you run the test by manually calling the built chamelium binary (generally useful for debugging the tests themselves with gdb) it won't actually get called in-between tests. This being said, I'm all for getting rid of a couple extraneous hotplug resets if possible, but I'd prefer to avoid breaking the ability to run the chamelium test binary manually. > --- > tests/chamelium.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/tests/chamelium.c b/tests/chamelium.c > index 01ae4cd7..7d6893da 100644 > --- a/tests/chamelium.c > +++ b/tests/chamelium.c > @@ -144,17 +144,17 @@ wait_for_connector(data_t *data, struct > chamelium_port *port, > } > > static void > -reset_state(data_t *data, struct chamelium_port *port) > +disconnect_connector(data_t *data, struct chamelium_port *port) > { > int p; > > - chamelium_reset(data->chamelium); > - > if (port) { > + chamelium_unplug(data->chamelium, port); > wait_for_connector(data, port, > DRM_MODE_DISCONNECTED, false); > } else { > for (p = 0; p < data->port_count; p++) { > port = data->ports[p]; > + chamelium_unplug(data->chamelium, port); > wait_for_connector(data, port, > DRM_MODE_DISCONNECTED, > false); > } > @@ -167,7 +167,7 @@ test_basic_hotplug(data_t *data, struct > chamelium_port *port, int toggle_count) > struct udev_monitor *mon = igt_watch_hotplug(); > int i; > > - reset_state(data, NULL); > + disconnect_connector(data, NULL); > igt_hpd_storm_set_threshold(data->drm_fd, 0); > > for (i = 0; i < toggle_count; i++) { > @@ -201,7 +201,7 @@ test_edid_read(data_t *data, struct > chamelium_port *port, > data->chamelium, port, false); > uint64_t edid_blob_id; > > - reset_state(data, port); > + disconnect_connector(data, port); > > chamelium_port_set_edid(data->chamelium, port, edid_id); > chamelium_plug(data->chamelium, port); > @@ -270,7 +270,7 @@ test_suspend_resume_hpd(data_t *data, struct > chamelium_port *port, > { > struct udev_monitor *mon = igt_watch_hotplug(); > > - reset_state(data, port); > + disconnect_connector(data, port); > > /* Make sure we notice new connectors after resuming */ > try_suspend_resume_hpd(data, port, state, test, mon, false); > @@ -294,7 +294,7 @@ test_suspend_resume_hpd_common(data_t *data, enum > igt_suspend_state state, > igt_debug("Testing port %s\n", > chamelium_port_get_name(port)); > } > > - reset_state(data, NULL); > + disconnect_connector(data, NULL); > > /* Make sure we notice new connectors after resuming */ > try_suspend_resume_hpd(data, NULL, state, test, mon, false); > @@ -315,7 +315,7 @@ test_suspend_resume_edid_change(data_t *data, > struct chamelium_port *port, > struct udev_monitor *mon = igt_watch_hotplug(); > int delay = chamelium_get_suspend_resume_delay(data- > >chamelium); > > - reset_state(data, port); > + disconnect_connector(data, port); > > igt_set_autoresume_delay(delay); > > @@ -585,7 +585,7 @@ test_hpd_without_ddc(data_t *data, struct > chamelium_port *port) > { > struct udev_monitor *mon = igt_watch_hotplug(); > > - reset_state(data, port); > + disconnect_connector(data, port); > igt_flush_hotplugs(mon); > > /* Disable the DDC on the connector and make sure we still > get a > @@ -607,7 +607,7 @@ test_hpd_storm_detect(data_t *data, struct > chamelium_port *port, int width) > int count = 0; > > igt_require_hpd_storm_ctl(data->drm_fd); > - reset_state(data, port); > + disconnect_connector(data, port); > > igt_hpd_storm_set_threshold(data->drm_fd, 1); > chamelium_fire_hpd_pulses(data->chamelium, port, width, 10); > @@ -632,7 +632,7 @@ static void > test_hpd_storm_disable(data_t *data, struct chamelium_port *port, > int width) > { > igt_require_hpd_storm_ctl(data->drm_fd); > - reset_state(data, port); > + disconnect_connector(data, port); > > igt_hpd_storm_set_threshold(data->drm_fd, 0); > chamelium_fire_hpd_pulses(data->chamelium, port,
Hey, On Mon, 2017-06-26 at 18:17 -0400, Lyude Paul wrote: > On Mon, 2017-06-26 at 16:59 +0300, Paul Kocialkowski wrote: > > This removes the reset call from the reset_state function and renames > > it > > to disconnect_connector. Since a call to reset is already done in > > chamelium_init, there is no need to do an extra reset in each test. > > > > This allows reducing the execution time a bit. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> > > NAK > It's annoying that we have to reset the connectors so much, but > unfortunately there is a reason in this case. The problem is that > chamelium_init() does not get called once per test. At first glance > when running through ./scripts/run_tests.sh this might appear to be the > case, however if you run the test by manually calling the built > chamelium binary (generally useful for debugging the tests themselves > with gdb) it won't actually get called in-between tests. I am well aware of that situation and made sure that it works correctly in that case too. Disconnecting the connector is really all that should be needed, and resetting the whole chamelium seems overkill. There was one occurence where not fully resetting caused an issue when running the whole test binary on i915 (when switching from poll-based detection to irq after hpd storm testing), which was definitely a kernel bug. I submitted a patch to fix it already. So this can even help discover new kernel bugs! > This being said, I'm all for getting rid of a couple extraneous hotplug > resets if possible, but I'd prefer to avoid breaking the ability to run > the chamelium test binary manually. Yeah, the real problem was that when running tests standalone, two resets are done in a row. So worst case, we could ditch the reset at init and make sure each test calls it. Thanks for the review! > > --- > > tests/chamelium.c | 22 +++++++++++----------- > > 1 file changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/tests/chamelium.c b/tests/chamelium.c > > index 01ae4cd7..7d6893da 100644 > > --- a/tests/chamelium.c > > +++ b/tests/chamelium.c > > @@ -144,17 +144,17 @@ wait_for_connector(data_t *data, struct > > chamelium_port *port, > > } > > > > static void > > -reset_state(data_t *data, struct chamelium_port *port) > > +disconnect_connector(data_t *data, struct chamelium_port *port) > > { > > int p; > > > > - chamelium_reset(data->chamelium); > > - > > if (port) { > > + chamelium_unplug(data->chamelium, port); > > wait_for_connector(data, port, > > DRM_MODE_DISCONNECTED, false); > > } else { > > for (p = 0; p < data->port_count; p++) { > > port = data->ports[p]; > > + chamelium_unplug(data->chamelium, port); > > wait_for_connector(data, port, > > DRM_MODE_DISCONNECTED, > > false); > > } > > @@ -167,7 +167,7 @@ test_basic_hotplug(data_t *data, struct > > chamelium_port *port, int toggle_count) > > struct udev_monitor *mon = igt_watch_hotplug(); > > int i; > > > > - reset_state(data, NULL); > > + disconnect_connector(data, NULL); > > igt_hpd_storm_set_threshold(data->drm_fd, 0); > > > > for (i = 0; i < toggle_count; i++) { > > @@ -201,7 +201,7 @@ test_edid_read(data_t *data, struct > > chamelium_port *port, > > data->chamelium, port, false); > > uint64_t edid_blob_id; > > > > - reset_state(data, port); > > + disconnect_connector(data, port); > > > > chamelium_port_set_edid(data->chamelium, port, edid_id); > > chamelium_plug(data->chamelium, port); > > @@ -270,7 +270,7 @@ test_suspend_resume_hpd(data_t *data, struct > > chamelium_port *port, > > { > > struct udev_monitor *mon = igt_watch_hotplug(); > > > > - reset_state(data, port); > > + disconnect_connector(data, port); > > > > /* Make sure we notice new connectors after resuming */ > > try_suspend_resume_hpd(data, port, state, test, mon, false); > > @@ -294,7 +294,7 @@ test_suspend_resume_hpd_common(data_t *data, enum > > igt_suspend_state state, > > igt_debug("Testing port %s\n", > > chamelium_port_get_name(port)); > > } > > > > - reset_state(data, NULL); > > + disconnect_connector(data, NULL); > > > > /* Make sure we notice new connectors after resuming */ > > try_suspend_resume_hpd(data, NULL, state, test, mon, false); > > @@ -315,7 +315,7 @@ test_suspend_resume_edid_change(data_t *data, > > struct chamelium_port *port, > > struct udev_monitor *mon = igt_watch_hotplug(); > > int delay = chamelium_get_suspend_resume_delay(data- > > > chamelium); > > > > > > - reset_state(data, port); > > + disconnect_connector(data, port); > > > > igt_set_autoresume_delay(delay); > > > > @@ -585,7 +585,7 @@ test_hpd_without_ddc(data_t *data, struct > > chamelium_port *port) > > { > > struct udev_monitor *mon = igt_watch_hotplug(); > > > > - reset_state(data, port); > > + disconnect_connector(data, port); > > igt_flush_hotplugs(mon); > > > > /* Disable the DDC on the connector and make sure we still > > get a > > @@ -607,7 +607,7 @@ test_hpd_storm_detect(data_t *data, struct > > chamelium_port *port, int width) > > int count = 0; > > > > igt_require_hpd_storm_ctl(data->drm_fd); > > - reset_state(data, port); > > + disconnect_connector(data, port); > > > > igt_hpd_storm_set_threshold(data->drm_fd, 1); > > chamelium_fire_hpd_pulses(data->chamelium, port, width, 10); > > @@ -632,7 +632,7 @@ static void > > test_hpd_storm_disable(data_t *data, struct chamelium_port *port, > > int width) > > { > > igt_require_hpd_storm_ctl(data->drm_fd); > > - reset_state(data, port); > > + disconnect_connector(data, port); > > > > igt_hpd_storm_set_threshold(data->drm_fd, 0); > > chamelium_fire_hpd_pulses(data->chamelium, port,
diff --git a/tests/chamelium.c b/tests/chamelium.c index 01ae4cd7..7d6893da 100644 --- a/tests/chamelium.c +++ b/tests/chamelium.c @@ -144,17 +144,17 @@ wait_for_connector(data_t *data, struct chamelium_port *port, } static void -reset_state(data_t *data, struct chamelium_port *port) +disconnect_connector(data_t *data, struct chamelium_port *port) { int p; - chamelium_reset(data->chamelium); - if (port) { + chamelium_unplug(data->chamelium, port); wait_for_connector(data, port, DRM_MODE_DISCONNECTED, false); } else { for (p = 0; p < data->port_count; p++) { port = data->ports[p]; + chamelium_unplug(data->chamelium, port); wait_for_connector(data, port, DRM_MODE_DISCONNECTED, false); } @@ -167,7 +167,7 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port, int toggle_count) struct udev_monitor *mon = igt_watch_hotplug(); int i; - reset_state(data, NULL); + disconnect_connector(data, NULL); igt_hpd_storm_set_threshold(data->drm_fd, 0); for (i = 0; i < toggle_count; i++) { @@ -201,7 +201,7 @@ test_edid_read(data_t *data, struct chamelium_port *port, data->chamelium, port, false); uint64_t edid_blob_id; - reset_state(data, port); + disconnect_connector(data, port); chamelium_port_set_edid(data->chamelium, port, edid_id); chamelium_plug(data->chamelium, port); @@ -270,7 +270,7 @@ test_suspend_resume_hpd(data_t *data, struct chamelium_port *port, { struct udev_monitor *mon = igt_watch_hotplug(); - reset_state(data, port); + disconnect_connector(data, port); /* Make sure we notice new connectors after resuming */ try_suspend_resume_hpd(data, port, state, test, mon, false); @@ -294,7 +294,7 @@ test_suspend_resume_hpd_common(data_t *data, enum igt_suspend_state state, igt_debug("Testing port %s\n", chamelium_port_get_name(port)); } - reset_state(data, NULL); + disconnect_connector(data, NULL); /* Make sure we notice new connectors after resuming */ try_suspend_resume_hpd(data, NULL, state, test, mon, false); @@ -315,7 +315,7 @@ test_suspend_resume_edid_change(data_t *data, struct chamelium_port *port, struct udev_monitor *mon = igt_watch_hotplug(); int delay = chamelium_get_suspend_resume_delay(data->chamelium); - reset_state(data, port); + disconnect_connector(data, port); igt_set_autoresume_delay(delay); @@ -585,7 +585,7 @@ test_hpd_without_ddc(data_t *data, struct chamelium_port *port) { struct udev_monitor *mon = igt_watch_hotplug(); - reset_state(data, port); + disconnect_connector(data, port); igt_flush_hotplugs(mon); /* Disable the DDC on the connector and make sure we still get a @@ -607,7 +607,7 @@ test_hpd_storm_detect(data_t *data, struct chamelium_port *port, int width) int count = 0; igt_require_hpd_storm_ctl(data->drm_fd); - reset_state(data, port); + disconnect_connector(data, port); igt_hpd_storm_set_threshold(data->drm_fd, 1); chamelium_fire_hpd_pulses(data->chamelium, port, width, 10); @@ -632,7 +632,7 @@ static void test_hpd_storm_disable(data_t *data, struct chamelium_port *port, int width) { igt_require_hpd_storm_ctl(data->drm_fd); - reset_state(data, port); + disconnect_connector(data, port); igt_hpd_storm_set_threshold(data->drm_fd, 0); chamelium_fire_hpd_pulses(data->chamelium, port,
This removes the reset call from the reset_state function and renames it to disconnect_connector. Since a call to reset is already done in chamelium_init, there is no need to do an extra reset in each test. This allows reducing the execution time a bit. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> --- tests/chamelium.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)