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

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

Maxime Ripard (13):
  drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate
  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: 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/bridge/synopsys/dw-hdmi.c  |   4 +-
 drivers/gpu/drm/drm_atomic_helper.c        | 109 ++++++++++
 drivers/gpu/drm/drm_atomic_state_helper.c  |  58 ++++++
 drivers/gpu/drm/drm_edid.c                 |   2 +-
 drivers/gpu/drm/drm_scdc_helper.c          |  13 ++
 drivers/gpu/drm/i915/display/intel_hdmi.c  |   2 +-
 drivers/gpu/drm/meson/meson_dw_hdmi.c      |   4 +-
 drivers/gpu/drm/radeon/radeon_encoders.c   |   2 +-
 drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c |   2 +-
 drivers/gpu/drm/tegra/sor.c                |  10 +-
 drivers/gpu/drm/vc4/vc4_hdmi.c             | 232 +++++++++------------
 drivers/gpu/drm/vc4/vc4_hdmi.h             |  17 +-
 include/drm/drm_atomic_helper.h            |   3 +
 include/drm/drm_atomic_state_helper.h      |   3 +
 include/drm/drm_connector.h                |  27 +++
 include/drm/drm_modes.h                    |  15 ++
 16 files changed, 342 insertions(+), 161 deletions(-)

-- 
2.32.0

[PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate

[Maxime Ripard]

A lot of drivers open-code the HDMI 1.4 maximum pixel rate in their
driver to test whether the resolutions are supported or if the
scrambling needs to be enabled.

Let's create a common define for everyone to use it.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: Emma Anholt <emma@anholt.net>
Cc: intel-gfx@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: linux-amlogic@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-tegra@vger.kernel.org
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c  | 4 ++--
 drivers/gpu/drm/drm_edid.c                 | 2 +-
 drivers/gpu/drm/i915/display/intel_hdmi.c  | 2 +-
 drivers/gpu/drm/meson/meson_dw_hdmi.c      | 4 ++--
 drivers/gpu/drm/radeon/radeon_encoders.c   | 2 +-
 drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c | 2 +-
 drivers/gpu/drm/tegra/sor.c                | 8 ++++----
 drivers/gpu/drm/vc4/vc4_hdmi.c             | 4 ++--
 include/drm/drm_connector.h                | 2 ++
 9 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 62ae63565d3a..3a58db357be0 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -46,7 +46,7 @@
 /* DW-HDMI Controller >= 0x200a are at least compliant with SCDC version 1 */
 #define SCDC_MIN_SOURCE_VERSION	0x1
 
-#define HDMI14_MAX_TMDSCLK	340000000
+#define HDMI14_MAX_TMDSCLK	(DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)
 
 enum hdmi_datamap {
 	RGB444_8B = 0x01,
@@ -1264,7 +1264,7 @@ static bool dw_hdmi_support_scdc(struct dw_hdmi *hdmi,
 	 * for low rates is not supported either
 	 */
 	if (!display->hdmi.scdc.scrambling.low_rates &&
-	    display->max_tmds_clock <= 340000)
+	    display->max_tmds_clock <= DRM_HDMI_14_MAX_TMDS_CLK_KHZ)
 		return false;
 
 	return true;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7aa2a56a71c8..ec8fb2d098ae 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
 		u32 max_tmds_clock = hf_vsdb[5] * 5000;
 		struct drm_scdc *scdc = &hdmi->scdc;
 
-		if (max_tmds_clock > 340000) {
+		if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
 			display->max_tmds_clock = max_tmds_clock;
 			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
 				display->max_tmds_clock);
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index d2e61f6c6e08..0666203d52b7 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
 		if (scdc->scrambling.low_rates)
 			pipe_config->hdmi_scrambling = true;
 
-		if (pipe_config->port_clock > 340000) {
+		if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
 			pipe_config->hdmi_scrambling = true;
 			pipe_config->hdmi_high_tmds_clock_ratio = true;
 		}
diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 0afbd1e70bfc..8078667aea0e 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -434,7 +434,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
 		readl_relaxed(priv->io_base + _REG(VPU_HDMI_SETTING));
 
 	DRM_DEBUG_DRIVER("\"%s\" div%d\n", mode->name,
-			 mode->clock > 340000 ? 40 : 10);
+			 mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ ? 40 : 10);
 
 	/* Enable clocks */
 	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL, 0xffff, 0x100);
@@ -457,7 +457,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
 	dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_BIST_CNTL, BIT(12));
 
 	/* TMDS pattern setup */
-	if (mode->clock > 340000 &&
+	if (mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ &&
 	    dw_hdmi->output_bus_fmt == MEDIA_BUS_FMT_YUV8_1X24) {
 		dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01,
 				  0);
diff --git a/drivers/gpu/drm/radeon/radeon_encoders.c b/drivers/gpu/drm/radeon/radeon_encoders.c
index 46549d5179ee..ddd8100e699f 100644
--- a/drivers/gpu/drm/radeon/radeon_encoders.c
+++ b/drivers/gpu/drm/radeon/radeon_encoders.c
@@ -384,7 +384,7 @@ bool radeon_dig_monitor_is_duallink(struct drm_encoder *encoder,
 		if (radeon_connector->use_digital) {
 			/* HDMI 1.3 supports up to 340 Mhz over single link */
 			if (ASIC_IS_DCE6(rdev) && drm_detect_hdmi_monitor(radeon_connector_edid(connector))) {
-				if (pixel_clock > 340000)
+				if (pixel_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ)
 					return true;
 				else
 					return false;
diff --git a/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c b/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c
index d25ecd4f4b67..bc213232a875 100644
--- a/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c
+++ b/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c
@@ -102,7 +102,7 @@ static bool sti_hdmi_tx3g4c28phy_start(struct sti_hdmi *hdmi)
 	tmdsck = ckpxpll;
 	pllctrl |= 40 << PLL_CFG_NDIV_SHIFT;
 
-	if (tmdsck > 340000000) {
+	if (tmdsck > (DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)) {
 		DRM_ERROR("output TMDS clock (%d) out of range\n", tmdsck);
 		goto err;
 	}
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index 0ea320c1092b..99a2d627bfeb 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -1814,7 +1814,7 @@ tegra_sor_encoder_atomic_check(struct drm_encoder *encoder,
 	 * For HBR2 modes, the SOR brick needs to use the x20 multiplier, so
 	 * the pixel clock must be corrected accordingly.
 	 */
-	if (pclk >= 340000000) {
+	if (pclk >= (DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)) {
 		state->link_speed = 20;
 		state->pclk = pclk / 2;
 	} else {
@@ -2196,7 +2196,7 @@ static void tegra_sor_hdmi_scdc_start(struct tegra_sor *sor)
 
 	mode = &sor->output.encoder.crtc->state->adjusted_mode;
 
-	if (mode->clock >= 340000 && scdc->supported) {
+	if (mode->clock >= DRM_HDMI_14_MAX_TMDS_CLK_KHZ && scdc->supported) {
 		schedule_delayed_work(&sor->scdc, msecs_to_jiffies(5000));
 		tegra_sor_hdmi_scdc_enable(sor);
 		sor->scdc_enabled = true;
@@ -2340,7 +2340,7 @@ static void tegra_sor_hdmi_enable(struct drm_encoder *encoder)
 	value &= ~SOR_CLK_CNTRL_DP_LINK_SPEED_MASK;
 	value &= ~SOR_CLK_CNTRL_DP_CLK_SEL_MASK;
 
-	if (mode->clock < 340000) {
+	if (mode->clock < DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
 		DRM_DEBUG_KMS("setting 2.7 GHz link speed\n");
 		value |= SOR_CLK_CNTRL_DP_LINK_SPEED_G2_70;
 	} else {
@@ -2423,7 +2423,7 @@ static void tegra_sor_hdmi_enable(struct drm_encoder *encoder)
 	/* adjust clock rate for HDMI 2.0 modes */
 	rate = clk_get_rate(sor->clk_parent);
 
-	if (mode->clock >= 340000)
+	if (mode->clock >= DRM_HDMI_14_MAX_TMDS_CLK_KHZ)
 		rate /= 2;
 
 	DRM_DEBUG_KMS("setting clock to %lu Hz, mode: %lu Hz\n", rate, pclk);
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index fab9b93e1b84..fc7247cc1022 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -97,11 +97,11 @@
 #define HSM_MIN_CLOCK_FREQ	120000000
 #define CEC_CLOCK_FREQ 40000
 
-#define HDMI_14_MAX_TMDS_CLK   (340 * 1000 * 1000)
+#define HDMI_14_MAX_TMDS_CLK	(DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)
 
 static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode)
 {
-	return (mode->clock * 1000) > HDMI_14_MAX_TMDS_CLK;
+	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
 }
 
 static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index b501d0badaea..030636635af1 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -260,6 +260,8 @@ struct drm_hdmi_info {
 	struct drm_hdmi_dsc_cap dsc_cap;
 };
 
+#define DRM_HDMI_14_MAX_TMDS_CLK_KHZ	(340 * 1000)
+
 /**
  * enum drm_link_status - connector's link_status property value
  *
-- 
2.32.0

Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate

[Alex Deucher]

On Tue, Nov 2, 2021 at 10:59 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> A lot of drivers open-code the HDMI 1.4 maximum pixel rate in their
> driver to test whether the resolutions are supported or if the
> scrambling needs to be enabled.
>
> Let's create a common define for everyone to use it.
>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: Emma Anholt <emma@anholt.net>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: linux-amlogic@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-tegra@vger.kernel.org
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c  | 4 ++--
>  drivers/gpu/drm/drm_edid.c                 | 2 +-
>  drivers/gpu/drm/i915/display/intel_hdmi.c  | 2 +-
>  drivers/gpu/drm/meson/meson_dw_hdmi.c      | 4 ++--
>  drivers/gpu/drm/radeon/radeon_encoders.c   | 2 +-

For radeon:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

Note that there are several instances of this in amdgpu as well:
drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c:                if
(pixel_clock > 340000)
drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c:                if
(pixel_clock > 340000)
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c:            if
(mode->clock > 340000)
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c:                if
(mode->clock > 340000)

Alex

>  drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c | 2 +-
>  drivers/gpu/drm/tegra/sor.c                | 8 ++++----
>  drivers/gpu/drm/vc4/vc4_hdmi.c             | 4 ++--
>  include/drm/drm_connector.h                | 2 ++
>  9 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 62ae63565d3a..3a58db357be0 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -46,7 +46,7 @@
>  /* DW-HDMI Controller >= 0x200a are at least compliant with SCDC version 1 */
>  #define SCDC_MIN_SOURCE_VERSION        0x1
>
> -#define HDMI14_MAX_TMDSCLK     340000000
> +#define HDMI14_MAX_TMDSCLK     (DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)
>
>  enum hdmi_datamap {
>         RGB444_8B = 0x01,
> @@ -1264,7 +1264,7 @@ static bool dw_hdmi_support_scdc(struct dw_hdmi *hdmi,
>          * for low rates is not supported either
>          */
>         if (!display->hdmi.scdc.scrambling.low_rates &&
> -           display->max_tmds_clock <= 340000)
> +           display->max_tmds_clock <= DRM_HDMI_14_MAX_TMDS_CLK_KHZ)
>                 return false;
>
>         return true;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7aa2a56a71c8..ec8fb2d098ae 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
>                 u32 max_tmds_clock = hf_vsdb[5] * 5000;
>                 struct drm_scdc *scdc = &hdmi->scdc;
>
> -               if (max_tmds_clock > 340000) {
> +               if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>                         display->max_tmds_clock = max_tmds_clock;
>                         DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>                                 display->max_tmds_clock);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index d2e61f6c6e08..0666203d52b7 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
>                 if (scdc->scrambling.low_rates)
>                         pipe_config->hdmi_scrambling = true;
>
> -               if (pipe_config->port_clock > 340000) {
> +               if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>                         pipe_config->hdmi_scrambling = true;
>                         pipe_config->hdmi_high_tmds_clock_ratio = true;
>                 }
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 0afbd1e70bfc..8078667aea0e 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -434,7 +434,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>                 readl_relaxed(priv->io_base + _REG(VPU_HDMI_SETTING));
>
>         DRM_DEBUG_DRIVER("\"%s\" div%d\n", mode->name,
> -                        mode->clock > 340000 ? 40 : 10);
> +                        mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ ? 40 : 10);
>
>         /* Enable clocks */
>         regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL, 0xffff, 0x100);
> @@ -457,7 +457,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>         dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_BIST_CNTL, BIT(12));
>
>         /* TMDS pattern setup */
> -       if (mode->clock > 340000 &&
> +       if (mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ &&
>             dw_hdmi->output_bus_fmt == MEDIA_BUS_FMT_YUV8_1X24) {
>                 dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01,
>                                   0);
> diff --git a/drivers/gpu/drm/radeon/radeon_encoders.c b/drivers/gpu/drm/radeon/radeon_encoders.c
> index 46549d5179ee..ddd8100e699f 100644
> --- a/drivers/gpu/drm/radeon/radeon_encoders.c
> +++ b/drivers/gpu/drm/radeon/radeon_encoders.c
> @@ -384,7 +384,7 @@ bool radeon_dig_monitor_is_duallink(struct drm_encoder *encoder,
>                 if (radeon_connector->use_digital) {
>                         /* HDMI 1.3 supports up to 340 Mhz over single link */
>                         if (ASIC_IS_DCE6(rdev) && drm_detect_hdmi_monitor(radeon_connector_edid(connector))) {
> -                               if (pixel_clock > 340000)
> +                               if (pixel_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ)
>                                         return true;
>                                 else
>                                         return false;
> diff --git a/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c b/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c
> index d25ecd4f4b67..bc213232a875 100644
> --- a/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c
> +++ b/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c
> @@ -102,7 +102,7 @@ static bool sti_hdmi_tx3g4c28phy_start(struct sti_hdmi *hdmi)
>         tmdsck = ckpxpll;
>         pllctrl |= 40 << PLL_CFG_NDIV_SHIFT;
>
> -       if (tmdsck > 340000000) {
> +       if (tmdsck > (DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)) {
>                 DRM_ERROR("output TMDS clock (%d) out of range\n", tmdsck);
>                 goto err;
>         }
> diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
> index 0ea320c1092b..99a2d627bfeb 100644
> --- a/drivers/gpu/drm/tegra/sor.c
> +++ b/drivers/gpu/drm/tegra/sor.c
> @@ -1814,7 +1814,7 @@ tegra_sor_encoder_atomic_check(struct drm_encoder *encoder,
>          * For HBR2 modes, the SOR brick needs to use the x20 multiplier, so
>          * the pixel clock must be corrected accordingly.
>          */
> -       if (pclk >= 340000000) {
> +       if (pclk >= (DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)) {
>                 state->link_speed = 20;
>                 state->pclk = pclk / 2;
>         } else {
> @@ -2196,7 +2196,7 @@ static void tegra_sor_hdmi_scdc_start(struct tegra_sor *sor)
>
>         mode = &sor->output.encoder.crtc->state->adjusted_mode;
>
> -       if (mode->clock >= 340000 && scdc->supported) {
> +       if (mode->clock >= DRM_HDMI_14_MAX_TMDS_CLK_KHZ && scdc->supported) {
>                 schedule_delayed_work(&sor->scdc, msecs_to_jiffies(5000));
>                 tegra_sor_hdmi_scdc_enable(sor);
>                 sor->scdc_enabled = true;
> @@ -2340,7 +2340,7 @@ static void tegra_sor_hdmi_enable(struct drm_encoder *encoder)
>         value &= ~SOR_CLK_CNTRL_DP_LINK_SPEED_MASK;
>         value &= ~SOR_CLK_CNTRL_DP_CLK_SEL_MASK;
>
> -       if (mode->clock < 340000) {
> +       if (mode->clock < DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>                 DRM_DEBUG_KMS("setting 2.7 GHz link speed\n");
>                 value |= SOR_CLK_CNTRL_DP_LINK_SPEED_G2_70;
>         } else {
> @@ -2423,7 +2423,7 @@ static void tegra_sor_hdmi_enable(struct drm_encoder *encoder)
>         /* adjust clock rate for HDMI 2.0 modes */
>         rate = clk_get_rate(sor->clk_parent);
>
> -       if (mode->clock >= 340000)
> +       if (mode->clock >= DRM_HDMI_14_MAX_TMDS_CLK_KHZ)
>                 rate /= 2;
>
>         DRM_DEBUG_KMS("setting clock to %lu Hz, mode: %lu Hz\n", rate, pclk);
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index fab9b93e1b84..fc7247cc1022 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -97,11 +97,11 @@
>  #define HSM_MIN_CLOCK_FREQ     120000000
>  #define CEC_CLOCK_FREQ 40000
>
> -#define HDMI_14_MAX_TMDS_CLK   (340 * 1000 * 1000)
> +#define HDMI_14_MAX_TMDS_CLK   (DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)
>
>  static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode)
>  {
> -       return (mode->clock * 1000) > HDMI_14_MAX_TMDS_CLK;
> +       return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
>  }
>
>  static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index b501d0badaea..030636635af1 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -260,6 +260,8 @@ struct drm_hdmi_info {
>         struct drm_hdmi_dsc_cap dsc_cap;
>  };
>
> +#define DRM_HDMI_14_MAX_TMDS_CLK_KHZ   (340 * 1000)
> +
>  /**
>   * enum drm_link_status - connector's link_status property value
>   *
> --
> 2.32.0
>

Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate

[Neil Armstrong]

On 02/11/2021 15:59, Maxime Ripard wrote:
> A lot of drivers open-code the HDMI 1.4 maximum pixel rate in their
> driver to test whether the resolutions are supported or if the
> scrambling needs to be enabled.
> 
> Let's create a common define for everyone to use it.
> 
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: Emma Anholt <emma@anholt.net>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: linux-amlogic@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-tegra@vger.kernel.org
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c  | 4 ++--
>  drivers/gpu/drm/drm_edid.c                 | 2 +-
>  drivers/gpu/drm/i915/display/intel_hdmi.c  | 2 +-
>  drivers/gpu/drm/meson/meson_dw_hdmi.c      | 4 ++--
>  drivers/gpu/drm/radeon/radeon_encoders.c   | 2 +-
>  drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c | 2 +-
>  drivers/gpu/drm/tegra/sor.c                | 8 ++++----
>  drivers/gpu/drm/vc4/vc4_hdmi.c             | 4 ++--
>  include/drm/drm_connector.h                | 2 ++
>  9 files changed, 16 insertions(+), 14 deletions(-)

For meson & bridge/synopsys/dw-hdmi:

Acked-by: Neil Armstrong <narmstrong@baylibre.com>

> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 62ae63565d3a..3a58db357be0 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -46,7 +46,7 @@
>  /* DW-HDMI Controller >= 0x200a are at least compliant with SCDC version 1 */
>  #define SCDC_MIN_SOURCE_VERSION	0x1
>  
> -#define HDMI14_MAX_TMDSCLK	340000000
> +#define HDMI14_MAX_TMDSCLK	(DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)
>  
>  enum hdmi_datamap {
>  	RGB444_8B = 0x01,
> @@ -1264,7 +1264,7 @@ static bool dw_hdmi_support_scdc(struct dw_hdmi *hdmi,
>  	 * for low rates is not supported either
>  	 */
>  	if (!display->hdmi.scdc.scrambling.low_rates &&
> -	    display->max_tmds_clock <= 340000)
> +	    display->max_tmds_clock <= DRM_HDMI_14_MAX_TMDS_CLK_KHZ)
>  		return false;
>  
>  	return true;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7aa2a56a71c8..ec8fb2d098ae 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
>  		u32 max_tmds_clock = hf_vsdb[5] * 5000;
>  		struct drm_scdc *scdc = &hdmi->scdc;
>  
> -		if (max_tmds_clock > 340000) {
> +		if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>  			display->max_tmds_clock = max_tmds_clock;
>  			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>  				display->max_tmds_clock);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index d2e61f6c6e08..0666203d52b7 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
>  		if (scdc->scrambling.low_rates)
>  			pipe_config->hdmi_scrambling = true;
>  
> -		if (pipe_config->port_clock > 340000) {
> +		if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>  			pipe_config->hdmi_scrambling = true;
>  			pipe_config->hdmi_high_tmds_clock_ratio = true;
>  		}
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 0afbd1e70bfc..8078667aea0e 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -434,7 +434,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>  		readl_relaxed(priv->io_base + _REG(VPU_HDMI_SETTING));
>  
>  	DRM_DEBUG_DRIVER("\"%s\" div%d\n", mode->name,
> -			 mode->clock > 340000 ? 40 : 10);
> +			 mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ ? 40 : 10);
>  
>  	/* Enable clocks */
>  	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL, 0xffff, 0x100);
> @@ -457,7 +457,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>  	dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_BIST_CNTL, BIT(12));
>  
>  	/* TMDS pattern setup */
> -	if (mode->clock > 340000 &&
> +	if (mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ &&
>  	    dw_hdmi->output_bus_fmt == MEDIA_BUS_FMT_YUV8_1X24) {
>  		dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01,
>  				  0);
> diff --git a/drivers/gpu/drm/radeon/radeon_encoders.c b/drivers/gpu/drm/radeon/radeon_encoders.c
> index 46549d5179ee..ddd8100e699f 100644
> --- a/drivers/gpu/drm/radeon/radeon_encoders.c
> +++ b/drivers/gpu/drm/radeon/radeon_encoders.c
> @@ -384,7 +384,7 @@ bool radeon_dig_monitor_is_duallink(struct drm_encoder *encoder,
>  		if (radeon_connector->use_digital) {
>  			/* HDMI 1.3 supports up to 340 Mhz over single link */
>  			if (ASIC_IS_DCE6(rdev) && drm_detect_hdmi_monitor(radeon_connector_edid(connector))) {
> -				if (pixel_clock > 340000)
> +				if (pixel_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ)
>  					return true;
>  				else
>  					return false;
> diff --git a/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c b/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c
> index d25ecd4f4b67..bc213232a875 100644
> --- a/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c
> +++ b/drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c
> @@ -102,7 +102,7 @@ static bool sti_hdmi_tx3g4c28phy_start(struct sti_hdmi *hdmi)
>  	tmdsck = ckpxpll;
>  	pllctrl |= 40 << PLL_CFG_NDIV_SHIFT;
>  
> -	if (tmdsck > 340000000) {
> +	if (tmdsck > (DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)) {
>  		DRM_ERROR("output TMDS clock (%d) out of range\n", tmdsck);
>  		goto err;
>  	}
> diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
> index 0ea320c1092b..99a2d627bfeb 100644
> --- a/drivers/gpu/drm/tegra/sor.c
> +++ b/drivers/gpu/drm/tegra/sor.c
> @@ -1814,7 +1814,7 @@ tegra_sor_encoder_atomic_check(struct drm_encoder *encoder,
>  	 * For HBR2 modes, the SOR brick needs to use the x20 multiplier, so
>  	 * the pixel clock must be corrected accordingly.
>  	 */
> -	if (pclk >= 340000000) {
> +	if (pclk >= (DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)) {
>  		state->link_speed = 20;
>  		state->pclk = pclk / 2;
>  	} else {
> @@ -2196,7 +2196,7 @@ static void tegra_sor_hdmi_scdc_start(struct tegra_sor *sor)
>  
>  	mode = &sor->output.encoder.crtc->state->adjusted_mode;
>  
> -	if (mode->clock >= 340000 && scdc->supported) {
> +	if (mode->clock >= DRM_HDMI_14_MAX_TMDS_CLK_KHZ && scdc->supported) {
>  		schedule_delayed_work(&sor->scdc, msecs_to_jiffies(5000));
>  		tegra_sor_hdmi_scdc_enable(sor);
>  		sor->scdc_enabled = true;
> @@ -2340,7 +2340,7 @@ static void tegra_sor_hdmi_enable(struct drm_encoder *encoder)
>  	value &= ~SOR_CLK_CNTRL_DP_LINK_SPEED_MASK;
>  	value &= ~SOR_CLK_CNTRL_DP_CLK_SEL_MASK;
>  
> -	if (mode->clock < 340000) {
> +	if (mode->clock < DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>  		DRM_DEBUG_KMS("setting 2.7 GHz link speed\n");
>  		value |= SOR_CLK_CNTRL_DP_LINK_SPEED_G2_70;
>  	} else {
> @@ -2423,7 +2423,7 @@ static void tegra_sor_hdmi_enable(struct drm_encoder *encoder)
>  	/* adjust clock rate for HDMI 2.0 modes */
>  	rate = clk_get_rate(sor->clk_parent);
>  
> -	if (mode->clock >= 340000)
> +	if (mode->clock >= DRM_HDMI_14_MAX_TMDS_CLK_KHZ)
>  		rate /= 2;
>  
>  	DRM_DEBUG_KMS("setting clock to %lu Hz, mode: %lu Hz\n", rate, pclk);
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index fab9b93e1b84..fc7247cc1022 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -97,11 +97,11 @@
>  #define HSM_MIN_CLOCK_FREQ	120000000
>  #define CEC_CLOCK_FREQ 40000
>  
> -#define HDMI_14_MAX_TMDS_CLK   (340 * 1000 * 1000)
> +#define HDMI_14_MAX_TMDS_CLK	(DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)
>  
>  static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode)
>  {
> -	return (mode->clock * 1000) > HDMI_14_MAX_TMDS_CLK;
> +	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
>  }
>  
>  static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index b501d0badaea..030636635af1 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -260,6 +260,8 @@ struct drm_hdmi_info {
>  	struct drm_hdmi_dsc_cap dsc_cap;
>  };
>  
> +#define DRM_HDMI_14_MAX_TMDS_CLK_KHZ	(340 * 1000)
> +
>  /**
>   * enum drm_link_status - connector's link_status property value
>   *
> 

Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate

[Ville Syrjälä]

On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
>  		u32 max_tmds_clock = hf_vsdb[5] * 5000;
>  		struct drm_scdc *scdc = &hdmi->scdc;
>  
> -		if (max_tmds_clock > 340000) {
> +		if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>  			display->max_tmds_clock = max_tmds_clock;
>  			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>  				display->max_tmds_clock);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index d2e61f6c6e08..0666203d52b7 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
>  		if (scdc->scrambling.low_rates)
>  			pipe_config->hdmi_scrambling = true;
>  
> -		if (pipe_config->port_clock > 340000) {
> +		if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>  			pipe_config->hdmi_scrambling = true;
>  			pipe_config->hdmi_high_tmds_clock_ratio = true;
>  		}

All of that is HDMI 2.0 stuff. So this just makes it all super
confusing IMO. Nak.

-- 
Ville Syrjälä
Intel

Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate

[Ville Syrjälä]

On Wed, Nov 03, 2021 at 01:02:11PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
> >  		u32 max_tmds_clock = hf_vsdb[5] * 5000;
> >  		struct drm_scdc *scdc = &hdmi->scdc;
> >  
> > -		if (max_tmds_clock > 340000) {
> > +		if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> >  			display->max_tmds_clock = max_tmds_clock;
> >  			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> >  				display->max_tmds_clock);
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > index d2e61f6c6e08..0666203d52b7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
> >  		if (scdc->scrambling.low_rates)
> >  			pipe_config->hdmi_scrambling = true;
> >  
> > -		if (pipe_config->port_clock > 340000) {
> > +		if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> >  			pipe_config->hdmi_scrambling = true;
> >  			pipe_config->hdmi_high_tmds_clock_ratio = true;
> >  		}
> 
> All of that is HDMI 2.0 stuff. So this just makes it all super
> confusing IMO. Nak.

So reading throgh HDMI 1.4 again it does specify 340 MHz as some kind
of upper limit for the physical cable. But nowhere else is that number
really mentioned AFAICS. HDMI 2.0 does talk quite a bit about the 340
Mcsc limit in various places.

I wonder what people would think of a couple of helpers like:
- drm_hdmi_{can,must}_use_scrambling()
- drm_hdmi_is_high_tmds_clock_ratio()
or something along those lines? At least with those the code would
read decently and I wouldn't have to wonder what this HDMI 1.4 TMDS
clock limit really is.

-- 
Ville Syrjälä
Intel

Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate

[Maxime Ripard]

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

Hi Ville,

On Wed, Nov 03, 2021 at 08:05:16PM +0200, Ville Syrjälä wrote:
> On Wed, Nov 03, 2021 at 01:02:11PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
> > >  		u32 max_tmds_clock = hf_vsdb[5] * 5000;
> > >  		struct drm_scdc *scdc = &hdmi->scdc;
> > >  
> > > -		if (max_tmds_clock > 340000) {
> > > +		if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > >  			display->max_tmds_clock = max_tmds_clock;
> > >  			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> > >  				display->max_tmds_clock);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > index d2e61f6c6e08..0666203d52b7 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
> > >  		if (scdc->scrambling.low_rates)
> > >  			pipe_config->hdmi_scrambling = true;
> > >  
> > > -		if (pipe_config->port_clock > 340000) {
> > > +		if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > >  			pipe_config->hdmi_scrambling = true;
> > >  			pipe_config->hdmi_high_tmds_clock_ratio = true;
> > >  		}
> > 
> > All of that is HDMI 2.0 stuff. So this just makes it all super
> > confusing IMO. Nak.
> 
> So reading throgh HDMI 1.4 again it does specify 340 MHz as some kind
> of upper limit for the physical cable. But nowhere else is that number
> really mentioned AFAICS. HDMI 2.0 does talk quite a bit about the 340
> Mcsc limit in various places.
> 
> I wonder what people would think of a couple of helpers like:
> - drm_hdmi_{can,must}_use_scrambling()
> - drm_hdmi_is_high_tmds_clock_ratio()
> or something along those lines? At least with those the code would
> read decently and I wouldn't have to wonder what this HDMI 1.4 TMDS
> clock limit really is.

Patch 2 introduces something along those lines.

It doesn't cover everything though, we're using this define in vc4 to
limit the available modes in mode_valid on HDMI controllers not
4k-capable

We could probably do better on the name, but I still believe a define
like this would be valuable.

Maxime

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

Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate

[Ville Syrjälä]

On Thu, Nov 04, 2021 at 09:48:41AM +0100, Maxime Ripard wrote:
> Hi Ville,
> 
> On Wed, Nov 03, 2021 at 08:05:16PM +0200, Ville Syrjälä wrote:
> > On Wed, Nov 03, 2021 at 01:02:11PM +0200, Ville Syrjälä wrote:
> > > On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
> > > >  		u32 max_tmds_clock = hf_vsdb[5] * 5000;
> > > >  		struct drm_scdc *scdc = &hdmi->scdc;
> > > >  
> > > > -		if (max_tmds_clock > 340000) {
> > > > +		if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > > >  			display->max_tmds_clock = max_tmds_clock;
> > > >  			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> > > >  				display->max_tmds_clock);
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > > index d2e61f6c6e08..0666203d52b7 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > > @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
> > > >  		if (scdc->scrambling.low_rates)
> > > >  			pipe_config->hdmi_scrambling = true;
> > > >  
> > > > -		if (pipe_config->port_clock > 340000) {
> > > > +		if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > > >  			pipe_config->hdmi_scrambling = true;
> > > >  			pipe_config->hdmi_high_tmds_clock_ratio = true;
> > > >  		}
> > > 
> > > All of that is HDMI 2.0 stuff. So this just makes it all super
> > > confusing IMO. Nak.
> > 
> > So reading throgh HDMI 1.4 again it does specify 340 MHz as some kind
> > of upper limit for the physical cable. But nowhere else is that number
> > really mentioned AFAICS. HDMI 2.0 does talk quite a bit about the 340
> > Mcsc limit in various places.
> > 
> > I wonder what people would think of a couple of helpers like:
> > - drm_hdmi_{can,must}_use_scrambling()
> > - drm_hdmi_is_high_tmds_clock_ratio()
> > or something along those lines? At least with those the code would
> > read decently and I wouldn't have to wonder what this HDMI 1.4 TMDS
> > clock limit really is.
> 
> Patch 2 introduces something along those lines.
> 
> It doesn't cover everything though, we're using this define in vc4 to
> limit the available modes in mode_valid on HDMI controllers not
> 4k-capable

I wouldn't want to use this kind of define for those kinds of checks
anyway. If the hardware has specific limits in what kind of clocks it
can generate (or what it was validated for) IMO you should spell
those out explicitly instead of assuming they happen to match
some standard defined max value.

-- 
Ville Syrjälä
Intel

Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate

[Maxime Ripard]

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

On Thu, Nov 04, 2021 at 05:41:13PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 04, 2021 at 09:48:41AM +0100, Maxime Ripard wrote:
> > Hi Ville,
> > 
> > On Wed, Nov 03, 2021 at 08:05:16PM +0200, Ville Syrjälä wrote:
> > > On Wed, Nov 03, 2021 at 01:02:11PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
> > > > >  		u32 max_tmds_clock = hf_vsdb[5] * 5000;
> > > > >  		struct drm_scdc *scdc = &hdmi->scdc;
> > > > >  
> > > > > -		if (max_tmds_clock > 340000) {
> > > > > +		if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > > > >  			display->max_tmds_clock = max_tmds_clock;
> > > > >  			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> > > > >  				display->max_tmds_clock);
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > > > index d2e61f6c6e08..0666203d52b7 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > > > @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
> > > > >  		if (scdc->scrambling.low_rates)
> > > > >  			pipe_config->hdmi_scrambling = true;
> > > > >  
> > > > > -		if (pipe_config->port_clock > 340000) {
> > > > > +		if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > > > >  			pipe_config->hdmi_scrambling = true;
> > > > >  			pipe_config->hdmi_high_tmds_clock_ratio = true;
> > > > >  		}
> > > > 
> > > > All of that is HDMI 2.0 stuff. So this just makes it all super
> > > > confusing IMO. Nak.
> > > 
> > > So reading throgh HDMI 1.4 again it does specify 340 MHz as some kind
> > > of upper limit for the physical cable. But nowhere else is that number
> > > really mentioned AFAICS. HDMI 2.0 does talk quite a bit about the 340
> > > Mcsc limit in various places.
> > > 
> > > I wonder what people would think of a couple of helpers like:
> > > - drm_hdmi_{can,must}_use_scrambling()
> > > - drm_hdmi_is_high_tmds_clock_ratio()
> > > or something along those lines? At least with those the code would
> > > read decently and I wouldn't have to wonder what this HDMI 1.4 TMDS
> > > clock limit really is.
> > 
> > Patch 2 introduces something along those lines.
> > 
> > It doesn't cover everything though, we're using this define in vc4 to
> > limit the available modes in mode_valid on HDMI controllers not
> > 4k-capable
> 
> I wouldn't want to use this kind of define for those kinds of checks
> anyway. If the hardware has specific limits in what kind of clocks it
> can generate (or what it was validated for) IMO you should spell
> those out explicitly instead of assuming they happen to match
> some standard defined max value.

AFAIK, in the vc4 case, this is the hardware limit.

And there's other cases where it still seems to make sense to have that
define:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_edid.c#n4978
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/radeon/radeon_encoders.c#n385
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c#n1174

etc..

Maxime

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

[PATCH 02/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.

Cc: Emma Anholt <emma@anholt.net>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
Cc: linux-tegra@vger.kernel.org
Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/tegra/sor.c    |  4 ++--
 drivers/gpu/drm/vc4/vc4_hdmi.c | 21 +++++++++------------
 include/drm/drm_modes.h        | 15 +++++++++++++++
 3 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index 99a2d627bfeb..390bd04b0d22 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -2192,11 +2192,11 @@ static void tegra_sor_hdmi_scdc_work(struct work_struct *work)
 static void tegra_sor_hdmi_scdc_start(struct tegra_sor *sor)
 {
 	struct drm_scdc *scdc = &sor->output.connector.display_info.hdmi.scdc;
-	struct drm_display_mode *mode;
+	const struct drm_display_mode *mode;
 
 	mode = &sor->output.encoder.crtc->state->adjusted_mode;
 
-	if (mode->clock >= DRM_HDMI_14_MAX_TMDS_CLK_KHZ && scdc->supported) {
+	if (drm_mode_hdmi_requires_scrambling(mode) && scdc->supported) {
 		schedule_delayed_work(&sor->scdc, msecs_to_jiffies(5000));
 		tegra_sor_hdmi_scdc_enable(sor);
 		sor->scdc_enabled = true;
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index fc7247cc1022..fa76ae2c94e4 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	(DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)
 
-static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode)
-{
-	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
-}
-
 static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
 {
 	struct drm_info_node *node = (struct drm_info_node *)m->private;
@@ -260,10 +255,10 @@ 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)) {
+			if (drm_mode_hdmi_requires_scrambling(mode)) {
 				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.");
 			}
@@ -574,7 +569,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 +592,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);
@@ -605,7 +600,7 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder)
 	if (!vc4_hdmi_supports_scrambling(encoder, mode))
 		return;
 
-	if (!vc4_hdmi_mode_needs_scrambling(mode))
+	if (!drm_mode_hdmi_requires_scrambling(mode))
 		return;
 
 	drm_scdc_set_high_tmds_clock_ratio(vc4_hdmi->ddc, true);
@@ -1283,7 +1278,8 @@ 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 &&
+	    drm_mode_hdmi_requires_scrambling(mode))
 		return -EINVAL;
 
 	vc4_state->pixel_rate = pixel_rate;
@@ -1305,7 +1301,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))
 		return MODE_CLOCK_HIGH;
 
 	return MODE_OK;
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 29ba4adf0c53..d22816d55836 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -424,6 +424,21 @@ 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
+ *
+ * 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)
+{
+	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
+}
+
 struct drm_connector;
 struct drm_cmdline_mode;
 
-- 
2.32.0

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

[Ville Syrjälä]

On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -424,6 +424,21 @@ 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
> + *
> + * 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)
> +{
> +	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> +}

That's only correct for 8bpc. The actual limit is on the TMDS clock (or
rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
magic for the actually transmitted TMDS clock).

-- 
Ville Syrjälä
Intel

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

[Daniel Vetter]

On Thu, Nov 04, 2021 at 05:37:21PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> > --- a/include/drm/drm_modes.h
> > +++ b/include/drm/drm_modes.h
> > @@ -424,6 +424,21 @@ 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
> > + *
> > + * 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)
> > +{
> > +	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> > +}
> 
> That's only correct for 8bpc. The actual limit is on the TMDS clock (or
> rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
> magic for the actually transmitted TMDS clock).

Yeah we might need to add the bus format for the cable here too, to make
this complete.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Ville Syrjälä]

On Fri, Nov 05, 2021 at 07:02:30PM +0100, Daniel Vetter wrote:
> On Thu, Nov 04, 2021 at 05:37:21PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> > > --- a/include/drm/drm_modes.h
> > > +++ b/include/drm/drm_modes.h
> > > @@ -424,6 +424,21 @@ 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
> > > + *
> > > + * 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)
> > > +{
> > > +	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> > > +}
> > 
> > That's only correct for 8bpc. The actual limit is on the TMDS clock (or
> > rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
> > magic for the actually transmitted TMDS clock).
> 
> Yeah we might need to add the bus format for the cable here too, to make
> this complete.

I don't think we have a usable thing for that on the drm level, so
would need to invent something. Oh, and the mode->clock vs. 
mode->crtc_clock funny business also limits the usability of this
approach. So probably just easiest to pass in the the driver calculated
TMDS clock instead.

-- 
Ville Syrjälä
Intel

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

[Maxime Ripard]

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

On Fri, Nov 05, 2021 at 08:14:04PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 05, 2021 at 07:02:30PM +0100, Daniel Vetter wrote:
> > On Thu, Nov 04, 2021 at 05:37:21PM +0200, Ville Syrjälä wrote:
> > > On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> > > > --- a/include/drm/drm_modes.h
> > > > +++ b/include/drm/drm_modes.h
> > > > @@ -424,6 +424,21 @@ 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
> > > > + *
> > > > + * 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)
> > > > +{
> > > > +	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> > > > +}
> > > 
> > > That's only correct for 8bpc. The actual limit is on the TMDS clock (or
> > > rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
> > > magic for the actually transmitted TMDS clock).
> > 
> > Yeah we might need to add the bus format for the cable here too, to make
> > this complete.
> 
> I don't think we have a usable thing for that on the drm level, so
> would need to invent something. Oh, and the mode->clock vs. 
> mode->crtc_clock funny business also limits the usability of this
> approach. So probably just easiest to pass in the the driver calculated
> TMDS clock instead.

If we look at all (I think?) the existing users of scrambling in KMS as
of 5.15, only i915 seems to use crtc_clock (which, in retrospect, seems
to be the right thing to do), and only i915 and dw-hdmi use an output
format, i915 rolling its own, and dw-hdmi using the mbus formats.

I think using the mbus format here makes the most sense: i915 already is
rolling a whole bunch of other code, and we use the mbus defines for the
bridge format enumeration as well which is probably going to have some
interaction.

I'm not quite sure what to do next though. The whole point of that
series is to streamline as much as possible the scrambling and TMDS
ratio setup to avoid the duplication we already have in the drivers that
support it, every one using the mostly-the-same-but-slightly-different
logic to configure it.

The mode is fortunately stored in generic structures so it's easy to
make that decision. Having to take into account the output format
however makes it mandatory to move the bus format in the
drm_connector_state(?) structure too.

It's already in the bridge_state though, so should we take the final
bridge format as the cable format if it's tied to a bridge?

Maxime

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

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

[Daniel Vetter]

On Mon, Nov 08, 2021 at 04:58:34PM +0100, Maxime Ripard wrote:
> On Fri, Nov 05, 2021 at 08:14:04PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 05, 2021 at 07:02:30PM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 04, 2021 at 05:37:21PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> > > > > --- a/include/drm/drm_modes.h
> > > > > +++ b/include/drm/drm_modes.h
> > > > > @@ -424,6 +424,21 @@ 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
> > > > > + *
> > > > > + * 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)
> > > > > +{
> > > > > +	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> > > > > +}
> > > > 
> > > > That's only correct for 8bpc. The actual limit is on the TMDS clock (or
> > > > rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
> > > > magic for the actually transmitted TMDS clock).
> > > 
> > > Yeah we might need to add the bus format for the cable here too, to make
> > > this complete.
> > 
> > I don't think we have a usable thing for that on the drm level, so
> > would need to invent something. Oh, and the mode->clock vs. 
> > mode->crtc_clock funny business also limits the usability of this
> > approach. So probably just easiest to pass in the the driver calculated
> > TMDS clock instead.
> 
> If we look at all (I think?) the existing users of scrambling in KMS as
> of 5.15, only i915 seems to use crtc_clock (which, in retrospect, seems
> to be the right thing to do), and only i915 and dw-hdmi use an output
> format, i915 rolling its own, and dw-hdmi using the mbus formats.
> 
> I think using the mbus format here makes the most sense: i915 already is
> rolling a whole bunch of other code, and we use the mbus defines for the
> bridge format enumeration as well which is probably going to have some
> interaction.
> 
> I'm not quite sure what to do next though. The whole point of that
> series is to streamline as much as possible the scrambling and TMDS
> ratio setup to avoid the duplication we already have in the drivers that
> support it, every one using the mostly-the-same-but-slightly-different
> logic to configure it.
> 
> The mode is fortunately stored in generic structures so it's easy to
> make that decision. Having to take into account the output format
> however makes it mandatory to move the bus format in the
> drm_connector_state(?) structure too.
> 
> It's already in the bridge_state though, so should we take the final
> bridge format as the cable format if it's tied to a bridge?

Maybe as a default, it nothing is set. Also if nothing is set in the
connector then just assume 8bpc rgb, and drivers can be fixed as we go.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Ville Syrjälä]

On Mon, Nov 08, 2021 at 04:58:34PM +0100, Maxime Ripard wrote:
> On Fri, Nov 05, 2021 at 08:14:04PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 05, 2021 at 07:02:30PM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 04, 2021 at 05:37:21PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> > > > > --- a/include/drm/drm_modes.h
> > > > > +++ b/include/drm/drm_modes.h
> > > > > @@ -424,6 +424,21 @@ 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
> > > > > + *
> > > > > + * 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)
> > > > > +{
> > > > > +	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> > > > > +}
> > > > 
> > > > That's only correct for 8bpc. The actual limit is on the TMDS clock (or
> > > > rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
> > > > magic for the actually transmitted TMDS clock).
> > > 
> > > Yeah we might need to add the bus format for the cable here too, to make
> > > this complete.
> > 
> > I don't think we have a usable thing for that on the drm level, so
> > would need to invent something. Oh, and the mode->clock vs. 
> > mode->crtc_clock funny business also limits the usability of this
> > approach. So probably just easiest to pass in the the driver calculated
> > TMDS clock instead.
> 
> If we look at all (I think?) the existing users of scrambling in KMS as
> of 5.15, only i915 seems to use crtc_clock (which, in retrospect, seems
> to be the right thing to do), and only i915 and dw-hdmi use an output
> format, i915 rolling its own, and dw-hdmi using the mbus formats.
> 
> I think using the mbus format here makes the most sense: i915 already is
> rolling a whole bunch of other code, and we use the mbus defines for the
> bridge format enumeration as well which is probably going to have some
> interaction.
> 
> I'm not quite sure what to do next though. The whole point of that
> series is to streamline as much as possible the scrambling and TMDS
> ratio setup to avoid the duplication we already have in the drivers that
> support it, every one using the mostly-the-same-but-slightly-different
> logic to configure it.
> 
> The mode is fortunately stored in generic structures so it's easy to
> make that decision. Having to take into account the output format
> however makes it mandatory to move the bus format in the
> drm_connector_state(?) structure too.

I think involving state objects and the like is just going to make
it harder to share code between all drivers, if that is the goal.
Just a few tiny helpers I think is what would allow the broadest 
reuse. I guess you could build additional midlayer on top of those
for some drivers if you wish.

As for the bus_format stuff, that looks at the same time overkill,
and insufficiently documented. I guess its main purpose is to exactly
defines how some digtal bus works, which makes sense when you're
chaining a bunch of random chips together. But looks overly complicated
to me for defining what to output from a HDMI encoder. Looking at the
defines I wouldn't even know what to use for HDMI actually. All we
really want is RGB 4:4:4 vs. YCbCr 4:4:4 vs. YCbCr 4:2:2 vs. YCbCr 4:2:0.
Beyond that level of detail we don't care how each bit gets transferred
etc. Hence enum intel_output_format in i915.

-- 
Ville Syrjälä
Intel

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

[Maxime Ripard]

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

On Mon, Nov 08, 2021 at 07:55:00PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 08, 2021 at 04:58:34PM +0100, Maxime Ripard wrote:
> > On Fri, Nov 05, 2021 at 08:14:04PM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 05, 2021 at 07:02:30PM +0100, Daniel Vetter wrote:
> > > > On Thu, Nov 04, 2021 at 05:37:21PM +0200, Ville Syrjälä wrote:
> > > > > On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> > > > > > --- a/include/drm/drm_modes.h
> > > > > > +++ b/include/drm/drm_modes.h
> > > > > > @@ -424,6 +424,21 @@ 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
> > > > > > + *
> > > > > > + * 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)
> > > > > > +{
> > > > > > +	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> > > > > > +}
> > > > > 
> > > > > That's only correct for 8bpc. The actual limit is on the TMDS clock (or
> > > > > rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
> > > > > magic for the actually transmitted TMDS clock).
> > > > 
> > > > Yeah we might need to add the bus format for the cable here too, to make
> > > > this complete.
> > > 
> > > I don't think we have a usable thing for that on the drm level, so
> > > would need to invent something. Oh, and the mode->clock vs. 
> > > mode->crtc_clock funny business also limits the usability of this
> > > approach. So probably just easiest to pass in the the driver calculated
> > > TMDS clock instead.
> > 
> > If we look at all (I think?) the existing users of scrambling in KMS as
> > of 5.15, only i915 seems to use crtc_clock (which, in retrospect, seems
> > to be the right thing to do), and only i915 and dw-hdmi use an output
> > format, i915 rolling its own, and dw-hdmi using the mbus formats.
> > 
> > I think using the mbus format here makes the most sense: i915 already is
> > rolling a whole bunch of other code, and we use the mbus defines for the
> > bridge format enumeration as well which is probably going to have some
> > interaction.
> > 
> > I'm not quite sure what to do next though. The whole point of that
> > series is to streamline as much as possible the scrambling and TMDS
> > ratio setup to avoid the duplication we already have in the drivers that
> > support it, every one using the mostly-the-same-but-slightly-different
> > logic to configure it.
> > 
> > The mode is fortunately stored in generic structures so it's easy to
> > make that decision. Having to take into account the output format
> > however makes it mandatory to move the bus format in the
> > drm_connector_state(?) structure too.
> 
> I think involving state objects and the like is just going to make
> it harder to share code between all drivers, if that is the goal.
> Just a few tiny helpers I think is what would allow the broadest 
> reuse. I guess you could build additional midlayer on top of those
> for some drivers if you wish.
> 
> As for the bus_format stuff, that looks at the same time overkill,
> and insufficiently documented. I guess its main purpose is to exactly
> defines how some digtal bus works, which makes sense when you're
> chaining a bunch of random chips together. But looks overly complicated
> to me for defining what to output from a HDMI encoder. Looking at the
> defines I wouldn't even know what to use for HDMI actually. All we
> really want is RGB 4:4:4 vs. YCbCr 4:4:4 vs. YCbCr 4:2:2 vs. YCbCr 4:2:0.
> Beyond that level of detail we don't care how each bit gets transferred
> etc. Hence enum intel_output_format in i915.

I have the same feeling about the mbus formats.

I tried to start a discussion about this some time back, without much
success:
https://lore.kernel.org/all/20210317154352.732095-1-maxime@cerno.tech/

The main issue for our current series is that it's tied to the bridges,
while it should work for any HDMI connector, backed by a bridge or not.

However, the main point of this series is to streamline as much as
possible the scrambling setup, including dealing with hotplug properly
just like i915 is doing.

A flag in the connector state to enable the scrambling and high tmds
ratio allows for the helper to perform the disable/enable cycle when we
detected the hotplug. If we wouldn't have it, it wouldn't know what to
do in the first place, and we would end up disabling/enabling the
display every time (which isn't great).

This also allows for less duplication since everyone is using a variant
of the same algorithm to do so, without proper consideration for corner
cases (like enabling scrambling for displays that supports it for rates
< 340MHz)

So we really need something generic there. Or maybe an intermediate
hdmi_connector_state?

Maxime

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

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

[Maxime Ripard]

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

On Mon, Nov 08, 2021 at 07:55:00PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 08, 2021 at 04:58:34PM +0100, Maxime Ripard wrote:
> > On Fri, Nov 05, 2021 at 08:14:04PM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 05, 2021 at 07:02:30PM +0100, Daniel Vetter wrote:
> > > > On Thu, Nov 04, 2021 at 05:37:21PM +0200, Ville Syrjälä wrote:
> > > > > On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> > > > > > --- a/include/drm/drm_modes.h
> > > > > > +++ b/include/drm/drm_modes.h
> > > > > > @@ -424,6 +424,21 @@ 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
> > > > > > + *
> > > > > > + * 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)
> > > > > > +{
> > > > > > +	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> > > > > > +}
> > > > > 
> > > > > That's only correct for 8bpc. The actual limit is on the TMDS clock (or
> > > > > rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
> > > > > magic for the actually transmitted TMDS clock).
> > > > 
> > > > Yeah we might need to add the bus format for the cable here too, to make
> > > > this complete.
> > > 
> > > I don't think we have a usable thing for that on the drm level, so
> > > would need to invent something. Oh, and the mode->clock vs. 
> > > mode->crtc_clock funny business also limits the usability of this
> > > approach. So probably just easiest to pass in the the driver calculated
> > > TMDS clock instead.
> > 
> > If we look at all (I think?) the existing users of scrambling in KMS as
> > of 5.15, only i915 seems to use crtc_clock (which, in retrospect, seems
> > to be the right thing to do), and only i915 and dw-hdmi use an output
> > format, i915 rolling its own, and dw-hdmi using the mbus formats.
> > 
> > I think using the mbus format here makes the most sense: i915 already is
> > rolling a whole bunch of other code, and we use the mbus defines for the
> > bridge format enumeration as well which is probably going to have some
> > interaction.
> > 
> > I'm not quite sure what to do next though. The whole point of that
> > series is to streamline as much as possible the scrambling and TMDS
> > ratio setup to avoid the duplication we already have in the drivers that
> > support it, every one using the mostly-the-same-but-slightly-different
> > logic to configure it.
> > 
> > The mode is fortunately stored in generic structures so it's easy to
> > make that decision. Having to take into account the output format
> > however makes it mandatory to move the bus format in the
> > drm_connector_state(?) structure too.
> 
> I think involving state objects and the like is just going to make
> it harder to share code between all drivers, if that is the goal.
> Just a few tiny helpers I think is what would allow the broadest 
> reuse. I guess you could build additional midlayer on top of those
> for some drivers if you wish.
> 
> As for the bus_format stuff, that looks at the same time overkill,
> and insufficiently documented. I guess its main purpose is to exactly
> defines how some digtal bus works, which makes sense when you're
> chaining a bunch of random chips together. But looks overly complicated
> to me for defining what to output from a HDMI encoder. Looking at the
> defines I wouldn't even know what to use for HDMI actually. All we
> really want is RGB 4:4:4 vs. YCbCr 4:4:4 vs. YCbCr 4:2:2 vs. YCbCr 4:2:0.
> Beyond that level of detail we don't care how each bit gets transferred
> etc. Hence enum intel_output_format in i915.

I was thinking about this some more, can we leverage struct
hdmi_colorspace for this? Chances are it's already used by any driver
that supports YCbCr output to setup the infoframes.

Maxime

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

[PATCH 03/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..93f40f2975c3 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);
+	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 030636635af1..78d3d6c78fcb 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -832,6 +832,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.32.0

[PATCH 04/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.32.0

[PATCH 05/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.32.0

[PATCH 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 fa76ae2c94e4..49bcb0342cc9 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -568,8 +568,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);
@@ -597,7 +596,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 (!drm_mode_hdmi_requires_scrambling(mode))
-- 
2.32.0

[PATCH 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 49bcb0342cc9..826ca7aaf8d7 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -183,7 +183,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));
 
@@ -215,13 +224,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;
 }
 
@@ -238,14 +245,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);
 
@@ -265,9 +279,6 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 		}
 	}
 
-out:
-	mutex_unlock(&vc4_hdmi->mutex);
-
 	return ret;
 }
 
@@ -1996,21 +2007,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);
@@ -2045,6 +2047,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;
 }
 
@@ -2053,16 +2057,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);
 
@@ -2074,6 +2069,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;
@@ -2092,22 +2089,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;
 }
@@ -2121,22 +2109,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)
@@ -2157,6 +2136,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 36c0b082a43b..32b0aa447717 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.32.0

[PATCH 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 826ca7aaf8d7..288c2bfbf88b 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -217,7 +217,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);
 			}
 		}
@@ -241,7 +240,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;
 
@@ -261,8 +259,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);
@@ -581,13 +577,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 ||
@@ -1120,11 +1115,12 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder,
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
 	struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
+	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);
@@ -1149,7 +1145,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_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
-	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
+	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;
@@ -1170,7 +1166,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);
@@ -1197,7 +1193,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 32b0aa447717..460112d68948 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.32.0

[PATCH 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 | 50 ++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 288c2bfbf88b..4f2f138f93e3 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -177,11 +177,35 @@ 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;
+
+	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
@@ -198,7 +222,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;
@@ -208,27 +232,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.32.0

[PATCH 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 4f2f138f93e3..2d7c34b306c9 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -996,30 +996,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);
 	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.32.0

[PATCH 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 2d7c34b306c9..82878718e5fc 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -201,8 +201,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;
@@ -353,7 +354,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,
@@ -362,6 +362,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.32.0

[PATCH 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 | 32 ++++++++++----------------------
 drivers/gpu/drm/vc4/vc4_hdmi.h |  6 ++++++
 2 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 82878718e5fc..aa6700622797 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -297,6 +297,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;
@@ -586,37 +594,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 (!drm_mode_hdmi_requires_scrambling(mode))
+	if (!vc4_hdmi->scdc_needed)
 		return;
 
 	drm_scdc_set_high_tmds_clock_ratio(vc4_hdmi->ddc, true);
@@ -1228,6 +1215,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));
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 460112d68948..1aabc51ede03 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.32.0

[PATCH 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 | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index aa6700622797..a1f40548dd48 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;
@@ -198,7 +197,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,
@@ -236,7 +235,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.32.0