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 at 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 at 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 at ideasonboard.com>
>>      Tested-by: Neil Armstrong <narmstrong at baylibre.com>
>>      Reviewed-by: Jose Abreu <joabreu at synopsys.com>
>>      Signed-off-by: Archit Taneja <architt at 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