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

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

[Nick Bowler]

[-- Attachment #1: Type: text/plain, Size: 1800 bytes --]

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:

  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.

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

[-- Attachment #2: aidos-4.14-rc7.log.gz --]
[-- Type: application/x-gzip, Size: 6784 bytes --]

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

[Nick Bowler]

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:
>
>   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.

    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;
 }

Cheers,
  Nick

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

[Nick Bowler]

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:
> >
> >   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.
> 
>     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

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

[Archit Taneja]

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:
>>>
>>>    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().

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
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Archit Taneja]

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:
>>>
>>>    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().

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
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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@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?

-- 
Regards,

Laurent Pinchart

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@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?

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Nick Bowler]

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

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

[Nick Bowler]

[-- Attachment #1: Type: text/plain, Size: 4883 bytes --]

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

[-- Attachment #2: aidos-hdmi-stat0.log.gz --]
[-- Type: application/octet-stream, Size: 7010 bytes --]

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

[Nick Bowler]

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;
 }
 
-- 
2.13.6

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

[Jose Abreu]

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

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

[Jose Abreu]

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

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

[Nick Bowler]

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

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

[Jose Abreu]

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

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

[Jose Abreu]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Nick Bowler]

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

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

[Laurent Pinchart]

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;
>  }

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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;
>  }

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Nick Bowler]

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,
-- 
Nick Bowler

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

[Laurent Pinchart]

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 :-)

-- 
Regards,

Laurent Pinchart

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

[Nick Bowler]

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;
 }

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

[Nick Bowler]

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

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

[Laurent Pinchart]

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;
>  }

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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;
>  }

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel