Re: PROBLEM: Asus C201 video mode problems on HDMI hotplug (regression)

[Laurent Pinchart]

Hi Archit,

Thank you for handling this, and sorry for missing the original bug report 
(and for breaking this in the first place).

On Monday, 27 November 2017 06:05:03 EET Archit Taneja wrote:
> On 11/16/2017 11:58 AM, Nick Bowler wrote:
> > On 2017-11-05 11:41 -0500, Nick Bowler wrote:
> >> On 2017-11-02, Nick Bowler <nbowler@xxxxxxxxxx> wrote:
> >>> ~50% of the time after a hotplug, there is a vertical pink bar on the
> >>> left of the display area and audio is not working at all.  According to
> >>> the sink device the display size is 1282x720 which seems pretty wrong
> >>> (normal and working situation is 1280x720).
> >>> 
> >>> I posted photos of non-working versus working states here:
> >>>    https://imgur.com/a/qhAZG
> >>> 
> >>> Unplugging and plugging the cable again will correct the issue (it seems
> >>> to, for the most part, alternate between working and not-working states,
> >>> although not always).  It always works on power up with the cable
> >>> initially connected.
> >>> 
> >>> This is a regression from 4.11, where hotplug works perfectly every
> >>> time.
> >> 
> >> Bisection implicates the following commit:
> >> 
> >> 181e0ef092a4952aa523c5b9cb21394cf43bcd46 is the first bad commit
> >> commit 181e0ef092a4952aa523c5b9cb21394cf43bcd46
> >> Author: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> >> Date:   Mon Mar 6 01:35:57 2017 +0200
> >> 
> >>      drm: bridge: dw-hdmi: Fix the PHY power up sequence
> >>      
> >>      When powering the PHY up we need to wait for the PLL to lock. This
> >>      is done by polling the TX_PHY_LOCK bit in the HDMI_PHY_STAT0
> >>      register (interrupt-based wait could be implemented as well but is
> >>      likely overkill). The bit is asserted when the PLL locks, but the
> >>      current code incorrectly waits for the bit to be deasserted. Fix it,
> >>      and while at it, replace the udelay() with a sleep as the code never
> >>      runs in non-sleepable context.
> >>      
> >>      To be consistent with the power down implementation move the poll
> >>      loop to the power off function.
> 
> The two main things the commit below does it to a) correctly wait on the
> TX_PHY_LOCK bit to be asserted and b) use usleep_range() instead of
> udelay().

Another difference is that the PWDN and TMDS signals, in theory needed for 
Gen1 PHYs only, are not set anymore for Gen2 PHYs. Nick, could you test the 
following change to see if it makes a difference ?

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/
bridge/synopsys/dw-hdmi.c
index b172139502d6..1c18ff1bf24a 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -1104,14 +1104,14 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi)
 	unsigned int i;
 	u8 val;
 
-	if (phy->gen == 1) {
-		dw_hdmi_phy_enable_powerdown(hdmi, false);
+	dw_hdmi_phy_enable_powerdown(hdmi, false);
 
-		/* Toggle TMDS enable. */
-		dw_hdmi_phy_enable_tmds(hdmi, 0);
-		dw_hdmi_phy_enable_tmds(hdmi, 1);
+	/* Toggle TMDS enable. */
+	dw_hdmi_phy_enable_tmds(hdmi, 0);
+	dw_hdmi_phy_enable_tmds(hdmi, 1);
+
+	if (phy->gen == 1)
 		return 0;
-	}
 
 	dw_hdmi_phy_gen2_txpwron(hdmi, 1);
 	dw_hdmi_phy_gen2_pddq(hdmi, 0);

> I don't see (b) being a problem. About (a), it's possible that the bit above
> is interpreted differently on a rockchip SoC versus a renesas chip. Could
> you print the value of HDMI_PHY_STAT0 that's read back?

The driver should print a "PHY PLL failed to lock" error message to the kernel 
log in that case. Nick, does that happen on your system ?

> If the code returns an -ETIMEDOUT, hdmi->phy.ops->init() will return an
> -ETIMEDOUT. This will cause dw_hdmi_setup() to bail out early, before we
> get a chance to configure the AVI infoframe and other stuff. I've seen
> other HDMI HW throwing up pink strips if the AVI infoframe stuff isn't
> configured properly.
> 
> As an experiment, could you forcefully return 0 instead of -ETIMEDOUT and
> see if things return back to normal?

-- 
Regards,

Laurent Pinchart