[PATCH v4 0/8] drm/vc4: Reset HDMI link at hotplug

[PATCH v4 0/8] drm/vc4: Reset HDMI link at hotplug

[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/
https://lore.kernel.org/dri-devel/20211118103814.524670-1-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.

While the previous versions of this series was moving the current scrambling
related functions into generic helpers to consolidate that logic, it proved to
be difficult to rework existing drivers to make use of it without hardware to
test it on and thus the code is (for now) private to vc4.

I still believe that long term, the code to decide if the scrambler needs to be
enabled or not should be moved into a generic helper.

This also means that we would need to move the format output decision to a
generic helper, which also makes sense to me but it probably going to be
controversial.

Let me know what you think,
Maxime

Changes from v3 (https://lore.kernel.org/dri-devel/20220714091252.2089015-1-maxime@cerno.tech/):
  - Rebased on drm-misc-next-2022-08-20-1

Changes from v2 (https://lore.kernel.org/dri-devel/20211118103814.524670-1-maxime@cerno.tech/):
  - Rebased on next-20220713
  - Dropped the generic helpers and put them into vc4

Changes from v1 (https://lore.kernel.org/dri-devel/20211102145944.259181-1-maxime@cerno.tech/):
  - 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 (8):
  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: Simplify the hotplug handling
  drm/vc4: hdmi: Switch to detect_ctx
  drm/vc4: hdmi: Move vc4_hdmi_supports_scrambling() around
  drm/vc4: hdmi: Reset link on hotplug
  drm/scdc: Document hotplug gotchas

 drivers/gpu/drm/display/drm_scdc_helper.c |  13 +
 drivers/gpu/drm/vc4/vc4_hdmi.c            | 308 ++++++++++++++--------
 drivers/gpu/drm/vc4/vc4_hdmi.h            |  12 +-
 3 files changed, 218 insertions(+), 115 deletions(-)

-- 
2.37.1

[PATCH v4 1/8] drm/vc4: hdmi: Constify drm_display_mode

[Maxime Ripard]

We don't modify the drm_display_mode pointer we have in the driver in
most places, so let's make them const.

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 84e5a91c2ea7..4a8c902dbbc8 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -335,7 +335,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, 8, VC4_HDMI_OUTPUT_RGB)) {
@@ -709,7 +709,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 *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
@@ -732,7 +732,7 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 	struct drm_device *drm = vc4_hdmi->connector.dev;
-	struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
+	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	unsigned long flags;
 	int idx;
 
@@ -1077,7 +1077,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
 
 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)
 {
 	struct drm_device *drm = vc4_hdmi->connector.dev;
 	bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
@@ -1141,7 +1141,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)
 {
 	struct drm_device *drm = vc4_hdmi->connector.dev;
 	const struct vc4_hdmi_connector_state *vc4_state =
@@ -1303,7 +1303,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 		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 drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
+	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	unsigned long tmds_char_rate = vc4_conn_state->tmds_char_rate;
 	unsigned long bvb_rate, hsm_rate;
 	unsigned long flags;
@@ -1416,7 +1416,7 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder,
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 	struct drm_device *drm = vc4_hdmi->connector.dev;
 	struct drm_connector *connector = &vc4_hdmi->connector;
-	struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
+	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	struct drm_connector_state *conn_state =
 		drm_atomic_get_new_connector_state(state, connector);
 	unsigned long flags;
@@ -1444,7 +1444,7 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder,
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 	struct drm_device *drm = vc4_hdmi->connector.dev;
-	struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
+	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	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;
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 99b0bc1297be..79495ef79f09 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -72,7 +72,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.37.1

[PATCH v4 2/8] 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 4a8c902dbbc8..faf52c20b95b 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -708,8 +708,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 *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
@@ -738,7 +737,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.37.1

[PATCH v4 3/8] 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 | 88 +++++++++++++---------------------
 drivers/gpu/drm/vc4/vc4_hdmi.h | 10 +---
 2 files changed, 34 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index faf52c20b95b..b786645eaeb7 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -279,7 +279,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));
 
@@ -304,13 +313,11 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 
 		vc4_hdmi_enable_scrambling(&vc4_hdmi->encoder.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;
 }
 
@@ -320,14 +327,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;
 
 	drm_connector_update_edid_property(connector, edid);
 	ret = drm_add_edid_modes(connector, edid);
@@ -345,9 +359,6 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 		}
 	}
 
-out:
-	mutex_unlock(&vc4_hdmi->mutex);
-
 	return ret;
 }
 
@@ -2663,17 +2674,6 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 	int ret;
 	int idx;
 
-	/*
-	 * 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 (!drm_dev_enter(drm, &idx))
 		/*
 		 * We can't return an error code, because the CEC
@@ -2688,6 +2688,8 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 		return ret;
 	}
 
+	mutex_lock(&vc4_hdmi->mutex);
+
 	spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
 
 	val = HDMI_READ(HDMI_CEC_CNTRL_5);
@@ -2722,6 +2724,7 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 
 	spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
 
+	mutex_unlock(&vc4_hdmi->mutex);
 	drm_dev_exit(idx);
 
 	return 0;
@@ -2742,16 +2745,7 @@ static int vc4_hdmi_cec_disable(struct cec_adapter *adap)
 		 */
 		return 0;
 
-	/*
-	 * 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);
 
@@ -2763,6 +2757,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);
 
 	drm_dev_exit(idx);
@@ -2785,17 +2781,6 @@ static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
 	unsigned long flags;
 	int idx;
 
-	/*
-	 * 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 (!drm_dev_enter(drm, &idx))
 		/*
 		 * We can't return an error code, because the CEC
@@ -2804,11 +2789,13 @@ static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
 		 */
 		return 0;
 
+	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);
 
 	drm_dev_exit(idx);
 
@@ -2825,17 +2812,6 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 	unsigned int i;
 	int idx;
 
-	/*
-	 * 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 (!drm_dev_enter(dev, &idx))
 		return -ENODEV;
 
@@ -2845,6 +2821,8 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 		return -ENOMEM;
 	}
 
+	mutex_lock(&vc4_hdmi->mutex);
+
 	spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
 
 	for (i = 0; i < msg->len; i += 4)
@@ -2864,7 +2842,7 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 	HDMI_WRITE(HDMI_CEC_CNTRL_1, val);
 
 	spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
-
+	mutex_unlock(&vc4_hdmi->mutex);
 	drm_dev_exit(idx);
 
 	return 0;
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 79495ef79f09..db823efb2563 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -195,15 +195,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.37.1

[PATCH v4 4/8] 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 b786645eaeb7..9fad57ebdd11 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -273,17 +273,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
@@ -294,31 +330,17 @@ 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 {
 		if (vc4_hdmi->variant->hp_detect &&
 		    vc4_hdmi->variant->hp_detect(vc4_hdmi))
-			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);
-		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 int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
-- 
2.37.1

[PATCH v4 5/8] 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 9fad57ebdd11..d385003d7f42 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -309,8 +309,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;
@@ -452,7 +453,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,
 	.reset = vc4_hdmi_connector_reset,
 	.atomic_duplicate_state = vc4_hdmi_connector_duplicate_state,
@@ -460,6 +460,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.37.1

[PATCH v4 6/8] drm/vc4: hdmi: Move vc4_hdmi_supports_scrambling() around

[Maxime Ripard]

We'll need it earlier in the driver, so let's move it next to the other
scrambling-related helpers.

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

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index d385003d7f42..a510da7462fd 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -124,6 +124,23 @@ static unsigned long long
 vc4_hdmi_encoder_compute_mode_clock(const struct drm_display_mode *mode,
 				    unsigned int bpc, enum vc4_hdmi_output_format fmt);
 
+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;
+}
+
 static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode,
 					   unsigned int bpc,
 					   enum vc4_hdmi_output_format fmt)
@@ -742,23 +759,6 @@ 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)
-- 
2.37.1

[PATCH v4 7/8] drm/vc4: hdmi: Reset link on hotplug

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

The solution we've had so far is 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 thus
breaks that expectation.

The solution used by i915 is 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.

As such, we took that code and plugged it into vc4. It probably could
have been turned into an helper, but it proved to be difficult for
several reasons:

  * i915 has fairly different structures than simpler KMS drivers such
    as vc4, so doing some code that works with both proved to be
    difficult;

  * Other simpler drivers could reuse some of it (tegra, dw-hdmi), but
    it would still require to move some parameters currently stored in
    private structure that are needed to compute whether the scrambling
    is needed or not, and then inform the driver that it needs to be
    enabled. Some of those parameters are already in core structures
    (drm_display_mode, drm_display_info, bpc), but the output format
    isnt't. Adding it is fairly challenging since unlike the TMDS char
    rate or mode, there's no consensus on what format to pick in
    drivers, so it's not possible to write some generic code that can
    depend on it.

For these reasons, we chose to duplicate the code for now, until someone
else really needs it as well, in which case we will be able to convert
it into a generic helper.

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

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index a510da7462fd..1b3131947a5b 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -288,9 +288,103 @@ 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 int reset_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;
+}
+
+static int vc4_hdmi_reset_link(struct drm_connector *connector,
+			       struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_device *drm = connector->dev;
+	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
+	struct drm_encoder *encoder = &vc4_hdmi->encoder.base;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	bool scrambling_needed;
+	u8 config;
+	int ret;
+
+	if (!connector)
+		return 0;
+
+	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 (!vc4_hdmi_supports_scrambling(encoder))
+		return 0;
+
+	scrambling_needed = vc4_hdmi_mode_needs_scrambling(&vc4_hdmi->saved_adjusted_mode,
+							   vc4_hdmi->output_bpc,
+							   vc4_hdmi->output_format);
+	if (!scrambling_needed)
+		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_SCRAMBLING_ENABLE) == scrambling_needed)
+		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 reset_pipe(crtc, ctx);
+}
 
 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;
@@ -303,6 +397,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.
@@ -323,7 +421,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);
+	vc4_hdmi_reset_link(connector, ctx);
 }
 
 static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector,
@@ -355,7 +453,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.37.1

[PATCH v4 8/8] 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/display/drm_scdc_helper.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
index 81881e81ceae..c3ad4ab2b456 100644
--- a/drivers/gpu/drm/display/drm_scdc_helper.c
+++ b/drivers/gpu/drm/display/drm_scdc_helper.c
@@ -35,6 +35,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 trigger an
+ * empty modeset in drm_connector_helper_funcs.detect_ctx(), like what vc4 does
+ * in vc4_hdmi_reset_link().
  */
 
 #define SCDC_I2C_SLAVE_ADDRESS 0x54
-- 
2.37.1

Re: [PATCH v4 4/8] drm/vc4: hdmi: Simplify the hotplug handling

[Ville Syrjälä]

On Mon, Aug 29, 2022 at 03:47:27PM +0200, Maxime Ripard wrote:
> 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 b786645eaeb7..9fad57ebdd11 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -273,17 +273,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;

This looks to have a change in behaviour to not call
vc4_hdmi_enable_scrambling() unless a change in the
connector status was detected. The previous code called
it regarless.

When I was doing the i915 stuff I did have a sink that
left hpd asserted while turned off, and when powering
back up it instead generated a pulse on the hpd line.
Not sure if such a pulse is always slow enough that
you can reasonably guarantee a detection of both edges...

Apart from that (and the cec locking mess that I dared
not even look at) the rest of the series looks OK to me.

> +
> +	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
> @@ -294,31 +330,17 @@ 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 {
>  		if (vc4_hdmi->variant->hp_detect &&
>  		    vc4_hdmi->variant->hp_detect(vc4_hdmi))
> -			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);
> -		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 int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
> -- 
> 2.37.1

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 4/8] drm/vc4: hdmi: Simplify the hotplug handling

[Maxime Ripard]

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

Hi Ville

Thanks for your review

On Mon, Sep 05, 2022 at 08:38:11PM +0300, Ville Syrjälä wrote:
> On Mon, Aug 29, 2022 at 03:47:27PM +0200, Maxime Ripard wrote:
> > 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 b786645eaeb7..9fad57ebdd11 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -273,17 +273,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;
> 
> This looks to have a change in behaviour to not call
> vc4_hdmi_enable_scrambling() unless a change in the
> connector status was detected. The previous code called
> it regarless.

Yeah, good point

> When I was doing the i915 stuff I did have a sink that
> left hpd asserted while turned off, and when powering
> back up it instead generated a pulse on the hpd line.
> Not sure if such a pulse is always slow enough that
> you can reasonably guarantee a detection of both edges...
> 
> Apart from that (and the cec locking mess that I dared
> not even look at) the rest of the series looks OK to me.

Can I add your R-B and remove the check you mentioned above while
applying, or would you prefer that I send a new version?

Thanks!
Maxime

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

Re: [PATCH v4 4/8] drm/vc4: hdmi: Simplify the hotplug handling

[Ville Syrjälä]

On Fri, Sep 09, 2022 at 04:36:44PM +0200, Maxime Ripard wrote:
> Hi Ville
> 
> Thanks for your review
> 
> On Mon, Sep 05, 2022 at 08:38:11PM +0300, Ville Syrjälä wrote:
> > On Mon, Aug 29, 2022 at 03:47:27PM +0200, Maxime Ripard wrote:
> > > 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 b786645eaeb7..9fad57ebdd11 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > @@ -273,17 +273,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;
> > 
> > This looks to have a change in behaviour to not call
> > vc4_hdmi_enable_scrambling() unless a change in the
> > connector status was detected. The previous code called
> > it regarless.
> 
> Yeah, good point
> 
> > When I was doing the i915 stuff I did have a sink that
> > left hpd asserted while turned off, and when powering
> > back up it instead generated a pulse on the hpd line.
> > Not sure if such a pulse is always slow enough that
> > you can reasonably guarantee a detection of both edges...
> > 
> > Apart from that (and the cec locking mess that I dared
> > not even look at) the rest of the series looks OK to me.
> 
> Can I add your R-B and remove the check you mentioned above while
> applying, or would you prefer that I send a new version?

Nah. Feel free slap on
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
to the lot if you don't think a resend is needed otherwise.

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 0/8] drm/vc4: Reset HDMI link at hotplug

[Maxime Ripard]

On Mon, 29 Aug 2022 15:47:23 +0200, 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/
> https://lore.kernel.org/dri-devel/20211118103814.524670-1-maxime@cerno.tech/
> 
> [...]

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

Thanks!
Maxime