The documentation of the drm_helper_hpd_irq_event() function didn't document the value that function was returning. Add that part as well.
Signed-off-by: Maxime Ripard maxime@cerno.tech
---
Changes from v2: - new patch --- drivers/gpu/drm/drm_probe_helper.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 5606bca3caa8..5b77fb5c1a32 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -817,6 +817,9 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini); * * Note that a connector can be both polled and probed from the hotplug handler, * in case the hotplug interrupt is known to be unreliable. + * + * Returns: + * A boolean indicating whether the connector status changed or not */ bool drm_helper_hpd_irq_event(struct drm_device *dev) {
The drm_helper_hpd_irq_event() function is iterating over all the connectors when an hotplug event is detected.
During that iteration, it will call each connector detect function and figure out if its status changed.
Finally, if any connector changed, it will notify the user-space and the clients that something changed on the DRM device.
This is supposed to be used for drivers that don't have a hotplug interrupt for individual connectors. However, drivers that can use an interrupt for a single connector are left in the dust and can either reimplement the logic used during the iteration for each connector or use that helper and iterate over all connectors all the time.
Since both are suboptimal, let's create a helper that will only perform the status detection on a single connector.
Signed-off-by: Maxime Ripard maxime@cerno.tech
--- Changes from v2: - Skip connectors with DRM_CONNECTOR_POLL_HPD in drm_helper_hpd_irq_event - Add drm_connector_helper_hpd_irq_event returned value documentation - Improve logging
Changes from v1: - Rename the shared function - Move the hotplug event notification out of the shared function - Added missing locks - Improve the documentation - Switched to drm_dbg_kms --- drivers/gpu/drm/drm_probe_helper.c | 117 +++++++++++++++++++++-------- include/drm/drm_probe_helper.h | 1 + 2 files changed, 87 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 5b77fb5c1a32..a1ffc0c30b3a 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -795,6 +795,87 @@ void drm_kms_helper_poll_fini(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_poll_fini);
+static bool check_connector_changed(struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + enum drm_connector_status old_status; + u64 old_epoch_counter; + bool changed = false; + + /* Only handle HPD capable connectors. */ + drm_WARN_ON(dev, !(connector->polled & DRM_CONNECTOR_POLL_HPD)); + + drm_WARN_ON(dev, !mutex_is_locked(&dev->mode_config.mutex)); + + old_status = connector->status; + old_epoch_counter = connector->epoch_counter; + connector->status = drm_helper_probe_detect(connector, NULL, false); + + if (old_epoch_counter == connector->epoch_counter) { + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Same epoch counter %llu\n", + connector->base.id, + connector->name, + connector->epoch_counter); + + return false; + } + + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] status updated from %s to %s\n", + connector->base.id, + connector->name, + drm_get_connector_status_name(old_status) + drm_get_connector_status_name(connector->status)); + + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Changed epoch counter %llu => %llu\n", + connector->base.id, + connector->name, + old_epoch_counter, + connector->epoch_counter); + + return true; +} + +/** + * drm_connector_helper_hpd_irq_event - hotplug processing + * @connector: drm_connector + * + * Drivers can use this helper function to run a detect cycle on a connector + * which has the DRM_CONNECTOR_POLL_HPD flag set in its &polled member. + * + * This helper function is useful for drivers which can track hotplug + * interrupts for a single connector. Drivers that want to send a + * hotplug event for all connectors or can't track hotplug interrupts + * per connector need to use drm_helper_hpd_irq_event(). + * + * This function must be called from process context with no mode + * setting locks held. + * + * Note that a connector can be both polled and probed from the hotplug + * handler, in case the hotplug interrupt is known to be unreliable. + * + * Returns: + * A boolean indicating whether the connector status changed or not + */ +bool drm_connector_helper_hpd_irq_event(struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + bool changed; + + mutex_lock(&dev->mode_config.mutex); + changed = check_connector_changed(connector); + mutex_unlock(&dev->mode_config.mutex); + + if (changed) { + drm_kms_helper_hotplug_event(dev); + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Sent hotplug event\n", + connector->base.id, + connector->name); + } + + return changed; +} +EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event); + /** * drm_helper_hpd_irq_event - hotplug processing * @dev: drm_device @@ -808,9 +889,10 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini); * interrupts for each connector. * * Drivers which support hotplug interrupts for each connector individually and - * which have a more fine-grained detect logic should bypass this code and - * directly call drm_kms_helper_hotplug_event() in case the connector state - * changed. + * which have a more fine-grained detect logic can use + * drm_connector_helper_hpd_irq_event(). Alternatively, they should bypass this + * code and directly call drm_kms_helper_hotplug_event() in case the connector + * state changed. * * This function must be called from process context with no mode * setting locks held. @@ -825,9 +907,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) { struct drm_connector *connector; struct drm_connector_list_iter conn_iter; - enum drm_connector_status old_status; bool changed = false; - u64 old_epoch_counter;
if (!dev->mode_config.poll_enabled) return false; @@ -839,33 +919,8 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
- old_status = connector->status; - - old_epoch_counter = connector->epoch_counter; - - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Old epoch counter %llu\n", connector->base.id, - connector->name, - old_epoch_counter); - - connector->status = drm_helper_probe_detect(connector, NULL, false); - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", - connector->base.id, - connector->name, - drm_get_connector_status_name(old_status), - drm_get_connector_status_name(connector->status)); - - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] New epoch counter %llu\n", - connector->base.id, - connector->name, - connector->epoch_counter); - - /* - * Check if epoch counter had changed, meaning that we need - * to send a uevent. - */ - if (old_epoch_counter != connector->epoch_counter) + if (check_connector_changed(connector)) changed = true; - } drm_connector_list_iter_end(&conn_iter); mutex_unlock(&dev->mode_config.mutex); diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h index 8d3ed2834d34..04c57564c397 100644 --- a/include/drm/drm_probe_helper.h +++ b/include/drm/drm_probe_helper.h @@ -18,6 +18,7 @@ int drm_helper_probe_detect(struct drm_connector *connector, void drm_kms_helper_poll_init(struct drm_device *dev); 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_poll_disable(struct drm_device *dev);
Hi Maxime,
On Tue, Sep 14, 2021 at 12:17:23PM +0200, Maxime Ripard wrote:
The drm_helper_hpd_irq_event() function is iterating over all the connectors when an hotplug event is detected.
During that iteration, it will call each connector detect function and figure out if its status changed.
Finally, if any connector changed, it will notify the user-space and the clients that something changed on the DRM device.
This is supposed to be used for drivers that don't have a hotplug interrupt for individual connectors. However, drivers that can use an interrupt for a single connector are left in the dust and can either reimplement the logic used during the iteration for each connector or use that helper and iterate over all connectors all the time.
Since both are suboptimal, let's create a helper that will only perform the status detection on a single connector.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Changes from v2:
- Skip connectors with DRM_CONNECTOR_POLL_HPD in drm_helper_hpd_irq_event
- Add drm_connector_helper_hpd_irq_event returned value documentation
- Improve logging
Changes from v1:
- Rename the shared function
- Move the hotplug event notification out of the shared function
- Added missing locks
- Improve the documentation
- Switched to drm_dbg_kms
drivers/gpu/drm/drm_probe_helper.c | 117 +++++++++++++++++++++-------- include/drm/drm_probe_helper.h | 1 + 2 files changed, 87 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 5b77fb5c1a32..a1ffc0c30b3a 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -795,6 +795,87 @@ void drm_kms_helper_poll_fini(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_poll_fini);
+static bool check_connector_changed(struct drm_connector *connector) +{
- struct drm_device *dev = connector->dev;
- enum drm_connector_status old_status;
- u64 old_epoch_counter;
- bool changed = false;
- /* Only handle HPD capable connectors. */
- drm_WARN_ON(dev, !(connector->polled & DRM_CONNECTOR_POLL_HPD));
This will WARN if DRM_CONNECTOR_POLL_HPD is not set - which the previous code did not. I am not sure this is intentional. Or have I missed something?
Sam
- drm_WARN_ON(dev, !mutex_is_locked(&dev->mode_config.mutex));
- old_status = connector->status;
- old_epoch_counter = connector->epoch_counter;
- connector->status = drm_helper_probe_detect(connector, NULL, false);
- if (old_epoch_counter == connector->epoch_counter) {
drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Same epoch counter %llu\n",connector->base.id,connector->name,connector->epoch_counter);return false;- }
- drm_dbg_kms(dev, "[CONNECTOR:%d:%s] status updated from %s to %s\n",
connector->base.id,connector->name,drm_get_connector_status_name(old_status)drm_get_connector_status_name(connector->status));- drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Changed epoch counter %llu => %llu\n",
connector->base.id,connector->name,old_epoch_counter,connector->epoch_counter);- return true;
+}
+/**
- drm_connector_helper_hpd_irq_event - hotplug processing
- @connector: drm_connector
- Drivers can use this helper function to run a detect cycle on a connector
- which has the DRM_CONNECTOR_POLL_HPD flag set in its &polled member.
- This helper function is useful for drivers which can track hotplug
- interrupts for a single connector. Drivers that want to send a
- hotplug event for all connectors or can't track hotplug interrupts
- per connector need to use drm_helper_hpd_irq_event().
- This function must be called from process context with no mode
- setting locks held.
- Note that a connector can be both polled and probed from the hotplug
- handler, in case the hotplug interrupt is known to be unreliable.
- Returns:
- A boolean indicating whether the connector status changed or not
- */
+bool drm_connector_helper_hpd_irq_event(struct drm_connector *connector) +{
- struct drm_device *dev = connector->dev;
- bool changed;
- mutex_lock(&dev->mode_config.mutex);
- changed = check_connector_changed(connector);
- mutex_unlock(&dev->mode_config.mutex);
- if (changed) {
drm_kms_helper_hotplug_event(dev);drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Sent hotplug event\n",connector->base.id,connector->name);- }
- return changed;
+} +EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event);
/**
- drm_helper_hpd_irq_event - hotplug processing
- @dev: drm_device
@@ -808,9 +889,10 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini);
- interrupts for each connector.
- Drivers which support hotplug interrupts for each connector individually and
- which have a more fine-grained detect logic should bypass this code and
- directly call drm_kms_helper_hotplug_event() in case the connector state
- changed.
- which have a more fine-grained detect logic can use
- drm_connector_helper_hpd_irq_event(). Alternatively, they should bypass this
- code and directly call drm_kms_helper_hotplug_event() in case the connector
- state changed.
- This function must be called from process context with no mode
- setting locks held.
@@ -825,9 +907,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) { struct drm_connector *connector; struct drm_connector_list_iter conn_iter;
enum drm_connector_status old_status; bool changed = false;
u64 old_epoch_counter;
if (!dev->mode_config.poll_enabled) return false;
@@ -839,33 +919,8 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) continue;
old_status = connector->status;old_epoch_counter = connector->epoch_counter;DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Old epoch counter %llu\n", connector->base.id,connector->name,old_epoch_counter);connector->status = drm_helper_probe_detect(connector, NULL, false);DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",connector->base.id,connector->name,drm_get_connector_status_name(old_status),drm_get_connector_status_name(connector->status));DRM_DEBUG_KMS("[CONNECTOR:%d:%s] New epoch counter %llu\n",connector->base.id,connector->name,connector->epoch_counter);/** Check if epoch counter had changed, meaning that we need* to send a uevent.*/if (old_epoch_counter != connector->epoch_counter)
if (check_connector_changed(connector)) changed = true;
- } drm_connector_list_iter_end(&conn_iter); mutex_unlock(&dev->mode_config.mutex);
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h index 8d3ed2834d34..04c57564c397 100644 --- a/include/drm/drm_probe_helper.h +++ b/include/drm/drm_probe_helper.h @@ -18,6 +18,7 @@ int drm_helper_probe_detect(struct drm_connector *connector, void drm_kms_helper_poll_init(struct drm_device *dev); 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_poll_disable(struct drm_device *dev);
2.31.1
Hi Sam,
On Tue, Sep 14, 2021 at 12:41:55PM +0200, Sam Ravnborg wrote:
On Tue, Sep 14, 2021 at 12:17:23PM +0200, Maxime Ripard wrote:
The drm_helper_hpd_irq_event() function is iterating over all the connectors when an hotplug event is detected.
During that iteration, it will call each connector detect function and figure out if its status changed.
Finally, if any connector changed, it will notify the user-space and the clients that something changed on the DRM device.
This is supposed to be used for drivers that don't have a hotplug interrupt for individual connectors. However, drivers that can use an interrupt for a single connector are left in the dust and can either reimplement the logic used during the iteration for each connector or use that helper and iterate over all connectors all the time.
Since both are suboptimal, let's create a helper that will only perform the status detection on a single connector.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Changes from v2:
- Skip connectors with DRM_CONNECTOR_POLL_HPD in drm_helper_hpd_irq_event
- Add drm_connector_helper_hpd_irq_event returned value documentation
- Improve logging
Changes from v1:
- Rename the shared function
- Move the hotplug event notification out of the shared function
- Added missing locks
- Improve the documentation
- Switched to drm_dbg_kms
drivers/gpu/drm/drm_probe_helper.c | 117 +++++++++++++++++++++-------- include/drm/drm_probe_helper.h | 1 + 2 files changed, 87 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 5b77fb5c1a32..a1ffc0c30b3a 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -795,6 +795,87 @@ void drm_kms_helper_poll_fini(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_poll_fini);
+static bool check_connector_changed(struct drm_connector *connector) +{
- struct drm_device *dev = connector->dev;
- enum drm_connector_status old_status;
- u64 old_epoch_counter;
- bool changed = false;
- /* Only handle HPD capable connectors. */
- drm_WARN_ON(dev, !(connector->polled & DRM_CONNECTOR_POLL_HPD));
This will WARN if DRM_CONNECTOR_POLL_HPD is not set - which the previous code did not. I am not sure this is intentional. Or have I missed something?
Sorry, I misunderstood your previous comment and thought you wanted to skip the !HPD connectors in the drm_helper_hpd_irq_event loop.
What do you think would be the proper scenario here? Just return false?
Maxime
On Tue, Sep 14, 2021 at 05:06:41PM +0200, Maxime Ripard wrote:
Hi Sam,
On Tue, Sep 14, 2021 at 12:41:55PM +0200, Sam Ravnborg wrote:
On Tue, Sep 14, 2021 at 12:17:23PM +0200, Maxime Ripard wrote:
The drm_helper_hpd_irq_event() function is iterating over all the connectors when an hotplug event is detected.
During that iteration, it will call each connector detect function and figure out if its status changed.
Finally, if any connector changed, it will notify the user-space and the clients that something changed on the DRM device.
This is supposed to be used for drivers that don't have a hotplug interrupt for individual connectors. However, drivers that can use an interrupt for a single connector are left in the dust and can either reimplement the logic used during the iteration for each connector or use that helper and iterate over all connectors all the time.
Since both are suboptimal, let's create a helper that will only perform the status detection on a single connector.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Changes from v2:
- Skip connectors with DRM_CONNECTOR_POLL_HPD in drm_helper_hpd_irq_event
- Add drm_connector_helper_hpd_irq_event returned value documentation
- Improve logging
Changes from v1:
- Rename the shared function
- Move the hotplug event notification out of the shared function
- Added missing locks
- Improve the documentation
- Switched to drm_dbg_kms
drivers/gpu/drm/drm_probe_helper.c | 117 +++++++++++++++++++++-------- include/drm/drm_probe_helper.h | 1 + 2 files changed, 87 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 5b77fb5c1a32..a1ffc0c30b3a 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -795,6 +795,87 @@ void drm_kms_helper_poll_fini(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_poll_fini);
+static bool check_connector_changed(struct drm_connector *connector) +{
- struct drm_device *dev = connector->dev;
- enum drm_connector_status old_status;
- u64 old_epoch_counter;
- bool changed = false;
- /* Only handle HPD capable connectors. */
- drm_WARN_ON(dev, !(connector->polled & DRM_CONNECTOR_POLL_HPD));
This will WARN if DRM_CONNECTOR_POLL_HPD is not set - which the previous code did not. I am not sure this is intentional. Or have I missed something?
Sorry, I misunderstood your previous comment and thought you wanted to skip the !HPD connectors in the drm_helper_hpd_irq_event loop.
What do you think would be the proper scenario here? Just return false?
Re-reading the patch it looks good. I missed that the check is no longer deleted - and it is now done exactly as I thought it should be.
So patch is: Reviewed-by: Sam Ravnborg sam@ravnborg.org
Sam
On Tue, 14 Sep 2021 12:17:23 +0200, Maxime Ripard wrote:
The drm_helper_hpd_irq_event() function is iterating over all the connectors when an hotplug event is detected.
During that iteration, it will call each connector detect function and figure out if its status changed.
Finally, if any connector changed, it will notify the user-space and the clients that something changed on the DRM device.
[...]
Applied to drm/drm-misc (drm-misc-next).
Thanks! Maxime
The drm_helper_hpd_irq_event() documentation states that this function is "useful for drivers which can't or don't track hotplug interrupts for each connector." and that "Drivers which support hotplug interrupts for each connector individually and which have a more fine-grained detect logic should bypass this code and directly call drm_kms_helper_hotplug_event()". This is thus what we ended-up doing.
However, what this actually means, and is further explained in the drm_kms_helper_hotplug_event() documentation, is that drm_kms_helper_hotplug_event() should be called by drivers that can track the connection status change, and if it has changed we should call that function.
This underlying expectation we failed to provide is that the caller of drm_kms_helper_hotplug_event() should call drm_helper_probe_detect() to probe the new status of the connector.
Since we didn't do it, it meant that even though we were sending the notification to user-space and the DRM clients that something changed we never probed or updated our internal connector status ourselves.
This went mostly unnoticed since the detect callback usually doesn't have any side-effect. Also, if we were using the DRM fbdev emulation (which is a DRM client), or any user-space application that can deal with hotplug events, chances are they would react to the hotplug event by probing the connector status eventually.
However, now that we have to enable the scrambler in detect() if it was enabled it has a side effect, and an application such as Kodi or modetest doesn't deal with hotplug events. This resulted with a black screen when Kodi or modetest was running when a screen was disconnected and then reconnected, or switched off and on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index a3dbd1fdff7d..d9e001b9314f 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1578,10 +1578,11 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) static irqreturn_t vc4_hdmi_hpd_irq_thread(int irq, void *priv) { struct vc4_hdmi *vc4_hdmi = priv; - struct drm_device *dev = vc4_hdmi->connector.dev; + struct drm_connector *connector = &vc4_hdmi->connector; + struct drm_device *dev = connector->dev;
if (dev && dev->registered) - drm_kms_helper_hotplug_event(dev); + drm_connector_helper_hpd_irq_event(connector);
return IRQ_HANDLED; }
On Tue, Sep 14, 2021 at 12:17:24PM +0200, Maxime Ripard wrote:
The drm_helper_hpd_irq_event() documentation states that this function is "useful for drivers which can't or don't track hotplug interrupts for each connector." and that "Drivers which support hotplug interrupts for each connector individually and which have a more fine-grained detect logic should bypass this code and directly call drm_kms_helper_hotplug_event()". This is thus what we ended-up doing.
However, what this actually means, and is further explained in the drm_kms_helper_hotplug_event() documentation, is that drm_kms_helper_hotplug_event() should be called by drivers that can track the connection status change, and if it has changed we should call that function.
This underlying expectation we failed to provide is that the caller of drm_kms_helper_hotplug_event() should call drm_helper_probe_detect() to probe the new status of the connector.
Since we didn't do it, it meant that even though we were sending the notification to user-space and the DRM clients that something changed we never probed or updated our internal connector status ourselves.
This went mostly unnoticed since the detect callback usually doesn't have any side-effect. Also, if we were using the DRM fbdev emulation (which is a DRM client), or any user-space application that can deal with hotplug events, chances are they would react to the hotplug event by probing the connector status eventually.
However, now that we have to enable the scrambler in detect() if it was enabled it has a side effect, and an application such as Kodi or modetest doesn't deal with hotplug events. This resulted with a black screen when Kodi or modetest was running when a screen was disconnected and then reconnected, or switched off and on.
Uh, why are you running this scrambler restore in your probe function? I guess it works, but most drivers that do expensive hotplug restore to handle the "no black screen for replug" use-case handle that in their own dedicated code.
But those also tend to have per-output hpd interrupt sources, so maybe that's why? -Daniel
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index a3dbd1fdff7d..d9e001b9314f 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1578,10 +1578,11 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) static irqreturn_t vc4_hdmi_hpd_irq_thread(int irq, void *priv) { struct vc4_hdmi *vc4_hdmi = priv;
- struct drm_device *dev = vc4_hdmi->connector.dev;
struct drm_connector *connector = &vc4_hdmi->connector;
struct drm_device *dev = connector->dev;
if (dev && dev->registered)
drm_kms_helper_hotplug_event(dev);
drm_connector_helper_hpd_irq_event(connector);return IRQ_HANDLED;
}
2.31.1
Hi Daniel,
On Tue, Sep 14, 2021 at 04:34:08PM +0200, Daniel Vetter wrote:
On Tue, Sep 14, 2021 at 12:17:24PM +0200, Maxime Ripard wrote:
The drm_helper_hpd_irq_event() documentation states that this function is "useful for drivers which can't or don't track hotplug interrupts for each connector." and that "Drivers which support hotplug interrupts for each connector individually and which have a more fine-grained detect logic should bypass this code and directly call drm_kms_helper_hotplug_event()". This is thus what we ended-up doing.
However, what this actually means, and is further explained in the drm_kms_helper_hotplug_event() documentation, is that drm_kms_helper_hotplug_event() should be called by drivers that can track the connection status change, and if it has changed we should call that function.
This underlying expectation we failed to provide is that the caller of drm_kms_helper_hotplug_event() should call drm_helper_probe_detect() to probe the new status of the connector.
Since we didn't do it, it meant that even though we were sending the notification to user-space and the DRM clients that something changed we never probed or updated our internal connector status ourselves.
This went mostly unnoticed since the detect callback usually doesn't have any side-effect. Also, if we were using the DRM fbdev emulation (which is a DRM client), or any user-space application that can deal with hotplug events, chances are they would react to the hotplug event by probing the connector status eventually.
However, now that we have to enable the scrambler in detect() if it was enabled it has a side effect, and an application such as Kodi or modetest doesn't deal with hotplug events. This resulted with a black screen when Kodi or modetest was running when a screen was disconnected and then reconnected, or switched off and on.
Uh, why are you running this scrambler restore in your probe function? I guess it works, but most drivers that do expensive hotplug restore to handle the "no black screen for replug" use-case handle that in their own dedicated code.
That's what I got from our discussion back in June (I think?). The discussion was about an issue we were having back then where one would disconnect / reconnect the display while it had been setup through SCDC to use the scrambler and higher bit ratio.
During that power cycle (that also happens when you turn a TV off and on), the display would obviously reset its SCDC status. But if there's nothing to handle the uevent, then the same mode remains applied resulting in a blank screen.
If we're not supposed to set the SCDC status again in detect(), how would we deal with this?
But those also tend to have per-output hpd interrupt sources, so maybe that's why?
We do have an per-output HPD interrupt here
Maximey
Hi Maxime,
On Tue, Sep 14, 2021 at 12:17:24PM +0200, Maxime Ripard wrote:
The drm_helper_hpd_irq_event() documentation states that this function is "useful for drivers which can't or don't track hotplug interrupts for each connector." and that "Drivers which support hotplug interrupts for each connector individually and which have a more fine-grained detect logic should bypass this code and directly call drm_kms_helper_hotplug_event()". This is thus what we ended-up doing.
However, what this actually means, and is further explained in the drm_kms_helper_hotplug_event() documentation, is that drm_kms_helper_hotplug_event() should be called by drivers that can track the connection status change, and if it has changed we should call that function.
This underlying expectation we failed to provide is that the caller of drm_kms_helper_hotplug_event() should call drm_helper_probe_detect() to probe the new status of the connector.
Since we didn't do it, it meant that even though we were sending the notification to user-space and the DRM clients that something changed we never probed or updated our internal connector status ourselves.
This went mostly unnoticed since the detect callback usually doesn't have any side-effect. Also, if we were using the DRM fbdev emulation (which is a DRM client), or any user-space application that can deal with hotplug events, chances are they would react to the hotplug event by probing the connector status eventually.
However, now that we have to enable the scrambler in detect() if it was enabled it has a side effect, and an application such as Kodi or modetest doesn't deal with hotplug events. This resulted with a black screen when Kodi or modetest was running when a screen was disconnected and then reconnected, or switched off and on.
Signed-off-by: Maxime Ripard maxime@cerno.tech
I leave it to you and Daniel to sort out this patch.
Sam
On Tue, 14 Sep 2021 12:17:24 +0200, Maxime Ripard wrote:
The drm_helper_hpd_irq_event() documentation states that this function is "useful for drivers which can't or don't track hotplug interrupts for each connector." and that "Drivers which support hotplug interrupts for each connector individually and which have a more fine-grained detect logic should bypass this code and directly call drm_kms_helper_hotplug_event()". This is thus what we ended-up doing.
[...]
Applied to drm/drm-misc (drm-misc-next).
Thanks! Maxime
On Tue, Sep 14, 2021 at 12:17:22PM +0200, Maxime Ripard wrote:
The documentation of the drm_helper_hpd_irq_event() function didn't document the value that function was returning. Add that part as well.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Sam Ravnborg sam@ravnborg.org
Changes from v2:
- new patch
drivers/gpu/drm/drm_probe_helper.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 5606bca3caa8..5b77fb5c1a32 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -817,6 +817,9 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini);
- Note that a connector can be both polled and probed from the hotplug handler,
- in case the hotplug interrupt is known to be unreliable.
- Returns:
*/
- A boolean indicating whether the connector status changed or not
bool drm_helper_hpd_irq_event(struct drm_device *dev) { -- 2.31.1
On Tue, 14 Sep 2021 12:17:22 +0200, Maxime Ripard wrote:
The documentation of the drm_helper_hpd_irq_event() function didn't document the value that function was returning. Add that part as well.
Applied to drm/drm-misc (drm-misc-next).
Thanks! Maxime
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maxime Ripard -
Sam Ravnborg