[PATCH] drm: bridge: dw_hdmi: only trigger hotplug event on real link state change

[PATCH] drm: bridge: dw_hdmi: only trigger hotplug event on real link state change

[Lucas Stach]

There are two events that signal a real change of the link state: HPD going
high means the sink is newly connected or wants the source to re-read the
EDID, RX sense going low is a indication that the link has been disconnected.

Ignore the other two events that also trigger interrupts, but don't need
immediate attention: HPD going low does not necessarily mean the link has
been lost and should not trigger a immediate read of the status. RX sense
going high also does not require a detect cycle, as HPD going high is the
right point in time to read the EDID.

Ignoring the negative HPD edge does make the detection much more robust
against spurious link status changes due to EMI or marginal signal levels.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 3e1be9894ed1..24f991b5248d 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -3095,6 +3095,7 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 {
 	struct dw_hdmi *hdmi = dev_id;
 	u8 intr_stat, phy_int_pol, phy_pol_mask, phy_stat;
+	enum drm_connector_status status = connector_status_unknown;
 
 	intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0);
 	phy_int_pol = hdmi_readb(hdmi, HDMI_PHY_POL0);
@@ -3133,13 +3134,15 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 			cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
 			mutex_unlock(&hdmi->cec_notifier_mutex);
 		}
-	}
 
-	if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
-		enum drm_connector_status status = phy_int_pol & HDMI_PHY_HPD
-						 ? connector_status_connected
-						 : connector_status_disconnected;
+		if (phy_stat & HDMI_PHY_HPD)
+			status = connector_status_connected;
+
+		if (!( phy_stat & HDMI_PHY_RX_SENSE))
+			status = connector_status_disconnected;
+	}
 
+	if (status != connector_status_unknown) {
 		dev_dbg(hdmi->dev, "EVENT=%s\n",
 			status == connector_status_connected ?
 			"plugin" : "plugout");
-- 
2.30.2

Re: [PATCH] drm: bridge: dw_hdmi: only trigger hotplug event on real link state change

[Neil Armstrong]

Hi,

On 27/06/2022 14:47, Lucas Stach wrote:
> There are two events that signal a real change of the link state: HPD going
> high means the sink is newly connected or wants the source to re-read the
> EDID, RX sense going low is a indication that the link has been disconnected.
> 
> Ignore the other two events that also trigger interrupts, but don't need
> immediate attention: HPD going low does not necessarily mean the link has
> been lost and should not trigger a immediate read of the status. RX sense
> going high also does not require a detect cycle, as HPD going high is the
> right point in time to read the EDID.
> 
> Ignoring the negative HPD edge does make the detection much more robust
> against spurious link status changes due to EMI or marginal signal levels.

Fair enough, but it means RX Sense must be totally functional with this change.

> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 3e1be9894ed1..24f991b5248d 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -3095,6 +3095,7 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>   {
>   	struct dw_hdmi *hdmi = dev_id;
>   	u8 intr_stat, phy_int_pol, phy_pol_mask, phy_stat;
> +	enum drm_connector_status status = connector_status_unknown;
>   
>   	intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0);
>   	phy_int_pol = hdmi_readb(hdmi, HDMI_PHY_POL0);
> @@ -3133,13 +3134,15 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>   			cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
>   			mutex_unlock(&hdmi->cec_notifier_mutex);
>   		}
> -	}
>   
> -	if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
> -		enum drm_connector_status status = phy_int_pol & HDMI_PHY_HPD
> -						 ? connector_status_connected
> -						 : connector_status_disconnected;
> +		if (phy_stat & HDMI_PHY_HPD)
> +			status = connector_status_connected;
> +
> +		if (!( phy_stat & HDMI_PHY_RX_SENSE))
> +			status = connector_status_disconnected;
> +	}
>   
> +	if (status != connector_status_unknown) {
>   		dev_dbg(hdmi->dev, "EVENT=%s\n",
>   			status == connector_status_connected ?
>   			"plugin" : "plugout");

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

It would be nice to have this tested on another platform using the Synopsys PHY (unlike Amlogic platforms)
like Renesas, Rockchip, Allwinner or Ingenic SoCs.

Neil

Re: [PATCH] drm: bridge: dw_hdmi: only trigger hotplug event on real link state change

[Lucas Stach]

Am Dienstag, dem 28.06.2022 um 09:16 +0200 schrieb Neil Armstrong:
> Hi,
> 
> On 27/06/2022 14:47, Lucas Stach wrote:
> > There are two events that signal a real change of the link state: HPD going
> > high means the sink is newly connected or wants the source to re-read the
> > EDID, RX sense going low is a indication that the link has been disconnected.
> > 
> > Ignore the other two events that also trigger interrupts, but don't need
> > immediate attention: HPD going low does not necessarily mean the link has
> > been lost and should not trigger a immediate read of the status. RX sense
> > going high also does not require a detect cycle, as HPD going high is the
> > right point in time to read the EDID.
> > 
> > Ignoring the negative HPD edge does make the detection much more robust
> > against spurious link status changes due to EMI or marginal signal levels.
> 
> Fair enough, but it means RX Sense must be totally functional with this change.

At least in my testing the condition used in this check of RX sense on
all lanes going low is very reliable. During plugin/-out sometimes the
RX sense status of some of lanes bounces a bit, but all of them going
low is a very reliable indicator.

> 
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 13 ++++++++-----
> >   1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > index 3e1be9894ed1..24f991b5248d 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -3095,6 +3095,7 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
> >   {
> >   	struct dw_hdmi *hdmi = dev_id;
> >   	u8 intr_stat, phy_int_pol, phy_pol_mask, phy_stat;
> > +	enum drm_connector_status status = connector_status_unknown;
> >   
> >   	intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0);
> >   	phy_int_pol = hdmi_readb(hdmi, HDMI_PHY_POL0);
> > @@ -3133,13 +3134,15 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
> >   			cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
> >   			mutex_unlock(&hdmi->cec_notifier_mutex);
> >   		}
> > -	}
> >   
> > -	if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
> > -		enum drm_connector_status status = phy_int_pol & HDMI_PHY_HPD
> > -						 ? connector_status_connected
> > -						 : connector_status_disconnected;
> > +		if (phy_stat & HDMI_PHY_HPD)
> > +			status = connector_status_connected;
> > +
> > +		if (!( phy_stat & HDMI_PHY_RX_SENSE))
> > +			status = connector_status_disconnected;
> > +	}
> >   
> > +	if (status != connector_status_unknown) {
> >   		dev_dbg(hdmi->dev, "EVENT=%s\n",
> >   			status == connector_status_connected ?
> >   			"plugin" : "plugout");
> 
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> It would be nice to have this tested on another platform using the Synopsys PHY (unlike Amlogic platforms)
> like Renesas, Rockchip, Allwinner or Ingenic SoCs.

For reference, I tested this change on i.MX6 (Synopsys PHY) and i.MX8MP
(Samsung PHY).

Regards,
Lucas

Re: [PATCH] drm: bridge: dw_hdmi: only trigger hotplug event on real link state change

[Philipp Zabel]

On Mo, 2022-06-27 at 14:47 +0200, Lucas Stach wrote:
There are two events that signal a real change of the link state: HPD going
high means the sink is newly connected or wants the source to re-read the
EDID, RX sense going low is a indication that the link has been disconnected.

Ignore the other two events that also trigger interrupts, but don't need
immediate attention: HPD going low does not necessarily mean the link has
been lost and should not trigger a immediate read of the status. RX sense
going high also does not require a detect cycle, as HPD going high is the
right point in time to read the EDID.

Ignoring the negative HPD edge does make the detection much more robust
against spurious link status changes due to EMI or marginal signal levels.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 3e1be9894ed1..24f991b5248d 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -3095,6 +3095,7 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 {
 	struct dw_hdmi *hdmi = dev_id;
 	u8 intr_stat, phy_int_pol, phy_pol_mask, phy_stat;
+	enum drm_connector_status status = connector_status_unknown;
 

 	intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0);
 	phy_int_pol = hdmi_readb(hdmi, HDMI_PHY_POL0);
@@ -3133,13 +3134,15 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 			cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
 			mutex_unlock(&hdmi->cec_notifier_mutex);
 		}
-	}
 

-	if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
-		enum drm_connector_status status = phy_int_pol & HDMI_PHY_HPD
-						 ? connector_status_connected
-						 : connector_status_disconnected;
+		if (phy_stat & HDMI_PHY_HPD)
+			status = connector_status_connected;
+
+		if (!( phy_stat & HDMI_PHY_RX_SENSE))

Too much space:       ^

Also, would it make sense to check

		if (!(phy_stat & (HDMI_PHY_HPD | HDMI_PHY_RX_SENSE)))

instead?

regards
Philipp