[PATCH v1 0/7] drm/vc4: Fix the core clock behaviour

[PATCH v1 0/7] drm/vc4: Fix the core clock behaviour

[Maxime Ripard]

Hi,

Those patches used to be part of a larger clock fixes series:
https://lore.kernel.org/linux-clk/20220715160014.2623107-1-maxime@cerno.tech/

However, that series doesn't seem to be getting anywhere, so I've split out
these patches that fix a regression that has been there since 5.18 and that
prevents the 4k output from working on the RaspberryPi4.

Hopefully, we will be able to merge those patches through the DRM tree to avoid
any further disruption.

Let me know what you think,
Maxime

---
Dom Cobley (1):
      drm/vc4: hdmi: Add more checks for 4k resolutions

Maxime Ripard (6):
      clk: bcm: rpi: Create helper to retrieve private data
      clk: bcm: rpi: Add a function to retrieve the maximum
      clk: bcm: rpi: Add a function to retrieve the minimum
      drm/vc4: hdmi: Fix hdmi_enable_4kp60 detection
      drm/vc4: hdmi: Rework hdmi_enable_4kp60 detection code
      drm/vc4: Make sure we don't end up with a core clock too high

 drivers/clk/bcm/clk-raspberrypi.c        | 73 ++++++++++++++++++++++++++++----
 drivers/gpu/drm/vc4/vc4_drv.h            | 14 ++++++
 drivers/gpu/drm/vc4/vc4_hdmi.c           | 25 +++++------
 drivers/gpu/drm/vc4/vc4_hdmi.h           |  8 ----
 drivers/gpu/drm/vc4/vc4_hvs.c            | 13 ++++++
 drivers/gpu/drm/vc4/vc4_kms.c            | 17 +++++---
 include/soc/bcm2835/raspberrypi-clocks.h | 21 +++++++++
 7 files changed, 138 insertions(+), 33 deletions(-)
---
base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
change-id: 20220815-rpi-fix-4k-60-17273650429d

Best regards,
-- 
Maxime Ripard <maxime@cerno.tech>

[PATCH v1 1/7] clk: bcm: rpi: Create helper to retrieve private data

[Maxime Ripard]

The RaspberryPi firmware clocks driver uses in several instances a
container_of to retrieve the struct raspberrypi_clk_data from a pointer
to struct clk_hw. Let's create a small function to avoid duplicating it
all over the place.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 73518009a0f2..6c0a0fd6cd79 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -73,6 +73,12 @@ struct raspberrypi_clk_data {
 	struct raspberrypi_clk *rpi;
 };
 
+static inline
+const struct raspberrypi_clk_data *clk_hw_to_data(const struct clk_hw *hw)
+{
+	return container_of(hw, struct raspberrypi_clk_data, hw);
+}
+
 struct raspberrypi_clk_variant {
 	bool		export;
 	char		*clkdev;
@@ -176,8 +182,7 @@ static int raspberrypi_clock_property(struct rpi_firmware *firmware,
 
 static int raspberrypi_fw_is_prepared(struct clk_hw *hw)
 {
-	struct raspberrypi_clk_data *data =
-		container_of(hw, struct raspberrypi_clk_data, hw);
+	const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
 	struct raspberrypi_clk *rpi = data->rpi;
 	u32 val = 0;
 	int ret;
@@ -194,8 +199,7 @@ static int raspberrypi_fw_is_prepared(struct clk_hw *hw)
 static unsigned long raspberrypi_fw_get_rate(struct clk_hw *hw,
 					     unsigned long parent_rate)
 {
-	struct raspberrypi_clk_data *data =
-		container_of(hw, struct raspberrypi_clk_data, hw);
+	const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
 	struct raspberrypi_clk *rpi = data->rpi;
 	u32 val = 0;
 	int ret;
@@ -211,8 +215,7 @@ static unsigned long raspberrypi_fw_get_rate(struct clk_hw *hw,
 static int raspberrypi_fw_set_rate(struct clk_hw *hw, unsigned long rate,
 				   unsigned long parent_rate)
 {
-	struct raspberrypi_clk_data *data =
-		container_of(hw, struct raspberrypi_clk_data, hw);
+	const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
 	struct raspberrypi_clk *rpi = data->rpi;
 	u32 _rate = rate;
 	int ret;
@@ -229,8 +232,7 @@ static int raspberrypi_fw_set_rate(struct clk_hw *hw, unsigned long rate,
 static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
 					      struct clk_rate_request *req)
 {
-	struct raspberrypi_clk_data *data =
-		container_of(hw, struct raspberrypi_clk_data, hw);
+	const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
 	struct raspberrypi_clk_variant *variant = data->variant;
 
 	/*

-- 
b4 0.10.0-dev-a76f5

[PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Maxime Ripard]

The RaspberryPi firmware can be configured by the end user using the
config.txt file.

Some of these options will affect the kernel capabilities, and we thus
need to be able to detect it to operate reliably.

One of such parameters is the hdmi_enable_4kp60 parameter that will
setup the clocks in a way that is suitable to reach the pixel
frequencies required by the 4k at 60Hz and higher modes.

If the user forgot to enable it, then those modes will simply not work
but are still likely to be picked up by the userspace, which is a poor
user-experience.

The kernel can't access the config.txt file directly, but one of the
effect that parameter has is that the core clock frequency maximum will
be much higher. Thus we can infer whether it was enabled or not by
querying the firmware for that maximum, and if it isn't prevent any of
the modes that wouldn't work.

The HDMI driver is already doing this, but was relying on a behaviour of
clk_round_rate() that got changed recently, and doesn't return the
result we would like anymore.

We also considered introducing a CCF function to access the maximum of a
given struct clk, but that wouldn't work if the clock is further
constrained by another user.

It was thus suggested to create a small, ad-hoc function to query the
RaspberryPi firmware for the maximum rate a given clock has.

Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 6c0a0fd6cd79..182e8817eac2 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
+#include <soc/bcm2835/raspberrypi-clocks.h>
 #include <soc/bcm2835/raspberrypi-firmware.h>
 
 enum rpi_firmware_clk_id {
@@ -254,6 +255,33 @@ static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
 	return 0;
 }
 
+unsigned long rpi_firmware_clk_get_max_rate(struct clk *clk)
+{
+	const struct raspberrypi_clk_data *data;
+	struct raspberrypi_clk *rpi;
+	struct clk_hw *hw;
+	u32 max_rate;
+	int ret;
+
+	if (!clk)
+		return 0;
+
+	hw =  __clk_get_hw(clk);
+	if (!hw)
+		return 0;
+
+	data = clk_hw_to_data(hw);
+	rpi = data->rpi;
+	ret = raspberrypi_clock_property(rpi->firmware, data,
+					 RPI_FIRMWARE_GET_MAX_CLOCK_RATE,
+					 &max_rate);
+	if (ret)
+		return 0;
+
+	return max_rate;
+}
+EXPORT_SYMBOL_GPL(rpi_firmware_clk_get_max_rate);
+
 static const struct clk_ops raspberrypi_firmware_clk_ops = {
 	.is_prepared	= raspberrypi_fw_is_prepared,
 	.recalc_rate	= raspberrypi_fw_get_rate,
diff --git a/include/soc/bcm2835/raspberrypi-clocks.h b/include/soc/bcm2835/raspberrypi-clocks.h
new file mode 100644
index 000000000000..ff0b608b51a8
--- /dev/null
+++ b/include/soc/bcm2835/raspberrypi-clocks.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __SOC_RASPBERRY_CLOCKS_H__
+#define __SOC_RASPBERRY_CLOCKS_H__
+
+#if IS_ENABLED(CONFIG_CLK_RASPBERRYPI)
+unsigned long rpi_firmware_clk_get_max_rate(struct clk *clk);
+#else
+static inline unsigned long rpi_firmware_clk_get_max_rate(struct clk *clk)
+{
+	return ULONG_MAX;
+}
+#endif
+
+#endif /* __SOC_RASPBERRY_CLOCKS_H__ */

-- 
b4 0.10.0-dev-a76f5

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Stephen Boyd]

Quoting Maxime Ripard (2022-08-15 08:31:24)
> @@ -254,6 +255,33 @@ static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
>         return 0;
>  }
>  
> +unsigned long rpi_firmware_clk_get_max_rate(struct clk *clk)
> +{
> +       const struct raspberrypi_clk_data *data;
> +       struct raspberrypi_clk *rpi;
> +       struct clk_hw *hw;
> +       u32 max_rate;
> +       int ret;
> +
> +       if (!clk)
> +               return 0;
> +
> +       hw =  __clk_get_hw(clk);

Ideally we don't add more users of this API. I should document that :/

It begs the question though, why do we need this API to take a 'struct
clk'?  Can it simply hardcode the data->id value for the clk you care
about and call rpi_firmware_property() directly (or some wrapper of it)?

Furthermore, I wonder if even that part needs to be implemented.  Why
not make a direct call to rpi_firmware_property() and get the max rate?
All of that can live in the drm driver. Making it a generic API that
takes a 'struct clk' means that it looks like any clk can be passed,
when that isn't true. It would be better to restrict it to the one use
case so that the scope of the problem doesn't grow. I understand that it
duplicates a few lines of code, but that looks like a fair tradeoff vs.
exposing an API that can be used for other clks in the future.

> +       if (!hw)
> +               return 0;
> +
> +       data = clk_hw_to_data(hw);
> +       rpi = data->rpi;
> +       ret = raspberrypi_clock_property(rpi->firmware, data,
> +                                        RPI_FIRMWARE_GET_MAX_CLOCK_RATE,
> +                                        &max_rate);

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Maxime Ripard]

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

Hi Stephen,

Thanks for reviewing that series

On Wed, Sep 14, 2022 at 08:50:33AM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-08-15 08:31:24)
> > @@ -254,6 +255,33 @@ static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
> >         return 0;
> >  }
> >  
> > +unsigned long rpi_firmware_clk_get_max_rate(struct clk *clk)
> > +{
> > +       const struct raspberrypi_clk_data *data;
> > +       struct raspberrypi_clk *rpi;
> > +       struct clk_hw *hw;
> > +       u32 max_rate;
> > +       int ret;
> > +
> > +       if (!clk)
> > +               return 0;
> > +
> > +       hw =  __clk_get_hw(clk);
> 
> Ideally we don't add more users of this API. I should document that :/

What should be the proper way to implement this?

> It begs the question though, why do we need this API to take a 'struct
> clk'?  Can it simply hardcode the data->id value for the clk you care
> about and call rpi_firmware_property() directly (or some wrapper of it)?

You mean push it down to the consumer?

We will have two users of that function eventually. The KMS driver, and
the codec driver that isn't upstream yet. AFAIK, both are using a
different clock, so we can' really hardcode it, and duplicating it at
the consumer level would be weird.

> Furthermore, I wonder if even that part needs to be implemented.  Why
> not make a direct call to rpi_firmware_property() and get the max rate?
> All of that can live in the drm driver. Making it a generic API that
> takes a 'struct clk' means that it looks like any clk can be passed,
> when that isn't true. It would be better to restrict it to the one use
> case so that the scope of the problem doesn't grow. I understand that it
> duplicates a few lines of code, but that looks like a fair tradeoff vs.
> exposing an API that can be used for other clks in the future.

So we'll want to have that function shared between the KMS and codec
drivers eventually. The clock id used by both drivers is stored in the
DT so we would create a function (outside of the clock drivers) that
would parse the clocks property, get the ID, and then queries the
firmware for it. Would that make sense?

Maxime

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

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Stephen Boyd]

Quoting Maxime Ripard (2022-09-14 09:15:02)
> Hi Stephen,
> 
> Thanks for reviewing that series
> 
> On Wed, Sep 14, 2022 at 08:50:33AM -0700, Stephen Boyd wrote:
> > Quoting Maxime Ripard (2022-08-15 08:31:24)
> > > @@ -254,6 +255,33 @@ static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
> > >         return 0;
> > >  }
> > >  
> > > +unsigned long rpi_firmware_clk_get_max_rate(struct clk *clk)
> > > +{
> > > +       const struct raspberrypi_clk_data *data;
> > > +       struct raspberrypi_clk *rpi;
> > > +       struct clk_hw *hw;
> > > +       u32 max_rate;
> > > +       int ret;
> > > +
> > > +       if (!clk)
> > > +               return 0;
> > > +
> > > +       hw =  __clk_get_hw(clk);
> > 
> > Ideally we don't add more users of this API. I should document that :/
> 
> What should be the proper way to implement this?
> 
> > It begs the question though, why do we need this API to take a 'struct
> > clk'?  Can it simply hardcode the data->id value for the clk you care
> > about and call rpi_firmware_property() directly (or some wrapper of it)?
> 
> You mean push it down to the consumer?
> 
> We will have two users of that function eventually. The KMS driver, and
> the codec driver that isn't upstream yet. AFAIK, both are using a
> different clock, so we can' really hardcode it, and duplicating it at
> the consumer level would be weird.

Can you make an API that returns 'max freq for KMS' and 'max freq for
codec'? For example, it could take the enum value that the clk driver
uses for data->id?

> 
> > Furthermore, I wonder if even that part needs to be implemented.  Why
> > not make a direct call to rpi_firmware_property() and get the max rate?
> > All of that can live in the drm driver. Making it a generic API that
> > takes a 'struct clk' means that it looks like any clk can be passed,
> > when that isn't true. It would be better to restrict it to the one use
> > case so that the scope of the problem doesn't grow. I understand that it
> > duplicates a few lines of code, but that looks like a fair tradeoff vs.
> > exposing an API that can be used for other clks in the future.
> 
> So we'll want to have that function shared between the KMS and codec
> drivers eventually. The clock id used by both drivers is stored in the
> DT so we would create a function (outside of the clock drivers) that
> would parse the clocks property, get the ID, and then queries the
> firmware for it. Would that make sense?
> 

Sure. Is the ID ever changing? If not then a simpler design would be to
ask for the particular ID and hardcode that in the driver.

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Stefan Wahren]

Hi,

Am 14.09.22 um 17:50 schrieb Stephen Boyd:
> Quoting Maxime Ripard (2022-08-15 08:31:24)
>> @@ -254,6 +255,33 @@ static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
>>          return 0;
>>   }
>>   
>> +unsigned long rpi_firmware_clk_get_max_rate(struct clk *clk)
>> +{
>> +       const struct raspberrypi_clk_data *data;
>> +       struct raspberrypi_clk *rpi;
>> +       struct clk_hw *hw;
>> +       u32 max_rate;
>> +       int ret;
>> +
>> +       if (!clk)
>> +               return 0;
>> +
>> +       hw =  __clk_get_hw(clk);
> Ideally we don't add more users of this API. I should document that :/
>
> It begs the question though, why do we need this API to take a 'struct
> clk'?  Can it simply hardcode the data->id value for the clk you care
> about and call rpi_firmware_property() directly (or some wrapper of it)?
>
> Furthermore, I wonder if even that part needs to be implemented.  Why
> not make a direct call to rpi_firmware_property() and get the max rate?
> All of that can live in the drm driver. Making it a generic API that
> takes a 'struct clk' means that it looks like any clk can be passed,
> when that isn't true. It would be better to restrict it to the one use
> case so that the scope of the problem doesn't grow. I understand that it
> duplicates a few lines of code, but that looks like a fair tradeoff vs.
> exposing an API that can be used for other clks in the future.
it would be nice to keep all the Rpi specific stuff out of the DRM 
driver, since there more users of it.
>
>> +       if (!hw)
>> +               return 0;
>> +
>> +       data = clk_hw_to_data(hw);
>> +       rpi = data->rpi;
>> +       ret = raspberrypi_clock_property(rpi->firmware, data,
>> +                                        RPI_FIRMWARE_GET_MAX_CLOCK_RATE,
>> +                                        &max_rate);
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Stephen Boyd]

Quoting Stefan Wahren (2022-09-14 10:45:48)
> Am 14.09.22 um 17:50 schrieb Stephen Boyd:
> >
> > Furthermore, I wonder if even that part needs to be implemented.  Why
> > not make a direct call to rpi_firmware_property() and get the max rate?
> > All of that can live in the drm driver. Making it a generic API that
> > takes a 'struct clk' means that it looks like any clk can be passed,
> > when that isn't true. It would be better to restrict it to the one use
> > case so that the scope of the problem doesn't grow. I understand that it
> > duplicates a few lines of code, but that looks like a fair tradeoff vs.
> > exposing an API that can be used for other clks in the future.
> it would be nice to keep all the Rpi specific stuff out of the DRM 
> driver, since there more users of it.

Instead of 'all' did you mean 'any'?

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Stefan Wahren]

Am 14.09.22 um 20:05 schrieb Stephen Boyd:
> Quoting Stefan Wahren (2022-09-14 10:45:48)
>> Am 14.09.22 um 17:50 schrieb Stephen Boyd:
>>> Furthermore, I wonder if even that part needs to be implemented.  Why
>>> not make a direct call to rpi_firmware_property() and get the max rate?
>>> All of that can live in the drm driver. Making it a generic API that
>>> takes a 'struct clk' means that it looks like any clk can be passed,
>>> when that isn't true. It would be better to restrict it to the one use
>>> case so that the scope of the problem doesn't grow. I understand that it
>>> duplicates a few lines of code, but that looks like a fair tradeoff vs.
>>> exposing an API that can be used for other clks in the future.
>> it would be nice to keep all the Rpi specific stuff out of the DRM
>> driver, since there more users of it.
> Instead of 'all' did you mean 'any'?
yes

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Stephen Boyd]

Quoting Stefan Wahren (2022-09-14 11:09:04)
> Am 14.09.22 um 20:05 schrieb Stephen Boyd:
> > Quoting Stefan Wahren (2022-09-14 10:45:48)
> >> Am 14.09.22 um 17:50 schrieb Stephen Boyd:
> >>> Furthermore, I wonder if even that part needs to be implemented.  Why
> >>> not make a direct call to rpi_firmware_property() and get the max rate?
> >>> All of that can live in the drm driver. Making it a generic API that
> >>> takes a 'struct clk' means that it looks like any clk can be passed,
> >>> when that isn't true. It would be better to restrict it to the one use
> >>> case so that the scope of the problem doesn't grow. I understand that it
> >>> duplicates a few lines of code, but that looks like a fair tradeoff vs.
> >>> exposing an API that can be used for other clks in the future.
> >> it would be nice to keep all the Rpi specific stuff out of the DRM
> >> driver, since there more users of it.
> > Instead of 'all' did you mean 'any'?
> yes

Why?

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Stefan Wahren]

Am 14.09.22 um 20:14 schrieb Stephen Boyd:
> Quoting Stefan Wahren (2022-09-14 11:09:04)
>> Am 14.09.22 um 20:05 schrieb Stephen Boyd:
>>> Quoting Stefan Wahren (2022-09-14 10:45:48)
>>>> Am 14.09.22 um 17:50 schrieb Stephen Boyd:
>>>>> Furthermore, I wonder if even that part needs to be implemented.  Why
>>>>> not make a direct call to rpi_firmware_property() and get the max rate?
>>>>> All of that can live in the drm driver. Making it a generic API that
>>>>> takes a 'struct clk' means that it looks like any clk can be passed,
>>>>> when that isn't true. It would be better to restrict it to the one use
>>>>> case so that the scope of the problem doesn't grow. I understand that it
>>>>> duplicates a few lines of code, but that looks like a fair tradeoff vs.
>>>>> exposing an API that can be used for other clks in the future.
>>>> it would be nice to keep all the Rpi specific stuff out of the DRM
>>>> driver, since there more users of it.
>>> Instead of 'all' did you mean 'any'?
>> yes
> Why?
This firmware is written specific for the Raspberry Pi and not stable 
from interface point of view. So i'm afraid that the DRM driver is only 
usable for the Raspberry Pi at the end with all these board specific 
dependencies. Emma invested a lot of time to make this open source and 
now it looks that like that more and more functionality moves back to 
firmware.

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Maxime Ripard]

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

On Wed, Sep 14, 2022 at 08:26:55PM +0200, Stefan Wahren wrote:
> Am 14.09.22 um 20:14 schrieb Stephen Boyd:
> > Quoting Stefan Wahren (2022-09-14 11:09:04)
> > > Am 14.09.22 um 20:05 schrieb Stephen Boyd:
> > > > Quoting Stefan Wahren (2022-09-14 10:45:48)
> > > > > Am 14.09.22 um 17:50 schrieb Stephen Boyd:
> > > > > > Furthermore, I wonder if even that part needs to be implemented.  Why
> > > > > > not make a direct call to rpi_firmware_property() and get the max rate?
> > > > > > All of that can live in the drm driver. Making it a generic API that
> > > > > > takes a 'struct clk' means that it looks like any clk can be passed,
> > > > > > when that isn't true. It would be better to restrict it to the one use
> > > > > > case so that the scope of the problem doesn't grow. I understand that it
> > > > > > duplicates a few lines of code, but that looks like a fair tradeoff vs.
> > > > > > exposing an API that can be used for other clks in the future.
> > > > > it would be nice to keep all the Rpi specific stuff out of the DRM
> > > > > driver, since there more users of it.
> > > > Instead of 'all' did you mean 'any'?
> > > yes
> > Why?
> This firmware is written specific for the Raspberry Pi and not stable from
> interface point of view. So i'm afraid that the DRM driver is only usable
> for the Raspberry Pi at the end with all these board specific dependencies.

I'm open for suggestions there, but is there any other bcm2711 device
that we support upstream?

If not, I'm not sure what the big deal is at this point. Chances are the
DRM driver won't work as is on a different board.

Plus, such a board wouldn't be using config.txt at all, so this whole
dance to find what was enabled or not wouldn't be used at all.

> Emma invested a lot of time to make this open source and now it looks that
> like that more and more functionality moves back to firmware.

What functionality has been moved back to firmware?

Maxime

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

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Stefan Wahren]

Hi Maxime,

Am 15.09.22 um 09:54 schrieb Maxime Ripard:
> On Wed, Sep 14, 2022 at 08:26:55PM +0200, Stefan Wahren wrote:
>> Am 14.09.22 um 20:14 schrieb Stephen Boyd:
>>> Quoting Stefan Wahren (2022-09-14 11:09:04)
>>>> Am 14.09.22 um 20:05 schrieb Stephen Boyd:
>>>>> Quoting Stefan Wahren (2022-09-14 10:45:48)
>>>>>> Am 14.09.22 um 17:50 schrieb Stephen Boyd:
>>>>>>> Furthermore, I wonder if even that part needs to be implemented.  Why
>>>>>>> not make a direct call to rpi_firmware_property() and get the max rate?
>>>>>>> All of that can live in the drm driver. Making it a generic API that
>>>>>>> takes a 'struct clk' means that it looks like any clk can be passed,
>>>>>>> when that isn't true. It would be better to restrict it to the one use
>>>>>>> case so that the scope of the problem doesn't grow. I understand that it
>>>>>>> duplicates a few lines of code, but that looks like a fair tradeoff vs.
>>>>>>> exposing an API that can be used for other clks in the future.
>>>>>> it would be nice to keep all the Rpi specific stuff out of the DRM
>>>>>> driver, since there more users of it.
>>>>> Instead of 'all' did you mean 'any'?
>>>> yes
>>> Why?
>> This firmware is written specific for the Raspberry Pi and not stable from
>> interface point of view. So i'm afraid that the DRM driver is only usable
>> for the Raspberry Pi at the end with all these board specific dependencies.
> I'm open for suggestions there, but is there any other bcm2711 device
> that we support upstream?
I meant the driver as a whole. According to the vc4 binding there are 
three compatibles bcm2835-vc4, cygnus-vc4 and bcm2711-vc5. Unfortunately 
i don't have access to any of these Cygnus boards, so i cannot do any 
regression tests or provide more information to your question.
> If not, I'm not sure what the big deal is at this point. Chances are the
> DRM driver won't work as is on a different board.
>
> Plus, such a board wouldn't be using config.txt at all, so this whole
> dance to find what was enabled or not wouldn't be used at all.
My concern is that we reach some point that we need to say this kernel 
version requires this firmware version. In the Raspberry Pi OS world 
this is not a problem, but not all distributions has this specific 
knowledge.
>
>> Emma invested a lot of time to make this open source and now it looks that
>> like that more and more functionality moves back to firmware.
> What functionality has been moved back to firmware?
This wasn't a offense against your great work. Just a slight warning 
that some functions of clock or power management moved back into 
firmware. We should watch out, but maybe i emote here.
>
> Maxime

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Maxime Ripard]

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

Hi Stefan,

On Thu, Sep 15, 2022 at 01:30:02PM +0200, Stefan Wahren wrote:
> Am 15.09.22 um 09:54 schrieb Maxime Ripard:
> > On Wed, Sep 14, 2022 at 08:26:55PM +0200, Stefan Wahren wrote:
> > > Am 14.09.22 um 20:14 schrieb Stephen Boyd:
> > > > Quoting Stefan Wahren (2022-09-14 11:09:04)
> > > > > Am 14.09.22 um 20:05 schrieb Stephen Boyd:
> > > > > > Quoting Stefan Wahren (2022-09-14 10:45:48)
> > > > > > > Am 14.09.22 um 17:50 schrieb Stephen Boyd:
> > > > > > > > Furthermore, I wonder if even that part needs to be implemented.  Why
> > > > > > > > not make a direct call to rpi_firmware_property() and get the max rate?
> > > > > > > > All of that can live in the drm driver. Making it a generic API that
> > > > > > > > takes a 'struct clk' means that it looks like any clk can be passed,
> > > > > > > > when that isn't true. It would be better to restrict it to the one use
> > > > > > > > case so that the scope of the problem doesn't grow. I understand that it
> > > > > > > > duplicates a few lines of code, but that looks like a fair tradeoff vs.
> > > > > > > > exposing an API that can be used for other clks in the future.
> > > > > > > it would be nice to keep all the Rpi specific stuff out of the DRM
> > > > > > > driver, since there more users of it.
> > > > > > Instead of 'all' did you mean 'any'?
> > > > > yes
> > > > Why?
> > > This firmware is written specific for the Raspberry Pi and not stable from
> > > interface point of view. So i'm afraid that the DRM driver is only usable
> > > for the Raspberry Pi at the end with all these board specific dependencies.
> > I'm open for suggestions there, but is there any other bcm2711 device
> > that we support upstream?
>
> I meant the driver as a whole. According to the vc4 binding there are three
> compatibles bcm2835-vc4, cygnus-vc4 and bcm2711-vc5. Unfortunately i don't
> have access to any of these Cygnus boards, so i cannot do any regression
> tests or provide more information to your question.

I don't have access to these boards either, and none of them are
upstream, so I'm not sure what we can do to improve their support by then.

> > If not, I'm not sure what the big deal is at this point. Chances are the
> > DRM driver won't work as is on a different board.
> > 
> > Plus, such a board wouldn't be using config.txt at all, so this whole
> > dance to find what was enabled or not wouldn't be used at all.
>
> My concern is that we reach some point that we need to say this kernel
> version requires this firmware version. In the Raspberry Pi OS world this is
> not a problem, but not all distributions has this specific knowledge.

The recent mess with the Intel GPU firmware
(https://lore.kernel.org/dri-devel/CAPM=9txdca1VnRpp-oNLXpBc2UWq3=ceeim5+Gw4N9tAriRY6A@mail.gmail.com/)
makes it fairly clear that such a situation should be considered a
regression and fixed. So it might be a situation that the downstream
tree will end up in, but it's not something we will allow to happen
upstream.

> > > Emma invested a lot of time to make this open source and now it looks that
> > > like that more and more functionality moves back to firmware.
> > What functionality has been moved back to firmware?
>
> This wasn't a offense against your great work. Just a slight warning that
> some functions of clock or power management moved back into firmware. We
> should watch out, but maybe i emote here.

Yeah, I guess we'll want to consider it on a case per case basis but
it's not like we merged fkms either :)

Maxime

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

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Stephen Boyd]

Quoting Stefan Wahren (2022-09-14 11:09:04)
> Am 14.09.22 um 20:05 schrieb Stephen Boyd:
> > Quoting Stefan Wahren (2022-09-14 10:45:48)
> >> Am 14.09.22 um 17:50 schrieb Stephen Boyd:
> >>> Furthermore, I wonder if even that part needs to be implemented.  Why
> >>> not make a direct call to rpi_firmware_property() and get the max rate?
> >>> All of that can live in the drm driver. Making it a generic API that
> >>> takes a 'struct clk' means that it looks like any clk can be passed,
> >>> when that isn't true. It would be better to restrict it to the one use
> >>> case so that the scope of the problem doesn't grow. I understand that it
> >>> duplicates a few lines of code, but that looks like a fair tradeoff vs.
> >>> exposing an API that can be used for other clks in the future.
> >> it would be nice to keep all the Rpi specific stuff out of the DRM
> >> driver, since there more users of it.
> > Instead of 'all' did you mean 'any'?
> yes

Another idea is to populate an OPP table in the rpi firmware driver for
this platform device with the adjusted max frequency. That would be an
SoC/firmware agnostic interface that expresses the constraints. I'm
almost certain we talked about this before.

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Stefan Wahren]

Hi Stephen,

Am 14.09.22 um 20:20 schrieb Stephen Boyd:
> Quoting Stefan Wahren (2022-09-14 11:09:04)
>> Am 14.09.22 um 20:05 schrieb Stephen Boyd:
>>> Quoting Stefan Wahren (2022-09-14 10:45:48)
>>>> Am 14.09.22 um 17:50 schrieb Stephen Boyd:
>>>>> Furthermore, I wonder if even that part needs to be implemented.  Why
>>>>> not make a direct call to rpi_firmware_property() and get the max rate?
>>>>> All of that can live in the drm driver. Making it a generic API that
>>>>> takes a 'struct clk' means that it looks like any clk can be passed,
>>>>> when that isn't true. It would be better to restrict it to the one use
>>>>> case so that the scope of the problem doesn't grow. I understand that it
>>>>> duplicates a few lines of code, but that looks like a fair tradeoff vs.
>>>>> exposing an API that can be used for other clks in the future.
>>>> it would be nice to keep all the Rpi specific stuff out of the DRM
>>>> driver, since there more users of it.
>>> Instead of 'all' did you mean 'any'?
>> yes
> Another idea is to populate an OPP table in the rpi firmware driver for
> this platform device with the adjusted max frequency. That would be an
> SoC/firmware agnostic interface that expresses the constraints.
Do you mean in the source code of this driver or in the DT?
> I'm
> almost certain we talked about this before.
I'm not sure about the context. Do you mean the CPU frequency handling? 
I remember it was a hard decision. In the end it was little benefit but 
a lot of disadvantages (hard to maintain all theses OPP tables for all 
Raspberry Pi boards, doesn't work with already deployed DT). So yes, i'm 
part of the problem i mentioned before ;-)

Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

[Maxime Ripard]

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

Hi,

On Wed, Sep 14, 2022 at 11:20:59AM -0700, Stephen Boyd wrote:
> Quoting Stefan Wahren (2022-09-14 11:09:04)
> > Am 14.09.22 um 20:05 schrieb Stephen Boyd:
> > > Quoting Stefan Wahren (2022-09-14 10:45:48)
> > >> Am 14.09.22 um 17:50 schrieb Stephen Boyd:
> > >>> Furthermore, I wonder if even that part needs to be implemented.  Why
> > >>> not make a direct call to rpi_firmware_property() and get the max rate?
> > >>> All of that can live in the drm driver. Making it a generic API that
> > >>> takes a 'struct clk' means that it looks like any clk can be passed,
> > >>> when that isn't true. It would be better to restrict it to the one use
> > >>> case so that the scope of the problem doesn't grow. I understand that it
> > >>> duplicates a few lines of code, but that looks like a fair tradeoff vs.
> > >>> exposing an API that can be used for other clks in the future.
> > >> it would be nice to keep all the Rpi specific stuff out of the DRM
> > >> driver, since there more users of it.
> > > Instead of 'all' did you mean 'any'?
> > yes
> 
> Another idea is to populate an OPP table in the rpi firmware driver for
> this platform device with the adjusted max frequency. That would be an
> SoC/firmware agnostic interface that expresses the constraints. I'm
> almost certain we talked about this before.

Yeah, that rings a bell too.

I found the thread:
https://lore.kernel.org/linux-clk/20220629092900.inpjgl7st33dwcak@houat/

Admittedly, I don't know the OPP stuff that well, but I was always under
the impression that it was to express the operating range of a device.
We're not quite in this case here, since we want to get the range of one
of the clock that feeds the device but might or might not affect the
frequency of the device itself.

I'm ok with your proposal to create a custom function in the firmware
driver though, and I don't believe it would be an obstacle to any board
we might upstream in the future that wouldn't have use the RPi firmware,
so I'll work on that.

Thanks!
Maxime

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

[PATCH v1 3/7] clk: bcm: rpi: Add a function to retrieve the minimum

[Maxime Ripard]

The RaspberryPi firmware can be configured by the end user using the
config.txt file.

Some of these options will affect the kernel capabilities, and we thus
need to be able to detect it to operate reliably.

One of such parameters is the core_clock parameter that allows users to
setup the clocks in a way that is suitable to reach the pixel
frequencies required by the 4096x2016 resolution at 60Hz and higher
modes.

If the user misconfigured it, then those modes will simply not work
but are still likely to be picked up by the userspace, which is a poor
user-experience.

The kernel can't access the config.txt file directly, but one of the
effect that parameter has is that the core clock frequency minimum will
be raised. Thus we can infer its setup by querying the firmware for that
minimum, and if it isn't ignore any of the modes that wouldn't work.

We had in the past a discussion for the maximum and it was suggested to
create a small, ad-hoc function to query the RaspberryPi firmware for
the minimum rate a given clock has, so let's do the same here.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 182e8817eac2..b81da5b1dd1e 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -282,6 +282,33 @@ unsigned long rpi_firmware_clk_get_max_rate(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(rpi_firmware_clk_get_max_rate);
 
+unsigned long rpi_firmware_clk_get_min_rate(struct clk *clk)
+{
+	const struct raspberrypi_clk_data *data;
+	struct raspberrypi_clk *rpi;
+	struct clk_hw *hw;
+	u32 min_rate;
+	int ret;
+
+	if (!clk)
+		return 0;
+
+	hw =  __clk_get_hw(clk);
+	if (!hw)
+		return 0;
+
+	data = clk_hw_to_data(hw);
+	rpi = data->rpi;
+	ret = raspberrypi_clock_property(rpi->firmware, data,
+					 RPI_FIRMWARE_GET_MIN_CLOCK_RATE,
+					 &min_rate);
+	if (ret)
+		return 0;
+
+	return min_rate;
+}
+EXPORT_SYMBOL_GPL(rpi_firmware_clk_get_min_rate);
+
 static const struct clk_ops raspberrypi_firmware_clk_ops = {
 	.is_prepared	= raspberrypi_fw_is_prepared,
 	.recalc_rate	= raspberrypi_fw_get_rate,
diff --git a/include/soc/bcm2835/raspberrypi-clocks.h b/include/soc/bcm2835/raspberrypi-clocks.h
index ff0b608b51a8..627535877964 100644
--- a/include/soc/bcm2835/raspberrypi-clocks.h
+++ b/include/soc/bcm2835/raspberrypi-clocks.h
@@ -5,11 +5,17 @@
 
 #if IS_ENABLED(CONFIG_CLK_RASPBERRYPI)
 unsigned long rpi_firmware_clk_get_max_rate(struct clk *clk);
+unsigned long rpi_firmware_clk_get_min_rate(struct clk *clk);
 #else
 static inline unsigned long rpi_firmware_clk_get_max_rate(struct clk *clk)
 {
 	return ULONG_MAX;
 }
+
+static inline unsigned long rpi_firmware_clk_get_min_rate(struct clk *clk)
+{
+	return 0;
+}
 #endif
 
 #endif /* __SOC_RASPBERRY_CLOCKS_H__ */

-- 
b4 0.10.0-dev-a76f5

[PATCH v1 4/7] drm/vc4: hdmi: Fix hdmi_enable_4kp60 detection

[Maxime Ripard]

In order to support higher HDMI frequencies, users have to set the
hdmi_enable_4kp60 parameter in their config.txt file.

We were detecting this so far by calling clk_round_rate() on the core
clock with the frequency we're supposed to run at when one of those
modes is enabled. Whether or not the parameter was enabled could then be
inferred by the returned rate since the maximum clock rate reported by
the firmware was one of the side effect of setting that parameter.

However, the recent clock rework we did changed what clk_round_rate()
was returning to always return the minimum allowed, and thus this test
wasn't reliable anymore.

Let's use the new clk_get_max_rate() function to reliably determine the
maximum rate allowed on that clock and fix the 4k@60Hz output.

Fixes: e9d6cea2af1c ("clk: bcm: rpi: Run some clocks at the minimum rate allowed")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 592c3b5d03e6..aa3ebda55e04 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -46,6 +46,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/rational.h>
 #include <linux/reset.h>
+#include <soc/bcm2835/raspberrypi-clocks.h>
 #include <sound/dmaengine_pcm.h>
 #include <sound/hdmi-codec.h>
 #include <sound/pcm_drm_eld.h>
@@ -2966,7 +2967,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 
 	if (variant->max_pixel_clock == 600000000) {
 		struct vc4_dev *vc4 = to_vc4_dev(drm);
-		long max_rate = clk_round_rate(vc4->hvs->core_clk, 550000000);
+		unsigned long max_rate = rpi_firmware_clk_get_max_rate(vc4->hvs->core_clk);
 
 		if (max_rate < 550000000)
 			vc4_hdmi->disable_4kp60 = true;

-- 
b4 0.10.0-dev-a76f5

[PATCH v1 5/7] drm/vc4: hdmi: Rework hdmi_enable_4kp60 detection code

[Maxime Ripard]

In order to support higher HDMI frequencies, users have to set the
hdmi_enable_4kp60 parameter in their config.txt file.

This will have the side-effect of raising the maximum of the core clock,
tied to the HVS, and managed by the HVS driver.

However, we are querying this in the HDMI driver by poking into the HVS
structure to get our struct clk handle.

Let's make this part of the HVS bind implementation to have all the core
clock related setup in the same place.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 1beb96b77b8c..d48ef302af42 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -339,6 +339,14 @@ struct vc4_hvs {
 	struct drm_mm_node mitchell_netravali_filter;
 
 	struct debugfs_regset32 regset;
+
+	/*
+	 * Even if HDMI0 on the RPi4 can output modes requiring a pixel
+	 * rate higher than 297MHz, it needs some adjustments in the
+	 * config.txt file to be able to do so and thus won't always be
+	 * available.
+	 */
+	bool vc5_hdmi_enable_scrambling;
 };
 
 struct vc4_plane {
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index aa3ebda55e04..371fbc05bf5a 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -46,7 +46,6 @@
 #include <linux/pm_runtime.h>
 #include <linux/rational.h>
 #include <linux/reset.h>
-#include <soc/bcm2835/raspberrypi-clocks.h>
 #include <sound/dmaengine_pcm.h>
 #include <sound/hdmi-codec.h>
 #include <sound/pcm_drm_eld.h>
@@ -277,6 +276,7 @@ static void vc4_hdmi_connector_destroy(struct drm_connector *connector)
 static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 {
 	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
+	struct vc4_dev *vc4 = to_vc4_dev(connector->dev);
 	int ret = 0;
 	struct edid *edid;
 
@@ -293,7 +293,7 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 	ret = drm_add_edid_modes(connector, edid);
 	kfree(edid);
 
-	if (vc4_hdmi->disable_4kp60) {
+	if (!vc4->hvs->vc5_hdmi_enable_scrambling) {
 		struct drm_device *drm = connector->dev;
 		struct drm_display_mode *mode;
 
@@ -1480,11 +1480,12 @@ vc4_hdmi_encoder_clock_valid(const struct vc4_hdmi *vc4_hdmi,
 {
 	const struct drm_connector *connector = &vc4_hdmi->connector;
 	const struct drm_display_info *info = &connector->display_info;
+	struct vc4_dev *vc4 = to_vc4_dev(connector->dev);
 
 	if (clock > vc4_hdmi->variant->max_pixel_clock)
 		return MODE_CLOCK_HIGH;
 
-	if (vc4_hdmi->disable_4kp60 && clock > HDMI_14_MAX_TMDS_CLK)
+	if (!vc4->hvs->vc5_hdmi_enable_scrambling && clock > HDMI_14_MAX_TMDS_CLK)
 		return MODE_CLOCK_HIGH;
 
 	if (info->max_tmds_clock && clock > (info->max_tmds_clock * 1000))
@@ -2965,14 +2966,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 	vc4_hdmi->disable_wifi_frequencies =
 		of_property_read_bool(dev->of_node, "wifi-2.4ghz-coexistence");
 
-	if (variant->max_pixel_clock == 600000000) {
-		struct vc4_dev *vc4 = to_vc4_dev(drm);
-		unsigned long max_rate = rpi_firmware_clk_get_max_rate(vc4->hvs->core_clk);
-
-		if (max_rate < 550000000)
-			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.
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index c3ed2b07df23..7506943050cf 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -155,14 +155,6 @@ struct vc4_hdmi {
 	 */
 	bool disable_wifi_frequencies;
 
-	/*
-	 * Even if HDMI0 on the RPi4 can output modes requiring a pixel
-	 * rate higher than 297MHz, it needs some adjustments in the
-	 * config.txt file to be able to do so and thus won't always be
-	 * available.
-	 */
-	bool disable_4kp60;
-
 	struct cec_adapter *cec_adap;
 	struct cec_msg cec_rx_msg;
 	bool cec_tx_ok;
diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
index fbaa741dda5f..3fdd2c4356f6 100644
--- a/drivers/gpu/drm/vc4/vc4_hvs.c
+++ b/drivers/gpu/drm/vc4/vc4_hvs.c
@@ -27,6 +27,8 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_vblank.h>
 
+#include <soc/bcm2835/raspberrypi-clocks.h>
+
 #include "vc4_drv.h"
 #include "vc4_regs.h"
 
@@ -671,12 +673,18 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
 	hvs->regset.nregs = ARRAY_SIZE(hvs_regs);
 
 	if (vc4->is_vc5) {
+		unsigned long max_rate;
+
 		hvs->core_clk = devm_clk_get(&pdev->dev, NULL);
 		if (IS_ERR(hvs->core_clk)) {
 			dev_err(&pdev->dev, "Couldn't get core clock\n");
 			return PTR_ERR(hvs->core_clk);
 		}
 
+		max_rate = rpi_firmware_clk_get_max_rate(hvs->core_clk);
+		if (max_rate >= 550000000)
+			hvs->vc5_hdmi_enable_scrambling = true;
+
 		ret = clk_prepare_enable(hvs->core_clk);
 		if (ret) {
 			dev_err(&pdev->dev, "Couldn't enable the core clock\n");

-- 
b4 0.10.0-dev-a76f5

[PATCH v1 6/7] drm/vc4: hdmi: Add more checks for 4k resolutions

[Maxime Ripard]

From: Dom Cobley <popcornmix@gmail.com>

At least the 4096x2160@60Hz mode requires some overclocking that isn't
available by default, even if hdmi_enable_4kp60 is enabled.

Let's add some logic to detect whether we can satisfy the core clock
requirements for that mode, and prevent it from being used otherwise.

Signed-off-by: Dom Cobley <popcornmix@gmail.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index d48ef302af42..e05f62a7eed6 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -347,6 +347,12 @@ struct vc4_hvs {
 	 * available.
 	 */
 	bool vc5_hdmi_enable_scrambling;
+
+	/*
+	 * 4096x2160@60 requires a core overclock to work, so register
+	 * whether that is sufficient.
+	 */
+	bool vc5_hdmi_enable_4096by2160;
 };
 
 struct vc4_plane {
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 371fbc05bf5a..5abbb2fe41ac 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1476,6 +1476,7 @@ vc4_hdmi_sink_supports_format_bpc(const struct vc4_hdmi *vc4_hdmi,
 
 static enum drm_mode_status
 vc4_hdmi_encoder_clock_valid(const struct vc4_hdmi *vc4_hdmi,
+			     const struct drm_display_mode *mode,
 			     unsigned long long clock)
 {
 	const struct drm_connector *connector = &vc4_hdmi->connector;
@@ -1488,6 +1489,12 @@ vc4_hdmi_encoder_clock_valid(const struct vc4_hdmi *vc4_hdmi,
 	if (!vc4->hvs->vc5_hdmi_enable_scrambling && clock > HDMI_14_MAX_TMDS_CLK)
 		return MODE_CLOCK_HIGH;
 
+	/* 4096x2160@60 is not reliable without overclocking core */
+	if (!vc4->hvs->vc5_hdmi_enable_4096by2160 &&
+	    mode->hdisplay > 3840 && mode->vdisplay >= 2160 &&
+	    drm_mode_vrefresh(mode) >= 50)
+		return MODE_CLOCK_HIGH;
+
 	if (info->max_tmds_clock && clock > (info->max_tmds_clock * 1000))
 		return MODE_CLOCK_HIGH;
 
@@ -1522,7 +1529,7 @@ vc4_hdmi_encoder_compute_clock(const struct vc4_hdmi *vc4_hdmi,
 	unsigned long long clock;
 
 	clock = vc4_hdmi_encoder_compute_mode_clock(mode, bpc, fmt);
-	if (vc4_hdmi_encoder_clock_valid(vc4_hdmi, clock) != MODE_OK)
+	if (vc4_hdmi_encoder_clock_valid(vc4_hdmi, mode, clock) != MODE_OK)
 		return -EINVAL;
 
 	vc4_state->tmds_char_rate = clock;
@@ -1685,7 +1692,7 @@ vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder,
 	     (mode->hsync_end % 2) || (mode->htotal % 2)))
 		return MODE_H_ILLEGAL;
 
-	return vc4_hdmi_encoder_clock_valid(vc4_hdmi, mode->clock * 1000);
+	return vc4_hdmi_encoder_clock_valid(vc4_hdmi, mode, mode->clock * 1000);
 }
 
 static const struct drm_encoder_helper_funcs vc4_hdmi_encoder_helper_funcs = {
diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
index 3fdd2c4356f6..6cfc1a4e7161 100644
--- a/drivers/gpu/drm/vc4/vc4_hvs.c
+++ b/drivers/gpu/drm/vc4/vc4_hvs.c
@@ -673,6 +673,7 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
 	hvs->regset.nregs = ARRAY_SIZE(hvs_regs);
 
 	if (vc4->is_vc5) {
+		unsigned long min_rate;
 		unsigned long max_rate;
 
 		hvs->core_clk = devm_clk_get(&pdev->dev, NULL);
@@ -685,6 +686,10 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
 		if (max_rate >= 550000000)
 			hvs->vc5_hdmi_enable_scrambling = true;
 
+		min_rate = rpi_firmware_clk_get_min_rate(hvs->core_clk);
+		if (min_rate >= 600000000)
+			hvs->vc5_hdmi_enable_4096by2160 = true;
+
 		ret = clk_prepare_enable(hvs->core_clk);
 		if (ret) {
 			dev_err(&pdev->dev, "Couldn't enable the core clock\n");

-- 
b4 0.10.0-dev-a76f5

[PATCH v1 7/7] drm/vc4: Make sure we don't end up with a core clock too high

[Maxime Ripard]

Following the clock rate range improvements to the clock framework,
trying to set a disjoint range on a clock will now result in an error.

Thus, we can't set a minimum rate higher than the maximum reported by
the firmware, or clk_set_min_rate() will fail.

Thus we need to clamp the rate we are about to ask for to the maximum
rate possible on that clock.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index b45dcdfd7306..4794e7235bb0 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -22,6 +22,8 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
+#include <soc/bcm2835/raspberrypi-clocks.h>
+
 #include "vc4_drv.h"
 #include "vc4_regs.h"
 
@@ -354,6 +356,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 	struct vc4_hvs_state *new_hvs_state;
 	struct drm_crtc *crtc;
 	struct vc4_hvs_state *old_hvs_state;
+	unsigned long max_clock_rate;
 	unsigned int channel;
 	int i;
 
@@ -394,11 +397,12 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 		old_hvs_state->fifo_state[channel].pending_commit = NULL;
 	}
 
+	max_clock_rate = rpi_firmware_clk_get_max_rate(hvs->core_clk);
 	if (vc4->is_vc5) {
 		unsigned long state_rate = max(old_hvs_state->core_clock_rate,
 					       new_hvs_state->core_clock_rate);
-		unsigned long core_rate = max_t(unsigned long,
-						500000000, state_rate);
+		unsigned long core_rate = clamp_t(unsigned long, state_rate,
+						  500000000, max_clock_rate);
 
 		drm_dbg(dev, "Raising the core clock at %lu Hz\n", core_rate);
 
@@ -432,14 +436,17 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 	drm_atomic_helper_cleanup_planes(dev, state);
 
 	if (vc4->is_vc5) {
-		drm_dbg(dev, "Running the core clock at %lu Hz\n",
-			new_hvs_state->core_clock_rate);
+		unsigned long core_rate = min_t(unsigned long,
+					       max_clock_rate,
+					       new_hvs_state->core_clock_rate);
+
+		drm_dbg(dev, "Running the core clock at %lu Hz\n", core_rate);
 
 		/*
 		 * Request a clock rate based on the current HVS
 		 * requirements.
 		 */
-		WARN_ON(clk_set_min_rate(hvs->core_clk, new_hvs_state->core_clock_rate));
+		WARN_ON(clk_set_min_rate(hvs->core_clk, core_rate));
 
 		drm_dbg(dev, "Core clock actual rate: %lu Hz\n",
 			clk_get_rate(hvs->core_clk));

-- 
b4 0.10.0-dev-a76f5

Re: [PATCH v1 0/7] drm/vc4: Fix the core clock behaviour

[Maxime Ripard]

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

Hi Stephen, Mike,

On Mon, Aug 15, 2022 at 05:31:22PM +0200, Maxime Ripard wrote:
> Those patches used to be part of a larger clock fixes series:
> https://lore.kernel.org/linux-clk/20220715160014.2623107-1-maxime@cerno.tech/
> 
> However, that series doesn't seem to be getting anywhere, so I've split out
> these patches that fix a regression that has been there since 5.18 and that
> prevents the 4k output from working on the RaspberryPi4.
> 
> Hopefully, we will be able to merge those patches through the DRM tree to avoid
> any further disruption.

I've ping'd Stephen privately on IRC multiple times, and it's basically
a resend of the previous clock series linked above that has been around
since almost a month and a half.

Can you Ack the first three patches so we can merge those patches
through the DRM tree and close this regression?

Maxime

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