[igt-dev] Improving DRM connector hotplug uevents
Daniel Vetter
daniel.vetter at ffwll.ch
Fri May 10 07:56:00 UTC 2019
On Fri, May 10, 2019 at 9:42 AM Paul Kocialkowski
<paul.kocialkowski at bootlin.com> wrote:
>
> Hi,
>
> So this thread has drifted off from IGT to a general DRM improvement,
> so renaming the thread and adding some CCs.
>
> It's about avoiding full reprobes (which may include slow EDID reads,
> etc) when a connector change was detected, by providing information
> about the connector and the new state to userspace. This way, userspace
> can reprobe the changed connector alone (or not reprobe at all in case
> of a disconnect).
>
> It feels like there is growing temptation to have per-driver hacks to
> avoid full reprobes when the driver knows that nothing changed, but I
> think it makes more sense to fixup the uevent we send to make sure that
> userspace knows what to probe exactly.
>
> On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> > On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > > I'm in support of this as well, we really could use better hotplug events.
> > > Feel free to cc patches to me for review :)
> >
> > Any idea(s) how the API would look like?
>
> From a quick chat on IRC yesterday, it seems that just adding entries
> to the uevent would do and would be backwards-compatible (well,
> provided that the userspace uevent parsers will not fail if something
> extra to "HOTPLUG=1" is provided).
>
> Currently, the notification is sent globally at each round of detected
> connector change (which may concern multiple connectors). So I think we
> should just list the connectors that changed in the uevent and their
> new state. So my proposal here is something like:
>
> HOTPLUG=1
> CONNECTOR_ID=47
> STATUS=Connected
> CONNECTOR_ID=48
> STATUS=Disconnected
>
> Where each STATUS entry would refer to the previous CONNECTOR_ID entry.
>
> What do you think?
We have the better uevent already, all we need is wiring up, lots of
code, and userspace for it. Ok, not quite, patch is still in flight:
https://patchwork.kernel.org/patch/10906677/
The real trouble isn't the new event, but making sure it goes out for
anything that might have changed. There's a lot more connector status
than just the STATUS property.
-Daniel
>
> Cheers,
>
> Paul
>
> > > On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > > > Hi,
> > > >
> > > > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > > > hotplug event for each connector that may be sent by the driver.
> > > > > > >
> > > > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > > > as
> > > > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > > > timing.
> > > > > > > So if we don't yet see the expected connector state on a given
> > > > > > > connector
> > > > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > > > state.
> > > > > >
> > > > > > This changes makes good sense to me, so:
> > > > > >
> > > > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski at bootlin.com>
> > > > > >
> > > > > > And it brings back hard feelings about the hotplug interface we have
> > > > > > today...
> > > > >
> > > > > Yes, I was surprised too not find any way to identify which connector(s)
> > > > > the hotplug event is associated with. We discussed with Ville if it'd
> > > > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > > > be useful outside of this test, or how that would align with Chris'
> > > > > connector probe state generation idea.
> > > >
> > > > Either way, I'm definitely in favor of having something more reliable
> > > > to expose to userspace. Having something to correlate each event to one
> > > > (or more) connector(s) sounds good to me.
> > > >
> > > > And I think there is a general need for this, it's not just IGT.
> > > >
> > > > Currently, every userspace application using DRM has to do a full
> > > > reprobe once a hotplug uevent is received, which is really not optimal.
> > > > If an entry identifying the connector was also provided, userspace
> > > > could only reprobe that connector. The step after that would be having
> > > > the indication of whether the connector was connected or disconnected
> > > > directly in uevent too, so that no probing needs to be done at all when
> > > > a given connector is disconnected.
> > > >
> > > > Anyway, if you're interested in the idea and that Ville and Chris are
> > > > in favor, I really think it would be worth fixing that. And we'd only
> > > > be adding new elements to uevent, so that would stay backward-
> > > > compatible too.
> > > >
> > > > Cheers,
> > > >
> > > > Paul
> > > >
> > > > > > > v2:
> > > > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > > > in
> > > > > > > the debugging output. (Lyude)
> > > > > > >
> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > > > Cc: Paul Kocialkowski <paul.kocialkowski at bootlin.com>
> > > > > > > Cc: Lyude Paul <lyude at redhat.com>
> > > > > > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > > > > > Reviewed-by: Lyude Paul <lyude at redhat.com>
> > > > > > > ---
> > > > > > > tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > > > -
> > > > > > > 1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > > > index 714e5e06..502f1efa 100644
> > > > > > > --- a/tests/kms_chamelium.c
> > > > > > > +++ b/tests/kms_chamelium.c
> > > > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > > > chamelium_port *port,
> > > > > > > drmModeFreeConnector(connector);
> > > > > > > }
> > > > > > >
> > > > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > > > */
> > > > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > > > +{
> > > > > > > + struct timespec start, end;
> > > > > > > + int elapsed;
> > > > > > > + bool detected;
> > > > > > > +
> > > > > > > + igt_assert_eq(igt_gettime(&start), 0);
> > > > > > > + detected = igt_hotplug_detected(mon, *timeout);
> > > > > > > + igt_assert_eq(igt_gettime(&end), 0);
> > > > > > > +
> > > > > > > + elapsed = igt_time_elapsed(&start, &end);
> > > > > > > + igt_assert_lte(0, elapsed);
> > > > > > > + *timeout = max(0, *timeout - elapsed);
> > > > > > > +
> > > > > > > + return detected;
> > > > > > > +}
> > > > > > > +
> > > > > > > static void
> > > > > > > try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > > > > enum igt_suspend_state state, enum
> > > > > > > igt_suspend_test test,
> > > > > > > struct udev_monitor *mon, bool connected)
> > > > > > > {
> > > > > > > + drmModeConnection target_state = connected ?
> > > > > > > DRM_MODE_DISCONNECTED :
> > > > > > > + DRM_MODE_CONNECTE
> > > > > > > D;
> > > > > > > + int timeout = HOTPLUG_TIMEOUT;
> > > > > > > int delay;
> > > > > > > int p;
> > > > > > >
> > > > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > > > chamelium_port *port,
> > > > > > > }
> > > > > > >
> > > > > > > igt_system_suspend_autoresume(state, test);
> > > > > > > + igt_assert(wait_for_hotplug(mon, &timeout));
> > > > > > >
> > > > > > > - igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > > > > if (port) {
> > > > > > > - igt_assert_eq(reprobe_connector(data, port), connected
> > > > > > > ?
> > > > > > > - DRM_MODE_DISCONNECTED :
> > > > > > > DRM_MODE_CONNECTED);
> > > > > > > + igt_assert_eq(reprobe_connector(data, port),
> > > > > > > target_state);
> > > > > > > } else {
> > > > > > > for (p = 0; p < data->port_count; p++) {
> > > > > > > + drmModeConnection current_state;
> > > > > > > +
> > > > > > > port = data->ports[p];
> > > > > > > - igt_assert_eq(reprobe_connector(data, port),
> > > > > > > connected ?
> > > > > > > - DRM_MODE_DISCONNECTED :
> > > > > > > - DRM_MODE_CONNECTED);
> > > > > > > + /*
> > > > > > > + * There could be as many hotplug events sent
> > > > > > > by
> > > > > > > + * driver as connectors we scheduled an HPD
> > > > > > > toggle on
> > > > > > > + * above, depending on timing. So if we're not
> > > > > > > seeing
> > > > > > > + * the expected connector state try to wait
> > > > > > > for an HPD
> > > > > > > + * event for each connector/port.
> > > > > > > + */
> > > > > > > + current_state = reprobe_connector(data, port);
> > > > > > > + if (p > 0 && current_state != target_state) {
> > > > > > > + igt_assert(wait_for_hotplug(mon,
> > > > > > > &timeout));
> > > > > > > + current_state =
> > > > > > > reprobe_connector(data, port);
> > > > > > > + }
> > > > > > > +
> > > > > > > + igt_assert_eq(current_state, target_state);
> > > > > > > }
> > > > > > >
> > > > > > > port = NULL;
> > > > > > --
> > > > > > Paul Kocialkowski, Bootlin
> > > > > > Embedded Linux and kernel engineering
> > > > > > https://bootlin.com
> > > > > >
> --
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the igt-dev
mailing list