diff mbox series

drm/bridge: ps8640: Add back the 50 ms mystery delay after HPD

Message ID 20221017121813.1.I59700c745fbc31559a5d5c8e2a960279c751dbd5@changeid
State New
Headers
Series drm/bridge: ps8640: Add back the 50 ms mystery delay after HPD |

Commit Message

Doug Anderson Oct. 17, 2022, 7:18 p.m. UTC 
  Back in commit 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable
runtime power management") we removed a mysterious 50 ms delay because
"Parade's support [couldn't] explain what the delay [was] for".

While I'm always a fan of removing mysterious delays, I suspect that
we need this mysterious delay to avoid some problems.

Specifically, what I found recently is that on sc7180-trogdor-homestar
sometimes the AUX backlight wasn't initializing properly. Some
debugging showed that the drm_dp_dpcd_read() function that the AUX
backlight driver was calling was returning bogus data about 1% of the
time when I booted up. This confused
drm_panel_dp_aux_backlight(). From continued debugging:
- If I retried the read then the read worked just fine.
- If I added a loop to perform the same read that
  drm_panel_dp_aux_backlight() was doing 30 times at bootup I could
  see that some percentage of the time the first read would give bogus
  data but all 29 additional reads would always be fine.
- If I added a large delay _after_ powering on the panel but before
  powering on PS8640 I could still reproduce the problem.
- If I added a delay after PS8640 powered on then I couldn't reproduce
  the problem.
- I couldn't reproduce the problem on a board with the same panel but
  the ti-sn65dsi86 bridge chip.

To me, the above indicated that there was a problem with PS8640 and
not the panel.

I don't really have any insight into what's going on in the MCU, but
my best guess is that when the MCU itself sees the HPD go high that it
does some AUX transfers itself and this is confusing things.

Let's go back and add back in the mysterious 50 ms delay. We only want
to do this the first time we see HPD go high after booting the MCU,
not every time we double-check HPD.

With this, the backlight initializes reliably on homestar.

Fixes: 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable runtime power management")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/parade-ps8640.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

Comments

Stephen Boyd Oct. 19, 2022, 6:18 p.m. UTC | #1 
Quoting Douglas Anderson (2022-10-17 12:18:51)
> Back in commit 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable
> runtime power management") we removed a mysterious 50 ms delay because
> "Parade's support [couldn't] explain what the delay [was] for".
>
> While I'm always a fan of removing mysterious delays, I suspect that
> we need this mysterious delay to avoid some problems.
>
> Specifically, what I found recently is that on sc7180-trogdor-homestar
> sometimes the AUX backlight wasn't initializing properly. Some
> debugging showed that the drm_dp_dpcd_read() function that the AUX
> backlight driver was calling was returning bogus data about 1% of the
> time when I booted up. This confused
> drm_panel_dp_aux_backlight(). From continued debugging:
> - If I retried the read then the read worked just fine.
> - If I added a loop to perform the same read that
>   drm_panel_dp_aux_backlight() was doing 30 times at bootup I could
>   see that some percentage of the time the first read would give bogus
>   data but all 29 additional reads would always be fine.
> - If I added a large delay _after_ powering on the panel but before
>   powering on PS8640 I could still reproduce the problem.
> - If I added a delay after PS8640 powered on then I couldn't reproduce
>   the problem.
> - I couldn't reproduce the problem on a board with the same panel but
>   the ti-sn65dsi86 bridge chip.
>
> To me, the above indicated that there was a problem with PS8640 and
> not the panel.
>
> I don't really have any insight into what's going on in the MCU, but
> my best guess is that when the MCU itself sees the HPD go high that it
> does some AUX transfers itself and this is confusing things.
>
> Let's go back and add back in the mysterious 50 ms delay. We only want
> to do this the first time we see HPD go high after booting the MCU,
> not every time we double-check HPD.
>
> With this, the backlight initializes reliably on homestar.
>
> Fixes: 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable runtime power management")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Doug Anderson Oct. 19, 2022, 6:22 p.m. UTC | #2 
Hi,

On Wed, Oct 19, 2022 at 11:18 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Douglas Anderson (2022-10-17 12:18:51)
> > Back in commit 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable
> > runtime power management") we removed a mysterious 50 ms delay because
> > "Parade's support [couldn't] explain what the delay [was] for".
> >
> > While I'm always a fan of removing mysterious delays, I suspect that
> > we need this mysterious delay to avoid some problems.
> >
> > Specifically, what I found recently is that on sc7180-trogdor-homestar
> > sometimes the AUX backlight wasn't initializing properly. Some
> > debugging showed that the drm_dp_dpcd_read() function that the AUX
> > backlight driver was calling was returning bogus data about 1% of the
> > time when I booted up. This confused
> > drm_panel_dp_aux_backlight(). From continued debugging:
> > - If I retried the read then the read worked just fine.
> > - If I added a loop to perform the same read that
> >   drm_panel_dp_aux_backlight() was doing 30 times at bootup I could
> >   see that some percentage of the time the first read would give bogus
> >   data but all 29 additional reads would always be fine.
> > - If I added a large delay _after_ powering on the panel but before
> >   powering on PS8640 I could still reproduce the problem.
> > - If I added a delay after PS8640 powered on then I couldn't reproduce
> >   the problem.
> > - I couldn't reproduce the problem on a board with the same panel but
> >   the ti-sn65dsi86 bridge chip.
> >
> > To me, the above indicated that there was a problem with PS8640 and
> > not the panel.
> >
> > I don't really have any insight into what's going on in the MCU, but
> > my best guess is that when the MCU itself sees the HPD go high that it
> > does some AUX transfers itself and this is confusing things.
> >
> > Let's go back and add back in the mysterious 50 ms delay. We only want
> > to do this the first time we see HPD go high after booting the MCU,
> > not every time we double-check HPD.
> >
> > With this, the backlight initializes reliably on homestar.
> >
> > Fixes: 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable runtime power management")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>

I'm not expecting any other reviews of this patch, though I'm happy to
be proven wrong. As a heads up, I'll plan to land this on Friday
(roughly 2 days from now) in "drm-misc-fixes" barring anything else.
If anyone else plans to offer any opinions about this patch or just
wants more time to review, please shout.

-Doug
Doug Anderson Oct. 21, 2022, 3:09 p.m. UTC | #3 
Hi,

On Wed, Oct 19, 2022 at 11:22 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Oct 19, 2022 at 11:18 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Douglas Anderson (2022-10-17 12:18:51)
> > > Back in commit 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable
> > > runtime power management") we removed a mysterious 50 ms delay because
> > > "Parade's support [couldn't] explain what the delay [was] for".
> > >
> > > While I'm always a fan of removing mysterious delays, I suspect that
> > > we need this mysterious delay to avoid some problems.
> > >
> > > Specifically, what I found recently is that on sc7180-trogdor-homestar
> > > sometimes the AUX backlight wasn't initializing properly. Some
> > > debugging showed that the drm_dp_dpcd_read() function that the AUX
> > > backlight driver was calling was returning bogus data about 1% of the
> > > time when I booted up. This confused
> > > drm_panel_dp_aux_backlight(). From continued debugging:
> > > - If I retried the read then the read worked just fine.
> > > - If I added a loop to perform the same read that
> > >   drm_panel_dp_aux_backlight() was doing 30 times at bootup I could
> > >   see that some percentage of the time the first read would give bogus
> > >   data but all 29 additional reads would always be fine.
> > > - If I added a large delay _after_ powering on the panel but before
> > >   powering on PS8640 I could still reproduce the problem.
> > > - If I added a delay after PS8640 powered on then I couldn't reproduce
> > >   the problem.
> > > - I couldn't reproduce the problem on a board with the same panel but
> > >   the ti-sn65dsi86 bridge chip.
> > >
> > > To me, the above indicated that there was a problem with PS8640 and
> > > not the panel.
> > >
> > > I don't really have any insight into what's going on in the MCU, but
> > > my best guess is that when the MCU itself sees the HPD go high that it
> > > does some AUX transfers itself and this is confusing things.
> > >
> > > Let's go back and add back in the mysterious 50 ms delay. We only want
> > > to do this the first time we see HPD go high after booting the MCU,
> > > not every time we double-check HPD.
> > >
> > > With this, the backlight initializes reliably on homestar.
> > >
> > > Fixes: 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable runtime power management")
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> >
> > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>
> I'm not expecting any other reviews of this patch, though I'm happy to
> be proven wrong. As a heads up, I'll plan to land this on Friday
> (roughly 2 days from now) in "drm-misc-fixes" barring anything else.
> If anyone else plans to offer any opinions about this patch or just
> wants more time to review, please shout.

As promised, pushed to drm-misc-fixes:

cb8e30ddb7e3 drm/bridge: ps8640: Add back the 50 ms mystery delay after HPD
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 5be6562c2a19..6a614e54b383 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -105,6 +105,7 @@  struct ps8640 {
 	struct gpio_desc *gpio_powerdown;
 	struct device_link *link;
 	bool pre_enabled;
+	bool need_post_hpd_delay;
 };
 
 static const struct regmap_config ps8640_regmap_config[] = {
@@ -173,14 +174,31 @@  static int _ps8640_wait_hpd_asserted(struct ps8640 *ps_bridge, unsigned long wai
 {
 	struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
 	int status;
+	int ret;
 
 	/*
 	 * Apparently something about the firmware in the chip signals that
 	 * HPD goes high by reporting GPIO9 as high (even though HPD isn't
 	 * actually connected to GPIO9).
 	 */
-	return regmap_read_poll_timeout(map, PAGE2_GPIO_H, status,
-					status & PS_GPIO9, wait_us / 10, wait_us);
+	ret = regmap_read_poll_timeout(map, PAGE2_GPIO_H, status,
+				       status & PS_GPIO9, wait_us / 10, wait_us);
+
+	/*
+	 * The first time we see HPD go high after a reset we delay an extra
+	 * 50 ms. The best guess is that the MCU is doing "stuff" during this
+	 * time (maybe talking to the panel) and we don't want to interrupt it.
+	 *
+	 * No locking is done around "need_post_hpd_delay". If we're here we
+	 * know we're holding a PM Runtime reference and the only other place
+	 * that touches this is PM Runtime resume.
+	 */
+	if (!ret && ps_bridge->need_post_hpd_delay) {
+		ps_bridge->need_post_hpd_delay = false;
+		msleep(50);
+	}
+
+	return ret;
 }
 
 static int ps8640_wait_hpd_asserted(struct drm_dp_aux *aux, unsigned long wait_us)
@@ -388,6 +406,9 @@  static int __maybe_unused ps8640_resume(struct device *dev)
 	msleep(50);
 	gpiod_set_value(ps_bridge->gpio_reset, 0);
 
+	/* We just reset things, so we need a delay after the first HPD */
+	ps_bridge->need_post_hpd_delay = true;
+
 	/*
 	 * Mystery 200 ms delay for the "MCU to be ready". It's unclear if
 	 * this is truly necessary since the MCU will already signal that