[PATCH 0/2] drm/vc4: hdmi: Fixes for the RaspberryPi 0-3 stalls

[PATCH 0/2] drm/vc4: hdmi: Fixes for the RaspberryPi 0-3 stalls

[Maxime Ripard]

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 <daniel@ffwll.ch>
To: David Airlie <airlied@linux.ie>
To: Emma Anholt <emma@anholt.net>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Link: https://lore.kernel.org/dri-devel/20220922145448.w3xfywkn5ecak2et@pengutronix.de/
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---
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 <maxime@cerno.tech>

[PATCH 1/2] drm/vc4: hdmi: Enforce the minimum rate at runtime_resume

[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/20220922145448.w3xfywkn5ecak2et@pengutronix.de/
Fixes: fd5894fa2413 ("drm/vc4: hdmi: Remove clock rate initialization")
Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 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

Re: [PATCH 1/2] drm/vc4: hdmi: Enforce the minimum rate at runtime_resume

[Javier Martinez Canillas]

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/20220922145448.w3xfywkn5ecak2et@pengutronix.de/
> Fixes: fd5894fa2413 ("drm/vc4: hdmi: Remove clock rate initialization")
> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  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 <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: (subset) [PATCH 1/2] drm/vc4: hdmi: Enforce the minimum rate at runtime_resume

[Maxime Ripard]

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

Re: [PATCH 1/2] drm/vc4: hdmi: Enforce the minimum rate at runtime_resume

[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/20220922145448.w3xfywkn5ecak2et@pengutronix.de/
> Fixes: fd5894fa2413 ("drm/vc4: hdmi: Remove clock rate initialization")
> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   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;
>

Re: [PATCH 1/2] drm/vc4: hdmi: Enforce the minimum rate at runtime_resume

[Stefan Wahren]

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/20220922145448.w3xfywkn5ecak2et@pengutronix.de/
>> Fixes: fd5894fa2413 ("drm/vc4: hdmi: Remove clock rate initialization")
>> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>> ---
>>   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;
>>

Re: [PATCH 1/2] drm/vc4: hdmi: Enforce the minimum rate at runtime_resume

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2822 bytes --]

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/20220922145448.w3xfywkn5ecak2et@pengutronix.de/
> > > Fixes: fd5894fa2413 ("drm/vc4: hdmi: Remove clock rate initialization")
> > > Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > ---
> > >   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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 2/2] drm/vc4: hdmi: Check the HSM rate at runtime_resume

[Maxime Ripard]

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/20220922145448.w3xfywkn5ecak2et@pengutronix.de/
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 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

Re: [PATCH 2/2] drm/vc4: hdmi: Check the HSM rate at runtime_resume

[Javier Martinez Canillas]

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/20220922145448.w3xfywkn5ecak2et@pengutronix.de/
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: (subset) [PATCH 2/2] drm/vc4: hdmi: Check the HSM rate at runtime_resume

[Maxime Ripard]

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

Re: [PATCH 0/2] drm/vc4: hdmi: Fixes for the RaspberryPi 0-3 stalls

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/2] drm/vc4: hdmi: Fixes for the RaspberryPi 0-3 stalls

[Stefan Wahren]

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 <stefan.wahren@i2se.com>

>
> Thanks
> Maxime