Hi,
Here is a series that enables the higher resolutions on the HDMI0 Controller found in the BCM2711 (RPi4).
In order to work it needs a few adjustments to config.txt, most notably to enable the enable_hdmi_4kp60 and force_turbo options.
The firmware also has a glitch at the moment and will not properly release the BSC controllers, which will make the EDID retrieval fail.
We can work around this using the following config.txt options:
disable_fw_kms_setup=1 hdmi_edid_file:0=1 hdmi_edid_filename:0=1366x768.bin hdmi_ignore_edid:0=1 hdmi_edid_file:1=1 hdmi_edid_filename:1=1366x768.bin hdmi_ignore_edid:1=1
A fix will come for the firmware eventually.
Let me know what you think, Maxime
Maxime Ripard (8): clk: Add range accessors drm/vc4: hvs: Make the HVS bind first drm/vc4: hdmi: Properly compute the BVB clock rate drm/vc4: hdmi: Check and warn if we can't reach 4kp60 frequencies drm/vc4: hdmi: Enable the scrambler drm/vc4: hdmi: Raise the maximum clock rate drm/vc4: plane: Fix typo in scaler width and height drm/vc4: plane: Remove redundant assignment
drivers/clk/clk.c | 30 ++++++++++ drivers/gpu/drm/vc4/vc4_drv.c | 11 +++- drivers/gpu/drm/vc4/vc4_hdmi.c | 88 ++++++++++++++++++++++++++--- drivers/gpu/drm/vc4/vc4_hdmi.h | 8 +++ drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 3 + drivers/gpu/drm/vc4/vc4_plane.c | 5 +- include/linux/clk.h | 16 ++++++ 7 files changed, 148 insertions(+), 13 deletions(-)
Some devices might need to access the current available range of a clock to discover their capabilities. Let's add those accessors.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++ include/linux/clk.h | 16 ++++++++++++++++ 2 files changed, 46 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8c1d04db990d..b7307d4f090d 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate) } EXPORT_SYMBOL_GPL(clk_set_max_rate);
+long clk_get_min_rate(struct clk *clk) +{ + unsigned long min_rate, max_rate; + + if (!clk) + return 0; + + clk_prepare_lock(); + clk_core_get_boundaries(clk->core, &min_rate, &max_rate); + clk_prepare_unlock(); + + return min_rate; +} +EXPORT_SYMBOL_GPL(clk_get_min_rate); + +long clk_get_max_rate(struct clk *clk) +{ + unsigned long min_rate, max_rate; + + if (!clk) + return 0; + + clk_prepare_lock(); + clk_core_get_boundaries(clk->core, &min_rate, &max_rate); + clk_prepare_unlock(); + + return max_rate; +} +EXPORT_SYMBOL_GPL(clk_get_max_rate); + /** * clk_get_parent - return the parent of a clk * @clk: the clk whose parent gets returned diff --git a/include/linux/clk.h b/include/linux/clk.h index 31ff1bf1b79f..6f0c00ddf3ac 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -709,6 +709,22 @@ int clk_set_min_rate(struct clk *clk, unsigned long rate); */ int clk_set_max_rate(struct clk *clk, unsigned long rate);
+/** + * clk_get_min_rate - get the minimum clock rate for a clock source + * @clk: clock source + * + * Returns the minimum rate or negative errno. + */ +long clk_get_min_rate(struct clk *clk); + +/** + * clk_get_max_rate - get the maximum clock rate for a clock source + * @clk: clock source + * + * Returns the maximum rate or negative errno. + */ +long clk_get_max_rate(struct clk *clk); + /** * clk_set_parent - set the parent clock source for this clock * @clk: clock source
Quoting Maxime Ripard (2021-02-25 07:59:02)
Some devices might need to access the current available range of a clock to discover their capabilities. Let's add those accessors.
This needs more than two sentences to describe what's required.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++ include/linux/clk.h | 16 ++++++++++++++++ 2 files changed, 46 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8c1d04db990d..b7307d4f090d 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate) } EXPORT_SYMBOL_GPL(clk_set_max_rate);
+long clk_get_min_rate(struct clk *clk)
I need to read the rest of the patches but I don't see the justification for this sort of API vs. having the consumer constrain the clk frequency that it wants. Is the code that's setting the min/max constraints not the same as the code that's calling this API? Would an OPP table better serve this so the device knows what frequencies are valid?s Please provide the use case/justification in the commit text.
Why two functions instead of one function to get both min and max?
+{
unsigned long min_rate, max_rate;if (!clk)return 0;clk_prepare_lock();
Please add a comment indicating why we grab the lock. Yes clk_core_get_boundaries() has a lock held assertion, but I don't know why we care about the lock here besides that we don't want the consumers to change while we calculate the boundaries as it may be some inaccurate number.
clk_core_get_boundaries(clk->core, &min_rate, &max_rate);clk_prepare_unlock();return min_rate;+} +EXPORT_SYMBOL_GPL(clk_get_min_rate);
+long clk_get_max_rate(struct clk *clk) +{
unsigned long min_rate, max_rate;if (!clk)return 0;
ULONG_MAX?
clk_prepare_lock();clk_core_get_boundaries(clk->core, &min_rate, &max_rate);clk_prepare_unlock();return max_rate;+} +EXPORT_SYMBOL_GPL(clk_get_max_rate);
/**
- clk_get_parent - return the parent of a clk
- @clk: the clk whose parent gets returned
diff --git a/include/linux/clk.h b/include/linux/clk.h index 31ff1bf1b79f..6f0c00ddf3ac 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -709,6 +709,22 @@ int clk_set_min_rate(struct clk *clk, unsigned long rate); */ int clk_set_max_rate(struct clk *clk, unsigned long rate);
+/**
- clk_get_min_rate - get the minimum clock rate for a clock source
- @clk: clock source
- Returns the minimum rate or negative errno.
Hmm?
- */
+long clk_get_min_rate(struct clk *clk);
Why long instead of unsigned long? Error values don't seem to be returned.
+/**
- clk_get_max_rate - get the maximum clock rate for a clock source
- @clk: clock source
- Returns the maximum rate or negative errno.
- */
+long clk_get_max_rate(struct clk *clk);
/**
- clk_set_parent - set the parent clock source for this clock
- @clk: clock source
--
Hi Stephen,
On Tue, Mar 02, 2021 at 03:18:58PM -0800, Stephen Boyd wrote:
Quoting Maxime Ripard (2021-02-25 07:59:02)
Some devices might need to access the current available range of a clock to discover their capabilities. Let's add those accessors.
This needs more than two sentences to describe what's required.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++ include/linux/clk.h | 16 ++++++++++++++++ 2 files changed, 46 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8c1d04db990d..b7307d4f090d 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate) } EXPORT_SYMBOL_GPL(clk_set_max_rate);
+long clk_get_min_rate(struct clk *clk)
I need to read the rest of the patches but I don't see the justification for this sort of API vs. having the consumer constrain the clk frequency that it wants. Is the code that's setting the min/max constraints not the same as the code that's calling this API? Would an OPP table better serve this so the device knows what frequencies are valid?s Please provide the use case/justification in the commit text.
The patch that uses it is the patch 4
The issue I'm trying to solve is that all the RaspberryPi have a configuration file for the firmware, and the firmware is in charge of the clocks communicating through a mailbox interface.
By default, one of the main clocks in the system can only reach 500MHz, and that's the range reported by the firmware when queried. However, in order to support display modes with a fairly high bandwidth such as 4k at 60Hz, that clock needs to be raised to at least 550MHz, and the firmware configuration has a special parameter for that one. Setting that parameter will increase the range of the clock to have proper boundaries for that display mode.
If a user doesn't enable it and tries to use those display modes, the display will be completely blank.
There's no way to query the firmware configuration directly, so we can instead query the range of the clock and see if the firmware enables us to use those modes, warn the user and ignore the modes that wouldn't work. That's what those accessors are here for
Why two functions instead of one function to get both min and max?
Since we have clk_set_min_rate and clk_set_max_rate, it made sense to me to mirror that, but I'd be happy to change if you think otherwise
I'll address your other commenst
Maxime
Quoting Maxime Ripard (2021-03-03 00:45:27)
Hi Stephen,
On Tue, Mar 02, 2021 at 03:18:58PM -0800, Stephen Boyd wrote:
Quoting Maxime Ripard (2021-02-25 07:59:02)
Some devices might need to access the current available range of a clock to discover their capabilities. Let's add those accessors.
This needs more than two sentences to describe what's required.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++ include/linux/clk.h | 16 ++++++++++++++++ 2 files changed, 46 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8c1d04db990d..b7307d4f090d 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate) } EXPORT_SYMBOL_GPL(clk_set_max_rate);
+long clk_get_min_rate(struct clk *clk)
I need to read the rest of the patches but I don't see the justification for this sort of API vs. having the consumer constrain the clk frequency that it wants. Is the code that's setting the min/max constraints not the same as the code that's calling this API? Would an OPP table better serve this so the device knows what frequencies are valid?s Please provide the use case/justification in the commit text.
The patch that uses it is the patch 4
The issue I'm trying to solve is that all the RaspberryPi have a configuration file for the firmware, and the firmware is in charge of the clocks communicating through a mailbox interface.
By default, one of the main clocks in the system can only reach 500MHz, and that's the range reported by the firmware when queried. However, in order to support display modes with a fairly high bandwidth such as 4k at 60Hz, that clock needs to be raised to at least 550MHz, and the firmware configuration has a special parameter for that one. Setting that parameter will increase the range of the clock to have proper boundaries for that display mode.
If a user doesn't enable it and tries to use those display modes, the display will be completely blank.
There's no way to query the firmware configuration directly, so we can instead query the range of the clock and see if the firmware enables us to use those modes, warn the user and ignore the modes that wouldn't work. That's what those accessors are here for
How does the clk driver query the firmware but it can't be done directly by the drm driver?
Why two functions instead of one function to get both min and max?
Since we have clk_set_min_rate and clk_set_max_rate, it made sense to me to mirror that, but I'd be happy to change if you think otherwise
Does using clk_round_rate() work just as well?
On Tue, Mar 16, 2021 at 06:06:40PM -0700, Stephen Boyd wrote:
Quoting Maxime Ripard (2021-03-03 00:45:27)
Hi Stephen,
On Tue, Mar 02, 2021 at 03:18:58PM -0800, Stephen Boyd wrote:
Quoting Maxime Ripard (2021-02-25 07:59:02)
Some devices might need to access the current available range of a clock to discover their capabilities. Let's add those accessors.
This needs more than two sentences to describe what's required.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++ include/linux/clk.h | 16 ++++++++++++++++ 2 files changed, 46 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8c1d04db990d..b7307d4f090d 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate) } EXPORT_SYMBOL_GPL(clk_set_max_rate);
+long clk_get_min_rate(struct clk *clk)
I need to read the rest of the patches but I don't see the justification for this sort of API vs. having the consumer constrain the clk frequency that it wants. Is the code that's setting the min/max constraints not the same as the code that's calling this API? Would an OPP table better serve this so the device knows what frequencies are valid?s Please provide the use case/justification in the commit text.
The patch that uses it is the patch 4
The issue I'm trying to solve is that all the RaspberryPi have a configuration file for the firmware, and the firmware is in charge of the clocks communicating through a mailbox interface.
By default, one of the main clocks in the system can only reach 500MHz, and that's the range reported by the firmware when queried. However, in order to support display modes with a fairly high bandwidth such as 4k at 60Hz, that clock needs to be raised to at least 550MHz, and the firmware configuration has a special parameter for that one. Setting that parameter will increase the range of the clock to have proper boundaries for that display mode.
If a user doesn't enable it and tries to use those display modes, the display will be completely blank.
There's no way to query the firmware configuration directly, so we can instead query the range of the clock and see if the firmware enables us to use those modes, warn the user and ignore the modes that wouldn't work. That's what those accessors are here for
How does the clk driver query the firmware but it can't be done directly by the drm driver?
The configuration is done through a text file accessed by the firmware. What I meant was that the kernel cannot access the content of that file to make sure the right options have been enabled.
However, it can indeed communicate with the firmware through the extent of the API it provides, but it's fairly limited. In our case, the only way to tell is to look for side effects of the configuration option, ie the maximum rate of the clock that has been increased.
Why two functions instead of one function to get both min and max?
Since we have clk_set_min_rate and clk_set_max_rate, it made sense to me to mirror that, but I'd be happy to change if you think otherwise
Does using clk_round_rate() work just as well?
I guess it could work too, I'll try it out
Maxime
We'll need to have the HVS binding before the HDMI controllers so that we can check whether the firmware allows to run in 4kp60. Reorder a bit the component list, and document the current constraints we're aware of.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_drv.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index 556ad0f02a0d..0310590ee66e 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -303,12 +303,21 @@ static const struct component_master_ops vc4_drm_ops = { .unbind = vc4_drm_unbind, };
+/* + * This list determines the binding order of our components, and we have + * a few constraints: + * - The TXP driver needs to be bound before the PixelValves (CRTC) + * but after the HVS to set the possible_crtc field properly + * - The HDMI driver needs to be bound after the HVS so that we can + * lookup the HVS maximum core clock rate and figure out if we + * support 4kp60 or not. + */ static struct platform_driver *const component_drivers[] = { + &vc4_hvs_driver, &vc4_hdmi_driver, &vc4_vec_driver, &vc4_dpi_driver, &vc4_dsi_driver, - &vc4_hvs_driver, &vc4_txp_driver, &vc4_crtc_driver, &vc4_v3d_driver,
On Thu, 25 Feb 2021 at 15:59, Maxime Ripard maxime@cerno.tech wrote:
We'll need to have the HVS binding before the HDMI controllers so that we can check whether the firmware allows to run in 4kp60. Reorder a bit the component list, and document the current constraints we're aware of.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Based on my understanding of bind ordering this makes sense, but I don't consider myself an expert there.
Acked-by: Dave Stevenson dave.stevenson@raspberrypi.com
drivers/gpu/drm/vc4/vc4_drv.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index 556ad0f02a0d..0310590ee66e 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -303,12 +303,21 @@ static const struct component_master_ops vc4_drm_ops = { .unbind = vc4_drm_unbind, };
+/*
- This list determines the binding order of our components, and we have
- a few constraints:
- The TXP driver needs to be bound before the PixelValves (CRTC)
but after the HVS to set the possible_crtc field properly
- The HDMI driver needs to be bound after the HVS so that we can
lookup the HVS maximum core clock rate and figure out if we
support 4kp60 or not.- */
static struct platform_driver *const component_drivers[] = {
&vc4_hvs_driver, &vc4_hdmi_driver, &vc4_vec_driver, &vc4_dpi_driver, &vc4_dsi_driver,
&vc4_hvs_driver, &vc4_txp_driver, &vc4_crtc_driver, &vc4_v3d_driver,-- 2.29.2
The BVB clock rate computation doesn't take into account a mode clock of 594MHz that we're going to need to support 4k60.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index eee9751009c2..b5bc742993a4 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -91,7 +91,6 @@ # define VC4_HD_M_ENABLE BIT(0)
#define CEC_CLOCK_FREQ 40000 -#define VC4_HSM_MID_CLOCK 149985000
#define HDMI_14_MAX_TMDS_CLK (340 * 1000 * 1000)
@@ -739,7 +738,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, conn_state_to_vc4_hdmi_conn_state(conn_state); struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); - unsigned long pixel_rate, hsm_rate; + unsigned long bvb_rate, pixel_rate, hsm_rate; int ret;
ret = pm_runtime_get_sync(&vc4_hdmi->pdev->dev); @@ -793,12 +792,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
vc4_hdmi_cec_update_clk_div(vc4_hdmi);
- /* - * FIXME: When the pixel freq is 594MHz (4k60), this needs to be setup - * at 300MHz. - */ - ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock, - (hsm_rate > VC4_HSM_MID_CLOCK ? 150000000 : 75000000)); + bvb_rate = roundup(mode->clock * 1000 / 2, 75000000); + ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock, bvb_rate); if (ret) { DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret); clk_disable_unprepare(vc4_hdmi->hsm_clock);
Hi Maxime
On Thu, 25 Feb 2021 at 15:59, Maxime Ripard maxime@cerno.tech wrote:
The BVB clock rate computation doesn't take into account a mode clock of 594MHz that we're going to need to support 4k60.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index eee9751009c2..b5bc742993a4 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -91,7 +91,6 @@ # define VC4_HD_M_ENABLE BIT(0)
#define CEC_CLOCK_FREQ 40000 -#define VC4_HSM_MID_CLOCK 149985000
#define HDMI_14_MAX_TMDS_CLK (340 * 1000 * 1000)
@@ -739,7 +738,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, conn_state_to_vc4_hdmi_conn_state(conn_state); struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
unsigned long pixel_rate, hsm_rate;
unsigned long bvb_rate, pixel_rate, hsm_rate; int ret; ret = pm_runtime_get_sync(&vc4_hdmi->pdev->dev);@@ -793,12 +792,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
vc4_hdmi_cec_update_clk_div(vc4_hdmi);
/** FIXME: When the pixel freq is 594MHz (4k60), this needs to be setup* at 300MHz.*/ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock,(hsm_rate > VC4_HSM_MID_CLOCK ? 150000000 : 75000000));
bvb_rate = roundup(mode->clock * 1000 / 2, 75000000);
Minor hesitation here that I would need to check the hardware over. As I read your code, if the clock falls 300MHz and 450MHz then you'd ask for a bvb_rate of 225MHz. Depending on what the source clock is that may not be valid. The firmware goes for 75, 150, or 300MHz only.
Dave
ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock, bvb_rate); if (ret) { DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret); clk_disable_unprepare(vc4_hdmi->hsm_clock);-- 2.29.2
On Thu, 25 Feb 2021 at 16:44, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi Maxime
On Thu, 25 Feb 2021 at 15:59, Maxime Ripard maxime@cerno.tech wrote:
The BVB clock rate computation doesn't take into account a mode clock of 594MHz that we're going to need to support 4k60.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index eee9751009c2..b5bc742993a4 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -91,7 +91,6 @@ # define VC4_HD_M_ENABLE BIT(0)
#define CEC_CLOCK_FREQ 40000 -#define VC4_HSM_MID_CLOCK 149985000
#define HDMI_14_MAX_TMDS_CLK (340 * 1000 * 1000)
@@ -739,7 +738,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, conn_state_to_vc4_hdmi_conn_state(conn_state); struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
unsigned long pixel_rate, hsm_rate;
unsigned long bvb_rate, pixel_rate, hsm_rate; int ret; ret = pm_runtime_get_sync(&vc4_hdmi->pdev->dev);@@ -793,12 +792,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
vc4_hdmi_cec_update_clk_div(vc4_hdmi);
/** FIXME: When the pixel freq is 594MHz (4k60), this needs to be setup* at 300MHz.*/ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock,(hsm_rate > VC4_HSM_MID_CLOCK ? 150000000 : 75000000));
bvb_rate = roundup(mode->clock * 1000 / 2, 75000000);Minor hesitation here that I would need to check the hardware over. As I read your code, if the clock falls 300MHz and 450MHz then you'd ask for a bvb_rate of 225MHz. Depending on what the source clock is that may not be valid. The firmware goes for 75, 150, or 300MHz only.
The information I have says it has to be an integer divide of 600MHz (PLLC @ 3GHz / 5), so 225MHz is not valid.
(Minor contradictory information though as PLLC is bumped to 3.3GHz with enable_4kp60, but we still appear to get 300MHz for BVB after the /5. A little more checking warranted around here).
Dave
ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock, bvb_rate); if (ret) { DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret); clk_disable_unprepare(vc4_hdmi->hsm_clock);-- 2.29.2
In order to reach the frequencies needed to output at 594MHz, the firmware needs to be configured with the appropriate parameters in the config.txt file (enable_hdmi_4kp60 and force_turbo).
Let's detect it at bind time, warn the user if we can't, and filter out the relevant modes.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++++ drivers/gpu/drm/vc4/vc4_hdmi.h | 8 ++++++++ 2 files changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b5bc742993a4..f05f6da286f7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -953,6 +953,9 @@ static int vc4_hdmi_encoder_atomic_check(struct drm_encoder *encoder, if (pixel_rate > vc4_hdmi->variant->max_pixel_clock) return -EINVAL;
+ if (vc4_hdmi->disable_4kp60 && (pixel_rate > HDMI_14_MAX_TMDS_CLK)) + return -EINVAL; + vc4_state->pixel_rate = pixel_rate;
return 0; @@ -972,6 +975,9 @@ vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder, if ((mode->clock * 1000) > vc4_hdmi->variant->max_pixel_clock) return MODE_CLOCK_HIGH;
+ if (vc4_hdmi->disable_4kp60 && ((mode->clock * 1000) > HDMI_14_MAX_TMDS_CLK)) + return MODE_CLOCK_HIGH; + return MODE_OK; }
@@ -1986,6 +1992,17 @@ 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); + long max_rate = clk_get_max_rate(vc4->hvs->core_clk); + + if (max_rate < 550000000) { + drm_warn(drm, "The core clock cannot reach frequencies high enough to support 4k @ 60Hz."); + drm_warn(drm, "Please change your config.txt file to add hdmi_enable_4kp60 and force_turbo"); + vc4_hdmi->disable_4kp60 = true; + } + } + if (vc4_hdmi->variant->reset) vc4_hdmi->variant->reset(vc4_hdmi);
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 3cebd1fd00fc..3cd021136402 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -154,6 +154,14 @@ 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;
Hi Maxime
On Thu, 25 Feb 2021 at 15:59, Maxime Ripard maxime@cerno.tech wrote:
In order to reach the frequencies needed to output at 594MHz, the firmware needs to be configured with the appropriate parameters in the config.txt file (enable_hdmi_4kp60 and force_turbo).
force_turbo isn't the right way to go about this as it permanently bumps all the clocks up, even if running the display at VGA.
Let's detect it at bind time, warn the user if we can't, and filter out the relevant modes.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++++ drivers/gpu/drm/vc4/vc4_hdmi.h | 8 ++++++++ 2 files changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b5bc742993a4..f05f6da286f7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -953,6 +953,9 @@ static int vc4_hdmi_encoder_atomic_check(struct drm_encoder *encoder, if (pixel_rate > vc4_hdmi->variant->max_pixel_clock) return -EINVAL;
if (vc4_hdmi->disable_4kp60 && (pixel_rate > HDMI_14_MAX_TMDS_CLK))return -EINVAL;vc4_state->pixel_rate = pixel_rate; return 0;@@ -972,6 +975,9 @@ vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder, if ((mode->clock * 1000) > vc4_hdmi->variant->max_pixel_clock) return MODE_CLOCK_HIGH;
if (vc4_hdmi->disable_4kp60 && ((mode->clock * 1000) > HDMI_14_MAX_TMDS_CLK))return MODE_CLOCK_HIGH;return MODE_OK;}
@@ -1986,6 +1992,17 @@ 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);long max_rate = clk_get_max_rate(vc4->hvs->core_clk);if (max_rate < 550000000) {drm_warn(drm, "The core clock cannot reach frequencies high enough to support 4k @ 60Hz.");drm_warn(drm, "Please change your config.txt file to add hdmi_enable_4kp60 and force_turbo");
Do we really want to warn in bind? Again you could have a VGA resolution monitor attached but that would trigger this warning. Can we warn (once) on processing the mode list and filtering out a clk
HDMI_14_MAX_TMDS_CLK mode instead?
And mentioning force_turbo is again wrong.
Dave
vc4_hdmi->disable_4kp60 = true;}}if (vc4_hdmi->variant->reset) vc4_hdmi->variant->reset(vc4_hdmi);diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 3cebd1fd00fc..3cd021136402 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -154,6 +154,14 @@ 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;-- 2.29.2
Hi Dave,
Thanks for your review
On Thu, Feb 25, 2021 at 04:38:37PM +0000, Dave Stevenson wrote:
On Thu, 25 Feb 2021 at 15:59, Maxime Ripard maxime@cerno.tech wrote:
In order to reach the frequencies needed to output at 594MHz, the firmware needs to be configured with the appropriate parameters in the config.txt file (enable_hdmi_4kp60 and force_turbo).
force_turbo isn't the right way to go about this as it permanently bumps all the clocks up, even if running the display at VGA.
so enable_hdmi_4kp60 is enough?
Let's detect it at bind time, warn the user if we can't, and filter out the relevant modes.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++++ drivers/gpu/drm/vc4/vc4_hdmi.h | 8 ++++++++ 2 files changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b5bc742993a4..f05f6da286f7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -953,6 +953,9 @@ static int vc4_hdmi_encoder_atomic_check(struct drm_encoder *encoder, if (pixel_rate > vc4_hdmi->variant->max_pixel_clock) return -EINVAL;
if (vc4_hdmi->disable_4kp60 && (pixel_rate > HDMI_14_MAX_TMDS_CLK))return -EINVAL;vc4_state->pixel_rate = pixel_rate; return 0;@@ -972,6 +975,9 @@ vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder, if ((mode->clock * 1000) > vc4_hdmi->variant->max_pixel_clock) return MODE_CLOCK_HIGH;
if (vc4_hdmi->disable_4kp60 && ((mode->clock * 1000) > HDMI_14_MAX_TMDS_CLK))return MODE_CLOCK_HIGH;return MODE_OK;}
@@ -1986,6 +1992,17 @@ 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);long max_rate = clk_get_max_rate(vc4->hvs->core_clk);if (max_rate < 550000000) {drm_warn(drm, "The core clock cannot reach frequencies high enough to support 4k @ 60Hz.");drm_warn(drm, "Please change your config.txt file to add hdmi_enable_4kp60 and force_turbo");Do we really want to warn in bind? Again you could have a VGA resolution monitor attached but that would trigger this warning. Can we warn (once) on processing the mode list and filtering out a clk
HDMI_14_MAX_TMDS_CLK mode instead?
That's a good idea indeed, I'll rework the patch to do that
Thanks! Maxime
On Tue, 2 Mar 2021 at 13:02, Maxime Ripard maxime@cerno.tech wrote:
Hi Dave,
Thanks for your review
On Thu, Feb 25, 2021 at 04:38:37PM +0000, Dave Stevenson wrote:
On Thu, 25 Feb 2021 at 15:59, Maxime Ripard maxime@cerno.tech wrote:
In order to reach the frequencies needed to output at 594MHz, the firmware needs to be configured with the appropriate parameters in the config.txt file (enable_hdmi_4kp60 and force_turbo).
force_turbo isn't the right way to go about this as it permanently bumps all the clocks up, even if running the display at VGA.
so enable_hdmi_4kp60 is enough?
No, but force_turbo=1 is the wrong way to go about fixing that.
I'll start a thread with Dom & Tim to discuss the best way of doing this. Immediate thoughts are that either vc4_hdmi needs to request the core clock be increased, or potentially the firmware could note the boosted BVB rate and increase core accordingly.
Dave
Let's detect it at bind time, warn the user if we can't, and filter out the relevant modes.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++++ drivers/gpu/drm/vc4/vc4_hdmi.h | 8 ++++++++ 2 files changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b5bc742993a4..f05f6da286f7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -953,6 +953,9 @@ static int vc4_hdmi_encoder_atomic_check(struct drm_encoder *encoder, if (pixel_rate > vc4_hdmi->variant->max_pixel_clock) return -EINVAL;
if (vc4_hdmi->disable_4kp60 && (pixel_rate > HDMI_14_MAX_TMDS_CLK))return -EINVAL;vc4_state->pixel_rate = pixel_rate; return 0;@@ -972,6 +975,9 @@ vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder, if ((mode->clock * 1000) > vc4_hdmi->variant->max_pixel_clock) return MODE_CLOCK_HIGH;
if (vc4_hdmi->disable_4kp60 && ((mode->clock * 1000) > HDMI_14_MAX_TMDS_CLK))return MODE_CLOCK_HIGH;return MODE_OK;}
@@ -1986,6 +1992,17 @@ 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);long max_rate = clk_get_max_rate(vc4->hvs->core_clk);if (max_rate < 550000000) {drm_warn(drm, "The core clock cannot reach frequencies high enough to support 4k @ 60Hz.");drm_warn(drm, "Please change your config.txt file to add hdmi_enable_4kp60 and force_turbo");Do we really want to warn in bind? Again you could have a VGA resolution monitor attached but that would trigger this warning. Can we warn (once) on processing the mode list and filtering out a clk
HDMI_14_MAX_TMDS_CLK mode instead?
That's a good idea indeed, I'll rework the patch to do that
Thanks! Maxime
The HDMI controller on the BCM2711 includes a scrambler in order to reach the modes that require it. Let's add the support for it.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 58 +++++++++++++++++++++++++++++ drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 3 ++ 2 files changed, 61 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index f05f6da286f7..1a6babb53cf4 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -35,6 +35,7 @@ #include <drm/drm_edid.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> +#include <drm/drm_scdc_helper.h> #include <linux/clk.h> #include <linux/component.h> #include <linux/i2c.h> @@ -76,6 +77,8 @@ #define VC5_HDMI_VERTB_VSPO_SHIFT 16 #define VC5_HDMI_VERTB_VSPO_MASK VC4_MASK(29, 16)
+#define VC5_HDMI_SCRAMBLER_CTL_ENABLE BIT(0) + #define VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE_SHIFT 8 #define VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE_MASK VC4_MASK(10, 8)
@@ -445,6 +448,58 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder) vc4_hdmi_set_audio_infoframe(encoder); }
+#define HDMI_14_MAX_TMDS_CLK (340 * 1000 * 1000) + +static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder, + struct drm_display_mode *mode) +{ + struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); + struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_display_info *display = &vc4_hdmi->connector.display_info; + + if (!vc4_encoder->hdmi_monitor) + return false; + + if (!display->hdmi.scdc.supported || + !display->hdmi.scdc.scrambling.supported) + return false; + + if ((mode->clock * 1000) < HDMI_14_MAX_TMDS_CLK) + return false; + + return true; +} + +static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) +{ + struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; + struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + + if (!vc4_hdmi_supports_scrambling(encoder, mode)) + return; + + drm_scdc_set_high_tmds_clock_ratio(vc4_hdmi->ddc, true); + drm_scdc_set_scrambling(vc4_hdmi->ddc, true); + + HDMI_WRITE(HDMI_SCRAMBLER_CTL, HDMI_READ(HDMI_SCRAMBLER_CTL) | + VC5_HDMI_SCRAMBLER_CTL_ENABLE); +} + +static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder) +{ + struct drm_display_mode *mode = &encoder->crtc->mode; + struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + + if (!vc4_hdmi_supports_scrambling(encoder, mode)) + return; + + HDMI_WRITE(HDMI_SCRAMBLER_CTL, HDMI_READ(HDMI_SCRAMBLER_CTL) & + ~VC5_HDMI_SCRAMBLER_CTL_ENABLE); + + drm_scdc_set_scrambling(vc4_hdmi->ddc, false); + drm_scdc_set_high_tmds_clock_ratio(vc4_hdmi->ddc, false); +} + static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { @@ -457,6 +512,8 @@ static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder,
HDMI_WRITE(HDMI_VID_CTL, HDMI_READ(HDMI_VID_CTL) | VC4_HD_VID_CTL_BLANKPIX); + + vc4_hdmi_disable_scrambling(encoder); }
static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder, @@ -901,6 +958,7 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, }
vc4_hdmi_recenter_fifo(vc4_hdmi); + vc4_hdmi_enable_scrambling(encoder); }
static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h index e1b58eac766f..6897586228ad 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h @@ -100,6 +100,7 @@ enum vc4_hdmi_field { HDMI_RM_FORMAT, HDMI_RM_OFFSET, HDMI_SCHEDULER_CONTROL, + HDMI_SCRAMBLER_CTL, HDMI_SW_RESET_CONTROL, HDMI_TX_PHY_CHANNEL_SWAP, HDMI_TX_PHY_CLK_DIV, @@ -234,6 +235,7 @@ static const struct vc4_hdmi_register __maybe_unused vc5_hdmi_hdmi0_fields[] = { VC4_HDMI_REG(HDMI_VERTB1, 0x0f8), VC4_HDMI_REG(HDMI_MAI_CHANNEL_MAP, 0x09c), VC4_HDMI_REG(HDMI_MAI_CONFIG, 0x0a0), + VC4_HDMI_REG(HDMI_SCRAMBLER_CTL, 0x1c4), VC4_HDMI_REG(HDMI_DEEP_COLOR_CONFIG_1, 0x170), VC4_HDMI_REG(HDMI_GCP_CONFIG, 0x178), VC4_HDMI_REG(HDMI_GCP_WORD_1, 0x17c), @@ -313,6 +315,7 @@ static const struct vc4_hdmi_register __maybe_unused vc5_hdmi_hdmi1_fields[] = { VC4_HDMI_REG(HDMI_VERTB1, 0x0f8), VC4_HDMI_REG(HDMI_MAI_CHANNEL_MAP, 0x09c), VC4_HDMI_REG(HDMI_MAI_CONFIG, 0x0a0), + VC4_HDMI_REG(HDMI_SCRAMBLER_CTL, 0x1c4), VC4_HDMI_REG(HDMI_DEEP_COLOR_CONFIG_1, 0x170), VC4_HDMI_REG(HDMI_GCP_CONFIG, 0x178), VC4_HDMI_REG(HDMI_GCP_WORD_1, 0x17c),
Hi Maxime
On Thu, 25 Feb 2021 at 15:59, Maxime Ripard maxime@cerno.tech wrote:
The HDMI controller on the BCM2711 includes a scrambler in order to reach the modes that require it. Let's add the support for it.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 58 +++++++++++++++++++++++++++++ drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 3 ++ 2 files changed, 61 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index f05f6da286f7..1a6babb53cf4 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -35,6 +35,7 @@ #include <drm/drm_edid.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> +#include <drm/drm_scdc_helper.h> #include <linux/clk.h> #include <linux/component.h> #include <linux/i2c.h> @@ -76,6 +77,8 @@ #define VC5_HDMI_VERTB_VSPO_SHIFT 16 #define VC5_HDMI_VERTB_VSPO_MASK VC4_MASK(29, 16)
+#define VC5_HDMI_SCRAMBLER_CTL_ENABLE BIT(0)
#define VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE_SHIFT 8 #define VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE_MASK VC4_MASK(10, 8)
@@ -445,6 +448,58 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder) vc4_hdmi_set_audio_infoframe(encoder); }
+#define HDMI_14_MAX_TMDS_CLK (340 * 1000 * 1000)
Something feels a bit funny here. drm-misc-next already has a commit [1] that adds a define HDMI_14_MAX_TMDS_CLK. Part of it is in the diff for 3/8. So is there a need to redefine it in this patch?
[1] https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/vc4/vc4_hdm...
+static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder,
struct drm_display_mode *mode)+{
struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);struct drm_display_info *display = &vc4_hdmi->connector.display_info;if (!vc4_encoder->hdmi_monitor)return false;if (!display->hdmi.scdc.supported ||!display->hdmi.scdc.scrambling.supported)return false;if ((mode->clock * 1000) < HDMI_14_MAX_TMDS_CLK)return false;return true;+}
+static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) +{
struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);if (!vc4_hdmi_supports_scrambling(encoder, mode))return;drm_scdc_set_high_tmds_clock_ratio(vc4_hdmi->ddc, true);drm_scdc_set_scrambling(vc4_hdmi->ddc, true);HDMI_WRITE(HDMI_SCRAMBLER_CTL, HDMI_READ(HDMI_SCRAMBLER_CTL) |VC5_HDMI_SCRAMBLER_CTL_ENABLE);+}
+static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder) +{
struct drm_display_mode *mode = &encoder->crtc->mode;struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);if (!vc4_hdmi_supports_scrambling(encoder, mode))return;HDMI_WRITE(HDMI_SCRAMBLER_CTL, HDMI_READ(HDMI_SCRAMBLER_CTL) &~VC5_HDMI_SCRAMBLER_CTL_ENABLE);drm_scdc_set_scrambling(vc4_hdmi->ddc, false);drm_scdc_set_high_tmds_clock_ratio(vc4_hdmi->ddc, false);+}
static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { @@ -457,6 +512,8 @@ static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder,
HDMI_WRITE(HDMI_VID_CTL, HDMI_READ(HDMI_VID_CTL) | VC4_HD_VID_CTL_BLANKPIX);
vc4_hdmi_disable_scrambling(encoder);}
static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder, @@ -901,6 +958,7 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, }
vc4_hdmi_recenter_fifo(vc4_hdmi);
vc4_hdmi_enable_scrambling(encoder);}
static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h index e1b58eac766f..6897586228ad 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h @@ -100,6 +100,7 @@ enum vc4_hdmi_field { HDMI_RM_FORMAT, HDMI_RM_OFFSET, HDMI_SCHEDULER_CONTROL,
HDMI_SCRAMBLER_CTL, HDMI_SW_RESET_CONTROL, HDMI_TX_PHY_CHANNEL_SWAP, HDMI_TX_PHY_CLK_DIV,@@ -234,6 +235,7 @@ static const struct vc4_hdmi_register __maybe_unused vc5_hdmi_hdmi0_fields[] = { VC4_HDMI_REG(HDMI_VERTB1, 0x0f8), VC4_HDMI_REG(HDMI_MAI_CHANNEL_MAP, 0x09c), VC4_HDMI_REG(HDMI_MAI_CONFIG, 0x0a0),
VC4_HDMI_REG(HDMI_SCRAMBLER_CTL, 0x1c4),
Nit pick: the rest of these registers are in numerical order, but these new additions aren't.
Dave
VC4_HDMI_REG(HDMI_DEEP_COLOR_CONFIG_1, 0x170), VC4_HDMI_REG(HDMI_GCP_CONFIG, 0x178), VC4_HDMI_REG(HDMI_GCP_WORD_1, 0x17c),@@ -313,6 +315,7 @@ static const struct vc4_hdmi_register __maybe_unused vc5_hdmi_hdmi1_fields[] = { VC4_HDMI_REG(HDMI_VERTB1, 0x0f8), VC4_HDMI_REG(HDMI_MAI_CHANNEL_MAP, 0x09c), VC4_HDMI_REG(HDMI_MAI_CONFIG, 0x0a0),
VC4_HDMI_REG(HDMI_SCRAMBLER_CTL, 0x1c4), VC4_HDMI_REG(HDMI_DEEP_COLOR_CONFIG_1, 0x170), VC4_HDMI_REG(HDMI_GCP_CONFIG, 0x178), VC4_HDMI_REG(HDMI_GCP_WORD_1, 0x17c),-- 2.29.2
Now that we have the infrastructure in place, we can raise the maximum pixel rate we can reach for HDMI0 on the BCM2711.
HDMI1 is left untouched since its pixelvalve has a smaller FIFO and would need a clock faster than what we can provide to support the same modes.
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 1a6babb53cf4..27464add6468 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2177,7 +2177,7 @@ static const struct vc4_hdmi_variant bcm2711_hdmi0_variant = { .encoder_type = VC4_ENCODER_TYPE_HDMI0, .debugfs_name = "hdmi0_regs", .card_name = "vc4-hdmi-0", - .max_pixel_clock = HDMI_14_MAX_TMDS_CLK, + .max_pixel_clock = 600000000, .registers = vc5_hdmi_hdmi0_fields, .num_registers = ARRAY_SIZE(vc5_hdmi_hdmi0_fields), .phy_lane_mapping = {
On Thu, 25 Feb 2021 at 15:59, Maxime Ripard maxime@cerno.tech wrote:
Now that we have the infrastructure in place, we can raise the maximum pixel rate we can reach for HDMI0 on the BCM2711.
HDMI1 is left untouched since its pixelvalve has a smaller FIFO and would need a clock faster than what we can provide to support the same modes.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
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 1a6babb53cf4..27464add6468 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2177,7 +2177,7 @@ static const struct vc4_hdmi_variant bcm2711_hdmi0_variant = { .encoder_type = VC4_ENCODER_TYPE_HDMI0, .debugfs_name = "hdmi0_regs", .card_name = "vc4-hdmi-0",
.max_pixel_clock = HDMI_14_MAX_TMDS_CLK,
.max_pixel_clock = 600000000, .registers = vc5_hdmi_hdmi0_fields, .num_registers = ARRAY_SIZE(vc5_hdmi_hdmi0_fields), .phy_lane_mapping = {-- 2.29.2
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_plane.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 4a075294ff4c..5e8b7f72dc05 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -912,9 +912,9 @@ static int vc4_plane_mode_set(struct drm_plane *plane, if (!vc4_state->is_unity) { vc4_dlist_write(vc4_state, VC4_SET_FIELD(vc4_state->crtc_w, - SCALER_POS1_SCL_WIDTH) | + SCALER5_POS1_SCL_WIDTH) | VC4_SET_FIELD(vc4_state->crtc_h, - SCALER_POS1_SCL_HEIGHT)); + SCALER5_POS1_SCL_HEIGHT)); }
/* Position Word 2: Source Image Size */
On Thu, 25 Feb 2021 at 15:59, Maxime Ripard maxime@cerno.tech wrote:
Signed-off-by: Maxime Ripard maxime@cerno.tech
Again no commit text body, but possibly not warranted
Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
drivers/gpu/drm/vc4/vc4_plane.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 4a075294ff4c..5e8b7f72dc05 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -912,9 +912,9 @@ static int vc4_plane_mode_set(struct drm_plane *plane, if (!vc4_state->is_unity) { vc4_dlist_write(vc4_state, VC4_SET_FIELD(vc4_state->crtc_w,
SCALER_POS1_SCL_WIDTH) |
SCALER5_POS1_SCL_WIDTH) | VC4_SET_FIELD(vc4_state->crtc_h,
SCALER_POS1_SCL_HEIGHT));
SCALER5_POS1_SCL_HEIGHT)); } /* Position Word 2: Source Image Size */-- 2.29.2
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_plane.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 5e8b7f72dc05..201831e924d9 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -1131,7 +1131,6 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane, plane->state->src_y = state->src_y; plane->state->src_w = state->src_w; plane->state->src_h = state->src_h; - plane->state->src_h = state->src_h; plane->state->alpha = state->alpha; plane->state->pixel_blend_mode = state->pixel_blend_mode; plane->state->rotation = state->rotation;
On Thu, 25 Feb 2021 at 15:59, Maxime Ripard maxime@cerno.tech wrote:
Signed-off-by: Maxime Ripard maxime@cerno.tech
Other than no commit text body (which is hardly needed in this case)
Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
drivers/gpu/drm/vc4/vc4_plane.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 5e8b7f72dc05..201831e924d9 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -1131,7 +1131,6 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane, plane->state->src_y = state->src_y; plane->state->src_w = state->src_w; plane->state->src_h = state->src_h;
plane->state->src_h = state->src_h; plane->state->alpha = state->alpha; plane->state->pixel_blend_mode = state->pixel_blend_mode; plane->state->rotation = state->rotation;-- 2.29.2
Hi Dave,
On Thu, Feb 25, 2021 at 04:46:48PM +0000, Dave Stevenson wrote:
On Thu, 25 Feb 2021 at 15:59, Maxime Ripard maxime@cerno.tech wrote:
Signed-off-by: Maxime Ripard maxime@cerno.tech
Other than no commit text body (which is hardly needed in this case)
Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
Yeah the last two patches weren't meant to be part of this series, I'll add a commit log and resend them.
Thanks! Maxime
dri-devel@lists.freedesktop.org
-
Dave Stevenson -
Maxime Ripard -
Stephen Boyd