[PATCH 0/8] drm/vc4: hdmi: Support the 4k @ 60Hz modes

[PATCH 0/8] drm/vc4: hdmi: Support the 4k @ 60Hz modes

[Maxime Ripard]

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(-)

-- 
2.29.2

[PATCH 0/8] drm/vc4: hdmi: Support the 4k @ 60Hz modes

[Maxime Ripard]

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(-)

-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/8] clk: Add range accessors

[Maxime Ripard]

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
-- 
2.29.2

[PATCH 1/8] clk: Add range accessors

[Maxime Ripard]

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
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/8] drm/vc4: hvs: Make the HVS bind first

[Maxime Ripard]

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,
-- 
2.29.2

[PATCH 2/8] drm/vc4: hvs: Make the HVS bind first

[Maxime Ripard]

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,
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/8] drm/vc4: hdmi: Properly compute the BVB clock rate

[Maxime Ripard]

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);
-- 
2.29.2

[PATCH 3/8] drm/vc4: hdmi: Properly compute the BVB clock rate

[Maxime Ripard]

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);
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/8] drm/vc4: hdmi: Check and warn if we can't reach 4kp60 frequencies

[Maxime Ripard]

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;
-- 
2.29.2

[PATCH 4/8] drm/vc4: hdmi: Check and warn if we can't reach 4kp60 frequencies

[Maxime Ripard]

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;
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/8] drm/vc4: hdmi: Enable the scrambler

[Maxime Ripard]

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),
-- 
2.29.2

[PATCH 5/8] drm/vc4: hdmi: Enable the scrambler

[Maxime Ripard]

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),
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/8] drm/vc4: hdmi: Raise the maximum clock rate

[Maxime Ripard]

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	= {
-- 
2.29.2

[PATCH 6/8] drm/vc4: hdmi: Raise the maximum clock rate

[Maxime Ripard]

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	= {
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/8] drm/vc4: plane: Fix typo in scaler width and height

[Maxime Ripard]

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 */
-- 
2.29.2

[PATCH 7/8] drm/vc4: plane: Fix typo in scaler width and height

[Maxime Ripard]

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 */
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 8/8] drm/vc4: plane: Remove redundant assignment

[Maxime Ripard]

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;
-- 
2.29.2

[PATCH 8/8] drm/vc4: plane: Remove redundant assignment

[Maxime Ripard]

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;
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/8] drm/vc4: hdmi: Enable the scrambler

[Dave Stevenson]

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_hdmi.c?id=24169a2b0533a6c4030c91a7a074039e7c98fde6

> +
> +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
>

Re: [PATCH 5/8] drm/vc4: hdmi: Enable the scrambler

[Dave Stevenson]

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_hdmi.c?id=24169a2b0533a6c4030c91a7a074039e7c98fde6

> +
> +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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/8] drm/vc4: hdmi: Check and warn if we can't reach 4kp60 frequencies

[Dave Stevenson]

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
>

Re: [PATCH 4/8] drm/vc4: hdmi: Check and warn if we can't reach 4kp60 frequencies

[Dave Stevenson]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm/vc4: hdmi: Properly compute the BVB clock rate

[Dave Stevenson]

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
>

Re: [PATCH 3/8] drm/vc4: hdmi: Properly compute the BVB clock rate

[Dave Stevenson]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm/vc4: hdmi: Raise the maximum clock rate

[Dave Stevenson]

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
>

Re: [PATCH 6/8] drm/vc4: hdmi: Raise the maximum clock rate

[Dave Stevenson]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm/vc4: plane: Remove redundant assignment

[Dave Stevenson]

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
>

Re: [PATCH 8/8] drm/vc4: plane: Remove redundant assignment

[Dave Stevenson]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] drm/vc4: plane: Fix typo in scaler width and height

[Dave Stevenson]

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
>

Re: [PATCH 7/8] drm/vc4: plane: Fix typo in scaler width and height

[Dave Stevenson]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] drm/vc4: hvs: Make the HVS bind first

[Dave Stevenson]

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
>

Re: [PATCH 2/8] drm/vc4: hvs: Make the HVS bind first

[Dave Stevenson]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm/vc4: hdmi: Properly compute the BVB clock rate

[Dave Stevenson]

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
> >

Re: [PATCH 3/8] drm/vc4: hdmi: Properly compute the BVB clock rate

[Dave Stevenson]

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
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/8] drm/vc4: hdmi: Check and warn if we can't reach 4kp60 frequencies

[Maxime Ripard]

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

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

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

Re: [PATCH 4/8] drm/vc4: hdmi: Check and warn if we can't reach 4kp60 frequencies

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2972 bytes --]

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

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm/vc4: plane: Remove redundant assignment

[Maxime Ripard]

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

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

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

Re: [PATCH 8/8] drm/vc4: plane: Remove redundant assignment

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 483 bytes --]

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

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/8] drm/vc4: hdmi: Check and warn if we can't reach 4kp60 frequencies

[Dave Stevenson]

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

Re: [PATCH 4/8] drm/vc4: hdmi: Check and warn if we can't reach 4kp60 frequencies

[Dave Stevenson]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] clk: Add range accessors

[Stephen Boyd]

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
> --

Re: [PATCH 1/8] clk: Add range accessors

[Stephen Boyd]

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
> --
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] clk: Add range accessors

[Maxime Ripard]

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

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

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

Re: [PATCH 1/8] clk: Add range accessors

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2677 bytes --]

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

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] clk: Add range accessors

[Stephen Boyd]

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?

Re: [PATCH 1/8] clk: Add range accessors

[Stephen Boyd]

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?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] clk: Add range accessors

[Maxime Ripard]

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

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

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

Re: [PATCH 1/8] clk: Add range accessors

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3661 bytes --]

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

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel