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

[Nick Bowler]

On 2017-11-27 11:00 +0200, Laurent Pinchart wrote:
> On Monday, 27 November 2017 06:05:03 EET Archit Taneja wrote:
> > On 2017-11-05 11:41 -0500, Nick Bowler wrote:
[...]
> > > 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
[...]
> > 
> > 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 ?

I do not notice any difference with this change applied on top of Linux
4.15-rc1.

A note about the test setup: I had to remove the test equipment so I
no longer have any information about the video mode from the sink side
(like in the photos).  Thus, with the current setup, I am using the
presense or absense of audio to determine whether the issue is present
or not.

The test procedure is: boot up, start music, then hotplug the hdmi four
times.  If sound is heard after all four connections, PASS; otherwise FAIL.

 - I retested on 4.15-rc1 to confirm that the issue is still present (it is).

 - I applied the functional revert from earlier on top of 4.15-rc1 and the
   problem is fixed.

 - Returning to 4.15-rc1 and applying this next patch -- the issue is
   present again (no change in behaviour compared to 4.15-rc1).

> 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?
[...]
> > As an experiment, could you forcefully return 0 instead of -ETIMEDOUT and
> > see if things return back to normal?

I did both of these tests at once by applying the below patch on top of
4.15-rc1.  There is no change in behaviour compared to 4.15-rc1 (except
for the added printouts).

With this, every time after inserting the cable the following is printed:

  [  128.002965] dwhdmi-rockchip ff980000.hdmi: 0: HDMI_PHY_STAT0: f2
  [  128.004614] dwhdmi-rockchip ff980000.hdmi: 1: HDMI_PHY_STAT0: f3
  [  128.013752] dwhdmi-rockchip ff980000.hdmi: 0: HDMI_PHY_STAT0: f2
  [  128.015605] dwhdmi-rockchip ff980000.hdmi: 1: HDMI_PHY_STAT0: f3

And there is no difference in output between working and non-working
cases.  I've attached the full log; I manually logged extra messages
to give context from the test procedure:

  "hdmi (not) working" - after bootup or connecting the cable
                         (indicating test pass/fail)
  "hdmi disconnect"    - after unplugging the cable.

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index bf14214fa464..0358f6020fb4 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -1118,7 +1118,11 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi)
 
        /* Wait for PHY PLL lock */
        for (i = 0; i < 5; ++i) {
-               val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;
+               val = hdmi_readb(hdmi, HDMI_PHY_STAT0);
+
+               dev_info(hdmi->dev, "%u: HDMI_PHY_STAT0: %.2hhx\n", i, val);
+
+               val &= HDMI_PHY_TX_PHY_LOCK;
                if (val)
                        break;
 
@@ -1127,7 +1131,7 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi)
 
        if (!val) {
                dev_err(hdmi->dev, "PHY PLL failed to lock\n");
-               return -ETIMEDOUT;
+               return 0;
        }
 
        dev_dbg(hdmi->dev, "PHY PLL locked %u iterations\n", i);

Let me know if there's anything else I should try.

Thanks,
  Nick

Attachment: aidos-hdmi-stat0.log.gz
Description: Binary data