Hi,
On my Asus C201 laptop (rk3288) the HDMI has been behaving weirdly after Linux upgrade.
~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:
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.
I attached dmesg output (gzipped) from 4.14-rc7 (I booted up and re-plugged the cable twice, which triggered non-working state and then back to working state -- although seems no messages are logged from these hotplugs).
I am working to bisect this, might take a while. Partial progress follows...
Let me know of anything else I should try.
Thanks, Nick
git bisect start # bad: [0b07194bb55ed836c2cc7c22e866b87a14681984] Linux 4.14-rc7 git bisect bad 0b07194bb55ed836c2cc7c22e866b87a14681984 # bad: [fa394784e74b918f44fca1e6a1f826cf818350d2] Linux 4.12.14 git bisect bad fa394784e74b918f44fca1e6a1f826cf818350d2 # good: [bd1a9eb6a755e1cb342725a11242251d2bfad567] Linux 4.11.12 git bisect good bd1a9eb6a755e1cb342725a11242251d2bfad567 # good: [a351e9b9fc24e982ec2f0e76379a49826036da12] Linux 4.11 git bisect good a351e9b9fc24e982ec2f0e76379a49826036da12 # bad: [8f28472a739e8e39adc6e64ee5b460df039f0e4f] Merge tag 'usb-4.12-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb git bisect bad 8f28472a739e8e39adc6e64ee5b460df039f0e4f
Hi,
I completed bisecting this issue. See below.
On 2017-11-02, Nick Bowler nbowler@draconx.ca 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:
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@ideasonboard.com 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.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Tested-by: Neil Armstrong narmstrong@baylibre.com Reviewed-by: Jose Abreu joabreu@synopsys.com Signed-off-by: Archit Taneja architt@codeaurora.org Link: http://patchwork.freedesktop.org/patch/msgid/20170305233557.11945-1-laurent....
:040000 040000 0defad9d1a61c0355f49c679b18eebae2c4b9495 5d260e6db25d6abc1211d61ec3405be99e693a23 M drivers
This commit does not revert cleanly, but on top of latest master (which has the problem) I manually changed the relevant code back to its original state and the problem is fixed, like this:
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index bf14214fa464..6618aac95a51 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1100,37 +1100,34 @@ static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi)
static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) { - const struct dw_hdmi_phy_data *phy = hdmi->phy.data; - unsigned int i; - u8 val; + u8 val, msec;
- 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); - return 0; - } + /* toggle TMDS enable */ + dw_hdmi_phy_enable_tmds(hdmi, 0); + dw_hdmi_phy_enable_tmds(hdmi, 1);
+ /* gen2 tx power on */ dw_hdmi_phy_gen2_txpwron(hdmi, 1); dw_hdmi_phy_gen2_pddq(hdmi, 0);
/* Wait for PHY PLL lock */ - for (i = 0; i < 5; ++i) { + msec = 5; + do { val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK; - if (val) + if (!val) break;
- usleep_range(1000, 2000); - } + if (msec == 0) { + dev_err(hdmi->dev, "PHY PLL not locked\n"); + return -ETIMEDOUT; + }
- if (!val) { - dev_err(hdmi->dev, "PHY PLL failed to lock\n"); - return -ETIMEDOUT; - } + udelay(1000); + msec--; + } while (1);
- dev_dbg(hdmi->dev, "PHY PLL locked %u iterations\n", i); return 0; }
Cheers, Nick
Hi,
Any ideas on this issue? Are there any additional tests I can perform to help debug this?
On 2017-11-05 11:41 -0500, Nick Bowler wrote:
I completed bisecting this issue. See below.
On 2017-11-02, Nick Bowler nbowler@draconx.ca 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:
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@ideasonboard.com 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. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Tested-by: Neil Armstrong <narmstrong@baylibre.com> Reviewed-by: Jose Abreu <joabreu@synopsys.com> Signed-off-by: Archit Taneja <architt@codeaurora.org> Link: http://patchwork.freedesktop.org/patch/msgid/20170305233557.11945-1-laurent.pinchart+renesas@ideasonboard.com:040000 040000 0defad9d1a61c0355f49c679b18eebae2c4b9495 5d260e6db25d6abc1211d61ec3405be99e693a23 M drivers
This commit does not revert cleanly, but on top of latest master (which has the problem) I manually changed the relevant code back to its original state and the problem is fixed, like this:
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index bf14214fa464..6618aac95a51 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1100,37 +1100,34 @@ static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi)
static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) {
- const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
- unsigned int i;
- u8 val;
- u8 val, msec;
- 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);return 0;- }
/* toggle TMDS enable */
dw_hdmi_phy_enable_tmds(hdmi, 0);
dw_hdmi_phy_enable_tmds(hdmi, 1);
/* gen2 tx power on */ dw_hdmi_phy_gen2_txpwron(hdmi, 1); dw_hdmi_phy_gen2_pddq(hdmi, 0);
/* Wait for PHY PLL lock */
- for (i = 0; i < 5; ++i) {
- msec = 5;
- do { val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;
if (val)
if (!val) break;
usleep_range(1000, 2000);- }
if (msec == 0) {dev_err(hdmi->dev, "PHY PLL not locked\n");return -ETIMEDOUT;}
- if (!val) {
dev_err(hdmi->dev, "PHY PLL failed to lock\n");return -ETIMEDOUT;- }
udelay(1000);msec--;- } while (1);
- dev_dbg(hdmi->dev, "PHY PLL locked %u iterations\n", i); return 0;
}
Thanks, Nick
Hi,
On 11/16/2017 11:58 AM, Nick Bowler wrote:
Hi,
Any ideas on this issue? Are there any additional tests I can perform to help debug this?
On 2017-11-05 11:41 -0500, Nick Bowler wrote:
I completed bisecting this issue. See below.
On 2017-11-02, Nick Bowler nbowler@draconx.ca 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:
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@ideasonboard.com 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().
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?
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?
Thanks, Archit
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Tested-by: Neil Armstrong <narmstrong@baylibre.com> Reviewed-by: Jose Abreu <joabreu@synopsys.com> Signed-off-by: Archit Taneja <architt@codeaurora.org> Link: http://patchwork.freedesktop.org/patch/msgid/20170305233557.11945-1-laurent.pinchart+renesas@ideasonboard.com:040000 040000 0defad9d1a61c0355f49c679b18eebae2c4b9495 5d260e6db25d6abc1211d61ec3405be99e693a23 M drivers
This commit does not revert cleanly, but on top of latest master (which has the problem) I manually changed the relevant code back to its original state and the problem is fixed, like this:
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index bf14214fa464..6618aac95a51 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1100,37 +1100,34 @@ static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi)
static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) {
- const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
- unsigned int i;
- u8 val;
- u8 val, msec;
- 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);return 0;- }
/* toggle TMDS enable */
dw_hdmi_phy_enable_tmds(hdmi, 0);
dw_hdmi_phy_enable_tmds(hdmi, 1);
/* gen2 tx power on */ dw_hdmi_phy_gen2_txpwron(hdmi, 1); dw_hdmi_phy_gen2_pddq(hdmi, 0);
/* Wait for PHY PLL lock */
- for (i = 0; i < 5; ++i) {
- msec = 5;
- do { val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;
if (val)
if (!val) break;
usleep_range(1000, 2000);- }
if (msec == 0) {dev_err(hdmi->dev, "PHY PLL not locked\n");return -ETIMEDOUT;}
- if (!val) {
dev_err(hdmi->dev, "PHY PLL failed to lock\n");return -ETIMEDOUT;- }
udelay(1000);msec--;- } while (1);
- dev_dbg(hdmi->dev, "PHY PLL locked %u iterations\n", i); return 0; }
Thanks, Nick
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@draconx.ca 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@ideasonboard.com 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?
Hi,
On 11/27/17, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
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 ?
I will try to test the other things later today, but after bootup there were no messages whatsoever printed to the kernel log during the test procedure.
Cheers, Nick
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@ideasonboard.com 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
Hi,
On 2017-11-27 22:30 -0500, Nick Bowler wrote:
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 [Laurent Pinchart's] patch -- the issue is present again (no change in behaviour compared to 4.15-rc1).
Another data point... the following patch appears sufficient to restore working behaviour.
Cheers, Nick
--- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 ----------------- 1 file changed, 17 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index bf14214fa464..3118fbd8433d 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1101,8 +1101,6 @@ static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) { const struct dw_hdmi_phy_data *phy = hdmi->phy.data; - unsigned int i; - u8 val;
if (phy->gen == 1) { dw_hdmi_phy_enable_powerdown(hdmi, false); @@ -1116,21 +1114,6 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) dw_hdmi_phy_gen2_txpwron(hdmi, 1); dw_hdmi_phy_gen2_pddq(hdmi, 0);
- /* Wait for PHY PLL lock */ - for (i = 0; i < 5; ++i) { - val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK; - if (val) - break; - - usleep_range(1000, 2000); - } - - if (!val) { - dev_err(hdmi->dev, "PHY PLL failed to lock\n"); - return -ETIMEDOUT; - } - - dev_dbg(hdmi->dev, "PHY PLL locked %u iterations\n", i); return 0; }
Hi Nick,
On 01-12-2017 00:11, Nick Bowler wrote:
Hi,
On 2017-11-27 22:30 -0500, Nick Bowler wrote:
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 [Laurent Pinchart's] patch -- the issue is present again (no change in behaviour compared to 4.15-rc1).
Another data point... the following patch appears sufficient to restore working behaviour.
Cheers, Nick
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 ----------------- 1 file changed, 17 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index bf14214fa464..3118fbd8433d 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1101,8 +1101,6 @@ static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) { const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
unsigned int i;
u8 val;
if (phy->gen == 1) { dw_hdmi_phy_enable_powerdown(hdmi, false);
@@ -1116,21 +1114,6 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) dw_hdmi_phy_gen2_txpwron(hdmi, 1); dw_hdmi_phy_gen2_pddq(hdmi, 0);
- /* Wait for PHY PLL lock */
- for (i = 0; i < 5; ++i) {
val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;if (val)break;usleep_range(1000, 2000);- }
- if (!val) {
dev_err(hdmi->dev, "PHY PLL failed to lock\n");return -ETIMEDOUT;- }
- dev_dbg(hdmi->dev, "PHY PLL locked %u iterations\n", i); return 0;
}
I don't think you can do this. The phy pll lock check is recommended and can indicate hw failure. Can you please check if this untested, uncompiled patch makes it work correctly ?
------------------>8--------------- diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index bf14214..456fc54 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1669,7 +1669,7 @@ static void hdmi_disable_overflow_interrupts(struct dw_hdmi *hdmi)
static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) { - int ret; + int ret, vsync_len = mode->vsync_end - mode->vsync_start;
hdmi_disable_overflow_interrupts(hdmi);
@@ -1722,6 +1722,14 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) return ret; hdmi->phy.enabled = true;
+ /* Reset all clock domains */ + hdmi_writeb(hdmi, 0x00, HDMI_MC_SWRSTZ); + + /* Rewrite vsync register to latch previous written values */ + if (mode->flags & DRM_MODE_FLAG_INTERLACE) + vsync_len /= 2; + hdmi_writeb(hdmi, vsync_len, HDMI_FC_VSYNCINWIDTH); + /* HDMI Initialization Step B.3 */ dw_hdmi_enable_video_path(hdmi);
------------------>8---------------
I would expect this patch to end your wrong image issue but the audio part may be a different problem.
Best Regards, Jose Miguel Abreu
Hi Jose,
On 2017-12-02 17:11 +0000, Jose Abreu wrote:
On 01-12-2017 00:11, Nick Bowler wrote:
Another data point... the following patch appears sufficient to restore working behaviour.
[...]
I don't think you can do this. The phy pll lock check is recommended and can indicate hw failure. Can you please check if this untested, uncompiled patch makes it work correctly ?
Your patch changes things. With this applied on top of 4.15-rc1 it is failing 100% of the time instead of only half of the time.
I brought the original test equipment back to the setup so I can see the video and pink bar again. The symptoms remain the same (unexpected size, pink bar, and no audio).
PS: your patch seems to have been line wrapped which made it a bit annoying to apply.
------------------>8--------------- diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index bf14214..456fc54 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1669,7 +1669,7 @@ static void hdmi_disable_overflow_interrupts(struct dw_hdmi *hdmi)
static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) {
int ret;
int ret, vsync_len = mode->vsync_end - mode->vsync_start; hdmi_disable_overflow_interrupts(hdmi);@@ -1722,6 +1722,14 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) return ret; hdmi->phy.enabled = true;
/* Reset all clock domains */hdmi_writeb(hdmi, 0x00, HDMI_MC_SWRSTZ);/* Rewrite vsync register to latch previous written values */if (mode->flags & DRM_MODE_FLAG_INTERLACE)vsync_len /= 2;hdmi_writeb(hdmi, vsync_len, HDMI_FC_VSYNCINWIDTH);/* HDMI Initialization Step B.3 */ dw_hdmi_enable_video_path(hdmi);------------------>8---------------
I would expect this patch to end your wrong image issue but the audio part may be a different problem.
It is very consistent: pink bar <=> no audio.
My suspicion is that the audio problem is just the wrong video mode on the sink side messing things up, but I have no way of confirming that (that I know of).
Thanks, Nick
On 03-12-2017 05:20, Nick Bowler wrote:
Your patch changes things. With this applied on top of 4.15-rc1 it is failing 100% of the time instead of only half of the time.
Ok, it was a long shot anyway.
I brought the original test equipment back to the setup so I can see the video and pink bar again. The symptoms remain the same (unexpected size, pink bar, and no audio).
Can you tell me which test equipment are you using?
It is very consistent: pink bar <=> no audio.
My suspicion is that the audio problem is just the wrong video mode on the sink side messing things up, but I have no way of confirming that (that I know of).
Hmmm, my first thought was that audio is being configured first because of the phy lock wait time, I've seen this happening before.
Lets try this: - Disable all alsa clients (e.g. pulseaudio, ...) so that no one tries to configure audio. - Plug out/in the cable until the issue appears - When the issue appears use aplay to play audio through the HDMI output - Repeat several times with different audio rates and with no resample (you can use the plughw interface in aplay).
Thanks, Nick
On 2017-12-04 10:04 +0000, Jose Abreu wrote:
On 03-12-2017 05:20, Nick Bowler wrote:
I brought the original test equipment back to the setup so I can see the video and pink bar again. The symptoms remain the same (unexpected size, pink bar, and no audio).
Can you tell me which test equipment are you using?
I am using an XRGB-Mini Framemeister as the sink device for testing which isn't "real" test equipment but does show details of the video and audio mode (which the monitor I have does not do).
The normal setup of this laptop has a small "audio splitter" as the sink, which among other things includes HDMI input and output ports and an S/PDIF audio output.
These devices can be connected together in various ways and the results seem to be consistent in any configuration.
It is very consistent: pink bar <=> no audio.
My suspicion is that the audio problem is just the wrong video mode on the sink side messing things up, but I have no way of confirming that (that I know of).
Hmmm, my first thought was that audio is being configured first because of the phy lock wait time, I've seen this happening before.
Lets try this:
- Disable all alsa clients (e.g. pulseaudio, ...) so that no one
tries to configure audio.
- Plug out/in the cable until the issue appears
- When the issue appears use aplay to play audio through the HDMI
output
- Repeat several times with different audio rates and with no
resample (you can use the plughw interface in aplay).
OK, I will give it a try later this evening.
Cheers, Nick
On 2017-12-04 13:33 -0500, Nick Bowler wrote:
On 2017-12-04 10:04 +0000, Jose Abreu wrote:
Hmmm, my first thought was that audio is being configured first because of the phy lock wait time, I've seen this happening before.
Lets try this:
- Disable all alsa clients (e.g. pulseaudio, ...) so that no one
tries to configure audio.
- Plug out/in the cable until the issue appears
- When the issue appears use aplay to play audio through the HDMI
output
- Repeat several times with different audio rates and with no
resample (you can use the plughw interface in aplay).
OK, I will give it a try later this evening.
Using the above sequence on unpatched 4.15-rc1 it seems there is no sound when starting audio output after the pink bar is visible.
However I am not confident of the results here, restarting aplay with different sample rates (or even restarting with the same rate) is causing some weird effects on my setup so I want to check the test setup with some different source devices.
Cheers, Nick
Hi Nick,
On Friday, 1 December 2017 02:11:46 EET Nick Bowler wrote:
On 2017-11-27 22:30 -0500, Nick Bowler wrote:
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 [Laurent Pinchart's] patch -- the issue is present again (no change in behaviour compared to 4.15-rc1).
Another data point... the following patch appears sufficient to restore working behaviour.
Cheers, Nick
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 ----------------- 1 file changed, 17 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index bf14214fa464..3118fbd8433d 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1101,8 +1101,6 @@ static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) { const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
unsigned int i;
u8 val;
if (phy->gen == 1) { dw_hdmi_phy_enable_powerdown(hdmi, false);
@@ -1116,21 +1114,6 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) dw_hdmi_phy_gen2_txpwron(hdmi, 1); dw_hdmi_phy_gen2_pddq(hdmi, 0);
- /* Wait for PHY PLL lock */
- for (i = 0; i < 5; ++i) {
val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;if (val)break;usleep_range(1000, 2000);- }
- if (!val) {
dev_err(hdmi->dev, "PHY PLL failed to lock\n");return -ETIMEDOUT;- }
- dev_dbg(hdmi->dev, "PHY PLL locked %u iterations\n", i);
As you reported that the PLL lock failure message is not printed, the failure can only come from either the extra delay introduced by the above loop, or from reading the HDMI_PHY_STAT0 register.
How many iterations of the for loop execute before the condition becomes true ?
return 0; }
Hi,
On 2017-12-04 21:06 +0200, Laurent Pinchart wrote:
As you reported that the PLL lock failure message is not printed, the failure can only come from either the extra delay introduced by the above loop, or from reading the HDMI_PHY_STAT0 register.
How many iterations of the for loop execute before the condition becomes true?
Judging from the log posted elsethread (where I added extra printouts), it seems to consistently become true on the second iteration.
I will try to rule out read side effects by replacing the polling loop with an unconditional delay.
Cheers,
Hi Nick,
On Monday, 4 December 2017 21:30:01 EET Nick Bowler wrote:
On 2017-12-04 21:06 +0200, Laurent Pinchart wrote:
As you reported that the PLL lock failure message is not printed, the failure can only come from either the extra delay introduced by the above loop, or from reading the HDMI_PHY_STAT0 register.
How many iterations of the for loop execute before the condition becomes true?
Judging from the log posted elsethread (where I added extra printouts), it seems to consistently become true on the second iteration.
I will try to rule out read side effects by replacing the polling loop with an unconditional delay.
You're reading my mind :-)
On 2017-12-04 21:34 +0200, Laurent Pinchart wrote:
On Monday, 4 December 2017 21:30:01 EET Nick Bowler wrote:
On 2017-12-04 21:06 +0200, Laurent Pinchart wrote:
As you reported that the PLL lock failure message is not printed, the failure can only come from either the extra delay introduced by the above loop, or from reading the HDMI_PHY_STAT0 register.
How many iterations of the for loop execute before the condition becomes true?
Judging from the log posted elsethread (where I added extra printouts), it seems to consistently become true on the second iteration.
I will try to rule out read side effects by replacing the polling loop with an unconditional delay.
You're reading my mind :-)
I did this test by applying the following patch on 4.15-rc1, and the problem remains. So it appears the delay is responsible somehow.
Cheers, Nick
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index bf14214fa464..4aec4d5c130e 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1101,8 +1101,6 @@ static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) { const struct dw_hdmi_phy_data *phy = hdmi->phy.data; - unsigned int i; - u8 val;
if (phy->gen == 1) { dw_hdmi_phy_enable_powerdown(hdmi, false); @@ -1116,21 +1114,7 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) dw_hdmi_phy_gen2_txpwron(hdmi, 1); dw_hdmi_phy_gen2_pddq(hdmi, 0);
- /* Wait for PHY PLL lock */ - for (i = 0; i < 5; ++i) { - val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK; - if (val) - break; - - usleep_range(1000, 2000); - } - - if (!val) { - dev_err(hdmi->dev, "PHY PLL failed to lock\n"); - return -ETIMEDOUT; - } - - dev_dbg(hdmi->dev, "PHY PLL locked %u iterations\n", i); + usleep_range(1000, 2000); return 0; }
Hi Nick,
On Tuesday, 5 December 2017 05:22:28 EET Nick Bowler wrote:
On 2017-12-04 21:34 +0200, Laurent Pinchart wrote:
On Monday, 4 December 2017 21:30:01 EET Nick Bowler wrote:
On 2017-12-04 21:06 +0200, Laurent Pinchart wrote:
As you reported that the PLL lock failure message is not printed, the failure can only come from either the extra delay introduced by the above loop, or from reading the HDMI_PHY_STAT0 register.
How many iterations of the for loop execute before the condition becomes true?
Judging from the log posted elsethread (where I added extra printouts), it seems to consistently become true on the second iteration.
I will try to rule out read side effects by replacing the polling loop with an unconditional delay.
You're reading my mind :-)
I did this test by applying the following patch on 4.15-rc1, and the problem remains. So it appears the delay is responsible somehow.
That's interesting. I think it also means we really need to find the root cause, as otherwise your system would be susceptible to random malfunction if the scheduler ends up interrupting the power on sequence. That might not happen frequently, but would be much harder to debug.
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index bf14214fa464..4aec4d5c130e 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1101,8 +1101,6 @@ static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) { const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
unsigned int i;u8 val; if (phy->gen == 1) { dw_hdmi_phy_enable_powerdown(hdmi, false);@@ -1116,21 +1114,7 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) dw_hdmi_phy_gen2_txpwron(hdmi, 1); dw_hdmi_phy_gen2_pddq(hdmi, 0);
/* Wait for PHY PLL lock */for (i = 0; i < 5; ++i) {val = hdmi_readb(hdmi, HDMI_PHY_STAT0) &HDMI_PHY_TX_PHY_LOCK; - if (val)
break;usleep_range(1000, 2000);}if (!val) {dev_err(hdmi->dev, "PHY PLL failed to lock\n");return -ETIMEDOUT;}dev_dbg(hdmi->dev, "PHY PLL locked %u iterations\n", i);
usleep_range(1000, 2000); return 0;}
dri-devel@lists.freedesktop.org
-
Archit Taneja -
Jose Abreu -
Laurent Pinchart -
Nick Bowler