[PATCH v2 00/13] drm: Add generic helpers for HDMI scrambling

[PATCH v2 00/13] drm: Add generic helpers for HDMI scrambling

[Maxime Ripard]

Hi,

This is a follow-up of the work to support the interactions between the hotplug
and the scrambling support for vc4:

https://lore.kernel.org/dri-devel/20210507150515.257424-11-maxime@cerno.tech/
https://lore.kernel.org/dri-devel/20211025152903.1088803-10-maxime@cerno.tech/

Ville feedback was that the same discussion happened some time ago for i915,
and resulted in a function to do an full disable/enable cycle on reconnection
to avoid breaking the HDMI 2.0 spec.

This series improves the current scrambling support by adding generic helpers
for usual scrambling-related operations, and builds upon them to provide a
generic alternative to intel_hdmi_reset_link.

Let me know what you think,
Maxime

Changes from v1:
  - Dropped the 340MHz define
  - Make drm_mode_hdmi_requires_scrambling use the bpc
  - Make more drm_display_mode const in vc4
  - Dropped the tegra conversion
  - Added more comments

Maxime Ripard (13):
  drm/connector: Add helper to check if a mode requires scrambling
  drm/atomic: Add HDMI scrambler state helper
  drm/atomic: Add HDMI reset link helper
  drm/scdc: Document hotplug gotchas
  drm/vc4: hdmi: Constify drm_display_mode
  drm/vc4: hdmi: Remove unused argument in vc4_hdmi_supports_scrambling
  drm/vc4: hdmi: Remove mutex in detect
  drm/vc4: hdmi: Remove HDMI flag from encoder
  drm/vc4: hdmi: Simplify the hotplug handling
  drm/vc4: hdmi: Simplify the connector state retrieval
  drm/vc4: hdmi: Switch to detect_ctx
  drm/vc4: hdmi: Leverage new SCDC atomic_check
  drm/vc4: hdmi: Reset link on hotplug

 drivers/gpu/drm/drm_atomic_helper.c       | 109 +++++++++
 drivers/gpu/drm/drm_atomic_state_helper.c |  58 +++++
 drivers/gpu/drm/drm_scdc_helper.c         |  13 ++
 drivers/gpu/drm/vc4/vc4_hdmi.c            | 257 ++++++++++------------
 drivers/gpu/drm/vc4/vc4_hdmi.h            |  19 +-
 include/drm/drm_atomic_helper.h           |   3 +
 include/drm/drm_atomic_state_helper.h     |   3 +
 include/drm/drm_connector.h               |  25 +++
 include/drm/drm_modes.h                   |  20 ++
 9 files changed, 353 insertions(+), 154 deletions(-)

-- 
2.33.1

[PATCH v2 01/13] drm/connector: Add helper to check if a mode requires scrambling

[Maxime Ripard]

Most drivers supporting the HDMI scrambling seem to have the HDMI 1.4
maximum frequency open-coded, and a function to test whether a display
mode is above that threshold to control whether or not scrambling should
be enabled.

Let's create a common define and helper for drivers to reuse.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 include/drm/drm_modes.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 29ba4adf0c53..3bbf98ae59ae 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -424,6 +424,26 @@ static inline bool drm_mode_is_stereo(const struct drm_display_mode *mode)
 	return mode->flags & DRM_MODE_FLAG_3D_MASK;
 }
 
+/**
+ * drm_mode_hdmi_requires_scrambling - Checks if a mode requires HDMI Scrambling
+ * @mode: DRM display mode
+ * @bpc: Pixels bit depth
+ *
+ * Checks if a given display mode requires the scrambling to be enabled.
+ *
+ * Returns:
+ * A boolean stating whether it's required or not.
+ */
+static inline bool
+drm_mode_hdmi_requires_scrambling(const struct drm_display_mode *mode,
+				  unsigned char bpc)
+{
+	unsigned long long clock = mode->crtc_clock * bpc;
+	do_div(clock, 8);
+
+	return mode->clock > 340000;
+}
+
 struct drm_connector;
 struct drm_cmdline_mode;
 
-- 
2.33.1

[PATCH v2 02/13] drm/atomic: Add HDMI scrambler state helper

[Maxime Ripard]

All the drivers that implement the HDMI scrambling setup (dw-hdmi, i915,
tegra, vc4) duplicate the same logic to see if the TMDS ratio or the
scrambling state needs to be modified depending on the current connector
state and CRTC mode.

Since it's basically the same logic everywhere, let's put these two
informations in the connector state, and filled by a atomic_check helper
so that drivers can just use it.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 58 +++++++++++++++++++++++
 include/drm/drm_atomic_state_helper.h     |  3 ++
 include/drm/drm_connector.h               | 25 ++++++++++
 3 files changed, 86 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index ddcf5c2c8e6a..08dfb2d1bf9b 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -454,6 +454,64 @@ void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset);
 
+/**
+ * drm_atomic_helper_connector_hdmi_check - Checks the state of an HDMI connector
+ * @connector: DRM connector
+ * @state: DRM atomic state to check
+ *
+ * Checks that an HDMI connector state is sane, and sets the various
+ * HDMI-specific flags in drm_connector_state related to HDMI support.
+ *
+ * Returns:
+ * 0 on success, a negative error code otherwise.
+ */
+int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
+					   struct drm_atomic_state *state)
+{
+	struct drm_connector_state *conn_state = drm_atomic_get_new_connector_state(state,
+										    connector);
+	struct drm_display_info *info = &connector->display_info;
+	const struct drm_display_mode *mode;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	bool required;
+
+	if (!conn_state)
+		return -EINVAL;
+
+	crtc = conn_state->crtc;
+	if (!crtc)
+		return -EINVAL;
+
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
+	mode = &crtc_state->mode;
+	crtc_state->connectors_changed = true;
+	conn_state->hdmi_needs_scrambling = false;
+	conn_state->hdmi_needs_high_tmds_ratio = false;
+
+	if (!info->is_hdmi)
+		return 0;
+
+	if (!info->hdmi.scdc.supported)
+		return 0;
+
+	required = drm_mode_hdmi_requires_scrambling(mode, conn_state->max_bpc);
+	if (required && !info->hdmi.scdc.scrambling.supported)
+		return -EINVAL;
+
+	if (info->hdmi.scdc.scrambling.low_rates || required)
+		conn_state->hdmi_needs_scrambling = true;
+
+	if (required)
+		conn_state->hdmi_needs_high_tmds_ratio = true;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check);
+
 /**
  * __drm_atomic_helper_connector_duplicate_state - copy atomic connector state
  * @connector: connector object
diff --git a/include/drm/drm_atomic_state_helper.h b/include/drm/drm_atomic_state_helper.h
index 3f8f1d627f7c..3d3d1ff355f4 100644
--- a/include/drm/drm_atomic_state_helper.h
+++ b/include/drm/drm_atomic_state_helper.h
@@ -26,6 +26,7 @@
 
 #include <linux/types.h>
 
+struct drm_atomic_state;
 struct drm_bridge;
 struct drm_bridge_state;
 struct drm_crtc;
@@ -71,6 +72,8 @@ void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
 					 struct drm_connector_state *conn_state);
 void drm_atomic_helper_connector_reset(struct drm_connector *connector);
 void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector);
+int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
+					   struct drm_atomic_state *state);
 void
 __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
 					   struct drm_connector_state *state);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index b501d0badaea..02c6f9f0d4f7 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -830,6 +830,31 @@ struct drm_connector_state {
 	 * DRM blob property for HDR output metadata
 	 */
 	struct drm_property_blob *hdr_output_metadata;
+
+	/**
+	 * @hdmi_needs_scrambling:
+	 *
+	 * Only relevant for HDMI sink. Tracks whether the scrambling
+	 * should be turned on for the current sink and mode.
+	 *
+	 * Drivers needing this should use
+	 * drm_atomic_helper_connector_hdmi_check() and use the value
+	 * set here to enable or disable their scrambler.
+	 */
+	bool hdmi_needs_scrambling;
+
+	/**
+	 * @hdmi_needs_high_tmds_ratio:
+	 *
+	 * Only relevant for HDMI sink. Tracks whether the TMDS clock
+	 * ratio should be 1/10 of the pixel clock (false), or 1/40
+	 * (true).
+	 *
+	 * Drivers needing this should use
+	 * drm_atomic_helper_connector_hdmi_check() and use the value
+	 * set here to enable or disable their scrambler.
+	 */
+	bool hdmi_needs_high_tmds_ratio;
 };
 
 /**
-- 
2.33.1

[PATCH v2 03/13] drm/atomic: Add HDMI reset link helper

[Maxime Ripard]

During a hotplug cycle (such as a TV going out of suspend, or when the
cable is disconnected and reconnected), the expectation is that the same
state used before the disconnection is reused until the next commit.

However, the HDMI scrambling requires that some flags are set in the
monitor, and those flags are very likely to be reset when the cable has
been disconnected. This will thus result in a blank display, even if the
display pipeline configuration hasn't been modified or is in the exact
same state.

One solution would be to enable the scrambling-related bits again on
reconnection, but the HDMI 2.0 specification (Section 6.1.3.1 -
Scrambling Control) requires that the scrambling enable bit is set
before sending any scrambled video signal. Using that solution would
break that specification expectation.

Thus, we need to do a full modeset on the connector so that we disable
the video signal, enable the scrambling bit, and enable the video signal
again.

The i915 code was doing this already, so let's take its code and
convert it into a generic helper.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_atomic_helper.c | 109 ++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h     |   3 +
 2 files changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 2c0c6ec92820..9f3fcc65e66e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -38,6 +38,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_print.h>
+#include <drm/drm_scdc_helper.h>
 #include <drm/drm_self_refresh_helper.h>
 #include <drm/drm_vblank.h>
 #include <drm/drm_writeback.h>
@@ -3524,3 +3525,111 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,
 	return input_fmts;
 }
 EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt);
+
+static int modeset_pipe(struct drm_crtc *crtc,
+			struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_atomic_state *state;
+	struct drm_crtc_state *crtc_state;
+	int ret;
+
+	state = drm_atomic_state_alloc(crtc->dev);
+	if (!state)
+		return -ENOMEM;
+
+	state->acquire_ctx = ctx;
+
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state)) {
+		ret = PTR_ERR(crtc_state);
+		goto out;
+	}
+
+	crtc_state->connectors_changed = true;
+
+	ret = drm_atomic_commit(state);
+out:
+	drm_atomic_state_put(state);
+
+	return ret;
+}
+
+/**
+ * drm_atomic_helper_connector_hdmi_reset_link() - Resets an HDMI link
+ * @connector: DRM connector we want to reset
+ * @ctx: Lock acquisition context
+ *
+ * This helper is here to restore the HDMI link state after the
+ * connector status has changed, typically when a TV has come out of
+ * suspend or when the HDMI cable has been disconnected and then
+ * reconnected.
+ *
+ * Returns:
+ * 0 on success, a negative error code otherwise.
+ */
+int drm_atomic_helper_connector_hdmi_reset_link(struct drm_connector *connector,
+						struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_device *drm = connector->dev;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	u8 config;
+	int ret;
+
+	if (!connector)
+		return 0;
+
+	drm_WARN_ON(drm,
+		    (connector->connector_type != DRM_MODE_CONNECTOR_HDMIA) &&
+		    (connector->connector_type != DRM_MODE_CONNECTOR_HDMIB));
+
+	ret = drm_modeset_lock(&drm->mode_config.connection_mutex, ctx);
+	if (ret)
+		return ret;
+
+	conn_state = connector->state;
+	crtc = conn_state->crtc;
+	if (!crtc)
+		return 0;
+
+	ret = drm_modeset_lock(&crtc->mutex, ctx);
+	if (ret)
+		return ret;
+
+	crtc_state = crtc->state;
+	if (!crtc_state->active)
+		return 0;
+
+	if (!conn_state->hdmi_needs_high_tmds_ratio &&
+	    !conn_state->hdmi_needs_scrambling)
+		return 0;
+
+	if (conn_state->commit &&
+	    !try_wait_for_completion(&conn_state->commit->hw_done))
+		return 0;
+
+	ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
+	if (ret < 0) {
+		drm_err(drm, "Failed to read TMDS config: %d\n", ret);
+		return 0;
+	}
+
+	if (!!(config & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40) ==
+	    conn_state->hdmi_needs_high_tmds_ratio &&
+	    !!(config & SCDC_SCRAMBLING_ENABLE) ==
+	    conn_state->hdmi_needs_scrambling)
+		return 0;
+
+	/*
+	 * HDMI 2.0 says that one should not send scrambled data
+	 * prior to configuring the sink scrambling, and that
+	 * TMDS clock/data transmission should be suspended when
+	 * changing the TMDS clock rate in the sink. So let's
+	 * just do a full modeset here, even though some sinks
+	 * would be perfectly happy if were to just reconfigure
+	 * the SCDC settings on the fly.
+	 */
+	return modeset_pipe(crtc, ctx);
+}
+EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_reset_link);
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 4045e2507e11..d7727f9a6fe9 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -231,4 +231,7 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,
 					u32 output_fmt,
 					unsigned int *num_input_fmts);
 
+int drm_atomic_helper_connector_hdmi_reset_link(struct drm_connector *connector,
+						struct drm_modeset_acquire_ctx *ctx);
+
 #endif /* DRM_ATOMIC_HELPER_H_ */
-- 
2.33.1

[PATCH v2 04/13] drm/scdc: Document hotplug gotchas

[Maxime Ripard]

There's some interactions between the SCDC setup and the disconnection /
reconnection of displays. Let's document it and a solution.

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

diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
index 48a382464d54..033a9e407acb 100644
--- a/drivers/gpu/drm/drm_scdc_helper.c
+++ b/drivers/gpu/drm/drm_scdc_helper.c
@@ -34,6 +34,19 @@
  * HDMI 2.0 specification. It is a point-to-point protocol that allows the
  * HDMI source and HDMI sink to exchange data. The same I2C interface that
  * is used to access EDID serves as the transport mechanism for SCDC.
+ *
+ * Note: The SCDC status is going to be lost when the display is
+ * disconnected. This can happen physically when the user disconnects
+ * the cable, but also when a display is switched on (such as waking up
+ * a TV).
+ *
+ * This is further complicated by the fact that, upon a disconnection /
+ * reconnection, KMS won't change the mode on its own. This means that
+ * one can't just rely on setting the SCDC status on enable, but also
+ * has to track the connector status changes using interrupts and
+ * restore the SCDC status. The typical solution for this is to call
+ * drm_atomic_helper_connector_hdmi_reset_link() in
+ * drm_connector_helper_funcs.detect_ctx().
  */
 
 #define SCDC_I2C_SLAVE_ADDRESS 0x54
-- 
2.33.1

[PATCH v2 05/13] drm/vc4: hdmi: Constify drm_display_mode

[Maxime Ripard]

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

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 053fbaf765ca..b00fedb5b880 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -260,7 +260,7 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 
 	if (vc4_hdmi->disable_4kp60) {
 		struct drm_device *drm = connector->dev;
-		struct drm_display_mode *mode;
+		const struct drm_display_mode *mode;
 
 		list_for_each_entry(mode, &connector->probed_modes, head) {
 			if (vc4_hdmi_mode_needs_scrambling(mode)) {
@@ -574,7 +574,7 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder)
 }
 
 static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder,
-					 struct drm_display_mode *mode)
+					 const struct drm_display_mode *mode)
 {
 	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
@@ -597,7 +597,7 @@ static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder,
 static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
+	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	unsigned long flags;
 
 	lockdep_assert_held(&vc4_hdmi->mutex);
@@ -812,7 +812,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi, bool enable)
 
 static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
 				 struct drm_connector_state *state,
-				 struct drm_display_mode *mode)
+				 const struct drm_display_mode *mode)
 {
 	bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
 	bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC;
@@ -863,7 +863,7 @@ static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
 
 static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
 				 struct drm_connector_state *state,
-				 struct drm_display_mode *mode)
+				 const struct drm_display_mode *mode)
 {
 	bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
 	bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC;
@@ -1010,7 +1010,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 	struct vc4_hdmi_connector_state *vc4_conn_state =
 		conn_state_to_vc4_hdmi_conn_state(conn_state);
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
+	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	unsigned long pixel_rate = vc4_conn_state->pixel_rate;
 	unsigned long bvb_rate, hsm_rate;
 	unsigned long flags;
@@ -1113,7 +1113,7 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder,
 					     struct drm_atomic_state *state)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
+	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
 	unsigned long flags;
 
@@ -1143,7 +1143,7 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder,
 					      struct drm_atomic_state *state)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
+	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
 	bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
 	bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC;
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 36c0b082a43b..e6562bfa6fae 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -82,7 +82,7 @@ struct vc4_hdmi_variant {
 	/* Callback to configure the video timings in the HDMI block */
 	void (*set_timings)(struct vc4_hdmi *vc4_hdmi,
 			    struct drm_connector_state *state,
-			    struct drm_display_mode *mode);
+			    const struct drm_display_mode *mode);
 
 	/* Callback to initialize the PHY according to the connector state */
 	void (*phy_init)(struct vc4_hdmi *vc4_hdmi,
-- 
2.33.1

[PATCH v2 06/13] drm/vc4: hdmi: Remove unused argument in vc4_hdmi_supports_scrambling

[Maxime Ripard]

Even though vc4_hdmi_supports_scrambling takes a mode as an argument, it
never uses it. Let's remove it.

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

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index b00fedb5b880..9a7864440d95 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -573,8 +573,7 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder)
 	vc4_hdmi_set_hdr_infoframe(encoder);
 }
 
-static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder,
-					 const struct drm_display_mode *mode)
+static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder)
 {
 	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
@@ -602,7 +601,7 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder)
 
 	lockdep_assert_held(&vc4_hdmi->mutex);
 
-	if (!vc4_hdmi_supports_scrambling(encoder, mode))
+	if (!vc4_hdmi_supports_scrambling(encoder))
 		return;
 
 	if (!vc4_hdmi_mode_needs_scrambling(mode))
-- 
2.33.1

[PATCH v2 07/13] drm/vc4: hdmi: Remove mutex in detect

[Maxime Ripard]

We recently introduced a new mutex to protect concurrent execution of
ALSA and KMS hooks, and the concurrent access to some of vc4_hdmi
fields.

However, using it in the detect hook was creating a reentrency issue
with CEC code. Indeed, calling cec_s_phys_addr_from_edid from detect
might call the CEC adap_enable hook with the lock held, eventually
resulting in a deadlock.

Since we didn't really need to protect anything at the moment in the CEC
code, the decision was made to ignore the mutex in those CEC hooks,
working around the issue.

However, we can have the same thing happening if we end up triggering a
mode set from the detect callback, for example using
drm_atomic_helper_connector_hdmi_reset_link().

Since we don't really need to protect anything in detect either, let's
just drop the lock in detect, and add it again in CEC.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 89 +++++++++++++---------------------
 drivers/gpu/drm/vc4/vc4_hdmi.h | 10 +---
 2 files changed, 36 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 9a7864440d95..e0b30c9e9559 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -188,7 +188,16 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
 	bool connected = false;
 
-	mutex_lock(&vc4_hdmi->mutex);
+	/*
+	 * NOTE: This function should really take vc4_hdmi->mutex, but
+	 * doing so results in reentrancy issues since
+	 * cec_s_phys_addr_from_edid might call .adap_enable, which
+	 * leads to that funtion being called with our mutex held.
+	 *
+	 * Concurrency isn't an issue at the moment since we don't share
+	 * any state with any of the other frameworks so we can ignore
+	 * the lock for now.
+	 */
 
 	WARN_ON(pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev));
 
@@ -220,13 +229,11 @@ 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;
 	}
 
 	cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
 	pm_runtime_put(&vc4_hdmi->pdev->dev);
-	mutex_unlock(&vc4_hdmi->mutex);
 	return connector_status_disconnected;
 }
 
@@ -243,14 +250,21 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 	int ret = 0;
 	struct edid *edid;
 
-	mutex_lock(&vc4_hdmi->mutex);
+	/*
+	 * NOTE: This function should really take vc4_hdmi->mutex, but
+	 * doing so results in reentrancy issues since
+	 * cec_s_phys_addr_from_edid might call .adap_enable, which
+	 * leads to that funtion being called with our mutex held.
+	 *
+	 * Concurrency isn't an issue at the moment since we don't share
+	 * any state with any of the other frameworks so we can ignore
+	 * the lock for now.
+	 */
 
 	edid = drm_get_edid(connector, vc4_hdmi->ddc);
 	cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
-	if (!edid) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (!edid)
+		return -ENODEV;
 
 	vc4_encoder->hdmi_monitor = drm_detect_hdmi_monitor(edid);
 
@@ -270,9 +284,6 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 		}
 	}
 
-out:
-	mutex_unlock(&vc4_hdmi->mutex);
-
 	return ret;
 }
 
@@ -1991,21 +2002,12 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 	u32 val;
 	int ret;
 
-	/*
-	 * NOTE: This function should really take vc4_hdmi->mutex, but doing so
-	 * results in a reentrancy since cec_s_phys_addr_from_edid() called in
-	 * .detect or .get_modes might call .adap_enable, which leads to this
-	 * function being called with that mutex held.
-	 *
-	 * Concurrency is not an issue for the moment since we don't share any
-	 * state with KMS, so we can ignore the lock for now, but we need to
-	 * keep it in mind if we were to change that assumption.
-	 */
-
 	ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev);
 	if (ret)
 		return ret;
 
+	mutex_lock(&vc4_hdmi->mutex);
+
 	spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
 
 	val = HDMI_READ(HDMI_CEC_CNTRL_5);
@@ -2040,6 +2042,8 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 
 	spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
 
+	mutex_unlock(&vc4_hdmi->mutex);
+
 	return 0;
 }
 
@@ -2048,16 +2052,7 @@ static int vc4_hdmi_cec_disable(struct cec_adapter *adap)
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
 	unsigned long flags;
 
-	/*
-	 * NOTE: This function should really take vc4_hdmi->mutex, but doing so
-	 * results in a reentrancy since cec_s_phys_addr_from_edid() called in
-	 * .detect or .get_modes might call .adap_enable, which leads to this
-	 * function being called with that mutex held.
-	 *
-	 * Concurrency is not an issue for the moment since we don't share any
-	 * state with KMS, so we can ignore the lock for now, but we need to
-	 * keep it in mind if we were to change that assumption.
-	 */
+	mutex_lock(&vc4_hdmi->mutex);
 
 	spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
 
@@ -2069,6 +2064,8 @@ static int vc4_hdmi_cec_disable(struct cec_adapter *adap)
 
 	spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
 
+	mutex_unlock(&vc4_hdmi->mutex);
+
 	pm_runtime_put(&vc4_hdmi->pdev->dev);
 
 	return 0;
@@ -2087,22 +2084,13 @@ static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
 	unsigned long flags;
 
-	/*
-	 * NOTE: This function should really take vc4_hdmi->mutex, but doing so
-	 * results in a reentrancy since cec_s_phys_addr_from_edid() called in
-	 * .detect or .get_modes might call .adap_enable, which leads to this
-	 * function being called with that mutex held.
-	 *
-	 * Concurrency is not an issue for the moment since we don't share any
-	 * state with KMS, so we can ignore the lock for now, but we need to
-	 * keep it in mind if we were to change that assumption.
-	 */
-
+	mutex_lock(&vc4_hdmi->mutex);
 	spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
 	HDMI_WRITE(HDMI_CEC_CNTRL_1,
 		   (HDMI_READ(HDMI_CEC_CNTRL_1) & ~VC4_HDMI_CEC_ADDR_MASK) |
 		   (log_addr & 0xf) << VC4_HDMI_CEC_ADDR_SHIFT);
 	spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
+	mutex_unlock(&vc4_hdmi->mutex);
 
 	return 0;
 }
@@ -2116,22 +2104,13 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 	u32 val;
 	unsigned int i;
 
-	/*
-	 * NOTE: This function should really take vc4_hdmi->mutex, but doing so
-	 * results in a reentrancy since cec_s_phys_addr_from_edid() called in
-	 * .detect or .get_modes might call .adap_enable, which leads to this
-	 * function being called with that mutex held.
-	 *
-	 * Concurrency is not an issue for the moment since we don't share any
-	 * state with KMS, so we can ignore the lock for now, but we need to
-	 * keep it in mind if we were to change that assumption.
-	 */
-
 	if (msg->len > 16) {
 		drm_err(dev, "Attempting to transmit too much data (%d)\n", msg->len);
 		return -ENOMEM;
 	}
 
+	mutex_lock(&vc4_hdmi->mutex);
+
 	spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
 
 	for (i = 0; i < msg->len; i += 4)
@@ -2152,6 +2131,8 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 
 	spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
 
+	mutex_unlock(&vc4_hdmi->mutex);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index e6562bfa6fae..85bf25824832 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -186,15 +186,7 @@ struct vc4_hdmi {
 
 	/**
 	 * @mutex: Mutex protecting the driver access across multiple
-	 * frameworks (KMS, ALSA).
-	 *
-	 * NOTE: While supported, CEC has been left out since
-	 * cec_s_phys_addr_from_edid() might call .adap_enable and lead to a
-	 * reentrancy issue between .get_modes (or .detect) and .adap_enable.
-	 * Since we don't share any state between the CEC hooks and KMS', it's
-	 * not a big deal. The only trouble might come from updating the CEC
-	 * clock divider which might be affected by a modeset, but CEC should
-	 * be resilient to that.
+	 * frameworks (KMS, ALSA, CEC).
 	 */
 	struct mutex mutex;
 
-- 
2.33.1

[PATCH v2 08/13] drm/vc4: hdmi: Remove HDMI flag from encoder

[Maxime Ripard]

The hdmi_monitor flag in the vc4_hdmi_encoder structure is redundant
with the is_hdmi flag in the drm_display_info structure.

Let's convert all the users.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 16 ++++++----------
 drivers/gpu/drm/vc4/vc4_hdmi.h |  1 -
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index e0b30c9e9559..98f15b11f135 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -222,7 +222,6 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 
 			if (edid) {
 				cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
-				vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid);
 				kfree(edid);
 			}
 		}
@@ -246,7 +245,6 @@ static void vc4_hdmi_connector_destroy(struct drm_connector *connector)
 static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 {
 	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
-	struct vc4_hdmi_encoder *vc4_encoder = &vc4_hdmi->encoder;
 	int ret = 0;
 	struct edid *edid;
 
@@ -266,8 +264,6 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 	if (!edid)
 		return -ENODEV;
 
-	vc4_encoder->hdmi_monitor = drm_detect_hdmi_monitor(edid);
-
 	drm_connector_update_edid_property(connector, edid);
 	ret = drm_add_edid_modes(connector, edid);
 	kfree(edid);
@@ -586,13 +582,12 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder)
 
 static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder)
 {
-	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
 
 	lockdep_assert_held(&vc4_hdmi->mutex);
 
-	if (!vc4_encoder->hdmi_monitor)
+	if (!display->is_hdmi)
 		return false;
 
 	if (!display->hdmi.scdc.supported ||
@@ -1125,11 +1120,12 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder,
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
+	const struct drm_display_info *display = &vc4_hdmi->connector.display_info;
 	unsigned long flags;
 
 	mutex_lock(&vc4_hdmi->mutex);
 
-	if (vc4_encoder->hdmi_monitor &&
+	if (display->is_hdmi &&
 	    drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED) {
 		if (vc4_hdmi->variant->csc_setup)
 			vc4_hdmi->variant->csc_setup(vc4_hdmi, true);
@@ -1154,7 +1150,7 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder,
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
-	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
+	const struct drm_display_info *display = &vc4_hdmi->connector.display_info;
 	bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
 	bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC;
 	unsigned long flags;
@@ -1175,7 +1171,7 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder,
 	HDMI_WRITE(HDMI_VID_CTL,
 		   HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_BLANKPIX);
 
-	if (vc4_encoder->hdmi_monitor) {
+	if (display->is_hdmi) {
 		HDMI_WRITE(HDMI_SCHEDULER_CONTROL,
 			   HDMI_READ(HDMI_SCHEDULER_CONTROL) |
 			   VC4_HDMI_SCHEDULER_CONTROL_MODE_HDMI);
@@ -1202,7 +1198,7 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder,
 			  "!VC4_HDMI_SCHEDULER_CONTROL_HDMI_ACTIVE\n");
 	}
 
-	if (vc4_encoder->hdmi_monitor) {
+	if (display->is_hdmi) {
 		spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
 
 		WARN_ON(!(HDMI_READ(HDMI_SCHEDULER_CONTROL) &
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 85bf25824832..4b9175f8aaaf 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -11,7 +11,6 @@
 /* VC4 HDMI encoder KMS struct */
 struct vc4_hdmi_encoder {
 	struct vc4_encoder base;
-	bool hdmi_monitor;
 	bool limited_rgb_range;
 };
 
-- 
2.33.1

[PATCH v2 09/13] drm/vc4: hdmi: Simplify the hotplug handling

[Maxime Ripard]

Our detect callback has a bunch of operations to perform depending on
the current and last status of the connector, such a setting the CEC
physical address or enabling the scrambling again.

This is currently dealt with a bunch of if / else statetements that make
it fairly difficult to read and extend.

Let's move all that logic to a function of its own.

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

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 98f15b11f135..7f44ef08f83a 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -182,17 +182,53 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
 
 static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder);
 
+static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
+				    enum drm_connector_status status)
+{
+	struct drm_connector *connector = &vc4_hdmi->connector;
+	struct edid *edid;
+
+	/*
+	 * NOTE: This function should really be called with
+	 * vc4_hdmi->mutex held, but doing so results in reentrancy
+	 * issues since cec_s_phys_addr_from_edid might call
+	 * .adap_enable, which leads to that funtion being called with
+	 * our mutex held.
+	 *
+	 * Concurrency isn't an issue at the moment since we don't share
+	 * any state with any of the other frameworks so we can ignore
+	 * the lock for now.
+	 */
+
+	if (status == connector->status)
+		return;
+
+	if (status == connector_status_disconnected) {
+		cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
+		return;
+	}
+
+	edid = drm_get_edid(connector, vc4_hdmi->ddc);
+	if (!edid)
+		return;
+
+	cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
+	kfree(edid);
+
+	vc4_hdmi_enable_scrambling(&vc4_hdmi->encoder.base.base);
+}
+
 static enum drm_connector_status
 vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 {
 	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
-	bool connected = false;
+	enum drm_connector_status status = connector_status_disconnected;
 
 	/*
 	 * NOTE: This function should really take vc4_hdmi->mutex, but
 	 * doing so results in reentrancy issues since
-	 * cec_s_phys_addr_from_edid might call .adap_enable, which
-	 * leads to that funtion being called with our mutex held.
+	 * vc4_hdmi_handle_hotplug() can call into other functions that
+	 * would take the mutex while it's held here.
 	 *
 	 * Concurrency isn't an issue at the moment since we don't share
 	 * any state with any of the other frameworks so we can ignore
@@ -203,7 +239,7 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 
 	if (vc4_hdmi->hpd_gpio) {
 		if (gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio))
-			connected = true;
+			status = connector_status_connected;
 	} else {
 		unsigned long flags;
 		u32 hotplug;
@@ -213,27 +249,13 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 		spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
 
 		if (hotplug & VC4_HDMI_HOTPLUG_CONNECTED)
-			connected = true;
+			status = connector_status_connected;
 	}
 
-	if (connected) {
-		if (connector->status != connector_status_connected) {
-			struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc);
-
-			if (edid) {
-				cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
-				kfree(edid);
-			}
-		}
-
-		vc4_hdmi_enable_scrambling(&vc4_hdmi->encoder.base.base);
-		pm_runtime_put(&vc4_hdmi->pdev->dev);
-		return connector_status_connected;
-	}
-
-	cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
+	vc4_hdmi_handle_hotplug(vc4_hdmi, status);
 	pm_runtime_put(&vc4_hdmi->pdev->dev);
-	return connector_status_disconnected;
+
+	return status;
 }
 
 static void vc4_hdmi_connector_destroy(struct drm_connector *connector)
-- 
2.33.1

[PATCH v2 10/13] drm/vc4: hdmi: Simplify the connector state retrieval

[Maxime Ripard]

When we have the entire DRM state, retrieving the connector state only
requires the drm_connector pointer. Fortunately for us, we have
allocated it as a part of the vc4_hdmi structure, so we can retrieve get
a pointer by simply accessing our field in that structure.

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

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 7f44ef08f83a..db647c01dc0a 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1013,30 +1013,15 @@ static void vc4_hdmi_recenter_fifo(struct vc4_hdmi *vc4_hdmi)
 		  "VC4_HDMI_FIFO_CTL_RECENTER_DONE");
 }
 
-static struct drm_connector_state *
-vc4_hdmi_encoder_get_connector_state(struct drm_encoder *encoder,
-				     struct drm_atomic_state *state)
-{
-	struct drm_connector_state *conn_state;
-	struct drm_connector *connector;
-	unsigned int i;
-
-	for_each_new_connector_in_state(state, connector, conn_state, i) {
-		if (conn_state->best_encoder == encoder)
-			return conn_state;
-	}
-
-	return NULL;
-}
-
 static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 						struct drm_atomic_state *state)
 {
+	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
+	struct drm_connector *connector = &vc4_hdmi->connector;
 	struct drm_connector_state *conn_state =
-		vc4_hdmi_encoder_get_connector_state(encoder, state);
+		drm_atomic_get_new_connector_state(state, connector);
 	struct vc4_hdmi_connector_state *vc4_conn_state =
 		conn_state_to_vc4_hdmi_conn_state(conn_state);
-	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	unsigned long pixel_rate = vc4_conn_state->pixel_rate;
 	unsigned long bvb_rate, hsm_rate;
-- 
2.33.1

[PATCH v2 11/13] drm/vc4: hdmi: Switch to detect_ctx

[Maxime Ripard]

We'll need the locking context in future patch, so let's convert .detect
to .detect_ctx.

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

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index db647c01dc0a..5858058165a2 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -218,8 +218,9 @@ static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
 	vc4_hdmi_enable_scrambling(&vc4_hdmi->encoder.base.base);
 }
 
-static enum drm_connector_status
-vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
+static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector,
+					 struct drm_modeset_acquire_ctx *ctx,
+					 bool force)
 {
 	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
 	enum drm_connector_status status = connector_status_disconnected;
@@ -370,7 +371,6 @@ vc4_hdmi_connector_duplicate_state(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
-	.detect = vc4_hdmi_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = vc4_hdmi_connector_destroy,
 	.reset = vc4_hdmi_connector_reset,
@@ -379,6 +379,7 @@ static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
 };
 
 static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs = {
+	.detect_ctx = vc4_hdmi_connector_detect_ctx,
 	.get_modes = vc4_hdmi_connector_get_modes,
 	.atomic_check = vc4_hdmi_connector_atomic_check,
 };
-- 
2.33.1

[PATCH v2 12/13] drm/vc4: hdmi: Leverage new SCDC atomic_check

[Maxime Ripard]

Now that we have a generic helper to fill the scrambling status, let's
use it.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 46 +++++++++++-----------------------
 drivers/gpu/drm/vc4/vc4_hdmi.h |  6 +++++
 2 files changed, 21 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 5858058165a2..ba939dab35c0 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -99,11 +99,6 @@
 
 #define HDMI_14_MAX_TMDS_CLK   (340 * 1000 * 1000)
 
-static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode)
-{
-	return (mode->clock * 1000) > HDMI_14_MAX_TMDS_CLK;
-}
-
 static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
 {
 	struct drm_info_node *node = (struct drm_info_node *)m->private;
@@ -296,7 +291,7 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 		const struct drm_display_mode *mode;
 
 		list_for_each_entry(mode, &connector->probed_modes, head) {
-			if (vc4_hdmi_mode_needs_scrambling(mode)) {
+			if (drm_mode_hdmi_requires_scrambling(mode, 8)) {
 				drm_warn_once(drm, "The core clock cannot reach frequencies high enough to support 4k @ 60Hz.");
 				drm_warn_once(drm, "Please change your config.txt file to add hdmi_enable_4kp60.");
 			}
@@ -314,6 +309,14 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
 	struct drm_connector_state *new_state =
 		drm_atomic_get_new_connector_state(state, connector);
 	struct drm_crtc *crtc = new_state->crtc;
+	int ret;
+
+	ret = drm_atomic_helper_connector_hdmi_check(connector, state);
+	if (ret)
+		return ret;
+
+	if (new_state->hdmi_needs_scrambling != new_state->hdmi_needs_high_tmds_ratio)
+		return -EINVAL;
 
 	if (!crtc)
 		return 0;
@@ -603,37 +606,16 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder)
 	vc4_hdmi_set_hdr_infoframe(encoder);
 }
 
-static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder)
-{
-	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
-
-	lockdep_assert_held(&vc4_hdmi->mutex);
-
-	if (!display->is_hdmi)
-		return false;
-
-	if (!display->hdmi.scdc.supported ||
-	    !display->hdmi.scdc.scrambling.supported)
-		return false;
-
-	return true;
-}
-
 #define SCRAMBLING_POLLING_DELAY_MS	1000
 
 static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	unsigned long flags;
 
 	lockdep_assert_held(&vc4_hdmi->mutex);
 
-	if (!vc4_hdmi_supports_scrambling(encoder))
-		return;
-
-	if (!vc4_hdmi_mode_needs_scrambling(mode))
+	if (!vc4_hdmi->scdc_needed)
 		return;
 
 	drm_scdc_set_high_tmds_clock_ratio(vc4_hdmi->ddc, true);
@@ -1245,6 +1227,7 @@ static void vc4_hdmi_encoder_atomic_mode_set(struct drm_encoder *encoder,
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 
 	mutex_lock(&vc4_hdmi->mutex);
+	vc4_hdmi->scdc_needed = conn_state->hdmi_needs_scrambling;
 	memcpy(&vc4_hdmi->saved_adjusted_mode,
 	       &crtc_state->adjusted_mode,
 	       sizeof(vc4_hdmi->saved_adjusted_mode));
@@ -1275,7 +1258,7 @@ static int vc4_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
 	 * bandwidth). Slightly lower the frequency to bring it out of
 	 * the WiFi range.
 	 */
-	tmds_rate = pixel_rate * 10;
+	tmds_rate = pixel_rate * (conn_state->hdmi_needs_high_tmds_ratio ? 40 : 10);
 	if (vc4_hdmi->disable_wifi_frequencies &&
 	    (tmds_rate >= WIFI_2_4GHz_CH1_MIN_FREQ &&
 	     tmds_rate <= WIFI_2_4GHz_CH1_MAX_FREQ)) {
@@ -1297,7 +1280,7 @@ static int vc4_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
 	if (pixel_rate > vc4_hdmi->variant->max_pixel_clock)
 		return -EINVAL;
 
-	if (vc4_hdmi->disable_4kp60 && (pixel_rate > HDMI_14_MAX_TMDS_CLK))
+	if (vc4_hdmi->disable_4kp60 && conn_state->hdmi_needs_scrambling)
 		return -EINVAL;
 
 	vc4_state->pixel_rate = pixel_rate;
@@ -1319,7 +1302,8 @@ vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder,
 	if ((mode->clock * 1000) > vc4_hdmi->variant->max_pixel_clock)
 		return MODE_CLOCK_HIGH;
 
-	if (vc4_hdmi->disable_4kp60 && vc4_hdmi_mode_needs_scrambling(mode))
+	if (vc4_hdmi->disable_4kp60 &&
+	    drm_mode_hdmi_requires_scrambling(mode, 8))
 		return MODE_CLOCK_HIGH;
 
 	return MODE_OK;
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 4b9175f8aaaf..36f6a208b729 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -206,6 +206,12 @@ struct vc4_hdmi {
 	 * the scrambler on? Protected by @mutex.
 	 */
 	bool scdc_enabled;
+
+	/**
+	 * @scdc_needed: Is the HDMI controller needs to have the
+	 * scrambling on? Protected by @mutex.
+	 */
+	bool scdc_needed;
 };
 
 static inline struct vc4_hdmi *
-- 
2.33.1

[PATCH v2 13/13] drm/vc4: hdmi: Reset link on hotplug

[Maxime Ripard]

Enabling the scrambling on reconnection seems to work so far but breaks
the HDMI2.0 specification and has introduced some issues in the past
with i915.

Let's do a mode set on the connector instead to follow the
specification.

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

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index ba939dab35c0..57310756d2cc 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -175,9 +175,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 void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
+				    struct drm_modeset_acquire_ctx *ctx,
 				    enum drm_connector_status status)
 {
 	struct drm_connector *connector = &vc4_hdmi->connector;
@@ -190,6 +189,10 @@ static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
 	 * .adap_enable, which leads to that funtion being called with
 	 * our mutex held.
 	 *
+	 * A similar situation occurs with
+	 * drm_atomic_helper_connector_hdmi_reset_link() that will call
+	 * into our KMS hooks if the scrambling was enabled.
+	 *
 	 * Concurrency isn't an issue at the moment since we don't share
 	 * any state with any of the other frameworks so we can ignore
 	 * the lock for now.
@@ -210,7 +213,7 @@ static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
 	cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
 	kfree(edid);
 
-	vc4_hdmi_enable_scrambling(&vc4_hdmi->encoder.base.base);
+	drm_atomic_helper_connector_hdmi_reset_link(connector, ctx);
 }
 
 static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector,
@@ -248,7 +251,7 @@ static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector,
 			status = connector_status_connected;
 	}
 
-	vc4_hdmi_handle_hotplug(vc4_hdmi, status);
+	vc4_hdmi_handle_hotplug(vc4_hdmi, ctx, status);
 	pm_runtime_put(&vc4_hdmi->pdev->dev);
 
 	return status;
-- 
2.33.1

Re: [PATCH v2 00/13] drm: Add generic helpers for HDMI scrambling

[Daniel Vetter]

On Thu, Nov 18, 2021 at 11:38:01AM +0100, Maxime Ripard wrote:
> Hi,
> 
> This is a follow-up of the work to support the interactions between the hotplug
> and the scrambling support for vc4:
> 
> https://lore.kernel.org/dri-devel/20210507150515.257424-11-maxime@cerno.tech/
> https://lore.kernel.org/dri-devel/20211025152903.1088803-10-maxime@cerno.tech/
> 
> Ville feedback was that the same discussion happened some time ago for i915,
> and resulted in a function to do an full disable/enable cycle on reconnection
> to avoid breaking the HDMI 2.0 spec.
> 
> This series improves the current scrambling support by adding generic helpers
> for usual scrambling-related operations, and builds upon them to provide a
> generic alternative to intel_hdmi_reset_link.

Out of curiosity, can we rebuild intel_hdmi_reset_link on top of these?
Always better to have two drivers to actually show the helpers help, than
just one.
-Daniel

> 
> Let me know what you think,
> Maxime
> 
> Changes from v1:
>   - Dropped the 340MHz define
>   - Make drm_mode_hdmi_requires_scrambling use the bpc
>   - Make more drm_display_mode const in vc4
>   - Dropped the tegra conversion
>   - Added more comments
> 
> Maxime Ripard (13):
>   drm/connector: Add helper to check if a mode requires scrambling
>   drm/atomic: Add HDMI scrambler state helper
>   drm/atomic: Add HDMI reset link helper
>   drm/scdc: Document hotplug gotchas
>   drm/vc4: hdmi: Constify drm_display_mode
>   drm/vc4: hdmi: Remove unused argument in vc4_hdmi_supports_scrambling
>   drm/vc4: hdmi: Remove mutex in detect
>   drm/vc4: hdmi: Remove HDMI flag from encoder
>   drm/vc4: hdmi: Simplify the hotplug handling
>   drm/vc4: hdmi: Simplify the connector state retrieval
>   drm/vc4: hdmi: Switch to detect_ctx
>   drm/vc4: hdmi: Leverage new SCDC atomic_check
>   drm/vc4: hdmi: Reset link on hotplug
> 
>  drivers/gpu/drm/drm_atomic_helper.c       | 109 +++++++++
>  drivers/gpu/drm/drm_atomic_state_helper.c |  58 +++++
>  drivers/gpu/drm/drm_scdc_helper.c         |  13 ++
>  drivers/gpu/drm/vc4/vc4_hdmi.c            | 257 ++++++++++------------
>  drivers/gpu/drm/vc4/vc4_hdmi.h            |  19 +-
>  include/drm/drm_atomic_helper.h           |   3 +
>  include/drm/drm_atomic_state_helper.h     |   3 +
>  include/drm/drm_connector.h               |  25 +++
>  include/drm/drm_modes.h                   |  20 ++
>  9 files changed, 353 insertions(+), 154 deletions(-)
> 
> -- 
> 2.33.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 00/13] drm: Add generic helpers for HDMI scrambling

[Maxime Ripard]

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

Hi Daniel,

On Fri, Nov 19, 2021 at 05:01:14PM +0100, Daniel Vetter wrote:
> On Thu, Nov 18, 2021 at 11:38:01AM +0100, Maxime Ripard wrote:
> > This is a follow-up of the work to support the interactions between the hotplug
> > and the scrambling support for vc4:
> > 
> > https://lore.kernel.org/dri-devel/20210507150515.257424-11-maxime@cerno.tech/
> > https://lore.kernel.org/dri-devel/20211025152903.1088803-10-maxime@cerno.tech/
> > 
> > Ville feedback was that the same discussion happened some time ago for i915,
> > and resulted in a function to do an full disable/enable cycle on reconnection
> > to avoid breaking the HDMI 2.0 spec.
> > 
> > This series improves the current scrambling support by adding generic helpers
> > for usual scrambling-related operations, and builds upon them to provide a
> > generic alternative to intel_hdmi_reset_link.
> 
> Out of curiosity, can we rebuild intel_hdmi_reset_link on top of these?
> Always better to have two drivers to actually show the helpers help, than
> just one.

Unfortunately, I don't have any Intel system I can test it on, and it
looks like the changes wouldn't be trivial.

Maybe we can use dw-hdmi instead?

Maxime

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

Re: [PATCH v2 00/13] drm: Add generic helpers for HDMI scrambling

[Daniel Vetter]

On Fri, Nov 26, 2021 at 04:43:49PM +0100, Maxime Ripard wrote:
> Hi Daniel,
> 
> On Fri, Nov 19, 2021 at 05:01:14PM +0100, Daniel Vetter wrote:
> > On Thu, Nov 18, 2021 at 11:38:01AM +0100, Maxime Ripard wrote:
> > > This is a follow-up of the work to support the interactions between the hotplug
> > > and the scrambling support for vc4:
> > > 
> > > https://lore.kernel.org/dri-devel/20210507150515.257424-11-maxime@cerno.tech/
> > > https://lore.kernel.org/dri-devel/20211025152903.1088803-10-maxime@cerno.tech/
> > > 
> > > Ville feedback was that the same discussion happened some time ago for i915,
> > > and resulted in a function to do an full disable/enable cycle on reconnection
> > > to avoid breaking the HDMI 2.0 spec.
> > > 
> > > This series improves the current scrambling support by adding generic helpers
> > > for usual scrambling-related operations, and builds upon them to provide a
> > > generic alternative to intel_hdmi_reset_link.
> > 
> > Out of curiosity, can we rebuild intel_hdmi_reset_link on top of these?
> > Always better to have two drivers to actually show the helpers help, than
> > just one.
> 
> Unfortunately, I don't have any Intel system I can test it on, and it
> looks like the changes wouldn't be trivial.
> 
> Maybe we can use dw-hdmi instead?

Hm I guess so, maybe Ville can be motivated. Just figured since this seems
at least inspired by i915 code.

Also we have CI running on intel-gfx, so if you just type well enough it
generally works out and CI catches anything you got wrong. Christian König
is pretty good at not breaking i915 with all the dma-buf and ttm changes
nowadays, much better than random arm socs dying.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 00/13] drm: Add generic helpers for HDMI scrambling

[Maxime Ripard]

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

On Fri, Nov 26, 2021 at 06:12:42PM +0100, Daniel Vetter wrote:
> On Fri, Nov 26, 2021 at 04:43:49PM +0100, Maxime Ripard wrote:
> > Hi Daniel,
> > 
> > On Fri, Nov 19, 2021 at 05:01:14PM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 18, 2021 at 11:38:01AM +0100, Maxime Ripard wrote:
> > > > This is a follow-up of the work to support the interactions between the hotplug
> > > > and the scrambling support for vc4:
> > > > 
> > > > https://lore.kernel.org/dri-devel/20210507150515.257424-11-maxime@cerno.tech/
> > > > https://lore.kernel.org/dri-devel/20211025152903.1088803-10-maxime@cerno.tech/
> > > > 
> > > > Ville feedback was that the same discussion happened some time ago for i915,
> > > > and resulted in a function to do an full disable/enable cycle on reconnection
> > > > to avoid breaking the HDMI 2.0 spec.
> > > > 
> > > > This series improves the current scrambling support by adding generic helpers
> > > > for usual scrambling-related operations, and builds upon them to provide a
> > > > generic alternative to intel_hdmi_reset_link.
> > > 
> > > Out of curiosity, can we rebuild intel_hdmi_reset_link on top of these?
> > > Always better to have two drivers to actually show the helpers help, than
> > > just one.
> > 
> > Unfortunately, I don't have any Intel system I can test it on, and it
> > looks like the changes wouldn't be trivial.
> > 
> > Maybe we can use dw-hdmi instead?
> 
> Hm I guess so, maybe Ville can be motivated. Just figured since this seems
> at least inspired by i915 code.

It's not really the conversion to the new helper that I suspect would be
hard to do, but rather the usage of the connector state that we do, and
the scrambling computation, and how it's all tied together in i915.

It seems to be fairly different from drivers that just rely on the
atomic helpers, and I'm not really confident about changing that. But
yeah, I guess I can give it a try and rely on the CI.

Maxime

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

Re: [PATCH v2 00/13] drm: Add generic helpers for HDMI scrambling

[Daniel Vetter]

On Mon, Nov 29, 2021 at 11:07:41AM +0100, Maxime Ripard wrote:
> On Fri, Nov 26, 2021 at 06:12:42PM +0100, Daniel Vetter wrote:
> > On Fri, Nov 26, 2021 at 04:43:49PM +0100, Maxime Ripard wrote:
> > > Hi Daniel,
> > > 
> > > On Fri, Nov 19, 2021 at 05:01:14PM +0100, Daniel Vetter wrote:
> > > > On Thu, Nov 18, 2021 at 11:38:01AM +0100, Maxime Ripard wrote:
> > > > > This is a follow-up of the work to support the interactions between the hotplug
> > > > > and the scrambling support for vc4:
> > > > > 
> > > > > https://lore.kernel.org/dri-devel/20210507150515.257424-11-maxime@cerno.tech/
> > > > > https://lore.kernel.org/dri-devel/20211025152903.1088803-10-maxime@cerno.tech/
> > > > > 
> > > > > Ville feedback was that the same discussion happened some time ago for i915,
> > > > > and resulted in a function to do an full disable/enable cycle on reconnection
> > > > > to avoid breaking the HDMI 2.0 spec.
> > > > > 
> > > > > This series improves the current scrambling support by adding generic helpers
> > > > > for usual scrambling-related operations, and builds upon them to provide a
> > > > > generic alternative to intel_hdmi_reset_link.
> > > > 
> > > > Out of curiosity, can we rebuild intel_hdmi_reset_link on top of these?
> > > > Always better to have two drivers to actually show the helpers help, than
> > > > just one.
> > > 
> > > Unfortunately, I don't have any Intel system I can test it on, and it
> > > looks like the changes wouldn't be trivial.
> > > 
> > > Maybe we can use dw-hdmi instead?
> > 
> > Hm I guess so, maybe Ville can be motivated. Just figured since this seems
> > at least inspired by i915 code.
> 
> It's not really the conversion to the new helper that I suspect would be
> hard to do, but rather the usage of the connector state that we do, and
> the scrambling computation, and how it's all tied together in i915.
> 
> It seems to be fairly different from drivers that just rely on the
> atomic helpers, and I'm not really confident about changing that. But
> yeah, I guess I can give it a try and rely on the CI.

Hm yeah if wiring through connector state to the right places is real work
then skip the conversion. But I thought i915 modeset code has also gone
through some of the state rollout that you've done for atomic helpers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 00/13] drm: Add generic helpers for HDMI scrambling

[Maxime Ripard]

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

On Tue, Nov 30, 2021 at 09:29:09AM +0100, Daniel Vetter wrote:
> On Mon, Nov 29, 2021 at 11:07:41AM +0100, Maxime Ripard wrote:
> > On Fri, Nov 26, 2021 at 06:12:42PM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 26, 2021 at 04:43:49PM +0100, Maxime Ripard wrote:
> > > > Hi Daniel,
> > > > 
> > > > On Fri, Nov 19, 2021 at 05:01:14PM +0100, Daniel Vetter wrote:
> > > > > On Thu, Nov 18, 2021 at 11:38:01AM +0100, Maxime Ripard wrote:
> > > > > > This is a follow-up of the work to support the interactions between the hotplug
> > > > > > and the scrambling support for vc4:
> > > > > > 
> > > > > > https://lore.kernel.org/dri-devel/20210507150515.257424-11-maxime@cerno.tech/
> > > > > > https://lore.kernel.org/dri-devel/20211025152903.1088803-10-maxime@cerno.tech/
> > > > > > 
> > > > > > Ville feedback was that the same discussion happened some time ago for i915,
> > > > > > and resulted in a function to do an full disable/enable cycle on reconnection
> > > > > > to avoid breaking the HDMI 2.0 spec.
> > > > > > 
> > > > > > This series improves the current scrambling support by adding generic helpers
> > > > > > for usual scrambling-related operations, and builds upon them to provide a
> > > > > > generic alternative to intel_hdmi_reset_link.
> > > > > 
> > > > > Out of curiosity, can we rebuild intel_hdmi_reset_link on top of these?
> > > > > Always better to have two drivers to actually show the helpers help, than
> > > > > just one.
> > > > 
> > > > Unfortunately, I don't have any Intel system I can test it on, and it
> > > > looks like the changes wouldn't be trivial.
> > > > 
> > > > Maybe we can use dw-hdmi instead?
> > > 
> > > Hm I guess so, maybe Ville can be motivated. Just figured since this seems
> > > at least inspired by i915 code.
> > 
> > It's not really the conversion to the new helper that I suspect would be
> > hard to do, but rather the usage of the connector state that we do, and
> > the scrambling computation, and how it's all tied together in i915.
> > 
> > It seems to be fairly different from drivers that just rely on the
> > atomic helpers, and I'm not really confident about changing that. But
> > yeah, I guess I can give it a try and rely on the CI.
> 
> Hm yeah if wiring through connector state to the right places is real work
> then skip the conversion. But I thought i915 modeset code has also gone
> through some of the state rollout that you've done for atomic helpers.

Maybe I'm just panicking without any particular reason, I'll give it a
try and will let you know

Maxime

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

Re: [PATCH v2 00/13] drm: Add generic helpers for HDMI scrambling

[Maxime Ripard]

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

On Tue, Nov 30, 2021 at 09:29:09AM +0100, Daniel Vetter wrote:
> On Mon, Nov 29, 2021 at 11:07:41AM +0100, Maxime Ripard wrote:
> > On Fri, Nov 26, 2021 at 06:12:42PM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 26, 2021 at 04:43:49PM +0100, Maxime Ripard wrote:
> > > > Hi Daniel,
> > > > 
> > > > On Fri, Nov 19, 2021 at 05:01:14PM +0100, Daniel Vetter wrote:
> > > > > On Thu, Nov 18, 2021 at 11:38:01AM +0100, Maxime Ripard wrote:
> > > > > > This is a follow-up of the work to support the interactions between the hotplug
> > > > > > and the scrambling support for vc4:
> > > > > > 
> > > > > > https://lore.kernel.org/dri-devel/20210507150515.257424-11-maxime@cerno.tech/
> > > > > > https://lore.kernel.org/dri-devel/20211025152903.1088803-10-maxime@cerno.tech/
> > > > > > 
> > > > > > Ville feedback was that the same discussion happened some time ago for i915,
> > > > > > and resulted in a function to do an full disable/enable cycle on reconnection
> > > > > > to avoid breaking the HDMI 2.0 spec.
> > > > > > 
> > > > > > This series improves the current scrambling support by adding generic helpers
> > > > > > for usual scrambling-related operations, and builds upon them to provide a
> > > > > > generic alternative to intel_hdmi_reset_link.
> > > > > 
> > > > > Out of curiosity, can we rebuild intel_hdmi_reset_link on top of these?
> > > > > Always better to have two drivers to actually show the helpers help, than
> > > > > just one.
> > > > 
> > > > Unfortunately, I don't have any Intel system I can test it on, and it
> > > > looks like the changes wouldn't be trivial.
> > > > 
> > > > Maybe we can use dw-hdmi instead?
> > > 
> > > Hm I guess so, maybe Ville can be motivated. Just figured since this seems
> > > at least inspired by i915 code.
> > 
> > It's not really the conversion to the new helper that I suspect would be
> > hard to do, but rather the usage of the connector state that we do, and
> > the scrambling computation, and how it's all tied together in i915.
> > 
> > It seems to be fairly different from drivers that just rely on the
> > atomic helpers, and I'm not really confident about changing that. But
> > yeah, I guess I can give it a try and rely on the CI.
> 
> Hm yeah if wiring through connector state to the right places is real work
> then skip the conversion. But I thought i915 modeset code has also gone
> through some of the state rollout that you've done for atomic helpers.

I kind of fell into a rabbit hole with this.

I looked at other drivers and which one I could convert too. Tegra was
easy enough, but dw-hdmi was more interesting.

dw-hdmi also has YUV output, and the output format also impacts the TMDS
rate and thus whether the scrambler needs to be enabled or not.

In parallel, I also worked on the vc4 HDMI YUV output, trying to mimic
what i915 is doing.

However, the requirements around which YUV output and bpc we support are
a bit non-trivial, and I think it should be part of some helpers to
avoid as much as possible someone getting it wrong.

For reference, i915 is starting with the highest bpc count it can, and
tries either RGB or YUV420 for that bpc count. If any succeeds, we stop
there, otherwise we repeat with a lower bit count.

The iteration over the available bpc and formats should be easy enough,
we would just need to have a bit field with supported bpc values and
formats and be done with it.

The part that would tell if the sink supports the formats and bpc can be
generic as well since it's part of the EDID and we parse it already.

However, one of the limitation we have to take into account is also the
maximum TMDS rate the controller can achieve, and thus if we could
output the TMDS clock for a bpc, format and mode triplet.

So, if we were to sum up, we'd need:

  - One bitfield for the formats supported by the connector (redundant
    with ycbcr420_allowed?)
  - One bitfield for the bpc supported by the connector
  - One field to store the maximum TMDS clock the connector can achieve

Then, in drm_connector_state, we'd have in addition to what we have so
far the format to output for a given commit.

Or maybe it's not worth the trouble and I should just stop trying to
make this a helper?

Maxime

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