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

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