Hi,
Here's a series addressing the current bug that has been reported for the
RaspberryPi3, where booting without an HDMI monitor attached and with vc4
compiled as a module will lead to a stuck system when the module is loaded.
This is due to the HSM clock not being initialized by anyone, and thus not
being functional once we start accessing the HDMI registers.
The first patch will fix this, and the second will make sure we avoid that
situation entirely in the future. This has been tested with 5.19.12. Earlier
versions might need a backport of 88110a9f6209 ("clk: bcm2835: fix
bcm2835_clock_choose_div").
Let me know what you think,
Maxime
To: Daniel Vetter <[email protected]>
To: David Airlie <[email protected]>
To: Emma Anholt <[email protected]>
To: Maarten Lankhorst <[email protected]>
To: Maxime Ripard <[email protected]>
To: Thomas Zimmermann <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Marc Kleine-Budde <[email protected]>
Cc: Stefan Wahren <[email protected]>
Link: https://lore.kernel.org/dri-devel/[email protected]/
Signed-off-by: Maxime Ripard <[email protected]>
---
Maxime Ripard (2):
drm/vc4: hdmi: Enforce the minimum rate at runtime_resume
drm/vc4: hdmi: Check the HSM rate at runtime_resume
drivers/gpu/drm/vc4/vc4_hdmi.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
---
base-commit: 58df6af8cea3c5377c0220d9fb47cbf85a216f54
change-id: 20220929-rpi-pi3-unplugged-fixes-5d61ea2cc383
Best regards,
--
Maxime Ripard <[email protected]>
This is a revert of commit fd5894fa2413 ("drm/vc4: hdmi: Remove clock
rate initialization"), with the code slightly moved around.
It turns out that we can't downright remove that code from the driver,
since the Pi0-3 and Pi4 are in different cases, and it only works for
the Pi4.
Indeed, the commit mentioned above was relying on the RaspberryPi
firmware clocks driver to initialize the rate if it wasn't done by the
firmware. However, the Pi0-3 are using the clk-bcm2835 clock driver that
wasn't doing this initialization. We therefore end up with the clock not
being assigned a rate, and the CPU stalling when trying to access a
register.
We can't move that initialization in the clk-bcm2835 driver, since the
HSM clock we depend on is actually part of the HDMI power domain, so any
rate setup is only valid when the power domain is enabled. Thus, we
reinstated the minimum rate setup at runtime_suspend, which should
address both issues.
Link: https://lore.kernel.org/dri-devel/[email protected]/
Fixes: fd5894fa2413 ("drm/vc4: hdmi: Remove clock rate initialization")
Reported-by: Marc Kleine-Budde <[email protected]>
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 199bc398817f..2e28fe16ed5e 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2891,6 +2891,15 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
u32 __maybe_unused value;
int ret;
+ /*
+ * The HSM clock is in the HDMI power domain, so we need to set
+ * its frequency while the power domain is active so that it
+ * keeps its rate.
+ */
+ ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ);
+ if (ret)
+ return ret;
+
ret = clk_prepare_enable(vc4_hdmi->hsm_clock);
if (ret)
return ret;
--
b4 0.10.1
If our HSM clock has not been properly initialized, any register access
will silently lock up the system.
Let's check that this can't happen by adding a check for the rate before
any register access, and error out otherwise.
Link: https://lore.kernel.org/dri-devel/[email protected]/
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 2e28fe16ed5e..eb3aaaca2b80 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2889,6 +2889,7 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
unsigned long __maybe_unused flags;
u32 __maybe_unused value;
+ unsigned long rate;
int ret;
/*
@@ -2904,6 +2905,21 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
if (ret)
return ret;
+ /*
+ * Whenever the RaspberryPi boots without an HDMI monitor
+ * plugged in, the firmware won't have initialized the HSM clock
+ * rate and it will be reported as 0.
+ *
+ * If we try to access a register of the controller in such a
+ * case, it will lead to a silent CPU stall. Let's make sure we
+ * prevent such a case.
+ */
+ rate = clk_get_rate(vc4_hdmi->hsm_clock);
+ if (!rate) {
+ ret = -EINVAL;
+ goto err_disable_clk;
+ }
+
if (vc4_hdmi->variant->reset)
vc4_hdmi->variant->reset(vc4_hdmi);
@@ -2925,6 +2941,10 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
#endif
return 0;
+
+err_disable_clk:
+ clk_disable_unprepare(vc4_hdmi->hsm_clock);
+ return ret;
}
static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
--
b4 0.10.1
Hi Mark, Stefan,
On Thu, Sep 29, 2022 at 11:21:16AM +0200, Maxime Ripard wrote:
> Here's a series addressing the current bug that has been reported for the
> RaspberryPi3, where booting without an HDMI monitor attached and with vc4
> compiled as a module will lead to a stuck system when the module is loaded.
>
> This is due to the HSM clock not being initialized by anyone, and thus not
> being functional once we start accessing the HDMI registers.
>
> The first patch will fix this, and the second will make sure we avoid that
> situation entirely in the future. This has been tested with 5.19.12. Earlier
> versions might need a backport of 88110a9f6209 ("clk: bcm2835: fix
> bcm2835_clock_choose_div").
Could you test/review this?
Thanks
Maxime
Am 10.10.22 um 10:50 schrieb Maxime Ripard:
> Hi Mark, Stefan,
>
> On Thu, Sep 29, 2022 at 11:21:16AM +0200, Maxime Ripard wrote:
>> Here's a series addressing the current bug that has been reported for the
>> RaspberryPi3, where booting without an HDMI monitor attached and with vc4
>> compiled as a module will lead to a stuck system when the module is loaded.
>>
>> This is due to the HSM clock not being initialized by anyone, and thus not
>> being functional once we start accessing the HDMI registers.
>>
>> The first patch will fix this, and the second will make sure we avoid that
>> situation entirely in the future. This has been tested with 5.19.12. Earlier
>> versions might need a backport of 88110a9f6209 ("clk: bcm2835: fix
>> bcm2835_clock_choose_div").
> Could you test/review this?
Looks good to me:
Tested-by: Stefan Wahren <[email protected]>
>
> Thanks
> Maxime
Hello Maxime,
On 9/29/22 11:21, Maxime Ripard wrote:
> This is a revert of commit fd5894fa2413 ("drm/vc4: hdmi: Remove clock
> rate initialization"), with the code slightly moved around.
>
> It turns out that we can't downright remove that code from the driver,
> since the Pi0-3 and Pi4 are in different cases, and it only works for
> the Pi4.
>
> Indeed, the commit mentioned above was relying on the RaspberryPi
> firmware clocks driver to initialize the rate if it wasn't done by the
> firmware. However, the Pi0-3 are using the clk-bcm2835 clock driver that
> wasn't doing this initialization. We therefore end up with the clock not
> being assigned a rate, and the CPU stalling when trying to access a
> register.
>
> We can't move that initialization in the clk-bcm2835 driver, since the
> HSM clock we depend on is actually part of the HDMI power domain, so any
> rate setup is only valid when the power domain is enabled. Thus, we
> reinstated the minimum rate setup at runtime_suspend, which should
> address both issues.
>
> Link: https://lore.kernel.org/dri-devel/[email protected]/
> Fixes: fd5894fa2413 ("drm/vc4: hdmi: Remove clock rate initialization")
> Reported-by: Marc Kleine-Budde <[email protected]>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> drivers/gpu/drm/vc4/vc4_hdmi.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 199bc398817f..2e28fe16ed5e 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -2891,6 +2891,15 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
> u32 __maybe_unused value;
> int ret;
>
> + /*
> + * The HSM clock is in the HDMI power domain, so we need to set
> + * its frequency while the power domain is active so that it
> + * keeps its rate.
> + */
> + ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ);
> + if (ret)
> + return ret;
> +
I'm not familiar with VC4 but your commit message has a great explanation
of the issue and the code is doing what you mention there. So this patch
looks good to me.
Reviewed-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On 9/29/22 11:21, Maxime Ripard wrote:
> If our HSM clock has not been properly initialized, any register access
> will silently lock up the system.
>
> Let's check that this can't happen by adding a check for the rate before
> any register access, and error out otherwise.
>
> Link: https://lore.kernel.org/dri-devel/[email protected]/
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
Reviewed-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On Thu, 29 Sep 2022 11:21:17 +0200, Maxime Ripard wrote:
> This is a revert of commit fd5894fa2413 ("drm/vc4: hdmi: Remove clock
> rate initialization"), with the code slightly moved around.
>
> It turns out that we can't downright remove that code from the driver,
> since the Pi0-3 and Pi4 are in different cases, and it only works for
> the Pi4.
>
> [...]
Applied to drm/drm-misc (drm-misc-fixes).
Thanks!
Maxime
On Thu, 29 Sep 2022 11:21:18 +0200, Maxime Ripard wrote:
> If our HSM clock has not been properly initialized, any register access
> will silently lock up the system.
>
> Let's check that this can't happen by adding a check for the rate before
> any register access, and error out otherwise.
>
>
> [...]
Applied to drm/drm-misc (drm-misc-fixes).
Thanks!
Maxime
Hi Maxime,
Am 29.09.22 um 11:21 schrieb Maxime Ripard:
> This is a revert of commit fd5894fa2413 ("drm/vc4: hdmi: Remove clock
> rate initialization"), with the code slightly moved around.
>
> It turns out that we can't downright remove that code from the driver,
> since the Pi0-3 and Pi4 are in different cases, and it only works for
> the Pi4.
>
> Indeed, the commit mentioned above was relying on the RaspberryPi
> firmware clocks driver to initialize the rate if it wasn't done by the
> firmware. However, the Pi0-3 are using the clk-bcm2835 clock driver that
> wasn't doing this initialization. We therefore end up with the clock not
> being assigned a rate, and the CPU stalling when trying to access a
> register.
>
> We can't move that initialization in the clk-bcm2835 driver, since the
> HSM clock we depend on is actually part of the HDMI power domain, so any
> rate setup is only valid when the power domain is enabled. Thus, we
> reinstated the minimum rate setup at runtime_suspend, which should
> address both issues.
>
> Link: https://lore.kernel.org/dri-devel/[email protected]/
> Fixes: fd5894fa2413 ("drm/vc4: hdmi: Remove clock rate initialization")
> Reported-by: Marc Kleine-Budde <[email protected]>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> drivers/gpu/drm/vc4/vc4_hdmi.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 199bc398817f..2e28fe16ed5e 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -2891,6 +2891,15 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
> u32 __maybe_unused value;
> int ret;
>
> + /*
> + * The HSM clock is in the HDMI power domain, so we need to set
> + * its frequency while the power domain is active so that it
> + * keeps its rate.
> + */
> + ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ);
> + if (ret)
> + return ret;
> +
unfortunately this breaks X on Raspberry Pi 4 in Linux 6.0.5
(multi_v7_defconfig + LPAE). Today i saw this report [1] and bisected
the issue down to this patch. Shame on me that i only tested this patch
with Rpi 3B+ :-(
Best regards
[1] - https://bugzilla.suse.com/show_bug.cgi?id=1205259
> ret = clk_prepare_enable(vc4_hdmi->hsm_clock);
> if (ret)
> return ret;
>
Am 11.11.22 um 22:08 schrieb Stefan Wahren:
> Hi Maxime,
>
> Am 29.09.22 um 11:21 schrieb Maxime Ripard:
>> This is a revert of commit fd5894fa2413 ("drm/vc4: hdmi: Remove clock
>> rate initialization"), with the code slightly moved around.
>>
>> It turns out that we can't downright remove that code from the driver,
>> since the Pi0-3 and Pi4 are in different cases, and it only works for
>> the Pi4.
>>
>> Indeed, the commit mentioned above was relying on the RaspberryPi
>> firmware clocks driver to initialize the rate if it wasn't done by the
>> firmware. However, the Pi0-3 are using the clk-bcm2835 clock driver that
>> wasn't doing this initialization. We therefore end up with the clock not
>> being assigned a rate, and the CPU stalling when trying to access a
>> register.
>>
>> We can't move that initialization in the clk-bcm2835 driver, since the
>> HSM clock we depend on is actually part of the HDMI power domain, so any
>> rate setup is only valid when the power domain is enabled. Thus, we
>> reinstated the minimum rate setup at runtime_suspend, which should
>> address both issues.
>>
>> Link:
>> https://lore.kernel.org/dri-devel/[email protected]/
>> Fixes: fd5894fa2413 ("drm/vc4: hdmi: Remove clock rate initialization")
>> Reported-by: Marc Kleine-Budde <[email protected]>
>> Signed-off-by: Maxime Ripard <[email protected]>
>> ---
>> drivers/gpu/drm/vc4/vc4_hdmi.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> index 199bc398817f..2e28fe16ed5e 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> @@ -2891,6 +2891,15 @@ static int vc4_hdmi_runtime_resume(struct
>> device *dev)
>> u32 __maybe_unused value;
>> int ret;
>> + /*
>> + * The HSM clock is in the HDMI power domain, so we need to set
>> + * its frequency while the power domain is active so that it
>> + * keeps its rate.
>> + */
>> + ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ);
>> + if (ret)
>> + return ret;
>> +
>
> unfortunately this breaks X on Raspberry Pi 4 in Linux 6.0.5
> (multi_v7_defconfig + LPAE). Today i saw this report [1] and bisected
> the issue down to this patch. Shame on me that i only tested this
> patch with Rpi 3B+ :-(
Looks like "drm/vc4: hdmi: Fix HSM clock too low on Pi4" addresses this
issue ...
>
> Best regards
>
> [1] - https://bugzilla.suse.com/show_bug.cgi?id=1205259
>
>> ret = clk_prepare_enable(vc4_hdmi->hsm_clock);
>> if (ret)
>> return ret;
>>
Hi Stefan,
On Mon, Nov 14, 2022 at 01:48:14AM +0100, Stefan Wahren wrote:
> Am 11.11.22 um 22:08 schrieb Stefan Wahren:
> > Hi Maxime,
> >
> > Am 29.09.22 um 11:21 schrieb Maxime Ripard:
> > > This is a revert of commit fd5894fa2413 ("drm/vc4: hdmi: Remove clock
> > > rate initialization"), with the code slightly moved around.
> > >
> > > It turns out that we can't downright remove that code from the driver,
> > > since the Pi0-3 and Pi4 are in different cases, and it only works for
> > > the Pi4.
> > >
> > > Indeed, the commit mentioned above was relying on the RaspberryPi
> > > firmware clocks driver to initialize the rate if it wasn't done by the
> > > firmware. However, the Pi0-3 are using the clk-bcm2835 clock driver that
> > > wasn't doing this initialization. We therefore end up with the clock not
> > > being assigned a rate, and the CPU stalling when trying to access a
> > > register.
> > >
> > > We can't move that initialization in the clk-bcm2835 driver, since the
> > > HSM clock we depend on is actually part of the HDMI power domain, so any
> > > rate setup is only valid when the power domain is enabled. Thus, we
> > > reinstated the minimum rate setup at runtime_suspend, which should
> > > address both issues.
> > >
> > > Link: https://lore.kernel.org/dri-devel/[email protected]/
> > > Fixes: fd5894fa2413 ("drm/vc4: hdmi: Remove clock rate initialization")
> > > Reported-by: Marc Kleine-Budde <[email protected]>
> > > Signed-off-by: Maxime Ripard <[email protected]>
> > > ---
> > > ? drivers/gpu/drm/vc4/vc4_hdmi.c | 9 +++++++++
> > > ? 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > index 199bc398817f..2e28fe16ed5e 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > @@ -2891,6 +2891,15 @@ static int vc4_hdmi_runtime_resume(struct
> > > device *dev)
> > > ????? u32 __maybe_unused value;
> > > ????? int ret;
> > > ? +??? /*
> > > +???? * The HSM clock is in the HDMI power domain, so we need to set
> > > +???? * its frequency while the power domain is active so that it
> > > +???? * keeps its rate.
> > > +???? */
> > > +??? ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ);
> > > +??? if (ret)
> > > +??????? return ret;
> > > +
> >
> > unfortunately this breaks X on Raspberry Pi 4 in Linux 6.0.5
> > (multi_v7_defconfig + LPAE). Today i saw this report [1] and bisected
> > the issue down to this patch. Shame on me that i only tested this patch
> > with Rpi 3B+ :-(
>
> Looks like "drm/vc4: hdmi: Fix HSM clock too low on Pi4" addresses this
> issue ...
Yes, indeed. The fix should be on its way to -stable already
Maxime