| Message ID | 20211015163336.95188-4-contact@emersion.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: add per-connector hotplug events | expand |
On Fri, Oct 15, 2021 at 04:33:43PM +0000, Simon Ser wrote: > In drm_connector_register, use drm_sysfs_connector_hotplug_event > instead of drm_sysfs_hotplug_event, because the hotplug event > only updates a single connector. > > Signed-off-by: Simon Ser <contact@emersion.fr> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
On Fri, 15 Oct 2021 16:33:43 +0000 Simon Ser <contact@emersion.fr> wrote: > In drm_connector_register, use drm_sysfs_connector_hotplug_event > instead of drm_sysfs_hotplug_event, because the hotplug event > only updates a single connector. > > Signed-off-by: Simon Ser <contact@emersion.fr> > --- > drivers/gpu/drm/drm_connector.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index ec3973e8963c..a50c82bc2b2f 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -547,7 +547,7 @@ int drm_connector_register(struct drm_connector *connector) > connector->registration_state = DRM_CONNECTOR_REGISTERED; > > /* Let userspace know we have a new connector */ > - drm_sysfs_hotplug_event(connector->dev); > + drm_sysfs_connector_hotplug_event(connector); > > if (connector->privacy_screen) > drm_privacy_screen_register_notifier(connector->privacy_screen, Hi Simon, this might not work for Weston if I understand this right. Kernel is adding a new connector, which means userspace does not recognise the connector id in the uevent. Weston as it is right now would ignore the event rather than add the connector. The missing piece is for Weston to revert to the old fashioned "recheck everything" behaviour when hotplug uevent carries anything unrecognised. Grep for drm_backend_update_conn_props if you want to see for yourself. However, I wouldn't NAK this patch just for Weston, but I wonder if other software would ignore events because of this as well. A whole another question is, would anyone notice. I guess this can only be an issue with MST. All the other changes in this series look fine to me, so them I can give Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com> Thanks, pq
On Wednesday, October 27th, 2021 at 15:15, Pekka Paalanen <ppaalanen@gmail.com> wrote: > On Fri, 15 Oct 2021 16:33:43 +0000 > Simon Ser <contact@emersion.fr> wrote: > > > In drm_connector_register, use drm_sysfs_connector_hotplug_event > > instead of drm_sysfs_hotplug_event, because the hotplug event > > only updates a single connector. > > > > Signed-off-by: Simon Ser <contact@emersion.fr> > > --- > > drivers/gpu/drm/drm_connector.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > index ec3973e8963c..a50c82bc2b2f 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -547,7 +547,7 @@ int drm_connector_register(struct drm_connector *connector) > > connector->registration_state = DRM_CONNECTOR_REGISTERED; > > > > /* Let userspace know we have a new connector */ > > - drm_sysfs_hotplug_event(connector->dev); > > + drm_sysfs_connector_hotplug_event(connector); > > > > if (connector->privacy_screen) > > drm_privacy_screen_register_notifier(connector->privacy_screen, > > Hi Simon, > > this might not work for Weston if I understand this right. Kernel is > adding a new connector, which means userspace does not recognise the > connector id in the uevent. Weston as it is right now would ignore the > event rather than add the connector. > > The missing piece is for Weston to revert to the old fashioned "recheck > everything" behaviour when hotplug uevent carries anything > unrecognised. Grep for drm_backend_update_conn_props if you want to see > for yourself. > > However, I wouldn't NAK this patch just for Weston, but I wonder if > other software would ignore events because of this as well. > > A whole another question is, would anyone notice. I guess this can only > be an issue with MST. I think Weston should be fine: udev_event_is_conn_prop_change returns false if there's no PROPERTY in the uevent. An uevent with just a CONNECTOR and no PROPERTY is something new. Weston already falls back to the old "reprobe the world" approach in this case. So far the CONNECTOR+PROPERTY uevent fields have only been used for content protection stuff. I'm not aware of other user-space using it (checked Kodi just in case, it doesn't do content protection nor handles uevents at all). > All the other changes in this series look fine to me, so them I can give > Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com> Thanks!
On Wed, 27 Oct 2021 13:26:45 +0000 Simon Ser <contact@emersion.fr> wrote: > On Wednesday, October 27th, 2021 at 15:15, Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > On Fri, 15 Oct 2021 16:33:43 +0000 > > Simon Ser <contact@emersion.fr> wrote: > > > > > In drm_connector_register, use drm_sysfs_connector_hotplug_event > > > instead of drm_sysfs_hotplug_event, because the hotplug event > > > only updates a single connector. > > > > > > Signed-off-by: Simon Ser <contact@emersion.fr> > > > --- > > > drivers/gpu/drm/drm_connector.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > > index ec3973e8963c..a50c82bc2b2f 100644 > > > --- a/drivers/gpu/drm/drm_connector.c > > > +++ b/drivers/gpu/drm/drm_connector.c > > > @@ -547,7 +547,7 @@ int drm_connector_register(struct drm_connector *connector) > > > connector->registration_state = DRM_CONNECTOR_REGISTERED; > > > > > > /* Let userspace know we have a new connector */ > > > - drm_sysfs_hotplug_event(connector->dev); > > > + drm_sysfs_connector_hotplug_event(connector); > > > > > > if (connector->privacy_screen) > > > drm_privacy_screen_register_notifier(connector->privacy_screen, > > > > Hi Simon, > > > > this might not work for Weston if I understand this right. Kernel is > > adding a new connector, which means userspace does not recognise the > > connector id in the uevent. Weston as it is right now would ignore the > > event rather than add the connector. > > > > The missing piece is for Weston to revert to the old fashioned "recheck > > everything" behaviour when hotplug uevent carries anything > > unrecognised. Grep for drm_backend_update_conn_props if you want to see > > for yourself. > > > > However, I wouldn't NAK this patch just for Weston, but I wonder if > > other software would ignore events because of this as well. > > > > A whole another question is, would anyone notice. I guess this can only > > be an issue with MST. > > I think Weston should be fine: udev_event_is_conn_prop_change returns false > if there's no PROPERTY in the uevent. An uevent with just a CONNECTOR and no > PROPERTY is something new. Weston already falls back to the old "reprobe the > world" approach in this case. > > So far the CONNECTOR+PROPERTY uevent fields have only been used for content > protection stuff. I'm not aware of other user-space using it (checked Kodi > just in case, it doesn't do content protection nor handles uevents at all). > > > All the other changes in this series look fine to me, so them I can give > > Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com> Hi Simon, you're right! Therefore my Ack applies to this patch too. Thanks, pq
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index ec3973e8963c..a50c82bc2b2f 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -547,7 +547,7 @@ int drm_connector_register(struct drm_connector *connector) connector->registration_state = DRM_CONNECTOR_REGISTERED; /* Let userspace know we have a new connector */ - drm_sysfs_hotplug_event(connector->dev); + drm_sysfs_connector_hotplug_event(connector); if (connector->privacy_screen) drm_privacy_screen_register_notifier(connector->privacy_screen,
In drm_connector_register, use drm_sysfs_connector_hotplug_event instead of drm_sysfs_hotplug_event, because the hotplug event only updates a single connector. Signed-off-by: Simon Ser <contact@emersion.fr> --- drivers/gpu/drm/drm_connector.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)