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.tec...
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(-)
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 *
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
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
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.
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.
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
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.
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/driv... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
etc..
Maxime
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;
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).
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
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.
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
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
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.
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
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
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; };
/**
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_ */
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
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))
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;
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; };
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)
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;
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, };
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 *
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;
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Daniel Vetter -
Maxime Ripard -
Neil Armstrong -
Ville Syrjälä