diff mbox series

[v6,17/22] drm: bridge: dw_hdmi: Drop call to drm_bridge_hpd_notify()

Message ID 20260516183838.2024991-18-jonas@kwiboo.se (mailing list archive)
State New
Headers
Series drm: bridge: dw_hdmi: Misc enable/disable, CEC and EDID cleanup |

Commit Message

Jonas Karlman May 16, 2026, 6:38 p.m. UTC 
The use of calls to both drm_helper_hpd_irq_event() and
drm_bridge_hpd_notify() from the HPD IRQ handler may cause multiple
hotplug uevents and modesets when the bridge connector is used.

Use of drm_helper_hpd_irq_event() cause the internal DRM function
check_connector_changed() to be called, which in turn calls the
connector detect()/force() funcs to detect any connection status or
epoch changes, and when changed trigger a hotplug uevent. This also
help ensure that EDID and CEC phys addr is updated.

If only a call drm_bridge_hpd_notify() would be used, a custom connector
status/EDID change detection logic needs to be implemented, to fully
match what check_connector_changed() already provides.

The bridge connector detect() func also ensures that any hpd_notify()
funcs are called for all bridges in the chain, so there is not really
any need to have a call to drm_bridge_hpd_notify() here.

With both calls there is two hotplug uevents, two modesets and a total
of four .hpd_notify() calls (using a bridge connector):

  dw_hdmi_irq(): EVENT=plugout
  drm_helper_hpd_irq_event():
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:check_connector_changed] [CONNECTOR:46:HDMI-A-1] status updated from connected to disconnected
    [drm:check_connector_changed] [CONNECTOR:46:HDMI-A-1] Changed epoch counter 1 => 2
    [drm:drm_sysfs_connector_hotplug_event] [CONNECTOR:46:HDMI-A-1] generating connector hotplug event
   drm_client_hotplug():
    [drm:drm_fb_helper_hotplug_event]
    [drm:drm_client_modeset_probe]
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1]
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1] disconnected
    [drm:drm_edid_connector_update] [CONNECTOR:46:HDMI-A-1] EDID changed, epoch counter 3
    [drm:drm_client_modeset_probe] No connectors reported connected with modes
    [drm:drm_client_modeset_probe] [CONNECTOR:46:HDMI-A-1] enabled? no
    [drm:drm_client_firmware_config.isra.0] Not using firmware configuration
    [drm:drm_client_modeset_probe] picking CRTCs for 3840x2160 config
    [drm:drm_client_hotplug] fbdev: ret=0
  drm_bridge_hpd_notify():
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:drm_sysfs_connector_hotplug_event] [CONNECTOR:46:HDMI-A-1] generating connector hotplug event
   drm_client_hotplug():
    [drm:drm_fb_helper_hotplug_event]
    [drm:drm_client_modeset_probe]
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1]
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1] disconnected
    [drm:drm_client_modeset_probe] No connectors reported connected with modes
    [drm:drm_client_modeset_probe] [CONNECTOR:46:HDMI-A-1] enabled? no
    [drm:drm_client_firmware_config.isra.0] Not using firmware configuration
    [drm:drm_client_modeset_probe] picking CRTCs for 3840x2160 config
    [drm:drm_client_hotplug] fbdev: ret=0

Change to only call drm_helper_hpd_irq_event() from HPD IRQ handler to
ensure that only one hotplug uevent is sent to userspace when connection
status or EDID changes.

With only a call the drm_helper_hpd_irq_event() there is only a single
hotplug uevent and only two .hpd_notify() calls:

  dw_hdmi_irq(): EVENT=plugout
  drm_helper_hpd_irq_event():
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:check_connector_changed] [CONNECTOR:46:HDMI-A-1] status updated from connected to disconnected
    [drm:check_connector_changed] [CONNECTOR:46:HDMI-A-1] Changed epoch counter 1 => 2
    [drm:drm_sysfs_connector_hotplug_event] [CONNECTOR:46:HDMI-A-1] generating connector hotplug event
   drm_client_hotplug():
    [drm:drm_fb_helper_hotplug_event]
    [drm:drm_client_modeset_probe]
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1]
    dw_hdmi_bridge_hpd_notify(status=2)
    [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:46:HDMI-A-1] disconnected
    [drm:drm_edid_connector_update] [CONNECTOR:46:HDMI-A-1] EDID changed, epoch counter 3
    [drm:drm_client_modeset_probe] No connectors reported connected with modes
    [drm:drm_client_modeset_probe] [CONNECTOR:46:HDMI-A-1] enabled? no
    [drm:drm_client_firmware_config.isra.0] Not using firmware configuration
    [drm:drm_client_modeset_probe] picking CRTCs for 3840x2160 config
    [drm:drm_client_hotplug] fbdev: ret=0

Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # Rock64, RockPro64, Quartz64-B
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v6: Drop the call from IRQ handler instead, prior to use of a HPD
    delayed work to avoid a possible deadlock with use of sync() calls
    in the bridge hpd_disable() ops,
    Update commit message,
    Collect t-b tag
v5: New patch
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

sashiko-bot@kernel.org May 16, 2026, 7:52 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing drm_bridge_hpd_notify() violates the DRM bridge API contract and causes hotplug events to be permanently lost during system suspend.
--

commit 6b3491e39082015ab62431ea0c93c74c9344b995
Author: Jonas Karlman <jonas@kwiboo.se>

drm: bridge: dw_hdmi: Drop call to drm_bridge_hpd_notify()

This commit removes the drm_bridge_hpd_notify() call from the dw_hdmi IRQ
handler, relying instead only on drm_helper_hpd_irq_event() to avoid sending
duplicate hotplug uevents to userspace.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index e9c4e24c090ca..6cc7b2a860bd8 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -3101,10 +3101,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>  			status == connector_status_connected ?
>  			"plugin" : "plugout");
>  
> -		if (hdmi->bridge.dev) {
> +		if (hdmi->bridge.dev)
>  			drm_helper_hpd_irq_event(hdmi->bridge.dev);
> -			drm_bridge_hpd_notify(&hdmi->bridge, status);
> -		}
>  	}

Will removing drm_bridge_hpd_notify() cause hotplug events to be permanently
lost if they occur during system suspend?

Since the driver advertises DRM_BRIDGE_OP_HPD, does the DRM bridge API
contract still require it to call drm_bridge_hpd_notify() to properly
propagate hotplug events to the rest of the bridge chain?

Looking at drm_helper_hpd_irq_event(), it aborts early if polling is
disabled:

drivers/gpu/drm/drm_probe_helper.c:drm_helper_hpd_irq_event() {
	struct drm_connector *connector, *first_changed_connector = NULL;
	struct drm_connector_list_iter conn_iter;
	int changed = 0;

	if (!dev->mode_config.poll_enabled)
		return false;
...
}

During system suspend, drm_kms_helper_poll_disable() sets poll_enabled
to false. If an HPD interrupt fires while the system is suspended, won't
drm_helper_hpd_irq_event() silently drop the event without updating the
connector status or scheduling any deferred work?

Furthermore, when the system resumes, output_poll_execute() explicitly skips
connectors that use the DRM_CONNECTOR_POLL_HPD flag, and the dw_hdmi resume
handler does not appear to manually trigger a hotplug event check.

Could this result in the system being completely unaware of any display
connections or disconnections that happened while suspended, at least until
a manual reprobe is forced by userspace?
Jonas Karlman May 16, 2026, 8 p.m. UTC | #2 
On 5/16/2026 9:52 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Removing drm_bridge_hpd_notify() violates the DRM bridge API contract and causes hotplug events to be permanently lost during system suspend.
> --
> 
> commit 6b3491e39082015ab62431ea0c93c74c9344b995
> Author: Jonas Karlman <jonas@kwiboo.se>
> 
> drm: bridge: dw_hdmi: Drop call to drm_bridge_hpd_notify()
> 
> This commit removes the drm_bridge_hpd_notify() call from the dw_hdmi IRQ
> handler, relying instead only on drm_helper_hpd_irq_event() to avoid sending
> duplicate hotplug uevents to userspace.
> 
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index e9c4e24c090ca..6cc7b2a860bd8 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -3101,10 +3101,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>>  			status == connector_status_connected ?
>>  			"plugin" : "plugout");
>>  
>> -		if (hdmi->bridge.dev) {
>> +		if (hdmi->bridge.dev)
>>  			drm_helper_hpd_irq_event(hdmi->bridge.dev);
>> -			drm_bridge_hpd_notify(&hdmi->bridge, status);
>> -		}
>>  	}
> 
> Will removing drm_bridge_hpd_notify() cause hotplug events to be permanently
> lost if they occur during system suspend?

No, drm_mode_config_helper_resume() is called at resume.

> Since the driver advertises DRM_BRIDGE_OP_HPD, does the DRM bridge API
> contract still require it to call drm_bridge_hpd_notify() to properly
> propagate hotplug events to the rest of the bridge chain?

No, hpd_notify() funcs are still called from bridge connector detect().

> Looking at drm_helper_hpd_irq_event(), it aborts early if polling is
> disabled:
> 
> drivers/gpu/drm/drm_probe_helper.c:drm_helper_hpd_irq_event() {
> 	struct drm_connector *connector, *first_changed_connector = NULL;
> 	struct drm_connector_list_iter conn_iter;
> 	int changed = 0;
> 
> 	if (!dev->mode_config.poll_enabled)
> 		return false;
> ...
> }
> 
> During system suspend, drm_kms_helper_poll_disable() sets poll_enabled
> to false. If an HPD interrupt fires while the system is suspended, won't
> drm_helper_hpd_irq_event() silently drop the event without updating the
> connector status or scheduling any deferred work?

No, drm_mode_config_helper_suspend/resume() will call hpd_enable/disable()
to signal when the connector/bridge should start listening for HPD. I.e.
not during suspend.

> Furthermore, when the system resumes, output_poll_execute() explicitly skips
> connectors that use the DRM_CONNECTOR_POLL_HPD flag, and the dw_hdmi resume
> handler does not appear to manually trigger a hotplug event check.
> 
> Could this result in the system being completely unaware of any display
> connections or disconnections that happened while suspended, at least until
> a manual reprobe is forced by userspace?

No, drm_mode_config_helper_resume() is called at resume.

Regards,
Jonas
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index e9c4e24c090c..6cc7b2a860bd 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -3101,10 +3101,8 @@  static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 			status == connector_status_connected ?
 			"plugin" : "plugout");
 
-		if (hdmi->bridge.dev) {
+		if (hdmi->bridge.dev)
 			drm_helper_hpd_irq_event(hdmi->bridge.dev);
-			drm_bridge_hpd_notify(&hdmi->bridge, status);
-		}
 	}
 
 	hdmi_writeb(hdmi, intr_stat, HDMI_IH_PHY_STAT0);