Re: [PATCH v7 09/11] drm: uevent for connector status change

[Daniel Vetter <daniel.vetter@ffwll.ch>]

On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski
<paul.kocialkowski@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.
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.
- 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.
- 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).

In a way PROPERTY=<status-prop-id> simply tells userspace that it
needs to reprobe this connector.

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.
- 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.

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.
-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@intel.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@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
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel