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

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

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,
-- 
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