[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 at 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 at 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