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

[Imre Deak <imre.deak@intel.com>]

On Mon, Mar 18, 2019 at 04:44:24PM -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.
> 
> v2:
> - Removing igt_assert() from the igt_hotplug_detected() when checking
> if device can act on hotplug fast enough
> - Checking time spend between hotplug and the enabling of DDC lines
> (Imre)
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  tests/kms_chamelium.c | 110 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
> 
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index c2090037..c79516ef 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_TIMEOUT (1) /* second */
> +
>  #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,96 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port, int toggle_count)
>  	igt_hpd_storm_reset(data->drm_fd);
>  }
>  
> +static void
> +test_fast_hotplug_handling(data_t *data, struct chamelium_port *port,
> +			   struct udev_monitor *mon)
> +{
> +	drmModeConnection status;
> +
> +	igt_flush_hotplugs(mon);
> +	chamelium_plug(data->chamelium, port);
> +	igt_hotplug_detected(mon, FAST_HOTPLUG_TIMEOUT);
> +	status = connector_status_get(data, port);
> +	igt_require(status == DRM_MODE_CONNECTED);
> +
> +	igt_flush_hotplugs(mon);
> +	chamelium_unplug(data->chamelium, port);
> +	igt_hotplug_detected(mon, FAST_HOTPLUG_TIMEOUT);
> +	status = connector_status_get(data, port);
> +	igt_require(status == DRM_MODE_DISCONNECTED);
> +}
> +
> +/*
> + * 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;
> +	struct timespec begin;
> +	uint64_t delta_nsec;
> +	uint8_t retries = 0;
> +
> +	/* Reset will unplug all connectors */
> +	reset_state(data, NULL);
> +
> +	/* Check if it device can act on hotplugs fast enough for this test */
> +	test_fast_hotplug_handling(data, port, mon);
> +
> +retry:
> +	/* 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_gettime(&begin);

Should start measuring already before chamelium_plug(), otherwise we
could miss time that elapsed since the beginning of HPD
interrupt->hotplug processing in the kernel.

> +	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_TIMEOUT);
> +	status = connector_status_get(data, port);
> +	igt_assert(status == DRM_MODE_DISCONNECTED);
> +
> +	/*
> +	 * Enable the DDC line and the kernel workaround should reprobe and
> +	 * report as connected
> +	 */
> +	chamelium_port_set_ddc_state(data->chamelium, port, true);
> +
> +	/*
> +	 * i915 uses the maximum timeout that each platform support to do aux
> +	 * transactions, this timeout can vary from 1.6msec to 4msec and i915
> +	 * driver tries the same aux transaction up to 5 times before return a
> +	 * error and additionally drm helpers will ask driver to do the same aux
> +	 * transaction up to 32 times, so it will take at least 256msec~640msec
> +	 * to kernel give up on a sink detection.
> +	 *
> +	 * The workaround will be schedule to run 1 second after the driver
> +	 * failed to probe the connector that signaled a hotplug, so if this
> +	 * test is preempt it could fail because the workaround is already
> +	 * running with the DDC lines still off, so lets try again until the
> +	 * time requirement is meet.
> +	 */
> +	delta_nsec = igt_nsec_elapsed(&begin);
> +	if (delta_nsec > (NSEC_PER_SEC * 1.3f)) {

1.3 sec is too late for the 256msec+1sec scenario.. Should be 1.2.

I guess you're still working on the HDMI test? Please also add it to the
meson and automake sources. I guess we can merge it before the kernel patches
letting it fail until that.

> +		igt_assert_f(retries != 5, "Test preempted too many times");
> +		retries++;
> +
> +		/* Wait for 1 more sec to make sure the workaround finished */
> +		chamelium_unplug(data->chamelium, port);
> +		igt_hotplug_detected(mon, FAST_HOTPLUG_TIMEOUT);
> +		goto retry;
> +	}
> +
> +	igt_assert(chamelium_port_get_ddc_state(data->chamelium, port));
> +	igt_assert(igt_hotplug_detected(mon, FAST_HOTPLUG_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 +1415,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
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev