diff mbox

[i-g-t,7/7] tests/chamelium: Disconnect connectors without extra reset

Message ID 20170626135906.32708-7-paul.kocialkowski@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paul Kocialkowki June 26, 2017, 1:59 p.m. UTC
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(-)

Comments

Lyude Paul June 26, 2017, 10:17 p.m. UTC | #1
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,
Paul Kocialkowki June 27, 2017, 7:22 a.m. UTC | #2
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 mbox

Patch

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,