| Message ID | 20220815-rpi-fix-4k-60-v4-4-a1b40526df3e@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Fix the core clock behaviour | expand |
On Thu, 20 Oct 2022 at 10:14, <maxime@cerno.tech> wrote: > > 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> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 64f9feabf43e..87961d4de5aa 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> > @@ -3429,7 +3430,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.1
On Wed, 26 Oct 2022 at 16:27, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > On Thu, 20 Oct 2022 at 10:14, <maxime@cerno.tech> wrote: > > > > 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> > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index 64f9feabf43e..87961d4de5aa 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> > > @@ -3429,7 +3430,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); Actually minor nit: rpi_firmware_clk_get_max_rate returns an unsigned int. AFAICT we don't need the range of unsigned long in any subsequent code, so I think it could just be unsigned int here. clk_round_rate returned a long, and therefore previously it did have to be that. Dave > > if (max_rate < 550000000) > > vc4_hdmi->disable_4kp60 = true; > > > > -- > > b4 0.10.1
Hi Dave, Thanks for your review On Wed, Oct 26, 2022 at 04:36:04PM +0100, Dave Stevenson wrote: > On Wed, 26 Oct 2022 at 16:27, Dave Stevenson > <dave.stevenson@raspberrypi.com> wrote: > > > > On Thu, 20 Oct 2022 at 10:14, <maxime@cerno.tech> wrote: > > > > > > 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> > > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > > --- > > > drivers/gpu/drm/vc4/vc4_hdmi.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > index 64f9feabf43e..87961d4de5aa 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> > > > @@ -3429,7 +3430,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); > > Actually minor nit: > rpi_firmware_clk_get_max_rate returns an unsigned int. > AFAICT we don't need the range of unsigned long in any subsequent > code, so I think it could just be unsigned int here. > > clk_round_rate returned a long, and therefore previously it did have to be that. Yeah, I was actually two-minded about this. rpi_firmware_clk_get_max_rate() indeed returns an unsigned long, because that's what the firmware returns. But the clock framework uses unsigned long to store all its frequencies, and in our case here in clk_set_min_rate(). I don't mind changing it to unsigned int here if you prefer to, and if you're fine with the rest of the patches I can fix it up while applying the patches. Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 64f9feabf43e..87961d4de5aa 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> @@ -3429,7 +3430,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;
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 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)