linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled
@ 2021-07-07  9:22 Maxime Ripard
  2021-07-07  9:22 ` [PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind Maxime Ripard
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Maxime Ripard @ 2021-07-07  9:22 UTC (permalink / raw)
  To: dri-devel, Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard
  Cc: Maxime Ripard, Emma Anholt, Dave Stevenson, Boris Brezillon,
	Phil Elwell, Tim Gover, Dom Cobley, linux-rpi-kernel,
	Nicolas Saenz Julienne, Hans Verkuil, linux-kernel,
	bcm-kernel-feedback-list

Hi,

This series aims at fixing a complete and silent hang when one tries to use CEC
while the display output is off.

This can be tested with:

echo off > /sys/class/drm/card0-HDMI-A-1/status
cec-ctl --tuner -p 1.0.0.0
cec-compliance

This series addresses it by making sure the HDMI controller is powered up as
soon as the CEC device is opened by the userspace.

Let me know what you think,
Maxime

Changes from v1:
  - More fixes
  - Added a big warning if we try to access a register while the device is
    disabled.
  - Fixed the pre_crtc_configure error path

Maxime Ripard (5):
  drm/vc4: hdmi: Make sure the controller is powered up during bind
  drm/vc4: hdmi: Rework the pre_crtc_configure error handling
  drm/vc4: hdmi: Split the CEC disable / enable functions in two
  drm/vc4: hdmi: Make sure the device is powered with CEC
  drm/vc4: hdmi: Warn if we access the controller while disabled

 drivers/gpu/drm/vc4/vc4_hdmi.c      | 123 +++++++++++++++++++---------
 drivers/gpu/drm/vc4/vc4_hdmi_regs.h |   6 ++
 2 files changed, 89 insertions(+), 40 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind
  2021-07-07  9:22 [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled Maxime Ripard
@ 2021-07-07  9:22 ` Maxime Ripard
  2021-07-28 13:50   ` Dave Stevenson
  2021-07-07  9:22 ` [PATCH v2 2/5] drm/vc4: hdmi: Rework the pre_crtc_configure error handling Maxime Ripard
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 8+ messages in thread
From: Maxime Ripard @ 2021-07-07  9:22 UTC (permalink / raw)
  To: dri-devel, Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard
  Cc: Maxime Ripard, Emma Anholt, Dave Stevenson, Boris Brezillon,
	Phil Elwell, Tim Gover, Dom Cobley, linux-rpi-kernel,
	Nicolas Saenz Julienne, Hans Verkuil, linux-kernel,
	bcm-kernel-feedback-list

In the bind hook, we actually need the device to have the HSM clock
running during the final part of the display initialisation where we
reset the controller and initialise the CEC component.

Failing to do so will result in a complete, silent, hang of the CPU.

Fixes: 411efa18e4b0 ("drm/vc4: hdmi: Move the HSM clock enable to runtime_pm")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index aab1b36ceb3c..dac47b100b8b 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2176,6 +2176,18 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 			vc4_hdmi->disable_4kp60 = true;
 	}
 
+	/*
+	 * We need to have the device powered up at this point to call
+	 * our reset hook and for the CEC init.
+	 */
+	ret = vc4_hdmi_runtime_resume(dev);
+	if (ret)
+		goto err_put_ddc;
+
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	if (vc4_hdmi->variant->reset)
 		vc4_hdmi->variant->reset(vc4_hdmi);
 
@@ -2187,8 +2199,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 		clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
 	}
 
-	pm_runtime_enable(dev);
-
 	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
 	drm_encoder_helper_add(encoder, &vc4_hdmi_encoder_helper_funcs);
 
@@ -2208,6 +2218,8 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 			     vc4_hdmi_debugfs_regs,
 			     vc4_hdmi);
 
+	pm_runtime_put_sync(dev);
+
 	return 0;
 
 err_free_cec:
@@ -2216,6 +2228,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 	vc4_hdmi_connector_destroy(&vc4_hdmi->connector);
 err_destroy_encoder:
 	drm_encoder_cleanup(encoder);
+	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
 err_put_ddc:
 	put_device(&vc4_hdmi->ddc->dev);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/5] drm/vc4: hdmi: Rework the pre_crtc_configure error handling
  2021-07-07  9:22 [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled Maxime Ripard
  2021-07-07  9:22 ` [PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind Maxime Ripard
@ 2021-07-07  9:22 ` Maxime Ripard
  2021-07-07  9:22 ` [PATCH v2 3/5] drm/vc4: hdmi: Split the CEC disable / enable functions in two Maxime Ripard
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Maxime Ripard @ 2021-07-07  9:22 UTC (permalink / raw)
  To: dri-devel, Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard
  Cc: Maxime Ripard, Emma Anholt, Dave Stevenson, Boris Brezillon,
	Phil Elwell, Tim Gover, Dom Cobley, linux-rpi-kernel,
	Nicolas Saenz Julienne, Hans Verkuil, linux-kernel,
	bcm-kernel-feedback-list

Since our pre_crtc_configure hook returned void, we didn't implement a
goto-based error path handling, leading to errors like failing to put
back the device in pm_runtime in all the error paths, but also failing
to disable the pixel clock if clk_set_min_rate on the HSM clock fails.

Move to a goto-based implementation to have an easier consitency.

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 | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index dac47b100b8b..38eb3caf6c44 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -913,13 +913,13 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 	ret = clk_set_rate(vc4_hdmi->pixel_clock, pixel_rate);
 	if (ret) {
 		DRM_ERROR("Failed to set pixel clock rate: %d\n", ret);
-		return;
+		goto err_put_runtime_pm;
 	}
 
 	ret = clk_prepare_enable(vc4_hdmi->pixel_clock);
 	if (ret) {
 		DRM_ERROR("Failed to turn on pixel clock: %d\n", ret);
-		return;
+		goto err_put_runtime_pm;
 	}
 
 	/*
@@ -942,7 +942,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 	ret = clk_set_min_rate(vc4_hdmi->hsm_clock, hsm_rate);
 	if (ret) {
 		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
-		return;
+		goto err_disable_pixel_clock;
 	}
 
 	vc4_hdmi_cec_update_clk_div(vc4_hdmi);
@@ -957,15 +957,13 @@ 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->pixel_clock);
-		return;
+		goto err_disable_pixel_clock;
 	}
 
 	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->pixel_clock);
-		return;
+		goto err_disable_pixel_clock;
 	}
 
 	if (vc4_hdmi->variant->phy_init)
@@ -978,6 +976,15 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 
 	if (vc4_hdmi->variant->set_timings)
 		vc4_hdmi->variant->set_timings(vc4_hdmi, conn_state, mode);
+
+	return;
+
+err_disable_pixel_clock:
+	clk_disable_unprepare(vc4_hdmi->pixel_clock);
+err_put_runtime_pm:
+	pm_runtime_put(&vc4_hdmi->pdev->dev);
+
+	return;
 }
 
 static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder,
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/5] drm/vc4: hdmi: Split the CEC disable / enable functions in two
  2021-07-07  9:22 [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled Maxime Ripard
  2021-07-07  9:22 ` [PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind Maxime Ripard
  2021-07-07  9:22 ` [PATCH v2 2/5] drm/vc4: hdmi: Rework the pre_crtc_configure error handling Maxime Ripard
@ 2021-07-07  9:22 ` Maxime Ripard
  2021-07-07  9:22 ` [PATCH v2 4/5] drm/vc4: hdmi: Make sure the device is powered with CEC Maxime Ripard
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Maxime Ripard @ 2021-07-07  9:22 UTC (permalink / raw)
  To: dri-devel, Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard
  Cc: Maxime Ripard, Emma Anholt, Dave Stevenson, Boris Brezillon,
	Phil Elwell, Tim Gover, Dom Cobley, linux-rpi-kernel,
	Nicolas Saenz Julienne, Hans Verkuil, linux-kernel,
	bcm-kernel-feedback-list

In order to ease further additions to the CEC enable and disable, let's
split the function into two functions, one to enable and the other to
disable.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 73 ++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 38eb3caf6c44..825696e6ef02 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1740,7 +1740,7 @@ static irqreturn_t vc4_cec_irq_handler(int irq, void *priv)
 	return ret;
 }
 
-static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
+static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 {
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
 	/* clock period in microseconds */
@@ -1753,38 +1753,53 @@ static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
 	val |= ((4700 / usecs) << VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT) |
 	       ((4500 / usecs) << VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT);
 
-	if (enable) {
-		HDMI_WRITE(HDMI_CEC_CNTRL_5, val |
-			   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
-		HDMI_WRITE(HDMI_CEC_CNTRL_5, val);
-		HDMI_WRITE(HDMI_CEC_CNTRL_2,
-			   ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) |
-			   ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) |
-			   ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) |
-			   ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) |
-			   ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT));
-		HDMI_WRITE(HDMI_CEC_CNTRL_3,
-			   ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) |
-			   ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) |
-			   ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) |
-			   ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT));
-		HDMI_WRITE(HDMI_CEC_CNTRL_4,
-			   ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) |
-			   ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) |
-			   ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) |
-			   ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
+	HDMI_WRITE(HDMI_CEC_CNTRL_5, val |
+		   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
+	HDMI_WRITE(HDMI_CEC_CNTRL_5, val);
+	HDMI_WRITE(HDMI_CEC_CNTRL_2,
+		   ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) |
+		   ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) |
+		   ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) |
+		   ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) |
+		   ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT));
+	HDMI_WRITE(HDMI_CEC_CNTRL_3,
+		   ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) |
+		   ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) |
+		   ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) |
+		   ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT));
+	HDMI_WRITE(HDMI_CEC_CNTRL_4,
+		   ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) |
+		   ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) |
+		   ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) |
+		   ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
+
+	if (!vc4_hdmi->variant->external_irq_controller)
+		HDMI_WRITE(HDMI_CEC_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
 
-		if (!vc4_hdmi->variant->external_irq_controller)
-			HDMI_WRITE(HDMI_CEC_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
-	} else {
-		if (!vc4_hdmi->variant->external_irq_controller)
-			HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
-		HDMI_WRITE(HDMI_CEC_CNTRL_5, val |
-			   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
-	}
 	return 0;
 }
 
+static int vc4_hdmi_cec_disable(struct cec_adapter *adap)
+{
+	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
+
+	if (!vc4_hdmi->variant->external_irq_controller)
+		HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
+
+	HDMI_WRITE(HDMI_CEC_CNTRL_5, HDMI_READ(HDMI_CEC_CNTRL_5) |
+		   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
+
+	return 0;
+}
+
+static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+	if (enable)
+		return vc4_hdmi_cec_enable(adap);
+	else
+		return vc4_hdmi_cec_disable(adap);
+}
+
 static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
 {
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 4/5] drm/vc4: hdmi: Make sure the device is powered with CEC
  2021-07-07  9:22 [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled Maxime Ripard
                   ` (2 preceding siblings ...)
  2021-07-07  9:22 ` [PATCH v2 3/5] drm/vc4: hdmi: Split the CEC disable / enable functions in two Maxime Ripard
@ 2021-07-07  9:22 ` Maxime Ripard
  2021-07-07  9:22 ` [PATCH v2 5/5] drm/vc4: hdmi: Warn if we access the controller while disabled Maxime Ripard
  2021-07-28 13:52 ` [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access " Dave Stevenson
  5 siblings, 0 replies; 8+ messages in thread
From: Maxime Ripard @ 2021-07-07  9:22 UTC (permalink / raw)
  To: dri-devel, Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard
  Cc: Maxime Ripard, Emma Anholt, Dave Stevenson, Boris Brezillon,
	Phil Elwell, Tim Gover, Dom Cobley, linux-rpi-kernel,
	Nicolas Saenz Julienne, Hans Verkuil, linux-kernel,
	bcm-kernel-feedback-list

Similarly to what we encountered with the detect hook with DRM, nothing
actually prevents any of the CEC callback from being run while the HDMI
output is disabled.

However, this is an issue since any register access to the controller
when it's powered down will result in a silent hang.

Let's make sure we run the runtime_pm hooks when the CEC adapter is
opened and closed by the userspace to avoid that issue.

Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 825696e6ef02..c37326f5e263 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1745,8 +1745,14 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
 	/* clock period in microseconds */
 	const u32 usecs = 1000000 / CEC_CLOCK_FREQ;
-	u32 val = HDMI_READ(HDMI_CEC_CNTRL_5);
+	u32 val;
+	int ret;
 
+	ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev);
+	if (ret)
+		return ret;
+
+	val = HDMI_READ(HDMI_CEC_CNTRL_5);
 	val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET |
 		 VC4_HDMI_CEC_CNT_TO_4700_US_MASK |
 		 VC4_HDMI_CEC_CNT_TO_4500_US_MASK);
@@ -1789,6 +1795,8 @@ static int vc4_hdmi_cec_disable(struct cec_adapter *adap)
 	HDMI_WRITE(HDMI_CEC_CNTRL_5, HDMI_READ(HDMI_CEC_CNTRL_5) |
 		   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
 
+	pm_runtime_put(&vc4_hdmi->pdev->dev);
+
 	return 0;
 }
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 5/5] drm/vc4: hdmi: Warn if we access the controller while disabled
  2021-07-07  9:22 [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled Maxime Ripard
                   ` (3 preceding siblings ...)
  2021-07-07  9:22 ` [PATCH v2 4/5] drm/vc4: hdmi: Make sure the device is powered with CEC Maxime Ripard
@ 2021-07-07  9:22 ` Maxime Ripard
  2021-07-28 13:52 ` [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access " Dave Stevenson
  5 siblings, 0 replies; 8+ messages in thread
From: Maxime Ripard @ 2021-07-07  9:22 UTC (permalink / raw)
  To: dri-devel, Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard
  Cc: Maxime Ripard, Emma Anholt, Dave Stevenson, Boris Brezillon,
	Phil Elwell, Tim Gover, Dom Cobley, linux-rpi-kernel,
	Nicolas Saenz Julienne, Hans Verkuil, linux-kernel,
	bcm-kernel-feedback-list

We've had many silent hangs where the kernel would look like it just
stalled due to the access to one of the HDMI registers while the
controller was disabled.

Add a warning if we're about to do that so that it's at least not silent
anymore.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
index 19d2fdc446bc..99dde6e06a37 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
@@ -1,6 +1,8 @@
 #ifndef _VC4_HDMI_REGS_H_
 #define _VC4_HDMI_REGS_H_
 
+#include <linux/pm_runtime.h>
+
 #include "vc4_hdmi.h"
 
 #define VC4_HDMI_PACKET_STRIDE			0x24
@@ -412,6 +414,8 @@ static inline u32 vc4_hdmi_read(struct vc4_hdmi *hdmi,
 	const struct vc4_hdmi_variant *variant = hdmi->variant;
 	void __iomem *base;
 
+	WARN_ON(!pm_runtime_active(&hdmi->pdev->dev));
+
 	if (reg >= variant->num_registers) {
 		dev_warn(&hdmi->pdev->dev,
 			 "Invalid register ID %u\n", reg);
@@ -438,6 +442,8 @@ static inline void vc4_hdmi_write(struct vc4_hdmi *hdmi,
 	const struct vc4_hdmi_variant *variant = hdmi->variant;
 	void __iomem *base;
 
+	WARN_ON(!pm_runtime_active(&hdmi->pdev->dev));
+
 	if (reg >= variant->num_registers) {
 		dev_warn(&hdmi->pdev->dev,
 			 "Invalid register ID %u\n", reg);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind
  2021-07-07  9:22 ` [PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind Maxime Ripard
@ 2021-07-28 13:50   ` Dave Stevenson
  0 siblings, 0 replies; 8+ messages in thread
From: Dave Stevenson @ 2021-07-28 13:50 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: DRI Development, Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard, Emma Anholt, Boris Brezillon,
	Phil Elwell, Tim Gover, Dom Cobley, linux-rpi-kernel,
	Nicolas Saenz Julienne, Hans Verkuil, LKML,
	bcm-kernel-feedback-list

Hi Maxime

On Wed, 7 Jul 2021 at 10:23, Maxime Ripard <maxime@cerno.tech> wrote:
>
> In the bind hook, we actually need the device to have the HSM clock
> running during the final part of the display initialisation where we
> reset the controller and initialise the CEC component.
>
> Failing to do so will result in a complete, silent, hang of the CPU.
>
> Fixes: 411efa18e4b0 ("drm/vc4: hdmi: Move the HSM clock enable to runtime_pm")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index aab1b36ceb3c..dac47b100b8b 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -2176,6 +2176,18 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>                         vc4_hdmi->disable_4kp60 = true;
>         }
>
> +       /*
> +        * We need to have the device powered up at this point to call
> +        * our reset hook and for the CEC init.
> +        */
> +       ret = vc4_hdmi_runtime_resume(dev);

vc4_hdmi_runtime_resume is within a #ifdef CONFIG_PM block, so
potentially isn't defined.
Admittedly we "select PM" in Kconfig so it should always be enabled,
so perhaps it's better to just remove the #ifdef CONFIG_PM around the
resume and suspend functions.

That's possibly a separate issue to the issue that this patch is
addressing, but may explain the test bot's failure. Otherwise
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> +       if (ret)
> +               goto err_put_ddc;
> +
> +       pm_runtime_get_noresume(dev);
> +       pm_runtime_set_active(dev);
> +       pm_runtime_enable(dev);
> +
>         if (vc4_hdmi->variant->reset)
>                 vc4_hdmi->variant->reset(vc4_hdmi);
>
> @@ -2187,8 +2199,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>                 clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
>         }
>
> -       pm_runtime_enable(dev);
> -
>         drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
>         drm_encoder_helper_add(encoder, &vc4_hdmi_encoder_helper_funcs);
>
> @@ -2208,6 +2218,8 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>                              vc4_hdmi_debugfs_regs,
>                              vc4_hdmi);
>
> +       pm_runtime_put_sync(dev);
> +
>         return 0;
>
>  err_free_cec:
> @@ -2216,6 +2228,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>         vc4_hdmi_connector_destroy(&vc4_hdmi->connector);
>  err_destroy_encoder:
>         drm_encoder_cleanup(encoder);
> +       pm_runtime_put_sync(dev);
>         pm_runtime_disable(dev);
>  err_put_ddc:
>         put_device(&vc4_hdmi->ddc->dev);
> --
> 2.31.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled
  2021-07-07  9:22 [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled Maxime Ripard
                   ` (4 preceding siblings ...)
  2021-07-07  9:22 ` [PATCH v2 5/5] drm/vc4: hdmi: Warn if we access the controller while disabled Maxime Ripard
@ 2021-07-28 13:52 ` Dave Stevenson
  5 siblings, 0 replies; 8+ messages in thread
From: Dave Stevenson @ 2021-07-28 13:52 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: DRI Development, Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard, Emma Anholt, Boris Brezillon,
	Phil Elwell, Tim Gover, Dom Cobley, linux-rpi-kernel,
	Nicolas Saenz Julienne, Hans Verkuil, LKML,
	bcm-kernel-feedback-list

Hi Maxime

On Wed, 7 Jul 2021 at 10:23, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> This series aims at fixing a complete and silent hang when one tries to use CEC
> while the display output is off.
>
> This can be tested with:
>
> echo off > /sys/class/drm/card0-HDMI-A-1/status
> cec-ctl --tuner -p 1.0.0.0
> cec-compliance
>
> This series addresses it by making sure the HDMI controller is powered up as
> soon as the CEC device is opened by the userspace.
>
> Let me know what you think,
> Maxime
>
> Changes from v1:
>   - More fixes
>   - Added a big warning if we try to access a register while the device is
>     disabled.
>   - Fixed the pre_crtc_configure error path
>
> Maxime Ripard (5):
>   drm/vc4: hdmi: Make sure the controller is powered up during bind
>   drm/vc4: hdmi: Rework the pre_crtc_configure error handling
>   drm/vc4: hdmi: Split the CEC disable / enable functions in two
>   drm/vc4: hdmi: Make sure the device is powered with CEC
>   drm/vc4: hdmi: Warn if we access the controller while disabled

Comment made on patch 1.

Patches 2-5:
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

  Dave

>
>  drivers/gpu/drm/vc4/vc4_hdmi.c      | 123 +++++++++++++++++++---------
>  drivers/gpu/drm/vc4/vc4_hdmi_regs.h |   6 ++
>  2 files changed, 89 insertions(+), 40 deletions(-)
>
> --
> 2.31.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-07-28 13:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-07  9:22 [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled Maxime Ripard
2021-07-07  9:22 ` [PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind Maxime Ripard
2021-07-28 13:50   ` Dave Stevenson
2021-07-07  9:22 ` [PATCH v2 2/5] drm/vc4: hdmi: Rework the pre_crtc_configure error handling Maxime Ripard
2021-07-07  9:22 ` [PATCH v2 3/5] drm/vc4: hdmi: Split the CEC disable / enable functions in two Maxime Ripard
2021-07-07  9:22 ` [PATCH v2 4/5] drm/vc4: hdmi: Make sure the device is powered with CEC Maxime Ripard
2021-07-07  9:22 ` [PATCH v2 5/5] drm/vc4: hdmi: Warn if we access the controller while disabled Maxime Ripard
2021-07-28 13:52 ` [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access " Dave Stevenson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).