| Message ID | 20240908132823.3308029-3-jonas@kwiboo.se (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: bridge: dw_hdmi: Misc enable/disable, CEC and EDID cleanup | expand |
On 08/09/2024 15:28, Jonas Karlman wrote: > drm_helper_hpd_irq_event() and drm_bridge_hpd_notify() may incorrectly > be called with a connected status when HPD is high and RX sense is > changed. This typically happen when the HDMI cable is unplugged, shortly > before the HPD is changed to low. > > Fix this by only notify connected status on the HPD interrupt when HPD > is going high, not on the RX sense interrupt when RX sense is changed. > > Fixes: da09daf88108 ("drm: bridge: dw_hdmi: only trigger hotplug event on link change") > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > --- > v2: New patch > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 9e7f86a0bf5c..055fc9848df4 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -3123,7 +3123,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) > mutex_unlock(&hdmi->cec_notifier_mutex); > } > > - if (phy_stat & HDMI_PHY_HPD) > + if ((intr_stat & HDMI_IH_PHY_STAT0_HPD) && > + (phy_stat & HDMI_PHY_HPD)) > status = connector_status_connected; > > if (!(phy_stat & (HDMI_PHY_HPD | HDMI_PHY_RX_SENSE))) Perhaps this should be also checked for the other lines checking HDMI_PHY_HPD ? Neil
Hi Neil, On 2024-09-09 15:16, Neil Armstrong wrote: > On 08/09/2024 15:28, Jonas Karlman wrote: >> drm_helper_hpd_irq_event() and drm_bridge_hpd_notify() may incorrectly >> be called with a connected status when HPD is high and RX sense is >> changed. This typically happen when the HDMI cable is unplugged, shortly >> before the HPD is changed to low. >> >> Fix this by only notify connected status on the HPD interrupt when HPD >> is going high, not on the RX sense interrupt when RX sense is changed. >> >> Fixes: da09daf88108 ("drm: bridge: dw_hdmi: only trigger hotplug event on link change") >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> >> --- >> v2: New patch >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> index 9e7f86a0bf5c..055fc9848df4 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> @@ -3123,7 +3123,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) >> mutex_unlock(&hdmi->cec_notifier_mutex); >> } >> >> - if (phy_stat & HDMI_PHY_HPD) >> + if ((intr_stat & HDMI_IH_PHY_STAT0_HPD) && >> + (phy_stat & HDMI_PHY_HPD)) >> status = connector_status_connected; >> >> if (!(phy_stat & (HDMI_PHY_HPD | HDMI_PHY_RX_SENSE))) > > Perhaps this should be also checked for the other lines checking HDMI_PHY_HPD ? I think this is the only place we need to check to match the original intent of da09daf88108 ("drm: bridge: dw_hdmi: only trigger hotplug event on link change"). The interrupt pattern I can see when physically unplugging are: - RX interrupt: HPD=high RX=low -> triggered connected event - HPD interrupt: HPD=low RX=low -> trigger disconnected event Based on the commit message the intent was to trigger hotplug event: - when HPD goes high (plugin) - when both HPD and RX sense has gone low (plugout) For plugin we should only trigger when HPD=high at HPD interrupt, as is done after this patch, to avoid unnecessary events when RX changes. For plugout the event should be triggered when both HPD=low and RX=low, that can happen at either HPD or RX interrupt. This should probably be revisited in future, I think we should ignore RX sense and instead use a work queue and a small debounce timeout after HPD interrupt or similar, to better support EDID refresh. Something for a future series. Regards, Jonas > > Neil
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 9e7f86a0bf5c..055fc9848df4 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -3123,7 +3123,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) mutex_unlock(&hdmi->cec_notifier_mutex); } - if (phy_stat & HDMI_PHY_HPD) + if ((intr_stat & HDMI_IH_PHY_STAT0_HPD) && + (phy_stat & HDMI_PHY_HPD)) status = connector_status_connected; if (!(phy_stat & (HDMI_PHY_HPD | HDMI_PHY_RX_SENSE)))
drm_helper_hpd_irq_event() and drm_bridge_hpd_notify() may incorrectly be called with a connected status when HPD is high and RX sense is changed. This typically happen when the HDMI cable is unplugged, shortly before the HPD is changed to low. Fix this by only notify connected status on the HPD interrupt when HPD is going high, not on the RX sense interrupt when RX sense is changed. Fixes: da09daf88108 ("drm: bridge: dw_hdmi: only trigger hotplug event on link change") Signed-off-by: Jonas Karlman <jonas@kwiboo.se> --- v2: New patch --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)