* [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-07 14:44 ` kernel test robot
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, 2 replies; 9+ 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: Nicolas Saenz Julienne, Emma Anholt, Tim Gover, Boris Brezillon,
Dave Stevenson, linux-kernel, bcm-kernel-feedback-list,
linux-rpi-kernel, Hans Verkuil, Phil Elwell, Dom Cobley
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] 9+ 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-07 14:44 ` kernel test robot
2021-07-28 13:50 ` Dave Stevenson
1 sibling, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-07-07 14:44 UTC (permalink / raw)
To: Maxime Ripard, dri-devel, Daniel Vetter, David Airlie,
Maarten Lankhorst, Thomas Zimmermann
Cc: Boris Brezillon, kbuild-all, Phil Elwell, Emma Anholt, Dave Stevenson
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ 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-07 14:44 ` kernel test robot
@ 2021-07-28 13:50 ` Dave Stevenson
1 sibling, 0 replies; 9+ messages in thread
From: Dave Stevenson @ 2021-07-28 13:50 UTC (permalink / raw)
To: Maxime Ripard
Cc: linux-rpi-kernel, Nicolas Saenz Julienne, Tim Gover, Emma Anholt,
Boris Brezillon, David Airlie, LKML, DRI Development,
bcm-kernel-feedback-list, Thomas Zimmermann, Hans Verkuil,
Daniel Vetter, Phil Elwell, Dom Cobley
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] 9+ 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; 9+ 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: Nicolas Saenz Julienne, Emma Anholt, Tim Gover, Boris Brezillon,
Dave Stevenson, linux-kernel, bcm-kernel-feedback-list,
linux-rpi-kernel, Hans Verkuil, Phil Elwell, Dom Cobley
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] 9+ 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; 9+ 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: Nicolas Saenz Julienne, Emma Anholt, Tim Gover, Boris Brezillon,
Dave Stevenson, linux-kernel, bcm-kernel-feedback-list,
linux-rpi-kernel, Hans Verkuil, Phil Elwell, Dom Cobley
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] 9+ 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; 9+ 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: Nicolas Saenz Julienne, Emma Anholt, Tim Gover, Boris Brezillon,
Dave Stevenson, linux-kernel, bcm-kernel-feedback-list,
linux-rpi-kernel, Hans Verkuil, Phil Elwell, Dom Cobley
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] 9+ 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; 9+ 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: Nicolas Saenz Julienne, Emma Anholt, Tim Gover, Boris Brezillon,
Dave Stevenson, linux-kernel, bcm-kernel-feedback-list,
linux-rpi-kernel, Hans Verkuil, Phil Elwell, Dom Cobley
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] 9+ 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; 9+ messages in thread
From: Dave Stevenson @ 2021-07-28 13:52 UTC (permalink / raw)
To: Maxime Ripard
Cc: linux-rpi-kernel, Nicolas Saenz Julienne, Tim Gover, Emma Anholt,
Boris Brezillon, David Airlie, LKML, DRI Development,
bcm-kernel-feedback-list, Thomas Zimmermann, Hans Verkuil,
Daniel Vetter, Phil Elwell, Dom Cobley
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] 9+ messages in thread