Re: [igt-dev] [PATCH i-g-t 1/3] lib/kms_chamelium: Use FireHpdPulse for chamelium_hpd_send_pulses()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Nov 20, 2018 at 07:33:36PM -0500, Lyude wrote:
> From: Lyude Paul <lyude@redhat.com>
> 
> While trying to implement a short HPD storm detection test, I noticed
> that I couldn't actually manage to get the Chamelium to properly fire
> short HPD signals, instead all of the signals it was sending would be
> interpreted as long. However-when using my chameleond client, I could
> get it to send short pulses just fine.
> 
> It turns out the difference between the two is the RPC call that they
> were making to send pulses. Currently the igt library uses
> FireMixedHpdPulses() on the chamelium for any kind of hotplug pulses,
> but my chameleond client was using FireHpdPulse().
> 
> So I did a benchmark of the two RPC calls, sending 21 pulses each with a
> width of 2ms per pulse. The problem then became very obvious:
> 
> FireMixedHpdPulses: 0.168565ms total, ~8ms per-pulse
> FireHpdPulse: 0.052269ms total, ~2.4ms per-pulse
> 
> FireMixedHpdPulses() is literally too slow to fire a proper short HPD
> pulse. This makes sense: internally FireMixedHpdPulses() sends each
> pulse by writing to the appropriate registers using only python2, and
> FireHpdPulse() just calls down to a C helper which handles sending the
> repeated pulses from there.
> 
> While this is definitely a bug that needs to be fixed in chameleond, we
> can at least workaround it for the time being by switching to using
> FireHpdPulse() in chamelium_hpd_send_pulses() instead of
> FireMixedHpdPulses(). This is probably a better idea in the long run
> anyway, since being able to specify whether we end in a low or high
> pulse is a lot less awkward for users then having to change that
> behavior by ensuring the count of pulses is even or odd, something which
> FireMixedHpdPulses() required. Additionally-it seems the C helper that
> chameleond is supposed to be (but isn't) using for mixed pulses only
> accepts up to 20 pulses anyway.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  lib/igt_chamelium.c   | 39 ++++++++++++++++++++++-----------------
>  lib/igt_chamelium.h   |  2 +-
>  tests/kms_chamelium.c |  6 +++---
>  3 files changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index a80ead16..fd17c1a0 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -394,6 +394,7 @@ bool chamelium_port_wait_video_input_stable(struct chamelium *chamelium,
>   * @port: The port to fire the HPD pulses on
>   * @width_msec: How long each pulse should last
>   * @count: The number of pulses to send
> + * @end_high: Whether or not to end with a high pulse

This terminology is a bit confusing. A hpd pulse is an
asserted->deasserted->asserted transition, so I guess by
a "high pulse" you mean we leave hpd deasserted at the end
instead of asserting it again? So kind of a half pulse.

And in which case I guess the width of the pulse just
specifies how long until a subsequent call can re-assert
hpd again?

>   *
>   * A convienence function for sending multiple hotplug pulses to the system.
>   * The pulses start at low (e.g. connector is disconnected), and then alternate
> @@ -401,29 +402,33 @@ bool chamelium_port_wait_video_input_stable(struct chamelium *chamelium,
>   * repeatedly calling #chamelium_plug and #chamelium_unplug, waiting
>   * @width_msec between each call.
>   *
> - * If @count is even, the last pulse sent will be high, and if it's odd then it
> - * will be low. Resetting the HPD line back to it's previous state, if desired,
> - * is the responsibility of the caller.
> + * If @end_high is set, then the last pulse that is sent will be high, otherwise
> + * it will be low. Keep in mind that enabling this option will potentially
> + * result in the system registering one additional hotplug signal.
> + *
> + * It should be noted that in many cases, sending HPD pulses with very short
> + * durations (like 2ms for a DP short pulse) will likely have the first and last
> + * pulse have longer durations then specified, while the pulses inbetween will
> + * be equivalent to @width_msec. Callers should compensate for this in their
> + * tests.
> + *
>   */
>  void chamelium_fire_hpd_pulses(struct chamelium *chamelium,
>  			       struct chamelium_port *port,
> -			       int width_msec, int count)
> +			       int width_msec, int count, bool end_high)
>  {
> -	xmlrpc_value *pulse_widths = xmlrpc_array_new(&chamelium->env);
> -	xmlrpc_value *width = xmlrpc_int_new(&chamelium->env, width_msec);
> -	int i;
> -
> -	igt_debug("Firing %d HPD pulses with width of %d msec on %s\n",
> -		  count, width_msec, port->name);
> -
> -	for (i = 0; i < count; i++)
> -		xmlrpc_array_append_item(&chamelium->env, pulse_widths, width);
> +	/* We half the width here and split it between the assert and deassert
> +	 * interval, since the sum of the two will be the real-world pulse
> +	 * length
> +	 */
> +	int width_usec = (width_msec * 1000) / 2;
>  
> -	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "FireMixedHpdPulses",
> -				    "(iA)", port->id, pulse_widths));
> +	igt_debug("Firing %d HPD pulses with width of %d msec on %s, ending %s\n",
> +		  count, width_msec, port->name, end_high ? "high" : "low");
>  
> -	xmlrpc_DECREF(width);
> -	xmlrpc_DECREF(pulse_widths);
> +	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "FireHpdPulse", "(iiiii)",
> +				    port->id, width_usec, width_usec, count,
> +				    end_high));
>  }
>  
>  /**
> diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
> index af9655a0..46fca1b1 100644
> --- a/lib/igt_chamelium.h
> +++ b/lib/igt_chamelium.h
> @@ -63,7 +63,7 @@ void chamelium_fire_mixed_hpd_pulses(struct chamelium *chamelium,
>  				     struct chamelium_port *port, ...);
>  void chamelium_fire_hpd_pulses(struct chamelium *chamelium,
>  			       struct chamelium_port *port,
> -			       int width_msec, int count);
> +			       int width_msec, int count, bool end_high);
>  void chamelium_schedule_hpd_toggle(struct chamelium *chamelium,
>  				   struct chamelium_port *port, int delay_ms,
>  				   bool rising_edge);
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index e0e3e3f1..f051344d 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -743,11 +743,11 @@ test_hpd_storm_detect(data_t *data, struct chamelium_port *port, int width)
>  	reset_state(data, port);
>  
>  	igt_hpd_storm_set_threshold(data->drm_fd, 1);
> -	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10);
> +	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10, false);
>  	igt_assert(igt_hpd_storm_detected(data->drm_fd));
>  
>  	mon = igt_watch_hotplug();
> -	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10);
> +	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10, false);
>  
>  	/*
>  	 * Polling should have been enabled by the HPD storm at this point,
> @@ -769,7 +769,7 @@ test_hpd_storm_disable(data_t *data, struct chamelium_port *port, int width)
>  
>  	igt_hpd_storm_set_threshold(data->drm_fd, 0);
>  	chamelium_fire_hpd_pulses(data->chamelium, port,
> -				  width, 10);
> +				  width, 10, false);
>  	igt_assert(!igt_hpd_storm_detected(data->drm_fd));
>  
>  	igt_hpd_storm_reset(data->drm_fd);
> -- 
> 2.19.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev