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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Hi Maxime
On Wed, 7 Jul 2021 at 10:23, Maxime Ripard <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> + 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
>
Hi Maxime
On Wed, 7 Jul 2021 at 10:23, Maxime Ripard <[email protected]> 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 <[email protected]>
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
>