If the of_get_named_gpio_flags call fails in vc4_hdmi_bind, we jump to the err_unprepare_hsm label. That label will then call pm_runtime_disable and put_device on the DDC device.
We just retrieved the DDC device, so the latter is definitely justified. However at that point we still haven't called pm_runtime_enable, so the call to pm_runtime_disable is not supposed to be there.
Fixes: 10ee275cb12f ("drm/vc4: prepare for CEC support") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index c27b287d2053..ccc6c8079dc6 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2039,7 +2039,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) &hpd_gpio_flags); if (vc4_hdmi->hpd_gpio < 0) { ret = vc4_hdmi->hpd_gpio; - goto err_unprepare_hsm; + goto err_put_ddc; }
vc4_hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW; @@ -2080,8 +2080,8 @@ 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); -err_unprepare_hsm: pm_runtime_disable(dev); +err_put_ddc: put_device(&vc4_hdmi->ddc->dev);
return ret;
The new gpiod interface takes care of parsing the GPIO flags and to return the logical value when accessing an active-low GPIO, so switching to it simplifies a lot the driver.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 24 +++++++----------------- drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +-- 2 files changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index ccc6c8079dc6..34622c59f6a7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -159,10 +159,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); bool connected = false;
- if (vc4_hdmi->hpd_gpio) { - if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ - vc4_hdmi->hpd_active_low) - connected = true; + if (vc4_hdmi->hpd_gpio && + gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) { + connected = true; } else if (drm_probe_ddc(vc4_hdmi->ddc)) { connected = true; } else if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) { @@ -1993,7 +1992,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) struct vc4_hdmi *vc4_hdmi; struct drm_encoder *encoder; struct device_node *ddc_node; - u32 value; int ret;
vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL); @@ -2031,18 +2029,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) /* Only use the GPIO HPD pin if present in the DT, otherwise * we'll use the HDMI core's register. */ - if (of_find_property(dev->of_node, "hpd-gpios", &value)) { - enum of_gpio_flags hpd_gpio_flags; - - vc4_hdmi->hpd_gpio = of_get_named_gpio_flags(dev->of_node, - "hpd-gpios", 0, - &hpd_gpio_flags); - if (vc4_hdmi->hpd_gpio < 0) { - ret = vc4_hdmi->hpd_gpio; - goto err_put_ddc; - } - - vc4_hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW; + vc4_hdmi->hpd_gpio = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN); + if (IS_ERR(vc4_hdmi->hpd_gpio)) { + ret = PTR_ERR(vc4_hdmi->hpd_gpio); + goto err_put_ddc; }
vc4_hdmi->disable_wifi_frequencies = diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 060bcaefbeb5..2688a55461d6 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -146,8 +146,7 @@ struct vc4_hdmi { /* VC5 Only */ void __iomem *rm_regs;
- int hpd_gpio; - bool hpd_active_low; + struct gpio_desc *hpd_gpio;
/* * On some systems (like the RPi4), some modes are in the same
On Mon, May 24, 2021 at 3:19 PM Maxime Ripard maxime@cerno.tech wrote:
The new gpiod interface takes care of parsing the GPIO flags and to return the logical value when accessing an active-low GPIO, so switching to it simplifies a lot the driver.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Thanks for fixing this! Reviewed-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
Hi Linus
On Fri, May 28, 2021 at 01:57:56AM +0200, Linus Walleij wrote:
On Mon, May 24, 2021 at 3:19 PM Maxime Ripard maxime@cerno.tech wrote:
The new gpiod interface takes care of parsing the GPIO flags and to return the logical value when accessing an active-low GPIO, so switching to it simplifies a lot the driver.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Thanks for fixing this! Reviewed-by: Linus Walleij linus.walleij@linaro.org
Is it for both patches or just this one?
Thanks! Maxime
On Fri, Jun 4, 2021 at 10:01 AM Maxime Ripard maxime@cerno.tech wrote:
On Fri, May 28, 2021 at 01:57:56AM +0200, Linus Walleij wrote:
On Mon, May 24, 2021 at 3:19 PM Maxime Ripard maxime@cerno.tech wrote:
The new gpiod interface takes care of parsing the GPIO flags and to return the logical value when accessing an active-low GPIO, so switching to it simplifies a lot the driver.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Thanks for fixing this! Reviewed-by: Linus Walleij linus.walleij@linaro.org
Is it for both patches or just this one?
I went and added Reviewed-by: to 1/2 as well so you can merge the patches. Was simple enough even though I'm not a VC4 expert.
Yours, Linus Walleij
On Fri, Jun 04, 2021 at 11:45:36PM +0200, Linus Walleij wrote:
On Fri, Jun 4, 2021 at 10:01 AM Maxime Ripard maxime@cerno.tech wrote:
On Fri, May 28, 2021 at 01:57:56AM +0200, Linus Walleij wrote:
On Mon, May 24, 2021 at 3:19 PM Maxime Ripard maxime@cerno.tech wrote:
The new gpiod interface takes care of parsing the GPIO flags and to return the logical value when accessing an active-low GPIO, so switching to it simplifies a lot the driver.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Thanks for fixing this! Reviewed-by: Linus Walleij linus.walleij@linaro.org
Is it for both patches or just this one?
I went and added Reviewed-by: to 1/2 as well so you can merge the patches. Was simple enough even though I'm not a VC4 expert.
Thanks :)
Applied both
Maxime
On Mon, May 24, 2021 at 03:18:52PM +0200, Maxime Ripard wrote:
The new gpiod interface takes care of parsing the GPIO flags and to return the logical value when accessing an active-low GPIO, so switching to it simplifies a lot the driver.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 24 +++++++----------------- drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +-- 2 files changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index ccc6c8079dc6..34622c59f6a7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -159,10 +159,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); bool connected = false;
- if (vc4_hdmi->hpd_gpio) {
if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^vc4_hdmi->hpd_active_low)connected = true;
- if (vc4_hdmi->hpd_gpio &&
gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) {connected = true;@@ -1993,7 +1992,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) struct vc4_hdmi *vc4_hdmi; struct drm_encoder *encoder; struct device_node *ddc_node;
u32 value; int ret;
vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL);
@@ -2031,18 +2029,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) /* Only use the GPIO HPD pin if present in the DT, otherwise * we'll use the HDMI core's register. */
- if (of_find_property(dev->of_node, "hpd-gpios", &value)) {
enum of_gpio_flags hpd_gpio_flags;vc4_hdmi->hpd_gpio = of_get_named_gpio_flags(dev->of_node,"hpd-gpios", 0,&hpd_gpio_flags);if (vc4_hdmi->hpd_gpio < 0) {ret = vc4_hdmi->hpd_gpio;goto err_put_ddc;}vc4_hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW;
vc4_hdmi->hpd_gpio = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN);
if (IS_ERR(vc4_hdmi->hpd_gpio)) {
ret = PTR_ERR(vc4_hdmi->hpd_gpio);goto err_put_ddc;}
vc4_hdmi->disable_wifi_frequencies =
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 060bcaefbeb5..2688a55461d6 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -146,8 +146,7 @@ struct vc4_hdmi { /* VC5 Only */ void __iomem *rm_regs;
- int hpd_gpio;
- bool hpd_active_low;
struct gpio_desc *hpd_gpio;
/*
- On some systems (like the RPi4), some modes are in the same
-- 2.31.1
Hi Maxime,
This patch as commit 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod") causes my Raspberry Pi 3 to lock up shortly after boot in combination with commit 411efa18e4b0 ("drm/vc4: hdmi: Move the HSM clock enable to runtime_pm"). The serial console and ssh are completely unresponsive and I do not see any messages in dmesg with "debug ignore_loglevel". The device is running with a 32-bit kernel (multi_v7_defconfig) with 32-bit userspace. If there is any further information that I can provide, please let me know.
Cheers, Nathan
Hi Nathan,
On Thu, Jul 01, 2021 at 08:29:34PM -0700, Nathan Chancellor wrote:
On Mon, May 24, 2021 at 03:18:52PM +0200, Maxime Ripard wrote:
The new gpiod interface takes care of parsing the GPIO flags and to return the logical value when accessing an active-low GPIO, so switching to it simplifies a lot the driver.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 24 +++++++----------------- drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +-- 2 files changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index ccc6c8079dc6..34622c59f6a7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -159,10 +159,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); bool connected = false;
- if (vc4_hdmi->hpd_gpio) {
if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^vc4_hdmi->hpd_active_low)connected = true;
- if (vc4_hdmi->hpd_gpio &&
gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) {connected = true;@@ -1993,7 +1992,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) struct vc4_hdmi *vc4_hdmi; struct drm_encoder *encoder; struct device_node *ddc_node;
u32 value; int ret;
vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL);
@@ -2031,18 +2029,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) /* Only use the GPIO HPD pin if present in the DT, otherwise * we'll use the HDMI core's register. */
- if (of_find_property(dev->of_node, "hpd-gpios", &value)) {
enum of_gpio_flags hpd_gpio_flags;vc4_hdmi->hpd_gpio = of_get_named_gpio_flags(dev->of_node,"hpd-gpios", 0,&hpd_gpio_flags);if (vc4_hdmi->hpd_gpio < 0) {ret = vc4_hdmi->hpd_gpio;goto err_put_ddc;}vc4_hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW;
vc4_hdmi->hpd_gpio = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN);
if (IS_ERR(vc4_hdmi->hpd_gpio)) {
ret = PTR_ERR(vc4_hdmi->hpd_gpio);goto err_put_ddc;}
vc4_hdmi->disable_wifi_frequencies =
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 060bcaefbeb5..2688a55461d6 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -146,8 +146,7 @@ struct vc4_hdmi { /* VC5 Only */ void __iomem *rm_regs;
- int hpd_gpio;
- bool hpd_active_low;
struct gpio_desc *hpd_gpio;
/*
- On some systems (like the RPi4), some modes are in the same
-- 2.31.1
This patch as commit 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod") causes my Raspberry Pi 3 to lock up shortly after boot in combination with commit 411efa18e4b0 ("drm/vc4: hdmi: Move the HSM clock enable to runtime_pm"). The serial console and ssh are completely unresponsive and I do not see any messages in dmesg with "debug ignore_loglevel". The device is running with a 32-bit kernel (multi_v7_defconfig) with 32-bit userspace. If there is any further information that I can provide, please let me know.
Thanks for reporting this. The same bug has been reported on wednesday on the RPi repo here: https://github.com/raspberrypi/linux/pull/4418
More specifically, this commit should fix it: https://github.com/raspberrypi/linux/pull/4418/commits/6d404373c20a794da3d6a...
Even though it's based on the 5.10 kernel, it should apply without any warning on a mainline tree. Let me know if it fixes your issue too
Maxime
On Fri, Jul 02, 2021 at 03:16:46PM +0200, Maxime Ripard wrote:
Hi Nathan,
On Thu, Jul 01, 2021 at 08:29:34PM -0700, Nathan Chancellor wrote:
On Mon, May 24, 2021 at 03:18:52PM +0200, Maxime Ripard wrote:
The new gpiod interface takes care of parsing the GPIO flags and to return the logical value when accessing an active-low GPIO, so switching to it simplifies a lot the driver.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 24 +++++++----------------- drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +-- 2 files changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index ccc6c8079dc6..34622c59f6a7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -159,10 +159,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); bool connected = false;
- if (vc4_hdmi->hpd_gpio) {
if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^vc4_hdmi->hpd_active_low)connected = true;
- if (vc4_hdmi->hpd_gpio &&
gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) {connected = true;@@ -1993,7 +1992,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) struct vc4_hdmi *vc4_hdmi; struct drm_encoder *encoder; struct device_node *ddc_node;
u32 value; int ret;
vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL);
@@ -2031,18 +2029,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) /* Only use the GPIO HPD pin if present in the DT, otherwise * we'll use the HDMI core's register. */
- if (of_find_property(dev->of_node, "hpd-gpios", &value)) {
enum of_gpio_flags hpd_gpio_flags;vc4_hdmi->hpd_gpio = of_get_named_gpio_flags(dev->of_node,"hpd-gpios", 0,&hpd_gpio_flags);if (vc4_hdmi->hpd_gpio < 0) {ret = vc4_hdmi->hpd_gpio;goto err_put_ddc;}vc4_hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW;
vc4_hdmi->hpd_gpio = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN);
if (IS_ERR(vc4_hdmi->hpd_gpio)) {
ret = PTR_ERR(vc4_hdmi->hpd_gpio);goto err_put_ddc;}
vc4_hdmi->disable_wifi_frequencies =
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 060bcaefbeb5..2688a55461d6 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -146,8 +146,7 @@ struct vc4_hdmi { /* VC5 Only */ void __iomem *rm_regs;
- int hpd_gpio;
- bool hpd_active_low;
struct gpio_desc *hpd_gpio;
/*
- On some systems (like the RPi4), some modes are in the same
-- 2.31.1
This patch as commit 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod") causes my Raspberry Pi 3 to lock up shortly after boot in combination with commit 411efa18e4b0 ("drm/vc4: hdmi: Move the HSM clock enable to runtime_pm"). The serial console and ssh are completely unresponsive and I do not see any messages in dmesg with "debug ignore_loglevel". The device is running with a 32-bit kernel (multi_v7_defconfig) with 32-bit userspace. If there is any further information that I can provide, please let me know.
Thanks for reporting this. The same bug has been reported on wednesday on the RPi repo here: https://github.com/raspberrypi/linux/pull/4418
More specifically, this commit should fix it: https://github.com/raspberrypi/linux/pull/4418/commits/6d404373c20a794da3d6a...
Even though it's based on the 5.10 kernel, it should apply without any warning on a mainline tree. Let me know if it fixes your issue too
Thank you for the links and the quick reply. Unfortunately, I applied this patch on top of commit d6b63b5b7d7f ("Merge tag 'sound-5.14-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound") in mainline, which does reproduce this issue still and it did not fix the issue. In fact, it did not even get to the raspberrypi login prompt before it locked up, yet again without any real output in the serial console except for maybe this message?
[ 7.582480] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
If there is anything further that I can provide or test, please let me know!
Cheers, Nathan
On Mon, May 24, 2021 at 3:19 PM Maxime Ripard maxime@cerno.tech wrote:
If the of_get_named_gpio_flags call fails in vc4_hdmi_bind, we jump to the err_unprepare_hsm label. That label will then call pm_runtime_disable and put_device on the DDC device.
We just retrieved the DDC device, so the latter is definitely justified. However at that point we still haven't called pm_runtime_enable, so the call to pm_runtime_disable is not supposed to be there.
Fixes: 10ee275cb12f ("drm/vc4: prepare for CEC support") Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
dri-devel@lists.freedesktop.org
-
Linus Walleij -
Maxime Ripard -
Nathan Chancellor