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(-)
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);
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-... 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/4342e12ac48418ce6366423771e887fa9fff... 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
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
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,
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);
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; }
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);
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
dri-devel@lists.freedesktop.org
-
Dave Stevenson -
kernel test robot -
Maxime Ripard