| Message ID | 20220516132527.328190-5-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | clk: More clock rate fixes and tests | expand |
Quoting Maxime Ripard (2022-05-16 06:25:03) > 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> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 6aadb65eb640..962a1b9b1c4f 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -2891,7 +2891,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 = clk_get_max_rate(vc4->hvs->core_clk); Ok, so this driver must want the new API. What is happening here though? The driver is setting 'disable_4kp60' at bind/probe time based on a clk_round_rate() returning a frequency. That returned value could change at runtime though based on rate constraints, or simply because the clk driver decides that the wind is blowing differently today and thus calling clk_set_rate() with that frequency will cause the clk to be wildly different than before. I don't understand how we can decide to disable 4kp60 at probe time. Why doesn't the driver try to set the rate it wants (or the rate range it wants) and then if that succeeds it knows 4kp60 is achievable and if not then it rejects the attempt by userspace to set such a resolution.
Hi, On Mon, Jun 27, 2022 at 04:31:04PM -0700, Stephen Boyd wrote: > Quoting Maxime Ripard (2022-05-16 06:25:03) > > 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> > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index 6aadb65eb640..962a1b9b1c4f 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > @@ -2891,7 +2891,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 = clk_get_max_rate(vc4->hvs->core_clk); > > Ok, so this driver must want the new API. > > What is happening here though? The driver is setting 'disable_4kp60' at > bind/probe time based on a clk_round_rate() returning a frequency. The main issue that we're trying to address is that whether or not HDMI modes with a rate over 340MHz (so most likely 4k/60Hz but others are affected) need a bootloader parameter to be set. If it isn't set, we can't output those modes. Since it's a bootloader parameter though the kernel can't access it. The main hint that we can use to figure out whether it's been enabled is that the maximum clock frequency reported by the firmware will be higher. So this code will try to round a frequency higher than the maximum allowed when that setting isn't there, and thus figure out whether it's enabled or not. If it's not, we prevent any of these modes from being exposed to userspace or being used. > That returned value could change at runtime though based on rate > constraints, or simply because the clk driver decides that the wind is > blowing differently today and thus calling clk_set_rate() with that > frequency will cause the clk to be wildly different than before. Yeah, that's true > I don't understand how we can decide to disable 4kp60 at probe time. We're trying to infer a bootloader/firmware parameter, so the only way it can change is through a reboot. > Why doesn't the driver try to set the rate it wants (or the rate range > it wants) and then if that succeeds it knows 4kp60 is achievable and > if not then it rejects the attempt by userspace to set such a > resolution. We can't really do that. The clock here drives the HDMI output so it can only change when we change the mode. However, because of the atomic commits in KMS, we can't fail when we actually change the mode, we have to fail beforehand when we check that the new state is sane. We also can't change the clock rate then, because there's no guarantee that the state being checked is actually going to be committed, and because we still have the hardware running when we check it so we would modify the clock while the hardware is running. I had another go in the RaspberryPi downstream kernel for this: https://github.com/raspberrypi/linux/commit/df368502ecbe1de26cf02a9b7837da9e967d64ca Would that be preferable? Maxime
Quoting Maxime Ripard (2022-06-28 02:47:53) > Hi, > > On Mon, Jun 27, 2022 at 04:31:04PM -0700, Stephen Boyd wrote: > > > > Ok, so this driver must want the new API. > > > > What is happening here though? The driver is setting 'disable_4kp60' at > > bind/probe time based on a clk_round_rate() returning a frequency. > > The main issue that we're trying to address is that whether or not HDMI > modes with a rate over 340MHz (so most likely 4k/60Hz but others are > affected) need a bootloader parameter to be set. If it isn't set, we > can't output those modes. > > Since it's a bootloader parameter though the kernel can't access it. The > main hint that we can use to figure out whether it's been enabled is > that the maximum clock frequency reported by the firmware will be > higher. So this code will try to round a frequency higher than the > maximum allowed when that setting isn't there, and thus figure out > whether it's enabled or not. So the kernel is at least able to ask for the max frequency during rounding and figure out that the frequency can't be larger than that. Is that right? > > If it's not, we prevent any of these modes from being exposed to > userspace or being used. > > > That returned value could change at runtime though based on rate > > constraints, or simply because the clk driver decides that the wind is > > blowing differently today and thus calling clk_set_rate() with that > > frequency will cause the clk to be wildly different than before. > > Yeah, that's true > > > I don't understand how we can decide to disable 4kp60 at probe time. > > We're trying to infer a bootloader/firmware parameter, so the only way > it can change is through a reboot. Got it. > > > Why doesn't the driver try to set the rate it wants (or the rate range > > it wants) and then if that succeeds it knows 4kp60 is achievable and > > if not then it rejects the attempt by userspace to set such a > > resolution. > > We can't really do that. The clock here drives the HDMI output so it can > only change when we change the mode. However, because of the atomic > commits in KMS, we can't fail when we actually change the mode, we have > to fail beforehand when we check that the new state is sane. Alright. I feel like we've discussed this before. > > We also can't change the clock rate then, because there's no guarantee > that the state being checked is actually going to be committed, and > because we still have the hardware running when we check it so we would > modify the clock while the hardware is running. > > I had another go in the RaspberryPi downstream kernel for this: > https://github.com/raspberrypi/linux/commit/df368502ecbe1de26cf02a9b7837da9e967d64ca > It really looks to me like we're trying to hide the firmware API behind the clk API. When the clk API doesn't provide what's needed, we add an API to expose internal clk details that the clk consumer should already know because it sets them. That's my main concern here. The driver is querying an OPP table through the clk framework. Why can't we either expose the firmware API directly to this driver or have the firmware driver probe and populate/trim an OPP table for this display device so that it can query the OPP table at probe time to determine the maximum frequency supported. The clk framework isn't in the business of exposing OPP tables, that's what the OPP framework is for.
On Wed, Jun 29, 2022 at 01:48:42AM -0700, Stephen Boyd wrote: > Quoting Maxime Ripard (2022-06-28 02:47:53) > > Hi, > > > > On Mon, Jun 27, 2022 at 04:31:04PM -0700, Stephen Boyd wrote: > > > > > > Ok, so this driver must want the new API. > > > > > > What is happening here though? The driver is setting 'disable_4kp60' at > > > bind/probe time based on a clk_round_rate() returning a frequency. > > > > The main issue that we're trying to address is that whether or not HDMI > > modes with a rate over 340MHz (so most likely 4k/60Hz but others are > > affected) need a bootloader parameter to be set. If it isn't set, we > > can't output those modes. > > > > Since it's a bootloader parameter though the kernel can't access it. The > > main hint that we can use to figure out whether it's been enabled is > > that the maximum clock frequency reported by the firmware will be > > higher. So this code will try to round a frequency higher than the > > maximum allowed when that setting isn't there, and thus figure out > > whether it's enabled or not. > > So the kernel is at least able to ask for the max frequency during > rounding and figure out that the frequency can't be larger than that. Is > that right? Yes, the firmware has a call that allows to query the boundaries of a given clock it manages, and we then used whatever value it returns to set the clk_hw rate range. See https://elixir.bootlin.com/linux/latest/source/drivers/clk/bcm/clk-raspberrypi.c#L287 > > If it's not, we prevent any of these modes from being exposed to > > userspace or being used. > > > > > That returned value could change at runtime though based on rate > > > constraints, or simply because the clk driver decides that the wind is > > > blowing differently today and thus calling clk_set_rate() with that > > > frequency will cause the clk to be wildly different than before. > > > > Yeah, that's true > > > > > I don't understand how we can decide to disable 4kp60 at probe time. > > > > We're trying to infer a bootloader/firmware parameter, so the only way > > it can change is through a reboot. > > Got it. > > > > > > Why doesn't the driver try to set the rate it wants (or the rate range > > > it wants) and then if that succeeds it knows 4kp60 is achievable and > > > if not then it rejects the attempt by userspace to set such a > > > resolution. > > > > We can't really do that. The clock here drives the HDMI output so it can > > only change when we change the mode. However, because of the atomic > > commits in KMS, we can't fail when we actually change the mode, we have > > to fail beforehand when we check that the new state is sane. > > Alright. I feel like we've discussed this before. > > > > > We also can't change the clock rate then, because there's no guarantee > > that the state being checked is actually going to be committed, and > > because we still have the hardware running when we check it so we would > > modify the clock while the hardware is running. > > > > I had another go in the RaspberryPi downstream kernel for this: > > https://github.com/raspberrypi/linux/commit/df368502ecbe1de26cf02a9b7837da9e967d64ca > > > > It really looks to me like we're trying to hide the firmware API behind > the clk API. When the clk API doesn't provide what's needed, we add an > API to expose internal clk details that the clk consumer should already > know because it sets them. That would work for me > That's my main concern here. The driver is querying an OPP table > through the clk framework. > > Why can't we either expose the firmware API directly to this driver or > have the firmware driver probe and populate/trim an OPP table for this > display device so that it can query the OPP table at probe time to > determine the maximum frequency supported. The clk framework isn't in > the business of exposing OPP tables, that's what the OPP framework is > for. Is it really an OPP? My understanding at least is that an OPP table is a discrete set of frequencies that a device can operate at, but you still need the frequency generator somewhere else? What we're discussing here definitely looks more like a clock to me: it's is the frequency generator, and can expose a continuous set of frequencies between a range. It just turns out that depending on a parameter that range changes a bit, and it then affect our capabilities. Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 6aadb65eb640..962a1b9b1c4f 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2891,7 +2891,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 = clk_get_max_rate(vc4->hvs->core_clk); if (max_rate < 550000000) vc4_hdmi->disable_4kp60 = true;
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> --- drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)