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

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

On Thu, Mar 14, 2019 at 07:59:34PM +0200, Souza, Jose wrote:
> 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@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@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).

Ok, sounds to me that HOTPLUG_TIMEOUT was added only to check if hotplug
detection works at all (all relevant places using it have an
igt_assert() on it).

Still not sure if we need the above two checks, since I think the
assumptions/checks later in the function should hold anyway.  Yes, we
may not exercise the hotplug retry path in the driver, but I don't think
there is a good way to ensure that anyway as I wrote below.

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

But then I don't see how it will work. The sequence is:

<connector is in disconnected state, corresponding event delivered>
1. disable DDC
2. generate a plug event
3. wait for the plug event delivery with 1 sec timeout
4. re-enable DDC
5. wait for the plug event delivery (that should be triggered by the new
   retry logic in the driver)

Since after 2. DDC is disabled the driver hotplug handler will conclude
that the connector is still disconnected and hence doesn't generate any
hotplug event. B/c of this 3. will time out after 1 sec.

So in 4. we'll re-enable DDC only after 1 sec after the plug event
(interrupt) generated in 2. Since the retry in the driver happens after
1 sec from the plug interrupt as well the retry processing could easily
race with the DDC re-enabling in 4. and thus the detection could fail.

Since I don't see a good way to ensure that we re-enable DDC after the
first detection cycle ran (but not too late missing the retry cycle) I
would rather suggest a simple wait at 3., let's say 500msec. With that
things should always work. We may not always exercise the driver's retry
path if there was a long scheduling delay, but that's unlikely.

However a scheduling delay after 2. and before 4. could cause a
detection failure. To avoid that I'd also check the elapsed time
starting from right before 2. until right after 4. and run the sequence
again if the elapsed time was too close to 1sec (and hence detection
possibly failed because of the race described above).

> > > +
> > > +	/*
> > > +	 * 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);

This also needs the retry logic to be added for DP ports on old
platforms (intel_dp_hotplug() in the driver).

> > >  	}
> > >  
> > >  	igt_subtest_group {
> > > -- 
> > > 2.21.0
> > > 


_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev