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
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
