[PATCH v8 00/10] drm/vc4: hdmi: Support the 4k @ 60Hz modes

[PATCH v8 00/10] 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 option.

Let me know what you think,
Maxime

---

Changes from v7:
  - Rebased on current drm-misc-next

Changes from v6:
  - Rebased on current drm-misc-next
  - Removed stale clk_request pointer

Changes from v5:
  - Fixed unused variables warning

Changes from v4:
  - Removed the patches already applied
  - Added various fixes for the issues that have been discovered on the
    downstream tree

Changes from v3:
  - Rework the encoder retrieval code that was broken on the RPi3 and older
  - Fix a scrambling enabling issue on some display

Changes from v2:
  - Gathered the various tags
  - Added Cc stable when relevant
  - Split out the check to test whether the scrambler is required into
    an helper
  - Fixed a bug where the scrambler state wouldn't be tracked properly
    if it was enabled at boot

Changes from v1:
  - Dropped the range accessors
  - Drop the mention of force_turbo
  - Reordered the SCRAMBLER_CTL register to match the offset
  - Removed duplicate HDMI_14_MAX_TMDS_CLK define
  - Warn about enable_hdmi_4kp60 only if there's some modes that can't be reached
  - Rework the BVB clock computation

Maxime Ripard (10):
  drm/vc4: hdmi: Remove the DDC probing for status detection
  drm/vc4: hdmi: Fix HPD GPIO detection
  drm/vc4: Make vc4_crtc_get_encoder public
  drm/vc4: crtc: Add encoder to vc4_crtc_config_pv prototype
  drm/vc4: crtc: Rework the encoder retrieval code (again)
  drm/vc4: crtc: Add some logging
  drm/vc4: Leverage the load tracker on the BCM2711
  drm/vc4: hdmi: Raise the maximum clock rate
  drm/vc4: hdmi: Enable the scrambler on reconnection
  drm/vc4: Increase the core clock based on HVS load

 drivers/gpu/drm/vc4/vc4_crtc.c    |  60 ++++++++------
 drivers/gpu/drm/vc4/vc4_debugfs.c |   7 +-
 drivers/gpu/drm/vc4/vc4_drv.h     |   8 +-
 drivers/gpu/drm/vc4/vc4_hdmi.c    |  13 +--
 drivers/gpu/drm/vc4/vc4_kms.c     | 126 +++++++++++++++++++++++++-----
 drivers/gpu/drm/vc4/vc4_plane.c   |   5 --
 6 files changed, 157 insertions(+), 62 deletions(-)

-- 
2.31.1

[PATCH v8 01/10] drm/vc4: hdmi: Remove the DDC probing for status detection

[Maxime Ripard]

Commit 9d44abbbb8d5 ("drm/vc4: Fall back to using an EDID probe in the
absence of a GPIO.") added some code to read the EDID through DDC in the
HDMI driver detect hook since the Pi3 had no HPD GPIO back then.
However, commit b1b8f45b3130 ("ARM: dts: bcm2837: Add missing GPIOs of
Expander") changed that a couple of years later.

This causes an issue though since some TV (like the LG 55C8) when it
comes out of standy will deassert the HPD line, but the EDID will
remain readable.

It causes an issues nn platforms without an HPD GPIO, like the Pi4,
where the DDC probing will be our primary mean to detect a display, and
thus we will never detect the HPD pulse. This was fine before since the
pulse was small enough that we would never detect it, and we also didn't
have anything (like the scrambler) that needed to be set up in the
display.

However, now that we have both, the display during the HPD pulse will
clear its scrambler status, and since we won't detect the
disconnect/reconnect cycle we will never enable the scrambler back.

As our main reason for that DDC probing is gone, let's just remove it.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 7b0cb08e6563..338968275724 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -193,8 +193,6 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 	if (vc4_hdmi->hpd_gpio &&
 	    gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) {
 		connected = true;
-	} else if (drm_probe_ddc(vc4_hdmi->ddc)) {
-		connected = true;
 	} else {
 		unsigned long flags;
 		u32 hotplug;
-- 
2.31.1

[PATCH v8 02/10] drm/vc4: hdmi: Fix HPD GPIO detection

[Maxime Ripard]

Prior to commit 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod"), in the
detect hook, if we had an HPD GPIO we would only rely on it and return
whatever state it was in.

However, that commit changed that by mistake to only consider the case
where we have a GPIO and it returns a logical high, and would fall back
to the other methods otherwise.

Since we can read the EDIDs when the HPD signal is low on some displays,
we changed the detection status from disconnected to connected, and we
would ignore an HPD pulse.

Fixes: 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 338968275724..dde67b991ae7 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -190,9 +190,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 
 	WARN_ON(pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev));
 
-	if (vc4_hdmi->hpd_gpio &&
-	    gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) {
-		connected = true;
+	if (vc4_hdmi->hpd_gpio) {
+		if (gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio))
+			connected = true;
 	} else {
 		unsigned long flags;
 		u32 hotplug;
-- 
2.31.1

[PATCH v8 03/10] drm/vc4: Make vc4_crtc_get_encoder public

[Maxime Ripard]

We'll need that function in vc4_kms to compute the core clock rate
requirements.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 8 ++++----
 drivers/gpu/drm/vc4/vc4_drv.h  | 5 +++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index e3ed52d96f42..7cfd4a097847 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -281,10 +281,10 @@ static u32 vc4_crtc_get_fifo_full_level_bits(struct vc4_crtc *vc4_crtc,
  * allows drivers to push pixels to more than one encoder from the
  * same CRTC.
  */
-static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
-						struct drm_atomic_state *state,
-						struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
-											 struct drm_connector *connector))
+struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
+					 struct drm_atomic_state *state,
+					 struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
+										  struct drm_connector *connector))
 {
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 4b550ebd9572..f5e678491502 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -544,6 +544,11 @@ vc4_crtc_to_vc4_pv_data(const struct vc4_crtc *crtc)
 	return container_of(data, struct vc4_pv_data, base);
 }
 
+struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
+					 struct drm_atomic_state *state,
+					 struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
+										  struct drm_connector *connector));
+
 struct vc4_crtc_state {
 	struct drm_crtc_state base;
 	/* Dlist area for this CRTC configuration. */
-- 
2.31.1

[PATCH v8 04/10] drm/vc4: crtc: Add encoder to vc4_crtc_config_pv prototype

[Maxime Ripard]

vc4_crtc_config_pv() retrieves the encoder again, even though its only
caller, vc4_crtc_atomic_enable(), already did.

Pass the encoder pointer as an argument instead of going through all the
connectors to retrieve it again.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 7cfd4a097847..e5c2e29a6f01 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -315,12 +315,11 @@ static void vc4_crtc_pixelvalve_reset(struct drm_crtc *crtc)
 	CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) | PV_CONTROL_FIFO_CLR);
 }
 
-static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_atomic_state *state)
+static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_encoder *encoder,
+			       struct drm_atomic_state *state)
 {
 	struct drm_device *dev = crtc->dev;
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
-	struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
-							   drm_atomic_get_new_connector_state);
 	struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
 	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 	const struct vc4_pv_data *pv_data = vc4_crtc_to_vc4_pv_data(vc4_crtc);
@@ -597,7 +596,7 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
 	if (vc4_encoder->pre_crtc_configure)
 		vc4_encoder->pre_crtc_configure(encoder, state);
 
-	vc4_crtc_config_pv(crtc, state);
+	vc4_crtc_config_pv(crtc, encoder, state);
 
 	CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) | PV_CONTROL_EN);
 
-- 
2.31.1

[PATCH v8 05/10] drm/vc4: crtc: Rework the encoder retrieval code (again)

[Maxime Ripard]

It turns out the encoder retrieval code, in addition to being
unnecessarily complicated, has a bug when only the planes and crtcs are
affected by a given atomic commit.

Indeed, in such a case, either drm_atomic_get_old_connector_state or
drm_atomic_get_new_connector_state will return NULL and thus our encoder
retrieval code will not match on anything.

We can however simplify the code by using drm_for_each_encoder_mask, the
drm_crtc_state storing the encoders a given CRTC is connected to
directly and without relying on any other state.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 30 +++++++++---------------------
 drivers/gpu/drm/vc4/vc4_drv.h  |  4 +---
 2 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index e5c2e29a6f01..fbc1d4638650 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -282,26 +282,14 @@ static u32 vc4_crtc_get_fifo_full_level_bits(struct vc4_crtc *vc4_crtc,
  * same CRTC.
  */
 struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
-					 struct drm_atomic_state *state,
-					 struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
-										  struct drm_connector *connector))
+					 struct drm_crtc_state *state)
 {
-	struct drm_connector *connector;
-	struct drm_connector_list_iter conn_iter;
+	struct drm_encoder *encoder;
 
-	drm_connector_list_iter_begin(crtc->dev, &conn_iter);
-	drm_for_each_connector_iter(connector, &conn_iter) {
-		struct drm_connector_state *conn_state = get_state(state, connector);
+	WARN_ON(hweight32(state->encoder_mask) > 1);
 
-		if (!conn_state)
-			continue;
-
-		if (conn_state->crtc == crtc) {
-			drm_connector_list_iter_end(&conn_iter);
-			return connector->encoder;
-		}
-	}
-	drm_connector_list_iter_end(&conn_iter);
+	drm_for_each_encoder_mask(encoder, crtc->dev, state->encoder_mask)
+		return encoder;
 
 	return NULL;
 }
@@ -550,8 +538,7 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
 	struct drm_crtc_state *old_state = drm_atomic_get_old_crtc_state(state,
 									 crtc);
 	struct vc4_crtc_state *old_vc4_state = to_vc4_crtc_state(old_state);
-	struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
-							   drm_atomic_get_old_connector_state);
+	struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, old_state);
 	struct drm_device *dev = crtc->dev;
 
 	require_hvs_enabled(dev);
@@ -578,10 +565,11 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
 static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
 				   struct drm_atomic_state *state)
 {
+	struct drm_crtc_state *new_state = drm_atomic_get_new_crtc_state(state,
+									 crtc);
 	struct drm_device *dev = crtc->dev;
 	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
-	struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
-							   drm_atomic_get_new_connector_state);
+	struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, new_state);
 	struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
 
 	require_hvs_enabled(dev);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index f5e678491502..60826aca9e5b 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -545,9 +545,7 @@ vc4_crtc_to_vc4_pv_data(const struct vc4_crtc *crtc)
 }
 
 struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
-					 struct drm_atomic_state *state,
-					 struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
-										  struct drm_connector *connector));
+					 struct drm_crtc_state *state);
 
 struct vc4_crtc_state {
 	struct drm_crtc_state base;
-- 
2.31.1

[PATCH v8 06/10] drm/vc4: crtc: Add some logging

[Maxime Ripard]

The encoder retrieval code has been a source of bugs and glitches in the
past and the crtc <-> encoder association been wrong in a number of
different ways.

Add some logging to quickly spot issues if they occur.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index fbc1d4638650..6decaa12a078 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -541,6 +541,9 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
 	struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, old_state);
 	struct drm_device *dev = crtc->dev;
 
+	drm_dbg(dev, "Disabling CRTC %s (%u) connected to Encoder %s (%u)",
+		crtc->name, crtc->base.id, encoder->name, encoder->base.id);
+
 	require_hvs_enabled(dev);
 
 	/* Disable vblank irq handling before crtc is disabled. */
@@ -572,6 +575,9 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
 	struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, new_state);
 	struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
 
+	drm_dbg(dev, "Enabling CRTC %s (%u) connected to Encoder %s (%u)",
+		crtc->name, crtc->base.id, encoder->name, encoder->base.id);
+
 	require_hvs_enabled(dev);
 
 	/* Enable vblank irq handling before crtc is started otherwise
-- 
2.31.1

[PATCH v8 07/10] drm/vc4: Leverage the load tracker on the BCM2711

[Maxime Ripard]

The load tracker was initially designed to report and warn about a load
too high for the HVS. To do so, it computes for each plane the impact
it's going to have on the HVS, and will warn (if it's enabled) if we go
over what the hardware can process.

While the limits being used are a bit irrelevant to the BCM2711, the
algorithm to compute the HVS load will be one component used in order to
compute the core clock rate on the BCM2711.

Let's remove the hooks to prevent the load tracker to do its
computation, but since we don't have the same limits, don't check them
against them, and prevent the debugfs file to enable it from being
created.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_debugfs.c |  7 +++++--
 drivers/gpu/drm/vc4/vc4_drv.h     |  3 ---
 drivers/gpu/drm/vc4/vc4_kms.c     | 16 +++++-----------
 drivers/gpu/drm/vc4/vc4_plane.c   |  5 -----
 4 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
index 6da22af4ee91..ba2d8ea562af 100644
--- a/drivers/gpu/drm/vc4/vc4_debugfs.c
+++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
@@ -7,6 +7,7 @@
 #include <linux/circ_buf.h>
 #include <linux/ctype.h>
 #include <linux/debugfs.h>
+#include <linux/platform_device.h>
 
 #include "vc4_drv.h"
 #include "vc4_regs.h"
@@ -26,8 +27,10 @@ vc4_debugfs_init(struct drm_minor *minor)
 	struct vc4_dev *vc4 = to_vc4_dev(minor->dev);
 	struct vc4_debugfs_info_entry *entry;
 
-	debugfs_create_bool("hvs_load_tracker", S_IRUGO | S_IWUSR,
-			    minor->debugfs_root, &vc4->load_tracker_enabled);
+	if (!of_device_is_compatible(vc4->hvs->pdev->dev.of_node,
+				     "brcm,bcm2711-vc5"))
+		debugfs_create_bool("hvs_load_tracker", S_IRUGO | S_IWUSR,
+				    minor->debugfs_root, &vc4->load_tracker_enabled);
 
 	list_for_each_entry(entry, &vc4->debugfs_list, link) {
 		drm_debugfs_create_files(&entry->info, 1,
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 60826aca9e5b..813c5d0ea98e 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -202,9 +202,6 @@ struct vc4_dev {
 
 	int power_refcount;
 
-	/* Set to true when the load tracker is supported. */
-	bool load_tracker_available;
-
 	/* Set to true when the load tracker is active. */
 	bool load_tracker_enabled;
 
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 028f19f7a5f8..41cb4869da50 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -552,9 +552,6 @@ static int vc4_load_tracker_atomic_check(struct drm_atomic_state *state)
 	struct drm_plane *plane;
 	int i;
 
-	if (!vc4->load_tracker_available)
-		return 0;
-
 	priv_state = drm_atomic_get_private_obj_state(state,
 						      &vc4->load_tracker);
 	if (IS_ERR(priv_state))
@@ -629,9 +626,6 @@ static void vc4_load_tracker_obj_fini(struct drm_device *dev, void *unused)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
 
-	if (!vc4->load_tracker_available)
-		return;
-
 	drm_atomic_private_obj_fini(&vc4->load_tracker);
 }
 
@@ -639,9 +633,6 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 {
 	struct vc4_load_tracker_state *load_state;
 
-	if (!vc4->load_tracker_available)
-		return 0;
-
 	load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
 	if (!load_state)
 		return -ENOMEM;
@@ -869,9 +860,12 @@ int vc4_kms_load(struct drm_device *dev)
 					      "brcm,bcm2711-vc5");
 	int ret;
 
+	/*
+	 * The limits enforced by the load tracker aren't relevant for
+	 * the BCM2711, but the load tracker computations are used for
+	 * the core clock rate calculation.
+	 */
 	if (!is_vc5) {
-		vc4->load_tracker_available = true;
-
 		/* Start with the load tracker enabled. Can be
 		 * disabled through the debugfs load_tracker file.
 		 */
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 19161b6ab27f..ac761c683663 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -529,11 +529,6 @@ static void vc4_plane_calc_load(struct drm_plane_state *state)
 	struct vc4_plane_state *vc4_state;
 	struct drm_crtc_state *crtc_state;
 	unsigned int vscale_factor;
-	struct vc4_dev *vc4;
-
-	vc4 = to_vc4_dev(state->plane->dev);
-	if (!vc4->load_tracker_available)
-		return;
 
 	vc4_state = to_vc4_plane_state(state);
 	crtc_state = drm_atomic_get_existing_crtc_state(state->state,
-- 
2.31.1

[PATCH v8 08/10] 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.

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
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 dde67b991ae7..d36b3b6ebed1 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2706,7 +2706,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.31.1

[PATCH v8 09/10] drm/vc4: hdmi: Enable the scrambler on reconnection

[Maxime Ripard]

If we have a state already and disconnect/reconnect the display, the
SCDC messages won't be sent again since we didn't go through a disable /
enable cycle.

In order to fix this, let's call the vc4_hdmi_enable_scrambling function
in the detect callback if there is a mode and it needs the scrambler to
be enabled.

Fixes: c85695a2016e ("drm/vc4: hdmi: Enable the scrambler")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index d36b3b6ebed1..fab9b93e1b84 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -180,6 +180,8 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
 static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
 #endif
 
+static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder);
+
 static enum drm_connector_status
 vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 {
@@ -216,6 +218,7 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 			}
 		}
 
+		vc4_hdmi_enable_scrambling(&vc4_hdmi->encoder.base.base);
 		pm_runtime_put(&vc4_hdmi->pdev->dev);
 		mutex_unlock(&vc4_hdmi->mutex);
 		return connector_status_connected;
-- 
2.31.1

[PATCH v8 10/10] drm/vc4: Increase the core clock based on HVS load

[Maxime Ripard]

Depending on a given HVS output (HVS to PixelValves) and input (planes
attached to a channel) load, the HVS needs for the core clock to be
raised above its boot time default.

Failing to do so will result in a vblank timeout and a stalled display
pipeline.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_crtc.c |  15 +++++
 drivers/gpu/drm/vc4/vc4_drv.h  |   2 +
 drivers/gpu/drm/vc4/vc4_kms.c  | 110 ++++++++++++++++++++++++++++++---
 3 files changed, 118 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 6decaa12a078..287dbc89ad64 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -659,12 +659,27 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
 	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
 	struct drm_connector *conn;
 	struct drm_connector_state *conn_state;
+	struct drm_encoder *encoder;
 	int ret, i;
 
 	ret = vc4_hvs_atomic_check(crtc, state);
 	if (ret)
 		return ret;
 
+	encoder = vc4_get_crtc_encoder(crtc, crtc_state);
+	if (encoder) {
+		const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
+		struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
+
+		mode = &crtc_state->adjusted_mode;
+		if (vc4_encoder->type == VC4_ENCODER_TYPE_HDMI0) {
+			vc4_state->hvs_load = max(mode->clock * mode->hdisplay / mode->htotal + 1000,
+						  mode->clock * 9 / 10) * 1000;
+		} else {
+			vc4_state->hvs_load = mode->clock * 1000;
+		}
+	}
+
 	for_each_new_connector_in_state(state, conn, conn_state,
 					i) {
 		if (conn_state->crtc != crtc)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 813c5d0ea98e..4329e09d357c 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -558,6 +558,8 @@ struct vc4_crtc_state {
 		unsigned int bottom;
 	} margins;
 
+	unsigned long hvs_load;
+
 	/* Transitional state below, only valid during atomic commits */
 	bool update_muxing;
 };
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 41cb4869da50..79d4d9dd1394 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -39,9 +39,11 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
 
 struct vc4_hvs_state {
 	struct drm_private_state base;
+	unsigned long core_clock_rate;
 
 	struct {
 		unsigned in_use: 1;
+		unsigned long fifo_load;
 		struct drm_crtc_commit *pending_commit;
 	} fifo_state[HVS_NUM_CHANNELS];
 };
@@ -340,10 +342,19 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 	struct vc4_hvs *hvs = vc4->hvs;
 	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_state *new_crtc_state;
+	struct vc4_hvs_state *new_hvs_state;
 	struct drm_crtc *crtc;
 	struct vc4_hvs_state *old_hvs_state;
 	int i;
 
+	old_hvs_state = vc4_hvs_get_old_global_state(state);
+	if (WARN_ON(!old_hvs_state))
+		return;
+
+	new_hvs_state = vc4_hvs_get_new_global_state(state);
+	if (WARN_ON(!new_hvs_state))
+		return;
+
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
 		struct vc4_crtc_state *vc4_crtc_state;
 
@@ -354,12 +365,13 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 		vc4_hvs_mask_underrun(dev, vc4_crtc_state->assigned_channel);
 	}
 
-	if (vc4->hvs->hvs5)
-		clk_set_min_rate(hvs->core_clk, 500000000);
+	if (vc4->hvs->hvs5) {
+		unsigned long core_rate = max_t(unsigned long,
+						500000000,
+						new_hvs_state->core_clock_rate);
 
-	old_hvs_state = vc4_hvs_get_old_global_state(state);
-	if (!old_hvs_state)
-		return;
+		clk_set_min_rate(hvs->core_clk, core_rate);
+	}
 
 	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
 		struct vc4_crtc_state *vc4_crtc_state =
@@ -399,8 +411,12 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 
 	drm_atomic_helper_cleanup_planes(dev, state);
 
-	if (vc4->hvs->hvs5)
-		clk_set_min_rate(hvs->core_clk, 0);
+	if (vc4->hvs->hvs5) {
+		drm_dbg(dev, "Running the core clock at %lu Hz\n",
+			new_hvs_state->core_clock_rate);
+
+		clk_set_min_rate(hvs->core_clk, new_hvs_state->core_clock_rate);
+	}
 }
 
 static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
@@ -657,9 +673,9 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
 
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
 
-
 	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
 		state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
+		state->fifo_state[i].fifo_load = old_state->fifo_state[i].fifo_load;
 
 		if (!old_state->fifo_state[i].pending_commit)
 			continue;
@@ -668,6 +684,8 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
 			drm_crtc_commit_get(old_state->fifo_state[i].pending_commit);
 	}
 
+	state->core_clock_rate = old_state->core_clock_rate;
+
 	return &state->base;
 }
 
@@ -822,6 +840,76 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 	return 0;
 }
 
+static int
+vc4_core_clock_atomic_check(struct drm_atomic_state *state)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
+	struct drm_private_state *priv_state;
+	struct vc4_hvs_state *hvs_new_state;
+	struct vc4_load_tracker_state *load_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct drm_crtc *crtc;
+	unsigned int num_outputs;
+	unsigned long pixel_rate;
+	unsigned long cob_rate;
+	unsigned int i;
+
+	priv_state = drm_atomic_get_private_obj_state(state,
+						      &vc4->load_tracker);
+	if (IS_ERR(priv_state))
+		return PTR_ERR(priv_state);
+
+	load_state = to_vc4_load_tracker_state(priv_state);
+
+	hvs_new_state = vc4_hvs_get_global_state(state);
+	if (!hvs_new_state)
+		return -EINVAL;
+
+	for_each_oldnew_crtc_in_state(state, crtc,
+				      old_crtc_state,
+				      new_crtc_state,
+				      i) {
+		if (old_crtc_state->active) {
+			struct vc4_crtc_state *old_vc4_state =
+				to_vc4_crtc_state(old_crtc_state);
+			unsigned int channel = old_vc4_state->assigned_channel;
+
+			hvs_new_state->fifo_state[channel].fifo_load = 0;
+		}
+
+		if (new_crtc_state->active) {
+			struct vc4_crtc_state *new_vc4_state =
+				to_vc4_crtc_state(new_crtc_state);
+			unsigned int channel = new_vc4_state->assigned_channel;
+
+			hvs_new_state->fifo_state[channel].fifo_load =
+				new_vc4_state->hvs_load;
+		}
+	}
+
+	cob_rate = 0;
+	num_outputs = 0;
+	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
+		if (!hvs_new_state->fifo_state[i].in_use)
+			continue;
+
+		num_outputs++;
+		cob_rate += hvs_new_state->fifo_state[i].fifo_load;
+	}
+
+	pixel_rate = load_state->hvs_load;
+	if (num_outputs > 1) {
+		pixel_rate = (pixel_rate * 40) / 100;
+	} else {
+		pixel_rate = (pixel_rate * 60) / 100;
+	}
+
+	hvs_new_state->core_clock_rate = max(cob_rate, pixel_rate);
+
+	return 0;
+}
+
+
 static int
 vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 {
@@ -839,7 +927,11 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 	if (ret)
 		return ret;
 
-	return vc4_load_tracker_atomic_check(state);
+	ret = vc4_load_tracker_atomic_check(state);
+	if (ret)
+		return ret;
+
+	return vc4_core_clock_atomic_check(state);
 }
 
 static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = {
-- 
2.31.1

Re: [PATCH v8 02/10] drm/vc4: hdmi: Fix HPD GPIO detection

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Prior to commit 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod"), in the
> detect hook, if we had an HPD GPIO we would only rely on it and return
> whatever state it was in.
>
> However, that commit changed that by mistake to only consider the case
> where we have a GPIO and it returns a logical high, and would fall back
> to the other methods otherwise.
>
> Since we can read the EDIDs when the HPD signal is low on some displays,
> we changed the detection status from disconnected to connected, and we
> would ignore an HPD pulse.
>
> Fixes: 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 338968275724..dde67b991ae7 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -190,9 +190,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
>
>         WARN_ON(pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev));
>
> -       if (vc4_hdmi->hpd_gpio &&
> -           gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) {
> -               connected = true;
> +       if (vc4_hdmi->hpd_gpio) {
> +               if (gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio))
> +                       connected = true;
>         } else {
>                 unsigned long flags;
>                 u32 hotplug;
> --
> 2.31.1
>

Re: [PATCH v8 02/10] drm/vc4: hdmi: Fix HPD GPIO detection

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Prior to commit 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod"), in the
> detect hook, if we had an HPD GPIO we would only rely on it and return
> whatever state it was in.
>
> However, that commit changed that by mistake to only consider the case
> where we have a GPIO and it returns a logical high, and would fall back
> to the other methods otherwise.
>
> Since we can read the EDIDs when the HPD signal is low on some displays,
> we changed the detection status from disconnected to connected, and we
> would ignore an HPD pulse.
>
> Fixes: 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 338968275724..dde67b991ae7 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -190,9 +190,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
>
>         WARN_ON(pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev));
>
> -       if (vc4_hdmi->hpd_gpio &&
> -           gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) {
> -               connected = true;
> +       if (vc4_hdmi->hpd_gpio) {
> +               if (gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio))
> +                       connected = true;
>         } else {
>                 unsigned long flags;
>                 u32 hotplug;
> --
> 2.31.1
>

Re: [PATCH v8 03/10] drm/vc4: Make vc4_crtc_get_encoder public

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> We'll need that function in vc4_kms to compute the core clock rate
> requirements.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 8 ++++----
>  drivers/gpu/drm/vc4/vc4_drv.h  | 5 +++++
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index e3ed52d96f42..7cfd4a097847 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -281,10 +281,10 @@ static u32 vc4_crtc_get_fifo_full_level_bits(struct vc4_crtc *vc4_crtc,
>   * allows drivers to push pixels to more than one encoder from the
>   * same CRTC.
>   */
> -static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
> -                                               struct drm_atomic_state *state,
> -                                               struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
> -                                                                                        struct drm_connector *connector))
> +struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
> +                                        struct drm_atomic_state *state,
> +                                        struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
> +                                                                                 struct drm_connector *connector))
>  {
>         struct drm_connector *connector;
>         struct drm_connector_list_iter conn_iter;
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 4b550ebd9572..f5e678491502 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -544,6 +544,11 @@ vc4_crtc_to_vc4_pv_data(const struct vc4_crtc *crtc)
>         return container_of(data, struct vc4_pv_data, base);
>  }
>
> +struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
> +                                        struct drm_atomic_state *state,
> +                                        struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
> +                                                                                 struct drm_connector *connector));
> +
>  struct vc4_crtc_state {
>         struct drm_crtc_state base;
>         /* Dlist area for this CRTC configuration. */
> --
> 2.31.1
>

Re: [PATCH v8 03/10] drm/vc4: Make vc4_crtc_get_encoder public

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> We'll need that function in vc4_kms to compute the core clock rate
> requirements.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 8 ++++----
>  drivers/gpu/drm/vc4/vc4_drv.h  | 5 +++++
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index e3ed52d96f42..7cfd4a097847 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -281,10 +281,10 @@ static u32 vc4_crtc_get_fifo_full_level_bits(struct vc4_crtc *vc4_crtc,
>   * allows drivers to push pixels to more than one encoder from the
>   * same CRTC.
>   */
> -static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
> -                                               struct drm_atomic_state *state,
> -                                               struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
> -                                                                                        struct drm_connector *connector))
> +struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
> +                                        struct drm_atomic_state *state,
> +                                        struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
> +                                                                                 struct drm_connector *connector))
>  {
>         struct drm_connector *connector;
>         struct drm_connector_list_iter conn_iter;
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 4b550ebd9572..f5e678491502 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -544,6 +544,11 @@ vc4_crtc_to_vc4_pv_data(const struct vc4_crtc *crtc)
>         return container_of(data, struct vc4_pv_data, base);
>  }
>
> +struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
> +                                        struct drm_atomic_state *state,
> +                                        struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
> +                                                                                 struct drm_connector *connector));
> +
>  struct vc4_crtc_state {
>         struct drm_crtc_state base;
>         /* Dlist area for this CRTC configuration. */
> --
> 2.31.1
>

Re: [PATCH v8 04/10] drm/vc4: crtc: Add encoder to vc4_crtc_config_pv prototype

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> vc4_crtc_config_pv() retrieves the encoder again, even though its only
> caller, vc4_crtc_atomic_enable(), already did.
>
> Pass the encoder pointer as an argument instead of going through all the
> connectors to retrieve it again.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 7cfd4a097847..e5c2e29a6f01 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -315,12 +315,11 @@ static void vc4_crtc_pixelvalve_reset(struct drm_crtc *crtc)
>         CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) | PV_CONTROL_FIFO_CLR);
>  }
>
> -static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_atomic_state *state)
> +static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_encoder *encoder,
> +                              struct drm_atomic_state *state)
>  {
>         struct drm_device *dev = crtc->dev;
>         struct vc4_dev *vc4 = to_vc4_dev(dev);
> -       struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
> -                                                          drm_atomic_get_new_connector_state);
>         struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
>         struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>         const struct vc4_pv_data *pv_data = vc4_crtc_to_vc4_pv_data(vc4_crtc);
> @@ -597,7 +596,7 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
>         if (vc4_encoder->pre_crtc_configure)
>                 vc4_encoder->pre_crtc_configure(encoder, state);
>
> -       vc4_crtc_config_pv(crtc, state);
> +       vc4_crtc_config_pv(crtc, encoder, state);
>
>         CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) | PV_CONTROL_EN);
>
> --
> 2.31.1
>

Re: [PATCH v8 04/10] drm/vc4: crtc: Add encoder to vc4_crtc_config_pv prototype

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> vc4_crtc_config_pv() retrieves the encoder again, even though its only
> caller, vc4_crtc_atomic_enable(), already did.
>
> Pass the encoder pointer as an argument instead of going through all the
> connectors to retrieve it again.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 7cfd4a097847..e5c2e29a6f01 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -315,12 +315,11 @@ static void vc4_crtc_pixelvalve_reset(struct drm_crtc *crtc)
>         CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) | PV_CONTROL_FIFO_CLR);
>  }
>
> -static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_atomic_state *state)
> +static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_encoder *encoder,
> +                              struct drm_atomic_state *state)
>  {
>         struct drm_device *dev = crtc->dev;
>         struct vc4_dev *vc4 = to_vc4_dev(dev);
> -       struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
> -                                                          drm_atomic_get_new_connector_state);
>         struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
>         struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>         const struct vc4_pv_data *pv_data = vc4_crtc_to_vc4_pv_data(vc4_crtc);
> @@ -597,7 +596,7 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
>         if (vc4_encoder->pre_crtc_configure)
>                 vc4_encoder->pre_crtc_configure(encoder, state);
>
> -       vc4_crtc_config_pv(crtc, state);
> +       vc4_crtc_config_pv(crtc, encoder, state);
>
>         CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) | PV_CONTROL_EN);
>
> --
> 2.31.1
>

Re: [PATCH v8 05/10] drm/vc4: crtc: Rework the encoder retrieval code (again)

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> It turns out the encoder retrieval code, in addition to being
> unnecessarily complicated, has a bug when only the planes and crtcs are
> affected by a given atomic commit.
>
> Indeed, in such a case, either drm_atomic_get_old_connector_state or
> drm_atomic_get_new_connector_state will return NULL and thus our encoder
> retrieval code will not match on anything.
>
> We can however simplify the code by using drm_for_each_encoder_mask, the
> drm_crtc_state storing the encoders a given CRTC is connected to
> directly and without relying on any other state.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 30 +++++++++---------------------
>  drivers/gpu/drm/vc4/vc4_drv.h  |  4 +---
>  2 files changed, 10 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index e5c2e29a6f01..fbc1d4638650 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -282,26 +282,14 @@ static u32 vc4_crtc_get_fifo_full_level_bits(struct vc4_crtc *vc4_crtc,
>   * same CRTC.
>   */
>  struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
> -                                        struct drm_atomic_state *state,
> -                                        struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
> -                                                                                 struct drm_connector *connector))
> +                                        struct drm_crtc_state *state)
>  {
> -       struct drm_connector *connector;
> -       struct drm_connector_list_iter conn_iter;
> +       struct drm_encoder *encoder;
>
> -       drm_connector_list_iter_begin(crtc->dev, &conn_iter);
> -       drm_for_each_connector_iter(connector, &conn_iter) {
> -               struct drm_connector_state *conn_state = get_state(state, connector);
> +       WARN_ON(hweight32(state->encoder_mask) > 1);
>
> -               if (!conn_state)
> -                       continue;
> -
> -               if (conn_state->crtc == crtc) {
> -                       drm_connector_list_iter_end(&conn_iter);
> -                       return connector->encoder;
> -               }
> -       }
> -       drm_connector_list_iter_end(&conn_iter);
> +       drm_for_each_encoder_mask(encoder, crtc->dev, state->encoder_mask)
> +               return encoder;
>
>         return NULL;
>  }
> @@ -550,8 +538,7 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
>         struct drm_crtc_state *old_state = drm_atomic_get_old_crtc_state(state,
>                                                                          crtc);
>         struct vc4_crtc_state *old_vc4_state = to_vc4_crtc_state(old_state);
> -       struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
> -                                                          drm_atomic_get_old_connector_state);
> +       struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, old_state);
>         struct drm_device *dev = crtc->dev;
>
>         require_hvs_enabled(dev);
> @@ -578,10 +565,11 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
>  static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
>                                    struct drm_atomic_state *state)
>  {
> +       struct drm_crtc_state *new_state = drm_atomic_get_new_crtc_state(state,
> +                                                                        crtc);
>         struct drm_device *dev = crtc->dev;
>         struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> -       struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
> -                                                          drm_atomic_get_new_connector_state);
> +       struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, new_state);
>         struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
>
>         require_hvs_enabled(dev);
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index f5e678491502..60826aca9e5b 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -545,9 +545,7 @@ vc4_crtc_to_vc4_pv_data(const struct vc4_crtc *crtc)
>  }
>
>  struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
> -                                        struct drm_atomic_state *state,
> -                                        struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
> -                                                                                 struct drm_connector *connector));
> +                                        struct drm_crtc_state *state);
>
>  struct vc4_crtc_state {
>         struct drm_crtc_state base;
> --
> 2.31.1
>

Re: [PATCH v8 05/10] drm/vc4: crtc: Rework the encoder retrieval code (again)

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> It turns out the encoder retrieval code, in addition to being
> unnecessarily complicated, has a bug when only the planes and crtcs are
> affected by a given atomic commit.
>
> Indeed, in such a case, either drm_atomic_get_old_connector_state or
> drm_atomic_get_new_connector_state will return NULL and thus our encoder
> retrieval code will not match on anything.
>
> We can however simplify the code by using drm_for_each_encoder_mask, the
> drm_crtc_state storing the encoders a given CRTC is connected to
> directly and without relying on any other state.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 30 +++++++++---------------------
>  drivers/gpu/drm/vc4/vc4_drv.h  |  4 +---
>  2 files changed, 10 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index e5c2e29a6f01..fbc1d4638650 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -282,26 +282,14 @@ static u32 vc4_crtc_get_fifo_full_level_bits(struct vc4_crtc *vc4_crtc,
>   * same CRTC.
>   */
>  struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
> -                                        struct drm_atomic_state *state,
> -                                        struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
> -                                                                                 struct drm_connector *connector))
> +                                        struct drm_crtc_state *state)
>  {
> -       struct drm_connector *connector;
> -       struct drm_connector_list_iter conn_iter;
> +       struct drm_encoder *encoder;
>
> -       drm_connector_list_iter_begin(crtc->dev, &conn_iter);
> -       drm_for_each_connector_iter(connector, &conn_iter) {
> -               struct drm_connector_state *conn_state = get_state(state, connector);
> +       WARN_ON(hweight32(state->encoder_mask) > 1);
>
> -               if (!conn_state)
> -                       continue;
> -
> -               if (conn_state->crtc == crtc) {
> -                       drm_connector_list_iter_end(&conn_iter);
> -                       return connector->encoder;
> -               }
> -       }
> -       drm_connector_list_iter_end(&conn_iter);
> +       drm_for_each_encoder_mask(encoder, crtc->dev, state->encoder_mask)
> +               return encoder;
>
>         return NULL;
>  }
> @@ -550,8 +538,7 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
>         struct drm_crtc_state *old_state = drm_atomic_get_old_crtc_state(state,
>                                                                          crtc);
>         struct vc4_crtc_state *old_vc4_state = to_vc4_crtc_state(old_state);
> -       struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
> -                                                          drm_atomic_get_old_connector_state);
> +       struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, old_state);
>         struct drm_device *dev = crtc->dev;
>
>         require_hvs_enabled(dev);
> @@ -578,10 +565,11 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
>  static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
>                                    struct drm_atomic_state *state)
>  {
> +       struct drm_crtc_state *new_state = drm_atomic_get_new_crtc_state(state,
> +                                                                        crtc);
>         struct drm_device *dev = crtc->dev;
>         struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> -       struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
> -                                                          drm_atomic_get_new_connector_state);
> +       struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, new_state);
>         struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
>
>         require_hvs_enabled(dev);
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index f5e678491502..60826aca9e5b 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -545,9 +545,7 @@ vc4_crtc_to_vc4_pv_data(const struct vc4_crtc *crtc)
>  }
>
>  struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
> -                                        struct drm_atomic_state *state,
> -                                        struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
> -                                                                                 struct drm_connector *connector));
> +                                        struct drm_crtc_state *state);
>
>  struct vc4_crtc_state {
>         struct drm_crtc_state base;
> --
> 2.31.1
>

Re: [PATCH v8 06/10] drm/vc4: crtc: Add some logging

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The encoder retrieval code has been a source of bugs and glitches in the
> past and the crtc <-> encoder association been wrong in a number of
> different ways.
>
> Add some logging to quickly spot issues if they occur.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index fbc1d4638650..6decaa12a078 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -541,6 +541,9 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
>         struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, old_state);
>         struct drm_device *dev = crtc->dev;
>
> +       drm_dbg(dev, "Disabling CRTC %s (%u) connected to Encoder %s (%u)",
> +               crtc->name, crtc->base.id, encoder->name, encoder->base.id);
> +
>         require_hvs_enabled(dev);
>
>         /* Disable vblank irq handling before crtc is disabled. */
> @@ -572,6 +575,9 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
>         struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, new_state);
>         struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
>
> +       drm_dbg(dev, "Enabling CRTC %s (%u) connected to Encoder %s (%u)",
> +               crtc->name, crtc->base.id, encoder->name, encoder->base.id);
> +
>         require_hvs_enabled(dev);
>
>         /* Enable vblank irq handling before crtc is started otherwise
> --
> 2.31.1
>

Re: [PATCH v8 06/10] drm/vc4: crtc: Add some logging

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The encoder retrieval code has been a source of bugs and glitches in the
> past and the crtc <-> encoder association been wrong in a number of
> different ways.
>
> Add some logging to quickly spot issues if they occur.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index fbc1d4638650..6decaa12a078 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -541,6 +541,9 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
>         struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, old_state);
>         struct drm_device *dev = crtc->dev;
>
> +       drm_dbg(dev, "Disabling CRTC %s (%u) connected to Encoder %s (%u)",
> +               crtc->name, crtc->base.id, encoder->name, encoder->base.id);
> +
>         require_hvs_enabled(dev);
>
>         /* Disable vblank irq handling before crtc is disabled. */
> @@ -572,6 +575,9 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
>         struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, new_state);
>         struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
>
> +       drm_dbg(dev, "Enabling CRTC %s (%u) connected to Encoder %s (%u)",
> +               crtc->name, crtc->base.id, encoder->name, encoder->base.id);
> +
>         require_hvs_enabled(dev);
>
>         /* Enable vblank irq handling before crtc is started otherwise
> --
> 2.31.1
>

Re: [PATCH v8 07/10] drm/vc4: Leverage the load tracker on the BCM2711

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The load tracker was initially designed to report and warn about a load
> too high for the HVS. To do so, it computes for each plane the impact
> it's going to have on the HVS, and will warn (if it's enabled) if we go
> over what the hardware can process.
>
> While the limits being used are a bit irrelevant to the BCM2711, the
> algorithm to compute the HVS load will be one component used in order to
> compute the core clock rate on the BCM2711.
>
> Let's remove the hooks to prevent the load tracker to do its
> computation, but since we don't have the same limits, don't check them
> against them, and prevent the debugfs file to enable it from being
> created.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_debugfs.c |  7 +++++--
>  drivers/gpu/drm/vc4/vc4_drv.h     |  3 ---
>  drivers/gpu/drm/vc4/vc4_kms.c     | 16 +++++-----------
>  drivers/gpu/drm/vc4/vc4_plane.c   |  5 -----
>  4 files changed, 10 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
> index 6da22af4ee91..ba2d8ea562af 100644
> --- a/drivers/gpu/drm/vc4/vc4_debugfs.c
> +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
> @@ -7,6 +7,7 @@
>  #include <linux/circ_buf.h>
>  #include <linux/ctype.h>
>  #include <linux/debugfs.h>
> +#include <linux/platform_device.h>
>
>  #include "vc4_drv.h"
>  #include "vc4_regs.h"
> @@ -26,8 +27,10 @@ vc4_debugfs_init(struct drm_minor *minor)
>         struct vc4_dev *vc4 = to_vc4_dev(minor->dev);
>         struct vc4_debugfs_info_entry *entry;
>
> -       debugfs_create_bool("hvs_load_tracker", S_IRUGO | S_IWUSR,
> -                           minor->debugfs_root, &vc4->load_tracker_enabled);
> +       if (!of_device_is_compatible(vc4->hvs->pdev->dev.of_node,
> +                                    "brcm,bcm2711-vc5"))
> +               debugfs_create_bool("hvs_load_tracker", S_IRUGO | S_IWUSR,
> +                                   minor->debugfs_root, &vc4->load_tracker_enabled);
>
>         list_for_each_entry(entry, &vc4->debugfs_list, link) {
>                 drm_debugfs_create_files(&entry->info, 1,
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 60826aca9e5b..813c5d0ea98e 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -202,9 +202,6 @@ struct vc4_dev {
>
>         int power_refcount;
>
> -       /* Set to true when the load tracker is supported. */
> -       bool load_tracker_available;
> -
>         /* Set to true when the load tracker is active. */
>         bool load_tracker_enabled;
>
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 028f19f7a5f8..41cb4869da50 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -552,9 +552,6 @@ static int vc4_load_tracker_atomic_check(struct drm_atomic_state *state)
>         struct drm_plane *plane;
>         int i;
>
> -       if (!vc4->load_tracker_available)
> -               return 0;
> -
>         priv_state = drm_atomic_get_private_obj_state(state,
>                                                       &vc4->load_tracker);
>         if (IS_ERR(priv_state))
> @@ -629,9 +626,6 @@ static void vc4_load_tracker_obj_fini(struct drm_device *dev, void *unused)
>  {
>         struct vc4_dev *vc4 = to_vc4_dev(dev);
>
> -       if (!vc4->load_tracker_available)
> -               return;
> -
>         drm_atomic_private_obj_fini(&vc4->load_tracker);
>  }
>
> @@ -639,9 +633,6 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
>  {
>         struct vc4_load_tracker_state *load_state;
>
> -       if (!vc4->load_tracker_available)
> -               return 0;
> -
>         load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
>         if (!load_state)
>                 return -ENOMEM;
> @@ -869,9 +860,12 @@ int vc4_kms_load(struct drm_device *dev)
>                                               "brcm,bcm2711-vc5");
>         int ret;
>
> +       /*
> +        * The limits enforced by the load tracker aren't relevant for
> +        * the BCM2711, but the load tracker computations are used for
> +        * the core clock rate calculation.
> +        */
>         if (!is_vc5) {
> -               vc4->load_tracker_available = true;
> -
>                 /* Start with the load tracker enabled. Can be
>                  * disabled through the debugfs load_tracker file.
>                  */
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index 19161b6ab27f..ac761c683663 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -529,11 +529,6 @@ static void vc4_plane_calc_load(struct drm_plane_state *state)
>         struct vc4_plane_state *vc4_state;
>         struct drm_crtc_state *crtc_state;
>         unsigned int vscale_factor;
> -       struct vc4_dev *vc4;
> -
> -       vc4 = to_vc4_dev(state->plane->dev);
> -       if (!vc4->load_tracker_available)
> -               return;
>
>         vc4_state = to_vc4_plane_state(state);
>         crtc_state = drm_atomic_get_existing_crtc_state(state->state,
> --
> 2.31.1
>

Re: [PATCH v8 07/10] drm/vc4: Leverage the load tracker on the BCM2711

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The load tracker was initially designed to report and warn about a load
> too high for the HVS. To do so, it computes for each plane the impact
> it's going to have on the HVS, and will warn (if it's enabled) if we go
> over what the hardware can process.
>
> While the limits being used are a bit irrelevant to the BCM2711, the
> algorithm to compute the HVS load will be one component used in order to
> compute the core clock rate on the BCM2711.
>
> Let's remove the hooks to prevent the load tracker to do its
> computation, but since we don't have the same limits, don't check them
> against them, and prevent the debugfs file to enable it from being
> created.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_debugfs.c |  7 +++++--
>  drivers/gpu/drm/vc4/vc4_drv.h     |  3 ---
>  drivers/gpu/drm/vc4/vc4_kms.c     | 16 +++++-----------
>  drivers/gpu/drm/vc4/vc4_plane.c   |  5 -----
>  4 files changed, 10 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
> index 6da22af4ee91..ba2d8ea562af 100644
> --- a/drivers/gpu/drm/vc4/vc4_debugfs.c
> +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
> @@ -7,6 +7,7 @@
>  #include <linux/circ_buf.h>
>  #include <linux/ctype.h>
>  #include <linux/debugfs.h>
> +#include <linux/platform_device.h>
>
>  #include "vc4_drv.h"
>  #include "vc4_regs.h"
> @@ -26,8 +27,10 @@ vc4_debugfs_init(struct drm_minor *minor)
>         struct vc4_dev *vc4 = to_vc4_dev(minor->dev);
>         struct vc4_debugfs_info_entry *entry;
>
> -       debugfs_create_bool("hvs_load_tracker", S_IRUGO | S_IWUSR,
> -                           minor->debugfs_root, &vc4->load_tracker_enabled);
> +       if (!of_device_is_compatible(vc4->hvs->pdev->dev.of_node,
> +                                    "brcm,bcm2711-vc5"))
> +               debugfs_create_bool("hvs_load_tracker", S_IRUGO | S_IWUSR,
> +                                   minor->debugfs_root, &vc4->load_tracker_enabled);
>
>         list_for_each_entry(entry, &vc4->debugfs_list, link) {
>                 drm_debugfs_create_files(&entry->info, 1,
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 60826aca9e5b..813c5d0ea98e 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -202,9 +202,6 @@ struct vc4_dev {
>
>         int power_refcount;
>
> -       /* Set to true when the load tracker is supported. */
> -       bool load_tracker_available;
> -
>         /* Set to true when the load tracker is active. */
>         bool load_tracker_enabled;
>
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 028f19f7a5f8..41cb4869da50 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -552,9 +552,6 @@ static int vc4_load_tracker_atomic_check(struct drm_atomic_state *state)
>         struct drm_plane *plane;
>         int i;
>
> -       if (!vc4->load_tracker_available)
> -               return 0;
> -
>         priv_state = drm_atomic_get_private_obj_state(state,
>                                                       &vc4->load_tracker);
>         if (IS_ERR(priv_state))
> @@ -629,9 +626,6 @@ static void vc4_load_tracker_obj_fini(struct drm_device *dev, void *unused)
>  {
>         struct vc4_dev *vc4 = to_vc4_dev(dev);
>
> -       if (!vc4->load_tracker_available)
> -               return;
> -
>         drm_atomic_private_obj_fini(&vc4->load_tracker);
>  }
>
> @@ -639,9 +633,6 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
>  {
>         struct vc4_load_tracker_state *load_state;
>
> -       if (!vc4->load_tracker_available)
> -               return 0;
> -
>         load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
>         if (!load_state)
>                 return -ENOMEM;
> @@ -869,9 +860,12 @@ int vc4_kms_load(struct drm_device *dev)
>                                               "brcm,bcm2711-vc5");
>         int ret;
>
> +       /*
> +        * The limits enforced by the load tracker aren't relevant for
> +        * the BCM2711, but the load tracker computations are used for
> +        * the core clock rate calculation.
> +        */
>         if (!is_vc5) {
> -               vc4->load_tracker_available = true;
> -
>                 /* Start with the load tracker enabled. Can be
>                  * disabled through the debugfs load_tracker file.
>                  */
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index 19161b6ab27f..ac761c683663 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -529,11 +529,6 @@ static void vc4_plane_calc_load(struct drm_plane_state *state)
>         struct vc4_plane_state *vc4_state;
>         struct drm_crtc_state *crtc_state;
>         unsigned int vscale_factor;
> -       struct vc4_dev *vc4;
> -
> -       vc4 = to_vc4_dev(state->plane->dev);
> -       if (!vc4->load_tracker_available)
> -               return;
>
>         vc4_state = to_vc4_plane_state(state);
>         crtc_state = drm_atomic_get_existing_crtc_state(state->state,
> --
> 2.31.1
>

Re: [PATCH v8 09/10] drm/vc4: hdmi: Enable the scrambler on reconnection

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> If we have a state already and disconnect/reconnect the display, the
> SCDC messages won't be sent again since we didn't go through a disable /
> enable cycle.
>
> In order to fix this, let's call the vc4_hdmi_enable_scrambling function
> in the detect callback if there is a mode and it needs the scrambler to
> be enabled.
>
> Fixes: c85695a2016e ("drm/vc4: hdmi: Enable the scrambler")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index d36b3b6ebed1..fab9b93e1b84 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -180,6 +180,8 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
>  static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
>  #endif
>
> +static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder);
> +
>  static enum drm_connector_status
>  vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
>  {
> @@ -216,6 +218,7 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
>                         }
>                 }
>
> +               vc4_hdmi_enable_scrambling(&vc4_hdmi->encoder.base.base);
>                 pm_runtime_put(&vc4_hdmi->pdev->dev);
>                 mutex_unlock(&vc4_hdmi->mutex);
>                 return connector_status_connected;
> --
> 2.31.1
>

Re: [PATCH v8 09/10] drm/vc4: hdmi: Enable the scrambler on reconnection

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> If we have a state already and disconnect/reconnect the display, the
> SCDC messages won't be sent again since we didn't go through a disable /
> enable cycle.
>
> In order to fix this, let's call the vc4_hdmi_enable_scrambling function
> in the detect callback if there is a mode and it needs the scrambler to
> be enabled.
>
> Fixes: c85695a2016e ("drm/vc4: hdmi: Enable the scrambler")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index d36b3b6ebed1..fab9b93e1b84 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -180,6 +180,8 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
>  static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
>  #endif
>
> +static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder);
> +
>  static enum drm_connector_status
>  vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
>  {
> @@ -216,6 +218,7 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
>                         }
>                 }
>
> +               vc4_hdmi_enable_scrambling(&vc4_hdmi->encoder.base.base);
>                 pm_runtime_put(&vc4_hdmi->pdev->dev);
>                 mutex_unlock(&vc4_hdmi->mutex);
>                 return connector_status_connected;
> --
> 2.31.1
>

Re: [PATCH v8 10/10] drm/vc4: Increase the core clock based on HVS load

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Depending on a given HVS output (HVS to PixelValves) and input (planes
> attached to a channel) load, the HVS needs for the core clock to be
> raised above its boot time default.
>
> Failing to do so will result in a vblank timeout and a stalled display
> pipeline.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

I will make the comment that this does a load of computation of hvs
load when running on hvs4 (BCM2835/6/7), even though it's redundant on
those chips.
The overhead is relatively minimal, but could be bypassed if viewed necessary.

Otherwise
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c |  15 +++++
>  drivers/gpu/drm/vc4/vc4_drv.h  |   2 +
>  drivers/gpu/drm/vc4/vc4_kms.c  | 110 ++++++++++++++++++++++++++++++---
>  3 files changed, 118 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 6decaa12a078..287dbc89ad64 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -659,12 +659,27 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
>         struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
>         struct drm_connector *conn;
>         struct drm_connector_state *conn_state;
> +       struct drm_encoder *encoder;
>         int ret, i;
>
>         ret = vc4_hvs_atomic_check(crtc, state);
>         if (ret)
>                 return ret;
>
> +       encoder = vc4_get_crtc_encoder(crtc, crtc_state);
> +       if (encoder) {
> +               const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> +               struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
> +
> +               mode = &crtc_state->adjusted_mode;
> +               if (vc4_encoder->type == VC4_ENCODER_TYPE_HDMI0) {
> +                       vc4_state->hvs_load = max(mode->clock * mode->hdisplay / mode->htotal + 1000,
> +                                                 mode->clock * 9 / 10) * 1000;
> +               } else {
> +                       vc4_state->hvs_load = mode->clock * 1000;
> +               }
> +       }
> +
>         for_each_new_connector_in_state(state, conn, conn_state,
>                                         i) {
>                 if (conn_state->crtc != crtc)
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 813c5d0ea98e..4329e09d357c 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -558,6 +558,8 @@ struct vc4_crtc_state {
>                 unsigned int bottom;
>         } margins;
>
> +       unsigned long hvs_load;
> +
>         /* Transitional state below, only valid during atomic commits */
>         bool update_muxing;
>  };
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 41cb4869da50..79d4d9dd1394 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -39,9 +39,11 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
>
>  struct vc4_hvs_state {
>         struct drm_private_state base;
> +       unsigned long core_clock_rate;
>
>         struct {
>                 unsigned in_use: 1;
> +               unsigned long fifo_load;
>                 struct drm_crtc_commit *pending_commit;
>         } fifo_state[HVS_NUM_CHANNELS];
>  };
> @@ -340,10 +342,19 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
>         struct vc4_hvs *hvs = vc4->hvs;
>         struct drm_crtc_state *old_crtc_state;
>         struct drm_crtc_state *new_crtc_state;
> +       struct vc4_hvs_state *new_hvs_state;
>         struct drm_crtc *crtc;
>         struct vc4_hvs_state *old_hvs_state;
>         int i;
>
> +       old_hvs_state = vc4_hvs_get_old_global_state(state);
> +       if (WARN_ON(!old_hvs_state))
> +               return;
> +
> +       new_hvs_state = vc4_hvs_get_new_global_state(state);
> +       if (WARN_ON(!new_hvs_state))
> +               return;
> +
>         for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>                 struct vc4_crtc_state *vc4_crtc_state;
>
> @@ -354,12 +365,13 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
>                 vc4_hvs_mask_underrun(dev, vc4_crtc_state->assigned_channel);
>         }
>
> -       if (vc4->hvs->hvs5)
> -               clk_set_min_rate(hvs->core_clk, 500000000);
> +       if (vc4->hvs->hvs5) {
> +               unsigned long core_rate = max_t(unsigned long,
> +                                               500000000,
> +                                               new_hvs_state->core_clock_rate);
>
> -       old_hvs_state = vc4_hvs_get_old_global_state(state);
> -       if (!old_hvs_state)
> -               return;
> +               clk_set_min_rate(hvs->core_clk, core_rate);
> +       }
>
>         for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>                 struct vc4_crtc_state *vc4_crtc_state =
> @@ -399,8 +411,12 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
>
>         drm_atomic_helper_cleanup_planes(dev, state);
>
> -       if (vc4->hvs->hvs5)
> -               clk_set_min_rate(hvs->core_clk, 0);
> +       if (vc4->hvs->hvs5) {
> +               drm_dbg(dev, "Running the core clock at %lu Hz\n",
> +                       new_hvs_state->core_clock_rate);
> +
> +               clk_set_min_rate(hvs->core_clk, new_hvs_state->core_clock_rate);
> +       }
>  }
>
>  static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
> @@ -657,9 +673,9 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>
>         __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
>
> -
>         for (i = 0; i < HVS_NUM_CHANNELS; i++) {
>                 state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
> +               state->fifo_state[i].fifo_load = old_state->fifo_state[i].fifo_load;
>
>                 if (!old_state->fifo_state[i].pending_commit)
>                         continue;
> @@ -668,6 +684,8 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>                         drm_crtc_commit_get(old_state->fifo_state[i].pending_commit);
>         }
>
> +       state->core_clock_rate = old_state->core_clock_rate;
> +
>         return &state->base;
>  }
>
> @@ -822,6 +840,76 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>         return 0;
>  }
>
> +static int
> +vc4_core_clock_atomic_check(struct drm_atomic_state *state)
> +{
> +       struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> +       struct drm_private_state *priv_state;
> +       struct vc4_hvs_state *hvs_new_state;
> +       struct vc4_load_tracker_state *load_state;
> +       struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +       struct drm_crtc *crtc;
> +       unsigned int num_outputs;
> +       unsigned long pixel_rate;
> +       unsigned long cob_rate;
> +       unsigned int i;
> +
> +       priv_state = drm_atomic_get_private_obj_state(state,
> +                                                     &vc4->load_tracker);
> +       if (IS_ERR(priv_state))
> +               return PTR_ERR(priv_state);
> +
> +       load_state = to_vc4_load_tracker_state(priv_state);
> +
> +       hvs_new_state = vc4_hvs_get_global_state(state);
> +       if (!hvs_new_state)
> +               return -EINVAL;
> +
> +       for_each_oldnew_crtc_in_state(state, crtc,
> +                                     old_crtc_state,
> +                                     new_crtc_state,
> +                                     i) {
> +               if (old_crtc_state->active) {
> +                       struct vc4_crtc_state *old_vc4_state =
> +                               to_vc4_crtc_state(old_crtc_state);
> +                       unsigned int channel = old_vc4_state->assigned_channel;
> +
> +                       hvs_new_state->fifo_state[channel].fifo_load = 0;
> +               }
> +
> +               if (new_crtc_state->active) {
> +                       struct vc4_crtc_state *new_vc4_state =
> +                               to_vc4_crtc_state(new_crtc_state);
> +                       unsigned int channel = new_vc4_state->assigned_channel;
> +
> +                       hvs_new_state->fifo_state[channel].fifo_load =
> +                               new_vc4_state->hvs_load;
> +               }
> +       }
> +
> +       cob_rate = 0;
> +       num_outputs = 0;
> +       for (i = 0; i < HVS_NUM_CHANNELS; i++) {
> +               if (!hvs_new_state->fifo_state[i].in_use)
> +                       continue;
> +
> +               num_outputs++;
> +               cob_rate += hvs_new_state->fifo_state[i].fifo_load;
> +       }
> +
> +       pixel_rate = load_state->hvs_load;
> +       if (num_outputs > 1) {
> +               pixel_rate = (pixel_rate * 40) / 100;
> +       } else {
> +               pixel_rate = (pixel_rate * 60) / 100;
> +       }
> +
> +       hvs_new_state->core_clock_rate = max(cob_rate, pixel_rate);
> +
> +       return 0;
> +}
> +
> +
>  static int
>  vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>  {
> @@ -839,7 +927,11 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>         if (ret)
>                 return ret;
>
> -       return vc4_load_tracker_atomic_check(state);
> +       ret = vc4_load_tracker_atomic_check(state);
> +       if (ret)
> +               return ret;
> +
> +       return vc4_core_clock_atomic_check(state);
>  }
>
>  static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = {
> --
> 2.31.1
>

Re: [PATCH v8 10/10] drm/vc4: Increase the core clock based on HVS load

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Depending on a given HVS output (HVS to PixelValves) and input (planes
> attached to a channel) load, the HVS needs for the core clock to be
> raised above its boot time default.
>
> Failing to do so will result in a vblank timeout and a stalled display
> pipeline.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

I will make the comment that this does a load of computation of hvs
load when running on hvs4 (BCM2835/6/7), even though it's redundant on
those chips.
The overhead is relatively minimal, but could be bypassed if viewed necessary.

Otherwise
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c |  15 +++++
>  drivers/gpu/drm/vc4/vc4_drv.h  |   2 +
>  drivers/gpu/drm/vc4/vc4_kms.c  | 110 ++++++++++++++++++++++++++++++---
>  3 files changed, 118 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 6decaa12a078..287dbc89ad64 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -659,12 +659,27 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
>         struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
>         struct drm_connector *conn;
>         struct drm_connector_state *conn_state;
> +       struct drm_encoder *encoder;
>         int ret, i;
>
>         ret = vc4_hvs_atomic_check(crtc, state);
>         if (ret)
>                 return ret;
>
> +       encoder = vc4_get_crtc_encoder(crtc, crtc_state);
> +       if (encoder) {
> +               const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> +               struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
> +
> +               mode = &crtc_state->adjusted_mode;
> +               if (vc4_encoder->type == VC4_ENCODER_TYPE_HDMI0) {
> +                       vc4_state->hvs_load = max(mode->clock * mode->hdisplay / mode->htotal + 1000,
> +                                                 mode->clock * 9 / 10) * 1000;
> +               } else {
> +                       vc4_state->hvs_load = mode->clock * 1000;
> +               }
> +       }
> +
>         for_each_new_connector_in_state(state, conn, conn_state,
>                                         i) {
>                 if (conn_state->crtc != crtc)
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 813c5d0ea98e..4329e09d357c 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -558,6 +558,8 @@ struct vc4_crtc_state {
>                 unsigned int bottom;
>         } margins;
>
> +       unsigned long hvs_load;
> +
>         /* Transitional state below, only valid during atomic commits */
>         bool update_muxing;
>  };
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 41cb4869da50..79d4d9dd1394 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -39,9 +39,11 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
>
>  struct vc4_hvs_state {
>         struct drm_private_state base;
> +       unsigned long core_clock_rate;
>
>         struct {
>                 unsigned in_use: 1;
> +               unsigned long fifo_load;
>                 struct drm_crtc_commit *pending_commit;
>         } fifo_state[HVS_NUM_CHANNELS];
>  };
> @@ -340,10 +342,19 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
>         struct vc4_hvs *hvs = vc4->hvs;
>         struct drm_crtc_state *old_crtc_state;
>         struct drm_crtc_state *new_crtc_state;
> +       struct vc4_hvs_state *new_hvs_state;
>         struct drm_crtc *crtc;
>         struct vc4_hvs_state *old_hvs_state;
>         int i;
>
> +       old_hvs_state = vc4_hvs_get_old_global_state(state);
> +       if (WARN_ON(!old_hvs_state))
> +               return;
> +
> +       new_hvs_state = vc4_hvs_get_new_global_state(state);
> +       if (WARN_ON(!new_hvs_state))
> +               return;
> +
>         for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>                 struct vc4_crtc_state *vc4_crtc_state;
>
> @@ -354,12 +365,13 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
>                 vc4_hvs_mask_underrun(dev, vc4_crtc_state->assigned_channel);
>         }
>
> -       if (vc4->hvs->hvs5)
> -               clk_set_min_rate(hvs->core_clk, 500000000);
> +       if (vc4->hvs->hvs5) {
> +               unsigned long core_rate = max_t(unsigned long,
> +                                               500000000,
> +                                               new_hvs_state->core_clock_rate);
>
> -       old_hvs_state = vc4_hvs_get_old_global_state(state);
> -       if (!old_hvs_state)
> -               return;
> +               clk_set_min_rate(hvs->core_clk, core_rate);
> +       }
>
>         for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>                 struct vc4_crtc_state *vc4_crtc_state =
> @@ -399,8 +411,12 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
>
>         drm_atomic_helper_cleanup_planes(dev, state);
>
> -       if (vc4->hvs->hvs5)
> -               clk_set_min_rate(hvs->core_clk, 0);
> +       if (vc4->hvs->hvs5) {
> +               drm_dbg(dev, "Running the core clock at %lu Hz\n",
> +                       new_hvs_state->core_clock_rate);
> +
> +               clk_set_min_rate(hvs->core_clk, new_hvs_state->core_clock_rate);
> +       }
>  }
>
>  static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
> @@ -657,9 +673,9 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>
>         __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
>
> -
>         for (i = 0; i < HVS_NUM_CHANNELS; i++) {
>                 state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
> +               state->fifo_state[i].fifo_load = old_state->fifo_state[i].fifo_load;
>
>                 if (!old_state->fifo_state[i].pending_commit)
>                         continue;
> @@ -668,6 +684,8 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>                         drm_crtc_commit_get(old_state->fifo_state[i].pending_commit);
>         }
>
> +       state->core_clock_rate = old_state->core_clock_rate;
> +
>         return &state->base;
>  }
>
> @@ -822,6 +840,76 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>         return 0;
>  }
>
> +static int
> +vc4_core_clock_atomic_check(struct drm_atomic_state *state)
> +{
> +       struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> +       struct drm_private_state *priv_state;
> +       struct vc4_hvs_state *hvs_new_state;
> +       struct vc4_load_tracker_state *load_state;
> +       struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +       struct drm_crtc *crtc;
> +       unsigned int num_outputs;
> +       unsigned long pixel_rate;
> +       unsigned long cob_rate;
> +       unsigned int i;
> +
> +       priv_state = drm_atomic_get_private_obj_state(state,
> +                                                     &vc4->load_tracker);
> +       if (IS_ERR(priv_state))
> +               return PTR_ERR(priv_state);
> +
> +       load_state = to_vc4_load_tracker_state(priv_state);
> +
> +       hvs_new_state = vc4_hvs_get_global_state(state);
> +       if (!hvs_new_state)
> +               return -EINVAL;
> +
> +       for_each_oldnew_crtc_in_state(state, crtc,
> +                                     old_crtc_state,
> +                                     new_crtc_state,
> +                                     i) {
> +               if (old_crtc_state->active) {
> +                       struct vc4_crtc_state *old_vc4_state =
> +                               to_vc4_crtc_state(old_crtc_state);
> +                       unsigned int channel = old_vc4_state->assigned_channel;
> +
> +                       hvs_new_state->fifo_state[channel].fifo_load = 0;
> +               }
> +
> +               if (new_crtc_state->active) {
> +                       struct vc4_crtc_state *new_vc4_state =
> +                               to_vc4_crtc_state(new_crtc_state);
> +                       unsigned int channel = new_vc4_state->assigned_channel;
> +
> +                       hvs_new_state->fifo_state[channel].fifo_load =
> +                               new_vc4_state->hvs_load;
> +               }
> +       }
> +
> +       cob_rate = 0;
> +       num_outputs = 0;
> +       for (i = 0; i < HVS_NUM_CHANNELS; i++) {
> +               if (!hvs_new_state->fifo_state[i].in_use)
> +                       continue;
> +
> +               num_outputs++;
> +               cob_rate += hvs_new_state->fifo_state[i].fifo_load;
> +       }
> +
> +       pixel_rate = load_state->hvs_load;
> +       if (num_outputs > 1) {
> +               pixel_rate = (pixel_rate * 40) / 100;
> +       } else {
> +               pixel_rate = (pixel_rate * 60) / 100;
> +       }
> +
> +       hvs_new_state->core_clock_rate = max(cob_rate, pixel_rate);
> +
> +       return 0;
> +}
> +
> +
>  static int
>  vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>  {
> @@ -839,7 +927,11 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>         if (ret)
>                 return ret;
>
> -       return vc4_load_tracker_atomic_check(state);
> +       ret = vc4_load_tracker_atomic_check(state);
> +       if (ret)
> +               return ret;
> +
> +       return vc4_core_clock_atomic_check(state);
>  }
>
>  static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = {
> --
> 2.31.1
>

Re: [PATCH v8 01/10] drm/vc4: hdmi: Remove the DDC probing for status detection

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Commit 9d44abbbb8d5 ("drm/vc4: Fall back to using an EDID probe in the
> absence of a GPIO.") added some code to read the EDID through DDC in the
> HDMI driver detect hook since the Pi3 had no HPD GPIO back then.
> However, commit b1b8f45b3130 ("ARM: dts: bcm2837: Add missing GPIOs of
> Expander") changed that a couple of years later.
>
> This causes an issue though since some TV (like the LG 55C8) when it
> comes out of standy will deassert the HPD line, but the EDID will
> remain readable.
>
> It causes an issues nn platforms without an HPD GPIO, like the Pi4,
> where the DDC probing will be our primary mean to detect a display, and
> thus we will never detect the HPD pulse. This was fine before since the
> pulse was small enough that we would never detect it, and we also didn't
> have anything (like the scrambler) that needed to be set up in the
> display.
>
> However, now that we have both, the display during the HPD pulse will
> clear its scrambler status, and since we won't detect the
> disconnect/reconnect cycle we will never enable the scrambler back.
>
> As our main reason for that DDC probing is gone, let's just remove it.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

A quick conversation with Dom does conclude that the old code was odd
in that DDC read attempt was before we checked the Pi4 HPD. If there
is a need to read the DDC, then it should be after HPD (in whatever
form it exists) has been checked.
No need for that to be reinstated at this point, so this patch is fine
as it stands.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 7b0cb08e6563..338968275724 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -193,8 +193,6 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
>         if (vc4_hdmi->hpd_gpio &&
>             gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) {
>                 connected = true;
> -       } else if (drm_probe_ddc(vc4_hdmi->ddc)) {
> -               connected = true;
>         } else {
>                 unsigned long flags;
>                 u32 hotplug;
> --
> 2.31.1
>

Re: [PATCH v8 01/10] drm/vc4: hdmi: Remove the DDC probing for status detection

[Dave Stevenson]

On Mon, 25 Oct 2021 at 16:29, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Commit 9d44abbbb8d5 ("drm/vc4: Fall back to using an EDID probe in the
> absence of a GPIO.") added some code to read the EDID through DDC in the
> HDMI driver detect hook since the Pi3 had no HPD GPIO back then.
> However, commit b1b8f45b3130 ("ARM: dts: bcm2837: Add missing GPIOs of
> Expander") changed that a couple of years later.
>
> This causes an issue though since some TV (like the LG 55C8) when it
> comes out of standy will deassert the HPD line, but the EDID will
> remain readable.
>
> It causes an issues nn platforms without an HPD GPIO, like the Pi4,
> where the DDC probing will be our primary mean to detect a display, and
> thus we will never detect the HPD pulse. This was fine before since the
> pulse was small enough that we would never detect it, and we also didn't
> have anything (like the scrambler) that needed to be set up in the
> display.
>
> However, now that we have both, the display during the HPD pulse will
> clear its scrambler status, and since we won't detect the
> disconnect/reconnect cycle we will never enable the scrambler back.
>
> As our main reason for that DDC probing is gone, let's just remove it.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

A quick conversation with Dom does conclude that the old code was odd
in that DDC read attempt was before we checked the Pi4 HPD. If there
is a need to read the DDC, then it should be after HPD (in whatever
form it exists) has been checked.
No need for that to be reinstated at this point, so this patch is fine
as it stands.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 7b0cb08e6563..338968275724 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -193,8 +193,6 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
>         if (vc4_hdmi->hpd_gpio &&
>             gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) {
>                 connected = true;
> -       } else if (drm_probe_ddc(vc4_hdmi->ddc)) {
> -               connected = true;
>         } else {
>                 unsigned long flags;
>                 u32 hotplug;
> --
> 2.31.1
>

Re: [PATCH v8 00/10] drm/vc4: hdmi: Support the 4k @ 60Hz modes

[Maxime Ripard]

On Mon, 25 Oct 2021 17:28:53 +0200, Maxime Ripard wrote:
> 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 option.
> 
> Let me know what you think,
> Maxime
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: [PATCH v8 00/10] drm/vc4: hdmi: Support the 4k @ 60Hz modes

[Maxime Ripard]

On Mon, 25 Oct 2021 17:28:53 +0200, Maxime Ripard wrote:
> 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 option.
> 
> Let me know what you think,
> Maxime
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime