[PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled

[PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled

[Maxime Ripard]

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

[PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled

[Maxime Ripard]

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

[PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind

[Maxime Ripard]

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

[PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind

[Maxime Ripard]

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

[PATCH v2 2/5] drm/vc4: hdmi: Rework the pre_crtc_configure error handling

[Maxime Ripard]

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

[PATCH v2 2/5] drm/vc4: hdmi: Rework the pre_crtc_configure error handling

[Maxime Ripard]

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

[PATCH v2 3/5] drm/vc4: hdmi: Split the CEC disable / enable functions in two

[Maxime Ripard]

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

[PATCH v2 3/5] drm/vc4: hdmi: Split the CEC disable / enable functions in two

[Maxime Ripard]

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

[PATCH v2 4/5] drm/vc4: hdmi: Make sure the device is powered with CEC

[Maxime Ripard]

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

[PATCH v2 4/5] drm/vc4: hdmi: Make sure the device is powered with CEC

[Maxime Ripard]

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

[PATCH v2 5/5] drm/vc4: hdmi: Warn if we access the controller while disabled

[Maxime Ripard]

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

[PATCH v2 5/5] drm/vc4: hdmi: Warn if we access the controller while disabled

[Maxime Ripard]

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

Re: [PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind

[kernel test robot]

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

Hi Maxime,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-tip/drm-tip]
[also build test ERROR on drm-intel/for-linux-next next-20210707]
[cannot apply to anholt/for-next v5.13]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Maxime-Ripard/drm-vc4-hdmi-Fix-CEC-access-while-disabled/20210707-172621
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4342e12ac48418ce6366423771e887fa9fff89eb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Maxime-Ripard/drm-vc4-hdmi-Fix-CEC-access-while-disabled/20210707-172621
        git checkout 4342e12ac48418ce6366423771e887fa9fff89eb
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/vc4/vc4_hdmi.c: In function 'vc4_hdmi_bind':
>> drivers/gpu/drm/vc4/vc4_hdmi.c:2178:8: error: implicit declaration of function 'vc4_hdmi_runtime_resume'; did you mean 'pm_runtime_resume'? [-Werror=implicit-function-declaration]
    2178 |  ret = vc4_hdmi_runtime_resume(dev);
         |        ^~~~~~~~~~~~~~~~~~~~~~~
         |        pm_runtime_resume
   At top level:
   drivers/gpu/drm/vc4/vc4_hdmi.c:1402:46: warning: 'vc4_hdmi_audio_component_drv' defined but not used [-Wunused-const-variable=]
    1402 | static const struct snd_soc_component_driver vc4_hdmi_audio_component_drv = {
         |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +2178 drivers/gpu/drm/vc4/vc4_hdmi.c

  2110	
  2111	static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
  2112	{
  2113		const struct vc4_hdmi_variant *variant = of_device_get_match_data(dev);
  2114		struct platform_device *pdev = to_platform_device(dev);
  2115		struct drm_device *drm = dev_get_drvdata(master);
  2116		struct vc4_hdmi *vc4_hdmi;
  2117		struct drm_encoder *encoder;
  2118		struct device_node *ddc_node;
  2119		int ret;
  2120	
  2121		vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL);
  2122		if (!vc4_hdmi)
  2123			return -ENOMEM;
  2124		INIT_DELAYED_WORK(&vc4_hdmi->scrambling_work, vc4_hdmi_scrambling_wq);
  2125	
  2126		dev_set_drvdata(dev, vc4_hdmi);
  2127		encoder = &vc4_hdmi->encoder.base.base;
  2128		vc4_hdmi->encoder.base.type = variant->encoder_type;
  2129		vc4_hdmi->encoder.base.pre_crtc_configure = vc4_hdmi_encoder_pre_crtc_configure;
  2130		vc4_hdmi->encoder.base.pre_crtc_enable = vc4_hdmi_encoder_pre_crtc_enable;
  2131		vc4_hdmi->encoder.base.post_crtc_enable = vc4_hdmi_encoder_post_crtc_enable;
  2132		vc4_hdmi->encoder.base.post_crtc_disable = vc4_hdmi_encoder_post_crtc_disable;
  2133		vc4_hdmi->encoder.base.post_crtc_powerdown = vc4_hdmi_encoder_post_crtc_powerdown;
  2134		vc4_hdmi->pdev = pdev;
  2135		vc4_hdmi->variant = variant;
  2136	
  2137		ret = variant->init_resources(vc4_hdmi);
  2138		if (ret)
  2139			return ret;
  2140	
  2141		ddc_node = of_parse_phandle(dev->of_node, "ddc", 0);
  2142		if (!ddc_node) {
  2143			DRM_ERROR("Failed to find ddc node in device tree\n");
  2144			return -ENODEV;
  2145		}
  2146	
  2147		vc4_hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
  2148		of_node_put(ddc_node);
  2149		if (!vc4_hdmi->ddc) {
  2150			DRM_DEBUG("Failed to get ddc i2c adapter by node\n");
  2151			return -EPROBE_DEFER;
  2152		}
  2153	
  2154		/* Only use the GPIO HPD pin if present in the DT, otherwise
  2155		 * we'll use the HDMI core's register.
  2156		 */
  2157		vc4_hdmi->hpd_gpio = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN);
  2158		if (IS_ERR(vc4_hdmi->hpd_gpio)) {
  2159			ret = PTR_ERR(vc4_hdmi->hpd_gpio);
  2160			goto err_put_ddc;
  2161		}
  2162	
  2163		vc4_hdmi->disable_wifi_frequencies =
  2164			of_property_read_bool(dev->of_node, "wifi-2.4ghz-coexistence");
  2165	
  2166		if (variant->max_pixel_clock == 600000000) {
  2167			struct vc4_dev *vc4 = to_vc4_dev(drm);
  2168			long max_rate = clk_round_rate(vc4->hvs->core_clk, 550000000);
  2169	
  2170			if (max_rate < 550000000)
  2171				vc4_hdmi->disable_4kp60 = true;
  2172		}
  2173	
  2174		/*
  2175		 * We need to have the device powered up at this point to call
  2176		 * our reset hook and for the CEC init.
  2177		 */
> 2178		ret = vc4_hdmi_runtime_resume(dev);
  2179		if (ret)
  2180			goto err_put_ddc;
  2181	
  2182		pm_runtime_get_noresume(dev);
  2183		pm_runtime_set_active(dev);
  2184		pm_runtime_enable(dev);
  2185	
  2186		if (vc4_hdmi->variant->reset)
  2187			vc4_hdmi->variant->reset(vc4_hdmi);
  2188	
  2189		if ((of_device_is_compatible(dev->of_node, "brcm,bcm2711-hdmi0") ||
  2190		     of_device_is_compatible(dev->of_node, "brcm,bcm2711-hdmi1")) &&
  2191		    HDMI_READ(HDMI_VID_CTL) & VC4_HD_VID_CTL_ENABLE) {
  2192			clk_prepare_enable(vc4_hdmi->pixel_clock);
  2193			clk_prepare_enable(vc4_hdmi->hsm_clock);
  2194			clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
  2195		}
  2196	
  2197		drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
  2198		drm_encoder_helper_add(encoder, &vc4_hdmi_encoder_helper_funcs);
  2199	
  2200		ret = vc4_hdmi_connector_init(drm, vc4_hdmi);
  2201		if (ret)
  2202			goto err_destroy_encoder;
  2203	
  2204		ret = vc4_hdmi_hotplug_init(vc4_hdmi);
  2205		if (ret)
  2206			goto err_destroy_conn;
  2207	
  2208		ret = vc4_hdmi_cec_init(vc4_hdmi);
  2209		if (ret)
  2210			goto err_destroy_conn;
  2211	
  2212		ret = vc4_hdmi_audio_init(vc4_hdmi);
  2213		if (ret)
  2214			goto err_free_cec;
  2215	
  2216		vc4_debugfs_add_file(drm, variant->debugfs_name,
  2217				     vc4_hdmi_debugfs_regs,
  2218				     vc4_hdmi);
  2219	
  2220		pm_runtime_put_sync(dev);
  2221	
  2222		return 0;
  2223	
  2224	err_free_cec:
  2225		vc4_hdmi_cec_exit(vc4_hdmi);
  2226	err_destroy_conn:
  2227		vc4_hdmi_connector_destroy(&vc4_hdmi->connector);
  2228	err_destroy_encoder:
  2229		drm_encoder_cleanup(encoder);
  2230		pm_runtime_put_sync(dev);
  2231		pm_runtime_disable(dev);
  2232	err_put_ddc:
  2233		put_device(&vc4_hdmi->ddc->dev);
  2234	
  2235		return ret;
  2236	}
  2237	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59722 bytes --]

Re: [PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind

[kernel test robot]

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

Hi Maxime,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-tip/drm-tip]
[also build test ERROR on drm-intel/for-linux-next next-20210707]
[cannot apply to anholt/for-next v5.13]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Maxime-Ripard/drm-vc4-hdmi-Fix-CEC-access-while-disabled/20210707-172621
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4342e12ac48418ce6366423771e887fa9fff89eb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Maxime-Ripard/drm-vc4-hdmi-Fix-CEC-access-while-disabled/20210707-172621
        git checkout 4342e12ac48418ce6366423771e887fa9fff89eb
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/vc4/vc4_hdmi.c: In function 'vc4_hdmi_bind':
>> drivers/gpu/drm/vc4/vc4_hdmi.c:2178:8: error: implicit declaration of function 'vc4_hdmi_runtime_resume'; did you mean 'pm_runtime_resume'? [-Werror=implicit-function-declaration]
    2178 |  ret = vc4_hdmi_runtime_resume(dev);
         |        ^~~~~~~~~~~~~~~~~~~~~~~
         |        pm_runtime_resume
   At top level:
   drivers/gpu/drm/vc4/vc4_hdmi.c:1402:46: warning: 'vc4_hdmi_audio_component_drv' defined but not used [-Wunused-const-variable=]
    1402 | static const struct snd_soc_component_driver vc4_hdmi_audio_component_drv = {
         |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +2178 drivers/gpu/drm/vc4/vc4_hdmi.c

  2110	
  2111	static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
  2112	{
  2113		const struct vc4_hdmi_variant *variant = of_device_get_match_data(dev);
  2114		struct platform_device *pdev = to_platform_device(dev);
  2115		struct drm_device *drm = dev_get_drvdata(master);
  2116		struct vc4_hdmi *vc4_hdmi;
  2117		struct drm_encoder *encoder;
  2118		struct device_node *ddc_node;
  2119		int ret;
  2120	
  2121		vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL);
  2122		if (!vc4_hdmi)
  2123			return -ENOMEM;
  2124		INIT_DELAYED_WORK(&vc4_hdmi->scrambling_work, vc4_hdmi_scrambling_wq);
  2125	
  2126		dev_set_drvdata(dev, vc4_hdmi);
  2127		encoder = &vc4_hdmi->encoder.base.base;
  2128		vc4_hdmi->encoder.base.type = variant->encoder_type;
  2129		vc4_hdmi->encoder.base.pre_crtc_configure = vc4_hdmi_encoder_pre_crtc_configure;
  2130		vc4_hdmi->encoder.base.pre_crtc_enable = vc4_hdmi_encoder_pre_crtc_enable;
  2131		vc4_hdmi->encoder.base.post_crtc_enable = vc4_hdmi_encoder_post_crtc_enable;
  2132		vc4_hdmi->encoder.base.post_crtc_disable = vc4_hdmi_encoder_post_crtc_disable;
  2133		vc4_hdmi->encoder.base.post_crtc_powerdown = vc4_hdmi_encoder_post_crtc_powerdown;
  2134		vc4_hdmi->pdev = pdev;
  2135		vc4_hdmi->variant = variant;
  2136	
  2137		ret = variant->init_resources(vc4_hdmi);
  2138		if (ret)
  2139			return ret;
  2140	
  2141		ddc_node = of_parse_phandle(dev->of_node, "ddc", 0);
  2142		if (!ddc_node) {
  2143			DRM_ERROR("Failed to find ddc node in device tree\n");
  2144			return -ENODEV;
  2145		}
  2146	
  2147		vc4_hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
  2148		of_node_put(ddc_node);
  2149		if (!vc4_hdmi->ddc) {
  2150			DRM_DEBUG("Failed to get ddc i2c adapter by node\n");
  2151			return -EPROBE_DEFER;
  2152		}
  2153	
  2154		/* Only use the GPIO HPD pin if present in the DT, otherwise
  2155		 * we'll use the HDMI core's register.
  2156		 */
  2157		vc4_hdmi->hpd_gpio = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN);
  2158		if (IS_ERR(vc4_hdmi->hpd_gpio)) {
  2159			ret = PTR_ERR(vc4_hdmi->hpd_gpio);
  2160			goto err_put_ddc;
  2161		}
  2162	
  2163		vc4_hdmi->disable_wifi_frequencies =
  2164			of_property_read_bool(dev->of_node, "wifi-2.4ghz-coexistence");
  2165	
  2166		if (variant->max_pixel_clock == 600000000) {
  2167			struct vc4_dev *vc4 = to_vc4_dev(drm);
  2168			long max_rate = clk_round_rate(vc4->hvs->core_clk, 550000000);
  2169	
  2170			if (max_rate < 550000000)
  2171				vc4_hdmi->disable_4kp60 = true;
  2172		}
  2173	
  2174		/*
  2175		 * We need to have the device powered up at this point to call
  2176		 * our reset hook and for the CEC init.
  2177		 */
> 2178		ret = vc4_hdmi_runtime_resume(dev);
  2179		if (ret)
  2180			goto err_put_ddc;
  2181	
  2182		pm_runtime_get_noresume(dev);
  2183		pm_runtime_set_active(dev);
  2184		pm_runtime_enable(dev);
  2185	
  2186		if (vc4_hdmi->variant->reset)
  2187			vc4_hdmi->variant->reset(vc4_hdmi);
  2188	
  2189		if ((of_device_is_compatible(dev->of_node, "brcm,bcm2711-hdmi0") ||
  2190		     of_device_is_compatible(dev->of_node, "brcm,bcm2711-hdmi1")) &&
  2191		    HDMI_READ(HDMI_VID_CTL) & VC4_HD_VID_CTL_ENABLE) {
  2192			clk_prepare_enable(vc4_hdmi->pixel_clock);
  2193			clk_prepare_enable(vc4_hdmi->hsm_clock);
  2194			clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
  2195		}
  2196	
  2197		drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
  2198		drm_encoder_helper_add(encoder, &vc4_hdmi_encoder_helper_funcs);
  2199	
  2200		ret = vc4_hdmi_connector_init(drm, vc4_hdmi);
  2201		if (ret)
  2202			goto err_destroy_encoder;
  2203	
  2204		ret = vc4_hdmi_hotplug_init(vc4_hdmi);
  2205		if (ret)
  2206			goto err_destroy_conn;
  2207	
  2208		ret = vc4_hdmi_cec_init(vc4_hdmi);
  2209		if (ret)
  2210			goto err_destroy_conn;
  2211	
  2212		ret = vc4_hdmi_audio_init(vc4_hdmi);
  2213		if (ret)
  2214			goto err_free_cec;
  2215	
  2216		vc4_debugfs_add_file(drm, variant->debugfs_name,
  2217				     vc4_hdmi_debugfs_regs,
  2218				     vc4_hdmi);
  2219	
  2220		pm_runtime_put_sync(dev);
  2221	
  2222		return 0;
  2223	
  2224	err_free_cec:
  2225		vc4_hdmi_cec_exit(vc4_hdmi);
  2226	err_destroy_conn:
  2227		vc4_hdmi_connector_destroy(&vc4_hdmi->connector);
  2228	err_destroy_encoder:
  2229		drm_encoder_cleanup(encoder);
  2230		pm_runtime_put_sync(dev);
  2231		pm_runtime_disable(dev);
  2232	err_put_ddc:
  2233		put_device(&vc4_hdmi->ddc->dev);
  2234	
  2235		return ret;
  2236	}
  2237	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 59722 bytes --]

Re: [PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind

[Dave Stevenson]

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
>

Re: [PATCH v2 1/5] drm/vc4: hdmi: Make sure the controller is powered up during bind

[Dave Stevenson]

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
>

Re: [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled

[Dave Stevenson]

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
>

Re: [PATCH v2 0/5] drm/vc4: hdmi: Fix CEC access while disabled

[Dave Stevenson]

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
>