diff mbox

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

Message ID d719bb3b-551c-3c15-6b9b-3717deaa57a7@synopsys.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jose Abreu Dec. 2, 2017, 5:11 p.m. UTC
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---------------
        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

Comments

Nick Bowler Dec. 3, 2017, 5:20 a.m. UTC | #1
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
Jose Abreu Dec. 4, 2017, 10:04 a.m. UTC | #2
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
Nick Bowler Dec. 4, 2017, 6:33 p.m. UTC | #3
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
Nick Bowler Dec. 5, 2017, 6:18 a.m. UTC | #4
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
diff mbox

Patch

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;