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
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
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
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;
> }
--
Regards,
Laurent Pinchart