[PATCH 0/5] drm/vc4: hdmi: Remove CPU hangs, take 2

[PATCH 0/5] drm/vc4: hdmi: Remove CPU hangs, take 2

[Maxime Ripard]

Hi,

Here's another attempt at fixing the complete CPU stall while retrieving the
HDMI connector status when the connector is disabled.

This was fixed already, but eventually got reverted by Linus due to the same
symptom happening in another situation. This was likely (but not confirmed by
the reporter) due to the kernel being booted without an HDMI display connected,
in which case the firmware won't initialise the HDMI State Machine clock.

This is fixed by patch 3. However, further changes in the clock drivers were
needed for clk_set_min_rate to be used, which are patches 1 and 2.

Finally, patches 4 and 5 are the original patches that were reverted. Patch 4
got a small modification to move the clk_set_min_rate() call before the HSM
clock is enabled.

Let me know what you think,
Maxime

Maxime Ripard (5):
  clk: bcm-2835: Pick the closest clock rate
  clk: bcm-2835: Remove rounding up the dividers
  drm/vc4: hdmi: Set a default HSM rate
  drm/vc4: hdmi: Move the HSM clock enable to runtime_pm
  drm/vc4: hdmi: Make sure the controller is powered in detect

 drivers/clk/bcm/clk-bcm2835.c  | 13 ++---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 90 ++++++++++++++++++++++++----------
 2 files changed, 68 insertions(+), 35 deletions(-)

-- 
2.31.1

[PATCH 1/5] clk: bcm-2835: Pick the closest clock rate

[Maxime Ripard]

The driver currently tries to pick the closest rate that is lower than
the rate being requested.

This causes an issue with clk_set_min_rate() since it actively checks
for the rounded rate to be above the minimum that was just set.

Let's change the logic a bit to pick the closest rate to the requested
rate, no matter if it's actually higher or lower.

Fixes: 6d18b8adbe67 ("clk: bcm2835: Support for clock parent selection")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-bcm2835.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index a254512965eb..bf97b2b2a63f 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1216,7 +1216,7 @@ static int bcm2835_clock_determine_rate(struct clk_hw *hw,
 		rate = bcm2835_clock_choose_div_and_prate(hw, i, req->rate,
 							  &div, &prate,
 							  &avgrate);
-		if (rate > best_rate && rate <= req->rate) {
+		if (abs(req->rate - rate) < abs(req->rate - best_rate)) {
 			best_parent = parent;
 			best_prate = prate;
 			best_rate = rate;
-- 
2.31.1

Re: [PATCH 1/5] clk: bcm-2835: Pick the closest clock rate

[nicolas saenz julienne]

On Wed, 2021-09-22 at 14:54 +0200, Maxime Ripard wrote:
> The driver currently tries to pick the closest rate that is lower than
> the rate being requested.
> 
> This causes an issue with clk_set_min_rate() since it actively checks
> for the rounded rate to be above the minimum that was just set.
> 
> Let's change the logic a bit to pick the closest rate to the requested
> rate, no matter if it's actually higher or lower.
> 
> Fixes: 6d18b8adbe67 ("clk: bcm2835: Support for clock parent selection")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Tested-by: Nicolas Saenz Julienne <nsaenz@kernel.org> # boot and basic functionality

Regards,
Nicolas

Re: [PATCH 1/5] clk: bcm-2835: Pick the closest clock rate

[Stephen Boyd]

Quoting Maxime Ripard (2021-09-22 05:54:15)
> The driver currently tries to pick the closest rate that is lower than
> the rate being requested.
> 
> This causes an issue with clk_set_min_rate() since it actively checks
> for the rounded rate to be above the minimum that was just set.
> 
> Let's change the logic a bit to pick the closest rate to the requested
> rate, no matter if it's actually higher or lower.
> 
> Fixes: 6d18b8adbe67 ("clk: bcm2835: Support for clock parent selection")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

Re: (subset) [PATCH 1/5] clk: bcm-2835: Pick the closest clock rate

[Maxime Ripard]

On Wed, 22 Sep 2021 14:54:15 +0200, Maxime Ripard wrote:
> The driver currently tries to pick the closest rate that is lower than
> the rate being requested.
> 
> This causes an issue with clk_set_min_rate() since it actively checks
> for the rounded rate to be above the minimum that was just set.
> 
> Let's change the logic a bit to pick the closest rate to the requested
> rate, no matter if it's actually higher or lower.
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime

[PATCH 2/5] clk: bcm-2835: Remove rounding up the dividers

[Maxime Ripard]

The driver, once it found a divider, tries to round it up by increasing
the least significant bit of the fractional part by one when the
round_up argument is set and there's a remainder.

However, since it increases the divider it will actually reduce the
clock rate below what we were asking for, leading to issues with
clk_set_min_rate() that will complain that our rounded clock rate is
below the minimum of the rate.

Since the dividers are fairly precise already, let's remove that part so
that we can have clk_set_min_rate() working.

This is effectively a revert of 9c95b32ca093 ("clk: bcm2835: add a round
up ability to the clock divisor").

Fixes: 9c95b32ca093 ("clk: bcm2835: add a round up ability to the clock divisor")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-bcm2835.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index bf97b2b2a63f..3667b4d731e7 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -932,8 +932,7 @@ static int bcm2835_clock_is_on(struct clk_hw *hw)
 
 static u32 bcm2835_clock_choose_div(struct clk_hw *hw,
 				    unsigned long rate,
-				    unsigned long parent_rate,
-				    bool round_up)
+				    unsigned long parent_rate)
 {
 	struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw);
 	const struct bcm2835_clock_data *data = clock->data;
@@ -945,10 +944,6 @@ static u32 bcm2835_clock_choose_div(struct clk_hw *hw,
 
 	rem = do_div(temp, rate);
 	div = temp;
-
-	/* Round up and mask off the unused bits */
-	if (round_up && ((div & unused_frac_mask) != 0 || rem != 0))
-		div += unused_frac_mask + 1;
 	div &= ~unused_frac_mask;
 
 	/* different clamping limits apply for a mash clock */
@@ -1079,7 +1074,7 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw,
 	struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw);
 	struct bcm2835_cprman *cprman = clock->cprman;
 	const struct bcm2835_clock_data *data = clock->data;
-	u32 div = bcm2835_clock_choose_div(hw, rate, parent_rate, false);
+	u32 div = bcm2835_clock_choose_div(hw, rate, parent_rate);
 	u32 ctl;
 
 	spin_lock(&cprman->regs_lock);
@@ -1130,7 +1125,7 @@ static unsigned long bcm2835_clock_choose_div_and_prate(struct clk_hw *hw,
 
 	if (!(BIT(parent_idx) & data->set_rate_parent)) {
 		*prate = clk_hw_get_rate(parent);
-		*div = bcm2835_clock_choose_div(hw, rate, *prate, true);
+		*div = bcm2835_clock_choose_div(hw, rate, *prate);
 
 		*avgrate = bcm2835_clock_rate_from_divisor(clock, *prate, *div);
 
-- 
2.31.1

Re: [PATCH 2/5] clk: bcm-2835: Remove rounding up the dividers

[nicolas saenz julienne]

On Wed, 2021-09-22 at 14:54 +0200, Maxime Ripard wrote:
> The driver, once it found a divider, tries to round it up by increasing
> the least significant bit of the fractional part by one when the
> round_up argument is set and there's a remainder.
> 
> However, since it increases the divider it will actually reduce the
> clock rate below what we were asking for, leading to issues with
> clk_set_min_rate() that will complain that our rounded clock rate is
> below the minimum of the rate.
> 
> Since the dividers are fairly precise already, let's remove that part so
> that we can have clk_set_min_rate() working.
> 
> This is effectively a revert of 9c95b32ca093 ("clk: bcm2835: add a round
> up ability to the clock divisor").
> 
> Fixes: 9c95b32ca093 ("clk: bcm2835: add a round up ability to the clock divisor")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Tested-by: Nicolas Saenz Julienne <nsaenz@kernel.org> # boot and basic functionality

Regards,
Nicolas

Re: [PATCH 2/5] clk: bcm-2835: Remove rounding up the dividers

[Maxime Ripard]

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

On Tue, Sep 28, 2021 at 06:26:55PM +0200, nicolas saenz julienne wrote:
> On Wed, 2021-09-22 at 14:54 +0200, Maxime Ripard wrote:
> > The driver, once it found a divider, tries to round it up by increasing
> > the least significant bit of the fractional part by one when the
> > round_up argument is set and there's a remainder.
> > 
> > However, since it increases the divider it will actually reduce the
> > clock rate below what we were asking for, leading to issues with
> > clk_set_min_rate() that will complain that our rounded clock rate is
> > below the minimum of the rate.
> > 
> > Since the dividers are fairly precise already, let's remove that part so
> > that we can have clk_set_min_rate() working.
> > 
> > This is effectively a revert of 9c95b32ca093 ("clk: bcm2835: add a round
> > up ability to the clock divisor").
> > 
> > Fixes: 9c95b32ca093 ("clk: bcm2835: add a round up ability to the clock divisor")
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> 
> Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
> Tested-by: Nicolas Saenz Julienne <nsaenz@kernel.org> # boot and basic functionality

Does that mean you're ok with merging it through the DRM-misc tree?
Florian, Mike, Stephen, any objection?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/5] clk: bcm-2835: Remove rounding up the dividers

[Stephen Boyd]

Quoting Maxime Ripard (2021-09-22 05:54:16)
> The driver, once it found a divider, tries to round it up by increasing
> the least significant bit of the fractional part by one when the
> round_up argument is set and there's a remainder.
> 
> However, since it increases the divider it will actually reduce the
> clock rate below what we were asking for, leading to issues with
> clk_set_min_rate() that will complain that our rounded clock rate is
> below the minimum of the rate.
> 
> Since the dividers are fairly precise already, let's remove that part so
> that we can have clk_set_min_rate() working.
> 
> This is effectively a revert of 9c95b32ca093 ("clk: bcm2835: add a round
> up ability to the clock divisor").
> 
> Fixes: 9c95b32ca093 ("clk: bcm2835: add a round up ability to the clock divisor")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

Re: (subset) [PATCH 2/5] clk: bcm-2835: Remove rounding up the dividers

[Maxime Ripard]

On Wed, 22 Sep 2021 14:54:16 +0200, Maxime Ripard wrote:
> The driver, once it found a divider, tries to round it up by increasing
> the least significant bit of the fractional part by one when the
> round_up argument is set and there's a remainder.
> 
> However, since it increases the divider it will actually reduce the
> clock rate below what we were asking for, leading to issues with
> clk_set_min_rate() that will complain that our rounded clock rate is
> below the minimum of the rate.
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime

[PATCH 3/5] drm/vc4: hdmi: Set a default HSM rate

[Maxime Ripard]

When the firmware doesn't setup the HSM rate (such as when booting
without an HDMI cable plugged in), its rate is 0 and thus any register
access results in a CPU stall, even though HSM is enabled.

Let's enforce a minimum rate at boot to avoid this issue.

Fixes: 4f6e3d66ac52 ("drm/vc4: Add runtime PM support to the HDMI encoder driver")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index b4b4653fe301..74222b12f8c8 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -94,6 +94,7 @@
 # define VC4_HD_M_SW_RST			BIT(2)
 # define VC4_HD_M_ENABLE			BIT(0)
 
+#define HSM_MIN_CLOCK_FREQ	120000000
 #define CEC_CLOCK_FREQ 40000
 
 #define HDMI_14_MAX_TMDS_CLK   (340 * 1000 * 1000)
@@ -2169,6 +2170,19 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 			vc4_hdmi->disable_4kp60 = true;
 	}
 
+	/*
+	 * If we boot without any cable connected to the HDMI connector,
+	 * the firmware will skip the HSM initialization and leave it
+	 * with a rate of 0, resulting in a bus lockup when we're
+	 * accessing the registers even if it's enabled.
+	 *
+	 * Let's put a sensible default at runtime_resume so that we
+	 * don't end up in this situation.
+	 */
+	ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ);
+	if (ret)
+		goto err_put_ddc;
+
 	if (vc4_hdmi->variant->reset)
 		vc4_hdmi->variant->reset(vc4_hdmi);
 
-- 
2.31.1

Re: [PATCH 3/5] drm/vc4: hdmi: Set a default HSM rate

[nicolas saenz julienne]

On Wed, 2021-09-22 at 14:54 +0200, Maxime Ripard wrote:
> When the firmware doesn't setup the HSM rate (such as when booting
> without an HDMI cable plugged in), its rate is 0 and thus any register
> access results in a CPU stall, even though HSM is enabled.
> 
> Let's enforce a minimum rate at boot to avoid this issue.
> 
> Fixes: 4f6e3d66ac52 ("drm/vc4: Add runtime PM support to the HDMI encoder driver")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Tested-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Regards,
Nicolas

Re: (subset) [PATCH 3/5] drm/vc4: hdmi: Set a default HSM rate

[Maxime Ripard]

On Wed, 22 Sep 2021 14:54:17 +0200, Maxime Ripard wrote:
> When the firmware doesn't setup the HSM rate (such as when booting
> without an HDMI cable plugged in), its rate is 0 and thus any register
> access results in a CPU stall, even though HSM is enabled.
> 
> Let's enforce a minimum rate at boot to avoid this issue.
> 
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime

[PATCH 4/5] drm/vc4: hdmi: Move the HSM clock enable to runtime_pm

[Maxime Ripard]

In order to access the HDMI controller, we need to make sure the HSM
clock is enabled. If we were to access it with the clock disabled, the
CPU would completely hang, resulting in an hard crash.

Since we have different code path that would require it, let's move that
clock enable / disable to runtime_pm that will take care of the
reference counting for us.

Since we also want to change the HSM clock rate and it's only valid
while the clock is disabled, we need to move the clk_set_min_rate() call
on the HSM clock above pm_runtime_get_and_sync().

Fixes: 4f6e3d66ac52 ("drm/vc4: Add runtime PM support to the HDMI encoder driver")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 72 ++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 74222b12f8c8..630b84ab73a2 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -628,7 +628,6 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder,
 		vc4_hdmi->variant->phy_disable(vc4_hdmi);
 
 	clk_disable_unprepare(vc4_hdmi->pixel_bvb_clock);
-	clk_disable_unprepare(vc4_hdmi->hsm_clock);
 	clk_disable_unprepare(vc4_hdmi->pixel_clock);
 
 	ret = pm_runtime_put(&vc4_hdmi->pdev->dev);
@@ -894,28 +893,10 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 		conn_state_to_vc4_hdmi_conn_state(conn_state);
 	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	unsigned long bvb_rate, pixel_rate, hsm_rate;
+	unsigned long pixel_rate = vc4_conn_state->pixel_rate;
+	unsigned long bvb_rate, hsm_rate;
 	int ret;
 
-	ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev);
-	if (ret < 0) {
-		DRM_ERROR("Failed to retain power domain: %d\n", ret);
-		return;
-	}
-
-	pixel_rate = vc4_conn_state->pixel_rate;
-	ret = clk_set_rate(vc4_hdmi->pixel_clock, pixel_rate);
-	if (ret) {
-		DRM_ERROR("Failed to set pixel clock rate: %d\n", ret);
-		return;
-	}
-
-	ret = clk_prepare_enable(vc4_hdmi->pixel_clock);
-	if (ret) {
-		DRM_ERROR("Failed to turn on pixel clock: %d\n", ret);
-		return;
-	}
-
 	/*
 	 * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must
 	 * be faster than pixel clock, infinitesimally faster, tested in
@@ -939,10 +920,21 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 		return;
 	}
 
-	ret = clk_prepare_enable(vc4_hdmi->hsm_clock);
+	ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev);
+	if (ret < 0) {
+		DRM_ERROR("Failed to retain power domain: %d\n", ret);
+		return;
+	}
+
+	ret = clk_set_rate(vc4_hdmi->pixel_clock, pixel_rate);
 	if (ret) {
-		DRM_ERROR("Failed to turn on HSM clock: %d\n", ret);
-		clk_disable_unprepare(vc4_hdmi->pixel_clock);
+		DRM_ERROR("Failed to set pixel clock rate: %d\n", ret);
+		return;
+	}
+
+	ret = clk_prepare_enable(vc4_hdmi->pixel_clock);
+	if (ret) {
+		DRM_ERROR("Failed to turn on pixel clock: %d\n", ret);
 		return;
 	}
 
@@ -958,7 +950,6 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 	ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock, bvb_rate);
 	if (ret) {
 		DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret);
-		clk_disable_unprepare(vc4_hdmi->hsm_clock);
 		clk_disable_unprepare(vc4_hdmi->pixel_clock);
 		return;
 	}
@@ -966,7 +957,6 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 	ret = clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
 	if (ret) {
 		DRM_ERROR("Failed to turn on pixel bvb clock: %d\n", ret);
-		clk_disable_unprepare(vc4_hdmi->hsm_clock);
 		clk_disable_unprepare(vc4_hdmi->pixel_clock);
 		return;
 	}
@@ -2107,6 +2097,29 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int vc4_hdmi_runtime_suspend(struct device *dev)
+{
+	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(vc4_hdmi->hsm_clock);
+
+	return 0;
+}
+
+static int vc4_hdmi_runtime_resume(struct device *dev)
+{
+	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(vc4_hdmi->hsm_clock);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+#endif
+
 static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 {
 	const struct vc4_hdmi_variant *variant = of_device_get_match_data(dev);
@@ -2374,11 +2387,18 @@ static const struct of_device_id vc4_hdmi_dt_match[] = {
 	{}
 };
 
+static const struct dev_pm_ops vc4_hdmi_pm_ops = {
+	SET_RUNTIME_PM_OPS(vc4_hdmi_runtime_suspend,
+			   vc4_hdmi_runtime_resume,
+			   NULL)
+};
+
 struct platform_driver vc4_hdmi_driver = {
 	.probe = vc4_hdmi_dev_probe,
 	.remove = vc4_hdmi_dev_remove,
 	.driver = {
 		.name = "vc4_hdmi",
 		.of_match_table = vc4_hdmi_dt_match,
+		.pm = &vc4_hdmi_pm_ops,
 	},
 };
-- 
2.31.1

Re: [PATCH 4/5] drm/vc4: hdmi: Move the HSM clock enable to runtime_pm

[nicolas saenz julienne]

On Wed, 2021-09-22 at 14:54 +0200, Maxime Ripard wrote:
> In order to access the HDMI controller, we need to make sure the HSM
> clock is enabled. If we were to access it with the clock disabled, the
> CPU would completely hang, resulting in an hard crash.
> 
> Since we have different code path that would require it, let's move that
> clock enable / disable to runtime_pm that will take care of the
> reference counting for us.
> 
> Since we also want to change the HSM clock rate and it's only valid
> while the clock is disabled, we need to move the clk_set_min_rate() call
> on the HSM clock above pm_runtime_get_and_sync().
> 
> Fixes: 4f6e3d66ac52 ("drm/vc4: Add runtime PM support to the HDMI encoder driver")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Tested-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Regards,
Nicolas

Re: (subset) [PATCH 4/5] drm/vc4: hdmi: Move the HSM clock enable to runtime_pm

[Maxime Ripard]

On Wed, 22 Sep 2021 14:54:18 +0200, Maxime Ripard wrote:
> In order to access the HDMI controller, we need to make sure the HSM
> clock is enabled. If we were to access it with the clock disabled, the
> CPU would completely hang, resulting in an hard crash.
> 
> Since we have different code path that would require it, let's move that
> clock enable / disable to runtime_pm that will take care of the
> reference counting for us.
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime

[PATCH 5/5] drm/vc4: hdmi: Make sure the controller is powered in detect

[Maxime Ripard]

If the HPD GPIO is not available and drm_probe_ddc fails, we end up
reading the HDMI_HOTPLUG register, but the controller might be powered
off resulting in a CPU hang. Make sure we have the power domain and the
HSM clock powered during the detect cycle to prevent the hang from
happening.

Fixes: 4f6e3d66ac52 ("drm/vc4: Add runtime PM support to the HDMI encoder driver")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 630b84ab73a2..8e6d7e1af200 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -168,6 +168,8 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
 	bool connected = false;
 
+	WARN_ON(pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev));
+
 	if (vc4_hdmi->hpd_gpio &&
 	    gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) {
 		connected = true;
@@ -188,10 +190,12 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 			}
 		}
 
+		pm_runtime_put(&vc4_hdmi->pdev->dev);
 		return connector_status_connected;
 	}
 
 	cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
+	pm_runtime_put(&vc4_hdmi->pdev->dev);
 	return connector_status_disconnected;
 }
 
-- 
2.31.1

Re: [PATCH 5/5] drm/vc4: hdmi: Make sure the controller is powered in detect

[nicolas saenz julienne]

On Wed, 2021-09-22 at 14:54 +0200, Maxime Ripard wrote:
> If the HPD GPIO is not available and drm_probe_ddc fails, we end up
> reading the HDMI_HOTPLUG register, but the controller might be powered
> off resulting in a CPU hang. Make sure we have the power domain and the
> HSM clock powered during the detect cycle to prevent the hang from
> happening.
> 
> Fixes: 4f6e3d66ac52 ("drm/vc4: Add runtime PM support to the HDMI encoder driver")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Tested-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Regards,
Nicolas

Re: (subset) [PATCH 5/5] drm/vc4: hdmi: Make sure the controller is powered in detect

[Maxime Ripard]

On Wed, 22 Sep 2021 14:54:19 +0200, Maxime Ripard wrote:
> If the HPD GPIO is not available and drm_probe_ddc fails, we end up
> reading the HDMI_HOTPLUG register, but the controller might be powered
> off resulting in a CPU hang. Make sure we have the power domain and the
> HSM clock powered during the detect cycle to prevent the hang from
> happening.
> 
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime

Re: [PATCH 0/5] drm/vc4: hdmi: Remove CPU hangs, take 2

[Michael Stapelberg]

I can confirm that this patch series (applied to linux commit
58e2cf5d794616b84f591d4d1276c8953278ce24) works for me — my Raspberry
Pi 3 still boots fine (without HDMI connected).

Thanks!


On Wed, 22 Sept 2021 at 23:18, Michael Stapelberg <michael@stapelberg.ch> wrote:
>
>
> On Wed, 22 Sept 2021 at 14:54, Maxime Ripard <maxime@cerno.tech> wrote:
>>
>> Hi,
>>
>> Here's another attempt at fixing the complete CPU stall while retrieving the
>> HDMI connector status when the connector is disabled.
>>
>> This was fixed already, but eventually got reverted by Linus due to the same
>> symptom happening in another situation. This was likely (but not confirmed by
>> the reporter) due to the kernel being booted without an HDMI display connected,
>> in which case the firmware won't initialise the HDMI State Machine clock.
>
>
> Sorry for the lack of confirmation: yes, this problem was encountered when no HDMI display was connected.
>
> I’ll try testing your patch series tomorrow.
>
> Thanks for taking care of this!
>
>>
>>
>> This is fixed by patch 3. However, further changes in the clock drivers were
>> needed for clk_set_min_rate to be used, which are patches 1 and 2.
>>
>> Finally, patches 4 and 5 are the original patches that were reverted. Patch 4
>> got a small modification to move the clk_set_min_rate() call before the HSM
>> clock is enabled.
>>
>> Let me know what you think,
>> Maxime
>>
>> Maxime Ripard (5):
>>   clk: bcm-2835: Pick the closest clock rate
>>   clk: bcm-2835: Remove rounding up the dividers
>>   drm/vc4: hdmi: Set a default HSM rate
>>   drm/vc4: hdmi: Move the HSM clock enable to runtime_pm
>>   drm/vc4: hdmi: Make sure the controller is powered in detect
>>
>>  drivers/clk/bcm/clk-bcm2835.c  | 13 ++---
>>  drivers/gpu/drm/vc4/vc4_hdmi.c | 90 ++++++++++++++++++++++++----------
>>  2 files changed, 68 insertions(+), 35 deletions(-)
>>
>> --
>> 2.31.1
>>
>
>
> --
> Best regards,
> Michael



-- 
Best regards,
Michael

Re: [PATCH 0/5] drm/vc4: hdmi: Remove CPU hangs, take 2

[Maxime Ripard]

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

Hi,

On Wed, Sep 22, 2021 at 02:54:14PM +0200, Maxime Ripard wrote:
> Hi,
> 
> Here's another attempt at fixing the complete CPU stall while retrieving the
> HDMI connector status when the connector is disabled.
> 
> This was fixed already, but eventually got reverted by Linus due to the same
> symptom happening in another situation. This was likely (but not confirmed by
> the reporter) due to the kernel being booted without an HDMI display connected,
> in which case the firmware won't initialise the HDMI State Machine clock.
> 
> This is fixed by patch 3. However, further changes in the clock drivers were
> needed for clk_set_min_rate to be used, which are patches 1 and 2.
> 
> Finally, patches 4 and 5 are the original patches that were reverted. Patch 4
> got a small modification to move the clk_set_min_rate() call before the HSM
> clock is enabled.

If we merge the clock patches and DRM patches separately we're going to
break bisectability. I guess the easiest approach would be to merge the
clk patches through DRM. Does that work for everyone?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/5] drm/vc4: hdmi: Remove CPU hangs, take 2

[Maxime Ripard]

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

On Fri, Sep 24, 2021 at 09:40:44AM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Sep 22, 2021 at 02:54:14PM +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > Here's another attempt at fixing the complete CPU stall while retrieving the
> > HDMI connector status when the connector is disabled.
> > 
> > This was fixed already, but eventually got reverted by Linus due to the same
> > symptom happening in another situation. This was likely (but not confirmed by
> > the reporter) due to the kernel being booted without an HDMI display connected,
> > in which case the firmware won't initialise the HDMI State Machine clock.
> > 
> > This is fixed by patch 3. However, further changes in the clock drivers were
> > needed for clk_set_min_rate to be used, which are patches 1 and 2.
> > 
> > Finally, patches 4 and 5 are the original patches that were reverted. Patch 4
> > got a small modification to move the clk_set_min_rate() call before the HSM
> > clock is enabled.
> 
> If we merge the clock patches and DRM patches separately we're going to
> break bisectability. I guess the easiest approach would be to merge the
> clk patches through DRM. Does that work for everyone?

Anyone? I can ask around for reviews on DRM, but I'd really like some
reviews on the clock patches here..

Maxime


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/5] drm/vc4: hdmi: Remove CPU hangs, take 2

[Stephen Boyd]

Quoting Maxime Ripard (2021-09-28 06:05:49)
> On Fri, Sep 24, 2021 at 09:40:44AM +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > On Wed, Sep 22, 2021 at 02:54:14PM +0200, Maxime Ripard wrote:
> > > Hi,
> > > 
> > > Here's another attempt at fixing the complete CPU stall while retrieving the
> > > HDMI connector status when the connector is disabled.
> > > 
> > > This was fixed already, but eventually got reverted by Linus due to the same
> > > symptom happening in another situation. This was likely (but not confirmed by
> > > the reporter) due to the kernel being booted without an HDMI display connected,
> > > in which case the firmware won't initialise the HDMI State Machine clock.
> > > 
> > > This is fixed by patch 3. However, further changes in the clock drivers were
> > > needed for clk_set_min_rate to be used, which are patches 1 and 2.
> > > 
> > > Finally, patches 4 and 5 are the original patches that were reverted. Patch 4
> > > got a small modification to move the clk_set_min_rate() call before the HSM
> > > clock is enabled.
> > 
> > If we merge the clock patches and DRM patches separately we're going to
> > break bisectability. I guess the easiest approach would be to merge the
> > clk patches through DRM. Does that work for everyone?
> 
> Anyone? I can ask around for reviews on DRM, but I'd really like some
> reviews on the clock patches here..
> 

Looks ok to me to take through drm.

Re: [PATCH 0/5] drm/vc4: hdmi: Remove CPU hangs, take 2

[Florian Fainelli]

On 9/30/21 11:09 AM, Stephen Boyd wrote:
> Quoting Maxime Ripard (2021-09-28 06:05:49)
>> On Fri, Sep 24, 2021 at 09:40:44AM +0200, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Wed, Sep 22, 2021 at 02:54:14PM +0200, Maxime Ripard wrote:
>>>> Hi,
>>>>
>>>> Here's another attempt at fixing the complete CPU stall while retrieving the
>>>> HDMI connector status when the connector is disabled.
>>>>
>>>> This was fixed already, but eventually got reverted by Linus due to the same
>>>> symptom happening in another situation. This was likely (but not confirmed by
>>>> the reporter) due to the kernel being booted without an HDMI display connected,
>>>> in which case the firmware won't initialise the HDMI State Machine clock.
>>>>
>>>> This is fixed by patch 3. However, further changes in the clock drivers were
>>>> needed for clk_set_min_rate to be used, which are patches 1 and 2.
>>>>
>>>> Finally, patches 4 and 5 are the original patches that were reverted. Patch 4
>>>> got a small modification to move the clk_set_min_rate() call before the HSM
>>>> clock is enabled.
>>>
>>> If we merge the clock patches and DRM patches separately we're going to
>>> break bisectability. I guess the easiest approach would be to merge the
>>> clk patches through DRM. Does that work for everyone?
>>
>> Anyone? I can ask around for reviews on DRM, but I'd really like some
>> reviews on the clock patches here..
>>
> 
> Looks ok to me to take through drm.

Same here, assuming I even have a say in this ;)
-- 
Florian