[igt-dev] [PATCH i-g-t 1/3] lib/kms_chamelium: Use FireHpdPulse for chamelium_hpd_send_pulses()
Lyude Paul
lyude at redhat.com
Tue Dec 11 21:28:21 UTC 2018
On Tue, 2018-12-11 at 18:52 +0200, Ville Syrjälä wrote:
> 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?
>
Agreed, unfortunately the end_high part is how the chamelium interface exposes
this.
So looking at the source code for chameleond, it looks like things are
actually reversed here:
@_FlowMethod
@_VideoMethod
def FireHpdPulse(self, port_id, deassert_interval_usec,
assert_interval_usec=None, repeat_count=1,
end_level=1):
"""Fires one or more HPD pulse (low -> high -> low -> ...).
Args:
port_id: The ID of the video input port.
deassert_interval_usec: The time in microsecond of the deassert pulse.
assert_interval_usec: The time in microsecond of the assert pulse.
If None, then use the same value as
deassert_interval_usec.
repeat_count: The count of HPD pulses to fire.
end_level: HPD ends with 0 for LOW (unplugged) or 1 for HIGH (plugged).
"""
if assert_interval_usec is None:
# Fall back to use the same value as deassertion if not given.
assert_interval_usec = deassert_interval_usec
logging.info('Fire HPD pulse on port #%d, ending with %s',
port_id, 'high' if end_level else 'low')
return self.flows[port_id].FireHpdPulse(
deassert_interval_usec, assert_interval_usec, repeat_count, end_level)
I wonder if the order being reversed here explains why i915 always seems
to see a long pulse at the start of a series of pulses.
Should we add something to the documentation for this that' similar to
the "low -> high -> low" stuff mentioned in the comments here?
> > *
> > * 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
--
Cheers,
Lyude Paul
More information about the igt-dev
mailing list