[Intel-gfx] [PATCH v7 09/11] drm: uevent for connector status change
Paul Kocialkowski
paul.kocialkowski at bootlin.com
Tue May 14 14:12:16 UTC 2019
Hi,
On Tue, 2019-05-14 at 13:09 +0200, Daniel Vetter wrote:
> On Mon, May 13, 2019 at 7:14 PM Paul Kocialkowski
> <paul.kocialkowski at bootlin.com> wrote:
> > Hey,
> >
> > Le lundi 13 mai 2019 à 11:34 +0200, Daniel Vetter a écrit :
> > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski
> > > <paul.kocialkowski at bootlin.com> wrote:
> > > > Hi,
> > > >
> > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote:
> > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski
> > > > > <paul.kocialkowski at bootlin.com> wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote:
> > > > > > > DRM API for generating uevent for a status changes of connector's
> > > > > > > property.
> > > > > > >
> > > > > > > This uevent will have following details related to the status change:
> > > > > > >
> > > > > > > HOTPLUG=1, CONNECTOR=<connector_id> and PROPERTY=<property_id>
> > > > > > >
> > > > > > > Need ACK from this uevent from userspace consumer.
> > > > > >
> > > > > > So we just had some discussions over on IRC and at about the hotplug
> > > > > > issue and came up with similar ideas:
> > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html
> > > > > >
> > > > > > The conclusions of these discussions so far would be to have a more or
> > > > > > less fine grain of uevent reporting depending on what happened. The
> > > > > > point is that we need to cover different cases:
> > > > > > - one or more properties changed;
> > > > > > - the connector status changed;
> > > > > > - something else about the connector changed (e.g. EDID/modes)
> > > > > >
> > > > > > For the first case, we can send out:
> > > > > > HOTPLUG=1
> > > > > > CONNECTOR=<id>
> > > > > > PROPERTY=<id>
> > > > > >
> > > > > > and no reprobe is required.
> > > > > >
> > > > > > For the second one, something like:
> > > > > > HOTPLUG=1
> > > > > > CONNECTOR=<id>
> > > > > > STATUS=Connected/Disconnected
> > > > > >
> > > > > > and a connector probe is needed for connected, but not for
> > > > > > disconnected;
> > > > > >
> > > > > > For the third one, we can only indicate the connector:
> > > > > > HOTPLUG=1
> > > > > > CONNECTOR=<id>
> > > > > >
> > > > > > and a reprobe of the connector is always needed
> > > > >
> > > > > There's no material difference between this one and the previous one.
> > > > > Plus there's no beenfit in supplying the actual value of the property,
> > > > > i.e. we can reuse the same PROPERTY=<id-of-status-property> trick.
> > > >
> > > > That's the idea, but we need to handle status changes differently than
> > > > properties, since as far as I know, connected/unconnected status is not
> > > > exposed as a prop for the connector.
> > >
> > > Oops, totally missed that. "Everything is a property" is kinda
> > > new-ish, at least compared to kms. Kinda tempted to just make status
> > > into a property. Or another excuse why we should expose the epoch
> > > property :-)
> >
> > Well I think it would make sense anyway, as long as we can make sure it
> > stays consistent with the one reported in the connector struct.
> >
> > > > > Here's why:
> > > > > - A side effect of forcing a probe on a connector is that you get to
> > > > > read all the properties, so supplying them is kinda pointless.
> > > >
> > > > Agreed, except for the status case where it's useful to know it's a
> > > > disconnect, because we don't need any probe step in that case.
> > > >
> > > > > - You can read STATUS without forcing a reprobe, if you want to avoid
> > > > > the reprobe for disconnected. I'd kinda not recommend that though,
> > > > > feels a bit like overoptimizing. And for reasonable connectors (i.e.
> > > > > dp) reprobing a disconnected output is fast. HDMI is ... less
> > > > > reasonable unfortunately, but oh well.
> > > >
> > > > How would that be retreived then? From the looks of it, that's a
> > > > MODE_GETCONNECTOR ioctl and I was under the impression this is what
> > > > does the full reprobe.
> > >
> > > drmGetConnector vs drmGetConnectorCurrent.
> >
> > Ah right, forgot about that one, thanks.
> >
> > > > Not sure what issues could arise in case of disconnect without reprobe
> > > > -- at least I don't see why userspace should have to do anything in
> > > > particular except no longer using the connector, even in complex DP MST
> > > > cases.
> > >
> > > connector->status might be a lie without a full reprobe, and wrongly
> > > indicate that the connector is disconnected while there's still
> > > something plugged in. I'm not sure we've fixed those bugs by now
> > > (usually it's around "hpd indicates disconnected" vs. "i2c indicates
> > > connected, and we can't break this because every intel platform ever
> > > shipped has a few devices where this is somehow broken, irrespective
> > > of the sink).
> >
> > Mhh either way, I think it's up to the driver to report that and make
> > it consistent. I think we have poll helpers to make up for cases where
> > hotplug is not available too. So I'm not sure why a full reprobe would
> > be needed: drivers just need to do the right thing.
> >
> > > > > - There's no way to only reprobe status, you can only ever reprobe
> > > > > everything with the current ioctl and implementations. Having an
> > > > > option to reprobe only parts of it doesn't seem useful to me (we need
> > > > > to read the EDID anyway, and that's the expensive part of reprobing in
> > > > > almost all cases).
> > > >
> > > > Agreed.
> > > >
> > > > > In a way PROPERTY=<status-prop-id> simply tells userspace that it
> > > > > needs to reprobe this connector.
> > > >
> > > > I thought we could access the props alone, which avoids doing a reprobe
> > > > when the kernel knows that only a prop or a set of props changed and do
> > > > not require a full reprobe. That's the first case I was mentionning.
> > > >
> > > > > At that point we need to figure out whether this is a good uapi or
> > > > > not, and that's where the epoch comes in. There's two reasons for an
> > > > > epoch:
> > > > > - We need it internally because I'm not goinig to wire a new return
> > > > > value through hundreds of connector probe functions. It's much easier
> > > > > to have an epoch counter which we set from e.g. drm_set_edid and
> > > > > similar functions that update probe state.
> > > >
> > > > I don't think I'm following what issue this is trying to solve
> > > > internally.
> > >
> > > So I'm assuming that if we handle a hotplug, we only want to generate
> > > one uevent for that, not one for every little thing that changed.
> > > There's two ways to implement this logic:
> > > - With some epoch counter and a helper function you can call everytime
> > > something changed (e.g. status, or edid, or anything else we care
> > > about e.g. from dp aux). This won't need much (if any) driver changes,
> > > because we can just put these into the relevant helper/core functions
> > > (like edid update, or dp aux reading or whatever).
> > > - Wiring a new return value through the entire stack (and _all_ the
> > > kms drivers) so that the probe helpers could aggregate this like they
> > > currently do.
> > >
> > > One of these is a lot less typing.
> >
> > Oh I had missed this issue. Yeah of course if we start reporting
> > property changes, a hotplug will be lots of such changes.
> >
> > So an epoch counter property would indeed also solve the reprobing
> > problem. But I think it would be nice to keep the ability to be
> > notified of what changed precisely via uevent. I'm not really buying
> > the "missing uevent" thing so much and I think we can reasonably expect
> > that it will be useful. Events could be aggregated (which the epoch
> > counter would probably also allow) and sent out altogether when the
> > connector status changes (along with the status information). I think
> > we're under-using uevent currently, and feel like this should be fixed
> > regardless of the full reprobe issue.
>
> I see two options:
>
> 1) kernel aggregates uevents a bit, and sends out one (per connector
> that changed) indicating that sink related state changed. Userspace
> listens to that, and does a drmGetConnector as a result to update
> itself. Most of this code needs to exist anyway.
>
> 2) kernel sends out updates as we go, userspace reassembles everything
> again (and not really an idea when all the updates are in, see Lyude's
> question. Plus userspace still needs to have the drmGetConnector path,
> at least for initial setup.
>
> I don't see why 2) is any better than 1), and it has some clear downsides.
My main point is that when the kernel knows some individual property
changed, it should update its internal state and report what prop
changed *in cases that don't require a full reprobe* and report only
which connector changed when a connector reprobe is needed anyway.
Maybe we could even have quirks for drivers to indicate that common
actions that shouldn't require a reprobe actually do for this driver in
particular, because of whatever instability we want to be confident
about handling. I believe this should be abstracted away in DRM and not
reflected on the userspace API.
So it's not just about providing per-property updates, the updates
reported this way have to be "standalone" and never imply that anything
else about the connector's state could have change. Driver's guarantee.
> > With that, I agree that a global epoch counter from the connector would
> > be a good quick way for userspace to tell if a connector changed or not
> > since the last time it checked.
> >
> > > > > - If userspace misses an event and there's no epoch, we're forcing
> > > > > userspace to reprobe everything. Use case would be if a compositor is
> > > > > switched away we probably don't want to piss of the current compositor
> > > > > by blocking it's own probe kernel calls by doing our own (probe is
> > > > > single-threaded in the kernel through the dev->mode_config.mutex). If
> > > > > it can read the epoch property (which it can do without forcing a
> > > > > reprobe) userspace would know which connectors it needs to check and
> > > > > reprobe.
> > > > >
> > > > > Hence why epoch, it's a bit more robust userspace api. Ofc you could
> > > > > also require that userspace needs to keep parsing all uevents and make
> > > > > a list of all connectors it needs to reprobe when it's back to being
> > > > > the active compositor. But just comparing a current epoch with the one
> > > > > you cached from the last full probe is much easier.
> > > >
> > > > Fair enough, I think it's a fine idea for robustness yes, but I think
> > > > we could also provide extra info in the uevent when relevant and not
> > > > rely on that entirely.
> > >
> > > See above, with drmGetConnectorCurrent there's no need to provide more
> > > than what's needed in the uevent, since userspace can get everything
> > > else at the cost of one ioctl, without reprobing. With a bit of
> > > engineering work we could even avoid taking the expensive
> > > dev->mode_config.mutex lock for this fastpath.
> > >
> > > > > Another thing: None of this we can for connectors with unreliable hdp.
> > > > > Or at least you'll piss of users if you cache always. The sad thing is
> > > > > that HDMI is unreliable, at least on some machines/screen combos (you
> > > > > never get a hpd irq if you plug in/unplug). So real compositors still
> > > > > need to reprobe when the user asks for it. igt can probably get away
> > > > > without reprobing.
> > > >
> > > > I wonder how that is handled currently and how a user action can solve
> > > > the issue without any notification from the kernel. Maybe a need a
> > > > better understanding of that case to have a clearer idea.
> > >
> > > User opens the screen configuration tool -> usually at that point the
> > > tool/compositor force a full reprobe, which then often triggers the
> > > automatic reconfiguring. E.g. on one laptop I have here when I plug in
> > > random shit projectors at conferences nothing happens, until I run
> > > xrandr, which triggers the full reprobe, which then makes the kernel
> > > realize something change, sending and uevent, which starts the
> > > automatic reconfigure machinery.
> >
> > Oh right hehe, I definitely do that blind alt+f2 gnome-control-center
> > to get me out of an off-panel situation much too often.
> >
> > > There's also the issue that there's machines with hpd storms (even on
> > > DP, where you really need hpd to work to be compliant), and we have to
> > > turn of the hpd irq to keep the machine useable.
> >
> > I was under the impression that we switch to polling when a hpd storm
> > is detected in i915 (but that's a vague memory from my summer
> > internship at Intel 2 years ago).
>
> We do poll, but the issue is still that polling doesn't do the same
> thing as full reprobe. One just calls ->detect, the other ->detect +
> ->get_modes. This is a bit a silliness of the helpers and source of
> lots of confusion. A possible fix might be to always call both.
I think that would make a lot of sense. I don't really get why we have
to wait for userspace to do a reprobe to get the new modes when the
kernel can be absolutely sure that the modes changed and need to be
refreshed.
> Plus even when that mess is sorted there's still the issue of broken
> hw, and we have no idea how much/where/which exactly. Except that
> every time we relied on hpd status, we got regression reports and had
> to revert.
I think we could configure the notification behavior per-driver and
some events that wouldn't require a full probe on some drivers could do
so on other drivers. Userspace should find out whether to reprobe or
not dynamically based on what's in the uevent anyway, so it's not even
an inconsistency in the interface that 2 events be reported differently
in 2 drivers. I think that'd be better than the current situation we're
in anyway. If some drivers want be paranoid and always ask for
a reprobe of the connectors because of unreliable hw, their choice.
Cheers,
Paul
> -Daniel
>
> > Cheers,
> >
> > Paul
> >
> > > Cheers, Daniel
> > >
> > > > Cheers,
> > > >
> > > > Paul
> > > >
> > > > > -Daniel
> > > > >
> > > > > > Then we still have the legacy case:
> > > > > > HOTPLUG=1
> > > > > >
> > > > > > where userspace is expected to reprobe all the connectors.
> > > > > >
> > > > > > I think this would deserve to be a separate series on its own. So I am
> > > > > > proposing to take this one off your plate and come up with another
> > > > > > seres implementing this proposal. What do you think?
> > > > > >
> > > > > > Cheers,
> > > > > >
> > > > > > Paul
> > > > > >
> > > > > > > v2:
> > > > > > > Minor fixes at KDoc comments [Daniel]
> > > > > > > v3:
> > > > > > > Check the property is really attached with connector [Daniel]
> > > > > > >
> > > > > > > Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> > > > > > > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > > > > ---
> > > > > > > drivers/gpu/drm/drm_sysfs.c | 35 +++++++++++++++++++++++++++++++++++
> > > > > > > include/drm/drm_sysfs.h | 5 ++++-
> > > > > > > 2 files changed, 39 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > > > > > > index 18b1ac442997..63fa951a20db 100644
> > > > > > > --- a/drivers/gpu/drm/drm_sysfs.c
> > > > > > > +++ b/drivers/gpu/drm/drm_sysfs.c
> > > > > > > @@ -21,6 +21,7 @@
> > > > > > > #include <drm/drm_sysfs.h>
> > > > > > > #include <drm/drmP.h>
> > > > > > > #include "drm_internal.h"
> > > > > > > +#include "drm_crtc_internal.h"
> > > > > > >
> > > > > > > #define to_drm_minor(d) dev_get_drvdata(d)
> > > > > > > #define to_drm_connector(d) dev_get_drvdata(d)
> > > > > > > @@ -320,6 +321,9 @@ void drm_sysfs_lease_event(struct drm_device *dev)
> > > > > > > * Send a uevent for the DRM device specified by @dev. Currently we only
> > > > > > > * set HOTPLUG=1 in the uevent environment, but this could be expanded to
> > > > > > > * deal with other types of events.
> > > > > > > + *
> > > > > > > + * Any new uapi should be using the drm_sysfs_connector_status_event()
> > > > > > > + * for uevents on connector status change.
> > > > > > > */
> > > > > > > void drm_sysfs_hotplug_event(struct drm_device *dev)
> > > > > > > {
> > > > > > > @@ -332,6 +336,37 @@ void drm_sysfs_hotplug_event(struct drm_device *dev)
> > > > > > > }
> > > > > > > EXPORT_SYMBOL(drm_sysfs_hotplug_event);
> > > > > > >
> > > > > > > +/**
> > > > > > > + * drm_sysfs_connector_status_event - generate a DRM uevent for connector
> > > > > > > + * property status change
> > > > > > > + * @connector: connector on which property status changed
> > > > > > > + * @property: connector property whoes status changed.
> > > > > > > + *
> > > > > > > + * Send a uevent for the DRM device specified by @dev. Currently we
> > > > > > > + * set HOTPLUG=1 and connector id along with the attached property id
> > > > > > > + * related to the status change.
> > > > > > > + */
> > > > > > > +void drm_sysfs_connector_status_event(struct drm_connector *connector,
> > > > > > > + struct drm_property *property)
> > > > > > > +{
> > > > > > > + struct drm_device *dev = connector->dev;
> > > > > > > + char hotplug_str[] = "HOTPLUG=1", conn_id[30], prop_id[30];
> > > > > > > + char *envp[4] = { hotplug_str, conn_id, prop_id, NULL };
> > > > > > > +
> > > > > > > + WARN_ON(!drm_mode_obj_find_prop_id(&connector->base,
> > > > > > > + property->base.id));
> > > > > > > +
> > > > > > > + snprintf(conn_id, ARRAY_SIZE(conn_id),
> > > > > > > + "CONNECTOR=%u", connector->base.id);
> > > > > > > + snprintf(prop_id, ARRAY_SIZE(prop_id),
> > > > > > > + "PROPERTY=%u", property->base.id);
> > > > > > > +
> > > > > > > + DRM_DEBUG("generating connector status event\n");
> > > > > > > +
> > > > > > > + kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE, envp);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL(drm_sysfs_connector_status_event);
> > > > > > > +
> > > > > > > static void drm_sysfs_release(struct device *dev)
> > > > > > > {
> > > > > > > kfree(dev);
> > > > > > > diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h
> > > > > > > index 4f311e836cdc..d454ef617b2c 100644
> > > > > > > --- a/include/drm/drm_sysfs.h
> > > > > > > +++ b/include/drm/drm_sysfs.h
> > > > > > > @@ -4,10 +4,13 @@
> > > > > > >
> > > > > > > struct drm_device;
> > > > > > > struct device;
> > > > > > > +struct drm_connector;
> > > > > > > +struct drm_property;
> > > > > > >
> > > > > > > int drm_class_device_register(struct device *dev);
> > > > > > > void drm_class_device_unregister(struct device *dev);
> > > > > > >
> > > > > > > void drm_sysfs_hotplug_event(struct drm_device *dev);
> > > > > > > -
> > > > > > > +void drm_sysfs_connector_status_event(struct drm_connector *connector,
> > > > > > > + struct drm_property *property);
> > > > > > > #endif
> > > > > > --
> > > > > > Paul Kocialkowski, Bootlin
> > > > > > Embedded Linux and kernel engineering
> > > > > > https://bootlin.com
> > > > > >
> > > > --
> > > > Paul Kocialkowski, Bootlin
> > > > Embedded Linux and kernel engineering
> > > > https://bootlin.com
> > > >
>
>
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
More information about the Intel-gfx
mailing list