Re: [Intel-gfx] [PATCH 5/6] drm/i915: Add encoder->is_clock_enabled()

["Kahola, Mika" <mika.kahola@intel.com>]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Wednesday, February 24, 2021 4:42 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 5/6] drm/i915: Add encoder->is_clock_enabled()
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Support reading out the current state of the DDI clock.
> 
> Not sure we really want this. Seems a bit excessive just to restore the debug
> print to icl_sanitize_encoder_pll_mapping()?
> But maybe there's more use for it?
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I guess there is no harm done if we have the state of the DDI clock in store.

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> ---
>  drivers/gpu/drm/i915/display/icl_dsi.c        |  19 +++
>  drivers/gpu/drm/i915/display/intel_crt.c      |   1 +
>  drivers/gpu/drm/i915/display/intel_ddi.c      | 123 +++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_ddi.h      |   1 +
>  .../drm/i915/display/intel_display_types.h    |   4 +
>  5 files changed, 146 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> b/drivers/gpu/drm/i915/display/icl_dsi.c
> index 29fe4919392a..7f2abc088a66 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -655,6 +655,24 @@ static void gen11_dsi_ungate_clocks(struct
> intel_encoder *encoder)
>  	mutex_unlock(&dev_priv->dpll.lock);
>  }
> 
> +static bool gen11_dsi_is_clock_enabled(struct intel_encoder *encoder) {
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder);
> +	bool clock_enabled = false;
> +	enum phy phy;
> +	u32 tmp;
> +
> +	tmp = intel_de_read(dev_priv, ICL_DPCLKA_CFGCR0);
> +
> +	for_each_dsi_phy(phy, intel_dsi->phys) {
> +		if (!(tmp & ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy)))
> +			clock_enabled = true;
> +	}
> +
> +	return clock_enabled;
> +}
> +
>  static void gen11_dsi_map_pll(struct intel_encoder *encoder,
>  			      const struct intel_crtc_state *crtc_state)  { @@ -
> 1939,6 +1957,7 @@ void icl_dsi_init(struct drm_i915_private *dev_priv)
>  	encoder->power_domain = POWER_DOMAIN_PORT_DSI;
>  	encoder->get_power_domains = gen11_dsi_get_power_domains;
>  	encoder->disable_clock = gen11_dsi_gate_clocks;
> +	encoder->is_clock_enabled = gen11_dsi_is_clock_enabled;
> 
>  	/* register DSI connector with DRM subsystem */
>  	drm_connector_init(dev, connector, &gen11_dsi_connector_funcs,
> diff --git a/drivers/gpu/drm/i915/display/intel_crt.c
> b/drivers/gpu/drm/i915/display/intel_crt.c
> index b03f74076f64..7f3d11c5ce3e 100644
> --- a/drivers/gpu/drm/i915/display/intel_crt.c
> +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> @@ -1078,6 +1078,7 @@ void intel_crt_init(struct drm_i915_private
> *dev_priv)
>  		crt->base.post_disable = hsw_post_disable_crt;
>  		crt->base.enable_clock = hsw_ddi_enable_clock;
>  		crt->base.disable_clock = hsw_ddi_disable_clock;
> +		crt->base.is_clock_enabled = hsw_ddi_is_clock_enabled;
>  	} else {
>  		if (HAS_PCH_SPLIT(dev_priv)) {
>  			crt->base.compute_config =
> pch_crt_compute_config; diff --git
> a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 56f5f55a7c8f..7d477c4007c7 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1589,6 +1589,12 @@ static void _cnl_ddi_disable_clock(struct
> drm_i915_private *i915, i915_reg_t reg
>  	mutex_unlock(&i915->dpll.lock);
>  }
> 
> +static bool _cnl_ddi_is_clock_enabled(struct drm_i915_private *i915,
> i915_reg_t reg,
> +				      u32 clk_off)
> +{
> +	return !(intel_de_read(i915, reg) & clk_off); }
> +
>  static struct intel_shared_dpll *
>  _cnl_ddi_get_pll(struct drm_i915_private *i915, i915_reg_t reg,
>  		 u32 clk_sel_mask, u32 clk_sel_shift) @@ -1625,6 +1631,15
> @@ static void adls_ddi_disable_clock(struct intel_encoder *encoder)
>  			       ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
>  }
> 
> +static bool adls_ddi_is_clock_enabled(struct intel_encoder *encoder) {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	enum phy phy = intel_port_to_phy(i915, encoder->port);
> +
> +	return _cnl_ddi_is_clock_enabled(i915, ADLS_DPCLKA_CFGCR(phy),
> +
> ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
> +}
> +
>  static struct intel_shared_dpll *adls_ddi_get_pll(struct intel_encoder
> *encoder)  {
>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev); @@ -
> 1660,6 +1675,15 @@ static void rkl_ddi_disable_clock(struct intel_encoder
> *encoder)
>  			       RKL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
>  }
> 
> +static bool rkl_ddi_is_clock_enabled(struct intel_encoder *encoder) {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	enum phy phy = intel_port_to_phy(i915, encoder->port);
> +
> +	return _cnl_ddi_is_clock_enabled(i915, ICL_DPCLKA_CFGCR0,
> +
> RKL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
> +}
> +
>  static struct intel_shared_dpll *rkl_ddi_get_pll(struct intel_encoder
> *encoder)  {
>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev); @@ -
> 1704,6 +1728,15 @@ static void dg1_ddi_disable_clock(struct intel_encoder
> *encoder)
>  			       DG1_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
>  }
> 
> +static bool dg1_ddi_is_clock_enabled(struct intel_encoder *encoder) {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	enum phy phy = intel_port_to_phy(i915, encoder->port);
> +
> +	return _cnl_ddi_is_clock_enabled(i915, DG1_DPCLKA_CFGCR0(phy),
> +
> DG1_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
> +}
> +
>  static struct intel_shared_dpll *dg1_ddi_get_pll(struct intel_encoder
> *encoder)  {
>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev); @@ -
> 1739,6 +1772,15 @@ static void icl_ddi_combo_disable_clock(struct
> intel_encoder *encoder)
>  			       ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
>  }
> 
> +static bool icl_ddi_combo_is_clock_enabled(struct intel_encoder
> +*encoder) {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	enum phy phy = intel_port_to_phy(i915, encoder->port);
> +
> +	return _cnl_ddi_is_clock_enabled(i915, ICL_DPCLKA_CFGCR0,
> +
> ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
> +}
> +
>  struct intel_shared_dpll *icl_ddi_combo_get_pll(struct intel_encoder
> *encoder)  {
>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev); @@ -
> 1778,6 +1820,20 @@ static void jsl_ddi_tc_disable_clock(struct intel_encoder
> *encoder)
>  	intel_de_write(i915, DDI_CLK_SEL(port), DDI_CLK_SEL_NONE);  }
> 
> +static bool jsl_ddi_tc_is_clock_enabled(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	enum port port = encoder->port;
> +	u32 tmp;
> +
> +	tmp = intel_de_read(i915, DDI_CLK_SEL(port));
> +
> +	if ((tmp & DDI_CLK_SEL_MASK) == DDI_CLK_SEL_NONE)
> +		return false;
> +
> +	return icl_ddi_combo_is_clock_enabled(encoder);
> +}
> +
>  static void icl_ddi_tc_enable_clock(struct intel_encoder *encoder,
>  				    const struct intel_crtc_state *crtc_state)  {
> @@ -1816,6 +1872,23 @@ static void icl_ddi_tc_disable_clock(struct
> intel_encoder *encoder)
>  	intel_de_write(i915, DDI_CLK_SEL(port), DDI_CLK_SEL_NONE);  }
> 
> +static bool icl_ddi_tc_is_clock_enabled(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	enum tc_port tc_port = intel_port_to_tc(i915, encoder->port);
> +	enum port port = encoder->port;
> +	u32 tmp;
> +
> +	tmp = intel_de_read(i915, DDI_CLK_SEL(port));
> +
> +	if ((tmp & DDI_CLK_SEL_MASK) == DDI_CLK_SEL_NONE)
> +		return false;
> +
> +	tmp = intel_de_read(i915, ICL_DPCLKA_CFGCR0);
> +
> +	return !(tmp & ICL_DPCLKA_CFGCR0_TC_CLK_OFF(tc_port));
> +}
> +
>  static struct intel_shared_dpll *icl_ddi_tc_get_pll(struct intel_encoder
> *encoder)  {
>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev); @@ -
> 1871,6 +1944,15 @@ static void cnl_ddi_disable_clock(struct intel_encoder
> *encoder)
>  			       DPCLKA_CFGCR0_DDI_CLK_OFF(port));  }
> 
> +static bool cnl_ddi_is_clock_enabled(struct intel_encoder *encoder) {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	enum port port = encoder->port;
> +
> +	return _cnl_ddi_is_clock_enabled(i915, DPCLKA_CFGCR0,
> +
> DPCLKA_CFGCR0_DDI_CLK_OFF(port)); }
> +
>  static struct intel_shared_dpll *cnl_ddi_get_pll(struct intel_encoder
> *encoder)  {
>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev); @@ -
> 1938,6 +2020,18 @@ static void skl_ddi_disable_clock(struct intel_encoder
> *encoder)
>  	mutex_unlock(&i915->dpll.lock);
>  }
> 
> +static bool skl_ddi_is_clock_enabled(struct intel_encoder *encoder) {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	enum port port = encoder->port;
> +
> +	/*
> +	 * FIXME Not sure if the override affects both
> +	 * the PLL selection and the CLK_OFF bit.
> +	 */
> +	return !(intel_de_read(i915, DPLL_CTRL2) &
> +DPLL_CTRL2_DDI_CLK_OFF(port)); }
> +
>  static struct intel_shared_dpll *skl_ddi_get_pll(struct intel_encoder
> *encoder)  {
>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev); @@ -
> 1981,6 +2075,14 @@ void hsw_ddi_disable_clock(struct intel_encoder
> *encoder)
>  	intel_de_write(i915, PORT_CLK_SEL(port), PORT_CLK_SEL_NONE);  }
> 
> +bool hsw_ddi_is_clock_enabled(struct intel_encoder *encoder) {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	enum port port = encoder->port;
> +
> +	return intel_de_read(i915, PORT_CLK_SEL(port)) !=
> PORT_CLK_SEL_NONE; }
> +
>  static struct intel_shared_dpll *hsw_ddi_get_pll(struct intel_encoder
> *encoder)  {
>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev); @@ -
> 2084,8 +2186,15 @@ void icl_sanitize_encoder_pll_mapping(struct
> intel_encoder *encoder)
>  		ddi_clk_needed = false;
>  	}
> 
> -	if (!ddi_clk_needed && encoder->disable_clock)
> -		encoder->disable_clock(encoder);
> +	if (ddi_clk_needed || !encoder->disable_clock ||
> +	    !encoder->is_clock_enabled(encoder))
> +		return;
> +
> +	drm_notice(&i915->drm,
> +		   "[ENCODER:%d:%s] is disabled/in DSI mode with an
> ungated DDI clock, gate it\n",
> +		   encoder->base.base.id, encoder->base.name);
> +
> +	encoder->disable_clock(encoder);
>  }
> 
>  static void
> @@ -4335,38 +4444,46 @@ void intel_ddi_init(struct drm_i915_private
> *dev_priv, enum port port)
>  	if (IS_ALDERLAKE_S(dev_priv)) {
>  		encoder->enable_clock = adls_ddi_enable_clock;
>  		encoder->disable_clock = adls_ddi_disable_clock;
> +		encoder->is_clock_enabled = adls_ddi_is_clock_enabled;
>  		encoder->get_config = adls_ddi_get_config;
>  	} else if (IS_ROCKETLAKE(dev_priv)) {
>  		encoder->enable_clock = rkl_ddi_enable_clock;
>  		encoder->disable_clock = rkl_ddi_disable_clock;
> +		encoder->is_clock_enabled = rkl_ddi_is_clock_enabled;
>  		encoder->get_config = rkl_ddi_get_config;
>  	} else if (IS_DG1(dev_priv)) {
>  		encoder->enable_clock = dg1_ddi_enable_clock;
>  		encoder->disable_clock = dg1_ddi_disable_clock;
> +		encoder->is_clock_enabled = dg1_ddi_is_clock_enabled;
>  		encoder->get_config = dg1_ddi_get_config;
>  	} else if (IS_JSL_EHL(dev_priv)) {
>  		if (intel_ddi_is_tc(dev_priv, port)) {
>  			encoder->enable_clock = jsl_ddi_tc_enable_clock;
>  			encoder->disable_clock = jsl_ddi_tc_disable_clock;
> +			encoder->is_clock_enabled =
> jsl_ddi_tc_is_clock_enabled;
>  			encoder->get_config = icl_ddi_combo_get_config;
>  		} else {
>  			encoder->enable_clock =
> icl_ddi_combo_enable_clock;
>  			encoder->disable_clock =
> icl_ddi_combo_disable_clock;
> +			encoder->is_clock_enabled =
> icl_ddi_combo_is_clock_enabled;
>  			encoder->get_config = icl_ddi_combo_get_config;
>  		}
>  	} else if (INTEL_GEN(dev_priv) >= 11) {
>  		if (intel_ddi_is_tc(dev_priv, port)) {
>  			encoder->enable_clock = icl_ddi_tc_enable_clock;
>  			encoder->disable_clock = icl_ddi_tc_disable_clock;
> +			encoder->is_clock_enabled =
> icl_ddi_tc_is_clock_enabled;
>  			encoder->get_config = icl_ddi_tc_get_config;
>  		} else {
>  			encoder->enable_clock =
> icl_ddi_combo_enable_clock;
>  			encoder->disable_clock =
> icl_ddi_combo_disable_clock;
> +			encoder->is_clock_enabled =
> icl_ddi_combo_is_clock_enabled;
>  			encoder->get_config = icl_ddi_combo_get_config;
>  		}
>  	} else if (IS_CANNONLAKE(dev_priv)) {
>  		encoder->enable_clock = cnl_ddi_enable_clock;
>  		encoder->disable_clock = cnl_ddi_disable_clock;
> +		encoder->is_clock_enabled = cnl_ddi_is_clock_enabled;
>  		encoder->get_config = cnl_ddi_get_config;
>  	} else if (IS_GEN9_LP(dev_priv)) {
>  		/* BXT/GLK have fixed PLL->port mapping */ @@ -4374,10
> +4491,12 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum
> port port)
>  	} else if (IS_GEN9_BC(dev_priv)) {
>  		encoder->enable_clock = skl_ddi_enable_clock;
>  		encoder->disable_clock = skl_ddi_disable_clock;
> +		encoder->is_clock_enabled = skl_ddi_is_clock_enabled;
>  		encoder->get_config = skl_ddi_get_config;
>  	} else if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) {
>  		encoder->enable_clock = hsw_ddi_enable_clock;
>  		encoder->disable_clock = hsw_ddi_disable_clock;
> +		encoder->is_clock_enabled = hsw_ddi_is_clock_enabled;
>  		encoder->get_config = hsw_ddi_get_config;
>  	}
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.h
> b/drivers/gpu/drm/i915/display/intel_ddi.h
> index 0780c47efe0f..99cebbe6b586 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.h
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.h
> @@ -36,6 +36,7 @@ void intel_ddi_get_clock(struct intel_encoder *encoder,
> void hsw_ddi_enable_clock(struct intel_encoder *encoder,
>  			  const struct intel_crtc_state *crtc_state);  void
> hsw_ddi_disable_clock(struct intel_encoder *encoder);
> +bool hsw_ddi_is_clock_enabled(struct intel_encoder *encoder);
>  void hsw_ddi_get_config(struct intel_encoder *encoder,
>  			struct intel_crtc_state *crtc_state);  struct
> intel_shared_dpll *icl_ddi_combo_get_pll(struct intel_encoder *encoder);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 1a76e1d9de7a..5b2e81db0a20 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -226,6 +226,10 @@ struct intel_encoder {
>  	void (*enable_clock)(struct intel_encoder *encoder,
>  			     const struct intel_crtc_state *crtc_state);
>  	void (*disable_clock)(struct intel_encoder *encoder);
> +	/*
> +	 * Returns whether the port clock is enabled or not.
> +	 */
> +	bool (*is_clock_enabled)(struct intel_encoder *encoder);
>  	enum hpd_pin hpd_pin;
>  	enum intel_display_power_domain power_domain;
>  	/* for communication with audio component; protected by av_mutex
> */
> --
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx