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;

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

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,

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;

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)

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, };

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;