When a uevent only updates a single connector, add a CONNECTOR property to the uevent. This allows user-space to ignore other connectors when handling the uevent. This is purely an optimization, drivers can still send a uevent without the CONNECTOR property.
The CONNECTOR property is already set when sending HDCP property update uevents, see drm_sysfs_connector_status_event.
This has been tested with a wlroots patch [1].
amdgpu and the probe-helper has been updated to use these new fine-grained uevents.
Changes in v3: rebase
[1]: https://github.com/swaywm/wlroots/pull/2959
Simon Ser (6): drm/sysfs: introduce drm_sysfs_connector_hotplug_event drm/probe-helper: add drm_kms_helper_connector_hotplug_event drm/connector: use drm_sysfs_connector_hotplug_event amdgpu: use drm_kms_helper_connector_hotplug_event drm/probe-helper: use drm_kms_helper_connector_hotplug_event i915/display/dp: send a more fine-grained link-status uevent
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++-- .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 4 +- drivers/gpu/drm/drm_connector.c | 2 +- drivers/gpu/drm/drm_probe_helper.c | 43 +++++++++++++++++-- drivers/gpu/drm/drm_sysfs.c | 25 +++++++++++ drivers/gpu/drm/i915/display/intel_dp.c | 2 + include/drm/drm_probe_helper.h | 1 + include/drm/drm_sysfs.h | 1 + 8 files changed, 75 insertions(+), 11 deletions(-)
base-commit: f6632721cd6231e1bf28b5317dcc7543e43359f7
This function sends a hotplug uevent with a CONNECTOR property.
Signed-off-by: Simon Ser contact@emersion.fr --- drivers/gpu/drm/drm_sysfs.c | 25 +++++++++++++++++++++++++ include/drm/drm_sysfs.h | 1 + 2 files changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 76ff6ec3421b..430e00b16eec 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -409,6 +409,31 @@ void drm_sysfs_hotplug_event(struct drm_device *dev) } EXPORT_SYMBOL(drm_sysfs_hotplug_event);
+/** + * drm_sysfs_connector_hotplug_event - generate a DRM uevent for any connector + * change + * @connector: connector which has changed + * + * Send a uevent for the DRM connector specified by @connector. This will send + * a uevent with the properties HOTPLUG=1 and CONNECTOR. + */ +void drm_sysfs_connector_hotplug_event(struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + char hotplug_str[] = "HOTPLUG=1", conn_id[21]; + char *envp[] = { hotplug_str, conn_id, NULL }; + + snprintf(conn_id, sizeof(conn_id), + "CONNECTOR=%u", connector->base.id); + + drm_dbg_kms(connector->dev, + "[CONNECTOR:%d:%s] generating connector hotplug event\n", + connector->base.id, connector->name); + + kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE, envp); +} +EXPORT_SYMBOL(drm_sysfs_connector_hotplug_event); + /** * drm_sysfs_connector_status_event - generate a DRM uevent for connector * property status change diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h index d454ef617b2c..6273cac44e47 100644 --- a/include/drm/drm_sysfs.h +++ b/include/drm/drm_sysfs.h @@ -11,6 +11,7 @@ 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_hotplug_event(struct drm_connector *connector); void drm_sysfs_connector_status_event(struct drm_connector *connector, struct drm_property *property); #endif
Hi Simon,
On Fri, Oct 15, 2021 at 04:33:41PM +0000, Simon Ser wrote:
This function sends a hotplug uevent with a CONNECTOR property.
A little late feedback.
Sam
Signed-off-by: Simon Ser contact@emersion.fr
drivers/gpu/drm/drm_sysfs.c | 25 +++++++++++++++++++++++++ include/drm/drm_sysfs.h | 1 + 2 files changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 76ff6ec3421b..430e00b16eec 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -409,6 +409,31 @@ void drm_sysfs_hotplug_event(struct drm_device *dev) } EXPORT_SYMBOL(drm_sysfs_hotplug_event);
+/**
- drm_sysfs_connector_hotplug_event - generate a DRM uevent for any connector
- change
- @connector: connector which has changed
- Send a uevent for the DRM connector specified by @connector. This will send
- a uevent with the properties HOTPLUG=1 and CONNECTOR.
- */
+void drm_sysfs_connector_hotplug_event(struct drm_connector *connector) +{
- struct drm_device *dev = connector->dev;
- char hotplug_str[] = "HOTPLUG=1", conn_id[21];
- char *envp[] = { hotplug_str, conn_id, NULL };
- snprintf(conn_id, sizeof(conn_id),
"CONNECTOR=%u", connector->base.id);
We have add_uevent_var() that seems a better choice than handrolling snprintf here.
Sam
- drm_dbg_kms(connector->dev,
"[CONNECTOR:%d:%s] generating connector hotplug event\n",connector->base.id, connector->name);- kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE, envp);
+} +EXPORT_SYMBOL(drm_sysfs_connector_hotplug_event);
/**
- drm_sysfs_connector_status_event - generate a DRM uevent for connector
- property status change
diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h index d454ef617b2c..6273cac44e47 100644 --- a/include/drm/drm_sysfs.h +++ b/include/drm/drm_sysfs.h @@ -11,6 +11,7 @@ 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_hotplug_event(struct drm_connector *connector); void drm_sysfs_connector_status_event(struct drm_connector *connector, struct drm_property *property);
#endif
2.33.1
On Fri, Oct 15, 2021 at 09:37:04PM +0200, Sam Ravnborg wrote:
Hi Simon,
On Fri, Oct 15, 2021 at 04:33:41PM +0000, Simon Ser wrote:
This function sends a hotplug uevent with a CONNECTOR property.
A little late feedback.
Sam
If you keep current code then patch is: Reviewed-by: Sam Ravnborg sam@ravnborg.org
As current code is perfectly fine and a similar pattern is used in the same file elsewhere.
Sam
This function is the same as drm_kms_helper_hotplug_event, but takes a connector instead of a device.
Signed-off-by: Simon Ser contact@emersion.fr --- drivers/gpu/drm/drm_probe_helper.c | 23 +++++++++++++++++++++++ include/drm/drm_probe_helper.h | 1 + 2 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 61d5c57f23e1..3aef3b188c99 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -604,6 +604,9 @@ EXPORT_SYMBOL(drm_helper_probe_single_connector_modes); * * This function must be called from process context with no mode * setting locks held. + * + * If only a single connector has changed, consider calling + * drm_kms_helper_connector_hotplug_event() instead. */ void drm_kms_helper_hotplug_event(struct drm_device *dev) { @@ -616,6 +619,26 @@ void drm_kms_helper_hotplug_event(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_hotplug_event);
+/** + * drm_kms_helper_connector_hotplug_event - fire off a KMS connector hotplug event + * @connector: drm_connector which has changed + * + * This is the same as drm_kms_helper_hotplug_event(), except it fires a more + * fine-grained uevent for a single connector. + */ +void drm_kms_helper_connector_hotplug_event(struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + + /* send a uevent + call fbdev */ + drm_sysfs_connector_hotplug_event(connector); + if (dev->mode_config.funcs->output_poll_changed) + dev->mode_config.funcs->output_poll_changed(dev); + + drm_client_dev_hotplug(dev); +} +EXPORT_SYMBOL(drm_kms_helper_connector_hotplug_event); + static void output_poll_execute(struct work_struct *work) { struct delayed_work *delayed_work = to_delayed_work(work); diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h index 04c57564c397..48300aa6ca71 100644 --- a/include/drm/drm_probe_helper.h +++ b/include/drm/drm_probe_helper.h @@ -20,6 +20,7 @@ void drm_kms_helper_poll_fini(struct drm_device *dev); bool drm_helper_hpd_irq_event(struct drm_device *dev); bool drm_connector_helper_hpd_irq_event(struct drm_connector *connector); void drm_kms_helper_hotplug_event(struct drm_device *dev); +void drm_kms_helper_connector_hotplug_event(struct drm_connector *connector);
void drm_kms_helper_poll_disable(struct drm_device *dev); void drm_kms_helper_poll_enable(struct drm_device *dev);
On Fri, Oct 15, 2021 at 04:33:42PM +0000, Simon Ser wrote:
This function is the same as drm_kms_helper_hotplug_event, but takes a connector instead of a device.
Signed-off-by: Simon Ser contact@emersion.fr
Reviewed-by: Sam Ravnborg sam@ravnborg.org
Sam
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,
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
When updating a single connector, use drm_kms_helper_connector_hotplug_event instead of drm_kms_helper_hotplug_event.
Signed-off-by: Simon Ser contact@emersion.fr --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++++---- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 9b1fc54555ee..c261e57d9a22 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -2652,7 +2652,7 @@ static void handle_hpd_irq(void *param) drm_modeset_unlock_all(dev);
if (aconnector->base.force == DRM_FORCE_UNSPECIFIED) - drm_kms_helper_hotplug_event(dev); + drm_kms_helper_connector_hotplug_event(connector);
} else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) { if (new_connection_type == dc_connection_none && @@ -2666,7 +2666,7 @@ static void handle_hpd_irq(void *param) drm_modeset_unlock_all(dev);
if (aconnector->base.force == DRM_FORCE_UNSPECIFIED) - drm_kms_helper_hotplug_event(dev); + drm_kms_helper_connector_hotplug_event(connector); } mutex_unlock(&aconnector->hpd_lock);
@@ -2833,7 +2833,7 @@ static void handle_hpd_rx_irq(void *param) dm_restore_drm_connector_state(dev, connector); drm_modeset_unlock_all(dev);
- drm_kms_helper_hotplug_event(dev); + drm_kms_helper_connector_hotplug_event(connector); } else if (dc_link_detect(dc_link, DETECT_REASON_HPDRX)) {
if (aconnector->fake_enable) @@ -2846,7 +2846,7 @@ static void handle_hpd_rx_irq(void *param) dm_restore_drm_connector_state(dev, connector); drm_modeset_unlock_all(dev);
- drm_kms_helper_hotplug_event(dev); + drm_kms_helper_connector_hotplug_event(connector); } } #ifdef CONFIG_DRM_AMD_DC_HDCP diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c index 87daa78a32b8..23e789855d17 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c @@ -1241,7 +1241,7 @@ static ssize_t trigger_hotplug(struct file *f, const char __user *buf, dm_restore_drm_connector_state(dev, connector); drm_modeset_unlock_all(dev);
- drm_kms_helper_hotplug_event(dev); + drm_kms_helper_connector_hotplug_event(connector); } else if (param[0] == 0) { if (!aconnector->dc_link) goto unlock; @@ -1263,7 +1263,7 @@ static ssize_t trigger_hotplug(struct file *f, const char __user *buf, dm_restore_drm_connector_state(dev, connector); drm_modeset_unlock_all(dev);
- drm_kms_helper_hotplug_event(dev); + drm_kms_helper_connector_hotplug_event(connector); }
unlock:
On 2021-10-15 12:33, Simon Ser wrote:
When updating a single connector, use drm_kms_helper_connector_hotplug_event instead of drm_kms_helper_hotplug_event.
Signed-off-by: Simon Ser contact@emersion.fr
Reviewed-by: Harry Wentland harry.wentland@amd.com
Patches 1-3 are also Acked-by: Harry Wentland harry.wentland@amd.com
Harry
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++++---- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 9b1fc54555ee..c261e57d9a22 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -2652,7 +2652,7 @@ static void handle_hpd_irq(void *param) drm_modeset_unlock_all(dev);
if (aconnector->base.force == DRM_FORCE_UNSPECIFIED)
drm_kms_helper_hotplug_event(dev);
drm_kms_helper_connector_hotplug_event(connector);} else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) { if (new_connection_type == dc_connection_none &&
@@ -2666,7 +2666,7 @@ static void handle_hpd_irq(void *param) drm_modeset_unlock_all(dev);
if (aconnector->base.force == DRM_FORCE_UNSPECIFIED)
drm_kms_helper_hotplug_event(dev);
drm_kms_helper_connector_hotplug_event(connector);@@ -2833,7 +2833,7 @@ static void handle_hpd_rx_irq(void *param) dm_restore_drm_connector_state(dev, connector); drm_modeset_unlock_all(dev);
drm_kms_helper_hotplug_event(dev);
drm_kms_helper_connector_hotplug_event(connector);} else if (dc_link_detect(dc_link, DETECT_REASON_HPDRX)) {
if (aconnector->fake_enable)@@ -2846,7 +2846,7 @@ static void handle_hpd_rx_irq(void *param) dm_restore_drm_connector_state(dev, connector); drm_modeset_unlock_all(dev);
drm_kms_helper_hotplug_event(dev);
drm_kms_helper_connector_hotplug_event(connector);#ifdef CONFIG_DRM_AMD_DC_HDCP diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c index 87daa78a32b8..23e789855d17 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c @@ -1241,7 +1241,7 @@ static ssize_t trigger_hotplug(struct file *f, const char __user *buf, dm_restore_drm_connector_state(dev, connector); drm_modeset_unlock_all(dev);
drm_kms_helper_hotplug_event(dev);
drm_kms_helper_connector_hotplug_event(connector);@@ -1263,7 +1263,7 @@ static ssize_t trigger_hotplug(struct file *f, const char __user *buf, dm_restore_drm_connector_state(dev, connector); drm_modeset_unlock_all(dev);
drm_kms_helper_hotplug_event(dev);
drm_kms_helper_connector_hotplug_event(connector);unlock:
If an hotplug event only updates a single connector, use drm_kms_helper_connector_hotplug_event instead of drm_kms_helper_hotplug_event.
Signed-off-by: Simon Ser contact@emersion.fr --- drivers/gpu/drm/drm_probe_helper.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 3aef3b188c99..6049dc92324b 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -927,7 +927,7 @@ EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event); */ bool drm_helper_hpd_irq_event(struct drm_device *dev) { - struct drm_connector *connector; + struct drm_connector *connector, *changed_connector = NULL; struct drm_connector_list_iter conn_iter; bool changed = false;
@@ -941,15 +941,27 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
- if (check_connector_changed(connector)) + if (check_connector_changed(connector)) { + if (changed) { + if (changed_connector) + drm_connector_put(changed_connector); + changed_connector = NULL; + } else { + drm_connector_get(connector); + changed_connector = connector; + } + changed = true; + } } drm_connector_list_iter_end(&conn_iter); mutex_unlock(&dev->mode_config.mutex);
- if (changed) { + if (changed_connector) { + drm_kms_helper_connector_hotplug_event(changed_connector); + drm_connector_put(changed_connector); + } else if (changed) { drm_kms_helper_hotplug_event(dev); - DRM_DEBUG_KMS("Sent hotplug event\n"); }
return changed;
On Fri, Oct 15, 2021 at 04:33:45PM +0000, Simon Ser wrote:
If an hotplug event only updates a single connector, use drm_kms_helper_connector_hotplug_event instead of drm_kms_helper_hotplug_event.
Signed-off-by: Simon Ser contact@emersion.fr
drivers/gpu/drm/drm_probe_helper.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 3aef3b188c99..6049dc92324b 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -927,7 +927,7 @@ EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event); */ bool drm_helper_hpd_irq_event(struct drm_device *dev) {
- struct drm_connector *connector;
- struct drm_connector *connector, *changed_connector = NULL; struct drm_connector_list_iter conn_iter; bool changed = false;
@@ -941,15 +941,27 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
if (check_connector_changed(connector))
if (check_connector_changed(connector)) {if (changed) {if (changed_connector)drm_connector_put(changed_connector);changed_connector = NULL;} else {drm_connector_get(connector);changed_connector = connector;}changed = true;
So many things that "changed". Would it not be simpler to just grab the first changed connector always, and count how many there were in total?
}
- if (changed) {
- if (changed_connector) {
drm_kms_helper_connector_hotplug_event(changed_connector);drm_connector_put(changed_connector);- } else if (changed) { drm_kms_helper_hotplug_event(dev);
DRM_DEBUG_KMS("Sent hotplug event\n");}
return changed;
-- 2.33.1
On Friday, October 15th, 2021 at 21:41, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
So many things that "changed". Would it not be simpler to just grab the first changed connector always, and count how many there were in total?
Indeed, sounds much better. Will do that in the next version.
Hi Simon,
On Fri, Oct 15, 2021 at 04:33:45PM +0000, Simon Ser wrote:
If an hotplug event only updates a single connector, use drm_kms_helper_connector_hotplug_event instead of drm_kms_helper_hotplug_event.
Signed-off-by: Simon Ser contact@emersion.fr
drivers/gpu/drm/drm_probe_helper.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 3aef3b188c99..6049dc92324b 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -927,7 +927,7 @@ EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event); */ bool drm_helper_hpd_irq_event(struct drm_device *dev) {
- struct drm_connector *connector;
- struct drm_connector *connector, *changed_connector = NULL; struct drm_connector_list_iter conn_iter; bool changed = false;
@@ -941,15 +941,27 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
if (check_connector_changed(connector))
if (check_connector_changed(connector)) {if (changed) {if (changed_connector)drm_connector_put(changed_connector);changed_connector = NULL;} else {drm_connector_get(connector);changed_connector = connector;}
This code is a little confusing to read.
In case we have only one connector with a change we hit the else part. What we really want to find out is if we have one or more connectors with a change. We could do something like:
struct drm_connector *changed_connector = NULL; int changes = 0;
...
/* Find if we have one or more changed connectors */ drm_for_each_connector_iter(connector, &conn_iter) { if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
if (check_connector_changed(connector)) { if (!changes) { changed_connector = connector; drm_connector_get(changed_connector); }
changes++; } } drm_connector_list_iter_end(&conn_iter); mutex_unlock(&dev->mode_config.mutex);
if (changes == 1) drm_kms_helper_connector_hotplug_event(changed_connector); else if (changes > 1) drm_kms_helper_hotplug_event(dev);
if (changed_connector) drm_connector_put(changed_connector);
Maybe the only reason why I think this is better is bc I wrote it?!?
Sam
On Friday, October 15th, 2021 at 22:03, Sam Ravnborg sam@ravnborg.org wrote:
This code is a little confusing to read.
In case we have only one connector with a change we hit the else part. What we really want to find out is if we have one or more connectors with a change. We could do something like:
struct drm_connector *changed_connector = NULL; int changes = 0;
...
/* Find if we have one or more changed connectors */ drm_for_each_connector_iter(connector, &conn_iter) { if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
if (check_connector_changed(connector)) { if (!changes) { changed_connector = connector; drm_connector_get(changed_connector); } changes++; }} drm_connector_list_iter_end(&conn_iter); mutex_unlock(&dev->mode_config.mutex);
if (changes == 1) drm_kms_helper_connector_hotplug_event(changed_connector); else if (changes > 1) drm_kms_helper_hotplug_event(dev);
if (changed_connector) drm_connector_put(changed_connector);
Maybe the only reason why I think this is better is bc I wrote it?!?
Ah, it's not just you, this version is much better. Thanks for the suggestion, will do that in the next version!
Hi Simon,
On Fri, Oct 15, 2021 at 04:33:45PM +0000, Simon Ser wrote:
If an hotplug event only updates a single connector, use drm_kms_helper_connector_hotplug_event instead of drm_kms_helper_hotplug_event.
Signed-off-by: Simon Ser contact@emersion.fr
I guess we'd also need to update drm_connector_helper_hpd_irq_event ?
Maxime
On Monday, October 18th, 2021 at 10:15, Maxime Ripard maxime@cerno.tech wrote:
I guess we'd also need to update drm_connector_helper_hpd_irq_event ?
Good catch! IIRC this function didn't exist when I first wrote the patchset.
When link-status changes, send a hotplug uevent which contains the connector and property ID. That way, user-space can more easily figure out that only the link-status property of this connector has been updated.
Signed-off-by: Simon Ser contact@emersion.fr --- drivers/gpu/drm/i915/display/intel_dp.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 04175f359fd6..afbe34b6e5be 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -5264,6 +5264,8 @@ static void intel_dp_modeset_retry_work_fn(struct work_struct *work) mutex_unlock(&connector->dev->mode_config.mutex); /* Send Hotplug uevent so userspace can reprobe */ drm_kms_helper_hotplug_event(connector->dev); + drm_sysfs_connector_status_event(connector, + connector->dev->mode_config.link_status_property); }
bool
On Fri, Oct 15, 2021 at 04:33:46PM +0000, Simon Ser wrote:
When link-status changes, send a hotplug uevent which contains the connector and property ID. That way, user-space can more easily figure out that only the link-status property of this connector has been updated.
Signed-off-by: Simon Ser contact@emersion.fr
drivers/gpu/drm/i915/display/intel_dp.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 04175f359fd6..afbe34b6e5be 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -5264,6 +5264,8 @@ static void intel_dp_modeset_retry_work_fn(struct work_struct *work) mutex_unlock(&connector->dev->mode_config.mutex); /* Send Hotplug uevent so userspace can reprobe */ drm_kms_helper_hotplug_event(connector->dev);
- drm_sysfs_connector_status_event(connector,
connector->dev->mode_config.link_status_property);
So we're now doing two uevents back to back? Is one not enough?
}
bool
2.33.1
On Friday, October 15th, 2021 at 21:44, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
/* Send Hotplug uevent so userspace can reprobe */ drm_kms_helper_hotplug_event(connector->dev);
- drm_sysfs_connector_status_event(connector,
connector->dev->mode_config.link_status_property);So we're now doing two uevents back to back? Is one not enough?
Oops, I think I missed a step in my rebase. Will fix!
Hi Simon,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on f6632721cd6231e1bf28b5317dcc7543e43359f7]
url: https://github.com/0day-ci/linux/commits/Simon-Ser/drm-add-per-connector-hot... base: f6632721cd6231e1bf28b5317dcc7543e43359f7 config: x86_64-randconfig-a006-20211015 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project a49f5386ce6b091da66ea7c3a1d9a588d53becf7) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/fb2b373f5b3b0f1e37e56270bf7aa0e6332f... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Simon-Ser/drm-add-per-connector-hotplug-events/20211016-003611 git checkout fb2b373f5b3b0f1e37e56270bf7aa0e6332f880d # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/display/intel_dp.c:5267:2: error: implicit declaration of function 'drm_sysfs_connector_status_event' [-Werror,-Wimplicit-function-declaration]
drm_sysfs_connector_status_event(connector, ^ drivers/gpu/drm/i915/display/intel_dp.c:5267:2: note: did you mean 'drm_get_connector_status_name'? include/drm/drm_connector.h:1729:13: note: 'drm_get_connector_status_name' declared here const char *drm_get_connector_status_name(enum drm_connector_status status); ^ 1 error generated.
vim +/drm_sysfs_connector_status_event +5267 drivers/gpu/drm/i915/display/intel_dp.c
5245 5246 static void intel_dp_modeset_retry_work_fn(struct work_struct *work) 5247 { 5248 struct intel_connector *intel_connector; 5249 struct drm_connector *connector; 5250 5251 intel_connector = container_of(work, typeof(*intel_connector), 5252 modeset_retry_work); 5253 connector = &intel_connector->base; 5254 DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, 5255 connector->name); 5256 5257 /* Grab the locks before changing connector property*/ 5258 mutex_lock(&connector->dev->mode_config.mutex); 5259 /* Set connector link status to BAD and send a Uevent to notify 5260 * userspace to do a modeset. 5261 */ 5262 drm_connector_set_link_status_property(connector, 5263 DRM_MODE_LINK_STATUS_BAD); 5264 mutex_unlock(&connector->dev->mode_config.mutex); 5265 /* Send Hotplug uevent so userspace can reprobe */ 5266 drm_kms_helper_hotplug_event(connector->dev);
5267 drm_sysfs_connector_status_event(connector,
5268 connector->dev->mode_config.link_status_property); 5269 } 5270
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
dri-devel@lists.freedesktop.org
-
Harry Wentland -
kernel test robot -
Maxime Ripard -
Pekka Paalanen -
Sam Ravnborg -
Simon Ser -
Ville Syrjälä