[igt-dev] [PATCH i-g-t 1/3] lib/kms_chamelium: Use FireHpdPulse for chamelium_hpd_send_pulses()
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Dec 11 16:52:45 UTC 2018
On Tue, Nov 20, 2018 at 07:33:36PM -0500, Lyude wrote:
> From: Lyude Paul <lyude at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
--
Ville Syrjälä
Intel
More information about the igt-dev
mailing list