[igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround

[Souza, Jose]

On Thu, 2019-03-14 at 18:59 +0200, Imre Deak wrote:
> Hi Jose,
> 
> On Tue, Mar 12, 2019 at 05:59:45PM -0700, José Roberto de Souza
> wrote:
> > It is know that some unpowered type-c dongles can take some time to
> > boot and be responsible in the DDC/aux transaction lines so a
> > workaround was implemented in kernel(drm/i915: Enable hotplug
> > retry)
> > to fix it but is possible that this could happen to other DP sinks.
> > 
> > So this test will try to simulate the sceneario described above, it
> > will disable the DDC lines and plug the connector, the hotplug
> > should
> > fail and then enabling the DDC lines kernel should report the
> > connector as connected.
> > 
> > The workaround will reprobe connector after 1 second after kernel
> > gives up on the first try to probe the connector, so that is why a
> > smaller timeout to detect hotplug was needed.
> > 
> > Cc: Imre Deak <imre.deak at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  tests/kms_chamelium.c | 70
> > +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> > 
> > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > index c2090037..50a553f2 100644
> > --- a/tests/kms_chamelium.c
> > +++ b/tests/kms_chamelium.c
> > @@ -45,6 +45,8 @@ typedef struct {
> >  
> >  #define HOTPLUG_TIMEOUT 20 /* seconds */
> >  
> > +#define FAST_HOTPLUG_SEC_TIMEOUT (1)
> > +
> >  #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
> >  #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
> >  
> > @@ -107,6 +109,21 @@ reprobe_connector(data_t *data, struct
> > chamelium_port *port)
> >  	return status;
> >  }
> >  
> > +static drmModeConnection
> > +connector_status_get(data_t *data, struct chamelium_port *port)
> > +{
> > +	drmModeConnector *connector;
> > +	drmModeConnection status;
> > +
> > +	igt_debug("Getting connector state %s...\n",
> > chamelium_port_get_name(port));
> > +	connector = chamelium_port_get_connector(data->chamelium, port,
> > false);
> > +	igt_assert(connector);
> > +	status = connector->connection;
> > +
> > +	drmModeFreeConnector(connector);
> > +	return status;
> > +}
> > +
> >  static void
> >  wait_for_connector(data_t *data, struct chamelium_port *port,
> >  		   drmModeConnection status)
> > @@ -253,6 +270,56 @@ test_basic_hotplug(data_t *data, struct
> > chamelium_port *port, int toggle_count)
> >  	igt_hpd_storm_reset(data->drm_fd);
> >  }
> >  
> > +/*
> > + * Test kernel workaround for sinks that takes some time to have
> > the DDC/aux
> > + * channel responsive after the hotplug
> > + */
> > +static void
> > +test_late_aux_wa(data_t *data, struct chamelium_port *port)
> > +{
> > +	struct udev_monitor *mon = igt_watch_hotplug();
> > +	drmModeConnection status;
> > +
> > +	/* Reset will unplug all connectors */
> > +	reset_state(data, NULL);
> > +
> > +	/* Check if it device can act on hotplugs fast enough for this
> > test */
> > +	igt_flush_hotplugs(mon);
> > +	chamelium_plug(data->chamelium, port);
> > +	igt_assert(igt_hotplug_detected(mon,
> > FAST_HOTPLUG_SEC_TIMEOUT));
> > +	status = connector_status_get(data, port);
> > +	igt_require(status == DRM_MODE_CONNECTED);
> > +
> > +	igt_flush_hotplugs(mon);
> > +	chamelium_unplug(data->chamelium, port);
> > +	igt_assert(igt_hotplug_detected(mon,
> > FAST_HOTPLUG_SEC_TIMEOUT));
> > +	status = connector_status_get(data, port);
> > +	igt_require(status == DRM_MODE_DISCONNECTED);
> 
> Do you know on which platforms the hotplug processing is that slow
> and
> what could be the reason?

I have tested on ICL and WHL and both don't need more than 1 second,
the 20s timeout was set by the first patch(c99f8b7a3) adding Chamelium
tests but there is not other information about why 20s(in the first
versions of the patch it was 30s).

I can send another patch reducing this value to 1s and hopefully if it
do not causes regressions we merge it.

> 
> > +
> > +	/* It is fast enough, lets disable the DDC lines and plug again
> > */
> > +	igt_flush_hotplugs(mon);
> > +	chamelium_port_set_ddc_state(data->chamelium, port, false);
> > +	chamelium_plug(data->chamelium, port);
> > +	igt_assert(!chamelium_port_get_ddc_state(data->chamelium,
> > port));
> > +
> > +	/*
> > +	 * Give some time to kernel try to process hotplug but it
> > should fail
> > +	 */
> > +	igt_hotplug_detected(mon, FAST_HOTPLUG_SEC_TIMEOUT);
> > +	status = connector_status_get(data, port);
> > +	igt_assert(status == DRM_MODE_DISCONNECTED);
> 
> Hm, how does a second disconnect event get signaled here? After the
> previous hotplug event above where the state was disconnected already
> there
> shouldn't have been any change to the state, hence there shouldn't be
> any event sent by the driver.

It is not signaled, that I why there is not igt_assert() on this
igt_hotplug_detected() but call it will poll/sleep for the time we
want.


> 
> > +
> > +	/*
> > +	 * Enable the DDC line and the kernel workaround should reprobe
> > and
> > +	 * report as connected
> > +	 */
> > +	chamelium_port_set_ddc_state(data->chamelium, port, true);
> > +	igt_assert(chamelium_port_get_ddc_state(data->chamelium,
> > port));
> > +	igt_assert(igt_hotplug_detected(mon,
> > FAST_HOTPLUG_SEC_TIMEOUT));
> > +	status = connector_status_get(data, port);
> > +	igt_assert(status == DRM_MODE_CONNECTED);
> > +}
> > +
> >  static void
> >  test_edid_read(data_t *data, struct chamelium_port *port,
> >  	       int edid_id, const unsigned char *edid)
> > @@ -1308,6 +1375,9 @@ igt_main
> >  
> >  		connector_subtest("dp-frame-dump", DisplayPort)
> >  			test_display_frame_dump(&data, port);
> > +
> > +		connector_subtest("dp-late-aux-wa", DisplayPort)
> > +			test_late_aux_wa(&data, port);
> >  	}
> >  
> >  	igt_subtest_group {
> > -- 
> > 2.21.0
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20190314/0fb22690/attachment.sig>