[RFC 0/3] Enable ICL DSI PLL

[RFC 0/3] Enable ICL DSI PLL

[Vandita Kulkarni]

Gen11/ICL DSI has to choose one of the free available DPLL which can also be
tied to DDI A/B combo phy ports. In legacy platforms that was not the case as
DSI had separate/exclusive PLLs.

ICL DPLL enable/disable steps are 80% common if DPLL is tied to DDI interface
(HDMI/DP) or DSI. If DSI implements separate PLL enable/disable
sequence like in legacy platform, lot of duplicate code will be added
which is not the right way.

Current DPLL enable/disable functions has some steps specific to DP/HDMI
and require some redesign if DSI has to use these existing DPLL functions.

RFC in this series alter existing DPLL functions to have common code for
enabling DPLL and move encoder specific code (DP/HDMI/DSI) to encoder
files.

Currently changes are for *enabling DPLL only*. If this approach looks
fine, RFC for disabling PLL will also be added. These RFC patches has
been tested and Single/Dual Link Video mode works fine tested with
https://github.com/madhavchauhan/Intel-DSI-Driver.git

These RFC patches are developed on top of following published series:
https://patchwork.freedesktop.org/series/44823/

Madhav Chauhan (3):
  drm/i915/icl: Restructure ICL DPLL enable functionality
  drm/i915/icl: Enable Gen11 DSI PLL
  drm/i915/icl: Calculate DPLL params for DSI

 drivers/gpu/drm/i915/icl_dsi.c        | 41 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++
 drivers/gpu/drm/i915/intel_display.c  |  4 +++-
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 26 +++++++---------------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
 5 files changed, 64 insertions(+), 21 deletions(-)

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Vandita Kulkarni]

From: Madhav Chauhan <madhav.chauhan@intel.com>

In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
Most of the steps for enabling DPLL are common across DDI
and DSI. This patch makes icl_dpll_enable() generic which
will be used by all the encoders.

Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++++
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index cd01a09..2942a24 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
 	mutex_lock(&dev_priv->dpll_lock);
 
 	if (IS_ICELAKE(dev_priv)) {
+		enum intel_dpll_id id = pll->info->id;
+		i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
+
+		val = I915_READ(enable_reg);
+		val |= PLL_ENABLE;
+		I915_WRITE(enable_reg, val);
+
+		/* TODO: wait times missing from the spec. */
+		if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
+					    PLL_LOCK, 5))
+			DRM_ERROR("PLL %d not locked\n", id);
+
 		if (port >= PORT_C)
 			I915_WRITE(DDI_CLK_SEL(port),
 				   icl_pll_to_ddi_pll_sel(encoder, pll));
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index e6cac92..36ed155 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state,
 	return pll;
 }
 
-static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
+i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
 {
 	switch (id) {
 	default:
@@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
 	default:
 		MISSING_CASE(id);
 	}
-
-	/*
-	 * DVFS pre sequence would be here, but in our driver the cdclk code
-	 * paths should already be setting the appropriate voltage, hence we do
-	 * nothign here.
-	 */
-
-	val = I915_READ(enable_reg);
-	val |= PLL_ENABLE;
-	I915_WRITE(enable_reg, val);
-
-	if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK,
-				    1)) /* 600us actually. */
-		DRM_ERROR("PLL %d not locked\n", id);
-
-	/* DVFS post sequence would be here. See the comment above. */
+	/* Encoder specific PLL enable steps are added in encoder file */
 }
 
 static void icl_pll_disable(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index bf0de8a..9e89265 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv,
 int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
 			       uint32_t pll_id);
 int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
-
+i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id);
 #endif /* _INTEL_DPLL_MGR_H_ */
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC 2/3] drm/i915/icl: Enable Gen11 DSI PLL

[Vandita Kulkarni]

From: Madhav Chauhan <madhav.chauhan@intel.com>

This patch implements steps specific to DSI for
enabling PLL.

Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
---
 drivers/gpu/drm/i915/icl_dsi.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
index 13830e4..c530e25 100644
--- a/drivers/gpu/drm/i915/icl_dsi.c
+++ b/drivers/gpu/drm/i915/icl_dsi.c
@@ -54,6 +54,45 @@ static void gen11_dsi_program_esc_clk_div(struct intel_encoder *encoder)
 	}
 }
 
+static void gen11_dsi_pll_enable(struct intel_encoder *encoder,
+				 const struct intel_crtc_state *pipe_config)
+{
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_shared_dpll *pll = intel_crtc->config->shared_dpll;
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	enum intel_dpll_id id = pll->info->id;
+	i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
+	enum port port;
+	uint32_t val;
+
+	/**
+	 * Note: Following PLL enable steps are specific to DSI,
+	 * some common steps for all encoder will be done by dpll_enable().
+	 */
+
+	mutex_lock(&dev_priv->dpll_lock);
+
+	for_each_dsi_port(port, intel_dsi->ports) {
+		val = I915_READ(DPCLKA_CFGCR0_ICL);
+		val &= ~DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
+		val |= DPCLKA_CFGCR0_DDI_CLK_SEL(pll->info->id, port);
+		I915_WRITE(DPCLKA_CFGCR0_ICL, val);
+		POSTING_READ(DPCLKA_CFGCR0_ICL);
+	}
+
+	mutex_unlock(&dev_priv->dpll_lock);
+
+	gen11_dsi_program_esc_clk_div(encoder);
+	val = I915_READ(enable_reg);
+	val |= PLL_ENABLE;
+	I915_WRITE(enable_reg, val);
+
+	if (wait_for_us((I915_READ(enable_reg) & PLL_LOCK), 600))
+		DRM_ERROR("PLL %d not locked\n", id);
+}
+
 static void gen11_dsi_enable_io_power(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
@@ -120,7 +159,7 @@ static void __attribute__((unused))
 	gen11_dsi_enable_io_power(encoder);
 
 	/* step3: enable DSI PLL */
-	gen11_dsi_program_esc_clk_div(encoder);
+	gen11_dsi_pll_enable(encoder, pipe_config);
 
 	/* step4: enable DSI port and DPHY */
 	gen11_dsi_enable_port_and_phy(encoder);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC 3/3] drm/i915/icl: Calculate DPLL params for DSI

[Vandita Kulkarni]

From: Madhav Chauhan <madhav.chauhan@intel.com>

This patch calculate various DPLL dividers and
parameters for DSI encoder and adjust AFE clock
for DSI. For DSI, 8x clock is AFE clock.

v2: Extend haswell_crtc_compute_clock() for Gen11 DSI
v3: Rebase

Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  | 4 +++-
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 7 ++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6928dcc..1a44c5e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9238,10 +9238,12 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
 static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
 				      struct intel_crtc_state *crtc_state)
 {
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	struct intel_atomic_state *state =
 		to_intel_atomic_state(crtc_state->base.state);
 
-	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI)) {
+	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) ||
+				 IS_ICELAKE(dev_priv)) {
 		struct intel_encoder *encoder =
 			intel_get_crtc_new_encoder(state, crtc_state);
 
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 36ed155..5175e44 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -22,6 +22,7 @@
  */
 
 #include "intel_drv.h"
+#include "intel_dsi.h"
 
 /**
  * DOC: Display PLLs
@@ -2532,7 +2533,11 @@ static bool icl_calc_dpll_state(struct intel_crtc_state *crtc_state,
 		ret = icl_calc_tbt_pll(dev_priv, clock, &pll_params);
 	else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
 		ret = cnl_ddi_calculate_wrpll(clock, dev_priv, &pll_params);
-	else
+	else if (encoder->type == INTEL_OUTPUT_DSI) {
+		struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+		ret = cnl_ddi_calculate_wrpll(intel_dsi->bitrate_khz/5,
+					      dev_priv, &pll_params);
+	} else
 		ret = icl_calc_dp_combo_pll(dev_priv, clock, &pll_params);
 
 	if (!ret)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Enable ICL DSI PLL

[Patchwork]

== Series Details ==

Series: Enable ICL DSI PLL
URL   : https://patchwork.freedesktop.org/series/49683/
State : failure

== Summary ==

CALL    scripts/checksyscalls.sh
  DESCEND  objtool
  CHK     include/generated/compile.h
  CC [M]  drivers/gpu/drm/i915/intel_dpll_mgr.o
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function ‘icl_calc_dpll_state’:
drivers/gpu/drm/i915/intel_dpll_mgr.c:2538:42: error: ‘struct intel_dsi’ has no member named ‘bitrate_khz’
   ret = cnl_ddi_calculate_wrpll(intel_dsi->bitrate_khz/5,
                                          ^~
scripts/Makefile.build:305: recipe for target 'drivers/gpu/drm/i915/intel_dpll_mgr.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_dpll_mgr.o] Error 1
scripts/Makefile.build:546: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:546: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:546: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1060: recipe for target 'drivers' failed
make: *** [drivers] Error 2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Ville Syrjälä]

On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
> From: Madhav Chauhan <madhav.chauhan@intel.com>
> 
> In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
> Most of the steps for enabling DPLL are common across DDI
> and DSI. This patch makes icl_dpll_enable() generic which
> will be used by all the encoders.
> 
> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++++
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
>  3 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index cd01a09..2942a24 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
>  	mutex_lock(&dev_priv->dpll_lock);
>  
>  	if (IS_ICELAKE(dev_priv)) {
> +		enum intel_dpll_id id = pll->info->id;
> +		i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> +
> +		val = I915_READ(enable_reg);
> +		val |= PLL_ENABLE;
> +		I915_WRITE(enable_reg, val);
> +
> +		/* TODO: wait times missing from the spec. */
> +		if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
> +					    PLL_LOCK, 5))
> +			DRM_ERROR("PLL %d not locked\n", id);
> +

I don't really see why this can't stay in the dpll mgr.

>  		if (port >= PORT_C)
>  			I915_WRITE(DDI_CLK_SEL(port),
>  				   icl_pll_to_ddi_pll_sel(encoder, pll));
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index e6cac92..36ed155 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state,
>  	return pll;
>  }
>  
> -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
>  {
>  	switch (id) {
>  	default:
> @@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>  	default:
>  		MISSING_CASE(id);
>  	}
> -
> -	/*
> -	 * DVFS pre sequence would be here, but in our driver the cdclk code
> -	 * paths should already be setting the appropriate voltage, hence we do
> -	 * nothign here.
> -	 */
> -
> -	val = I915_READ(enable_reg);
> -	val |= PLL_ENABLE;
> -	I915_WRITE(enable_reg, val);
> -
> -	if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK,
> -				    1)) /* 600us actually. */
> -		DRM_ERROR("PLL %d not locked\n", id);
> -
> -	/* DVFS post sequence would be here. See the comment above. */
> +	/* Encoder specific PLL enable steps are added in encoder file */
>  }
>  
>  static void icl_pll_disable(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index bf0de8a..9e89265 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv,
>  int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
>  			       uint32_t pll_id);
>  int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
> -
> +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id);
>  #endif /* _INTEL_DPLL_MGR_H_ */
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 3/3] drm/i915/icl: Calculate DPLL params for DSI

[Ville Syrjälä]

On Fri, Sep 14, 2018 at 12:24:14PM +0530, Vandita Kulkarni wrote:
> From: Madhav Chauhan <madhav.chauhan@intel.com>
> 
> This patch calculate various DPLL dividers and
> parameters for DSI encoder and adjust AFE clock
> for DSI. For DSI, 8x clock is AFE clock.
> 
> v2: Extend haswell_crtc_compute_clock() for Gen11 DSI
> v3: Rebase
> 
> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  | 4 +++-
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 7 ++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6928dcc..1a44c5e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9238,10 +9238,12 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>  				      struct intel_crtc_state *crtc_state)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct intel_atomic_state *state =
>  		to_intel_atomic_state(crtc_state->base.state);
>  
> -	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI)) {
> +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) ||
> +				 IS_ICELAKE(dev_priv)) {
>  		struct intel_encoder *encoder =
>  			intel_get_crtc_new_encoder(state, crtc_state);
>  
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 36ed155..5175e44 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -22,6 +22,7 @@
>   */
>  
>  #include "intel_drv.h"
> +#include "intel_dsi.h"
>  
>  /**
>   * DOC: Display PLLs
> @@ -2532,7 +2533,11 @@ static bool icl_calc_dpll_state(struct intel_crtc_state *crtc_state,
>  		ret = icl_calc_tbt_pll(dev_priv, clock, &pll_params);
>  	else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
>  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv, &pll_params);
> -	else
> +	else if (encoder->type == INTEL_OUTPUT_DSI) {
> +		struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +		ret = cnl_ddi_calculate_wrpll(intel_dsi->bitrate_khz/5,

Missing port_clock=whatever in compute config?

> +					      dev_priv, &pll_params);
> +	} else
>  		ret = icl_calc_dp_combo_pll(dev_priv, clock, &pll_params);
>  
>  	if (!ret)
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Kulkarni, Vandita]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Friday, September 14, 2018 9:37 PM
> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable
> functionality
> 
> On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
> > From: Madhav Chauhan <madhav.chauhan@intel.com>
> >
> > In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
> > Most of the steps for enabling DPLL are common across DDI and DSI.
> > This patch makes icl_dpll_enable() generic which will be used by all
> > the encoders.
> >
> > Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++++
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
> > drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
> >  3 files changed, 15 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index cd01a09..2942a24 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct
> intel_encoder *encoder,
> >  	mutex_lock(&dev_priv->dpll_lock);
> >
> >  	if (IS_ICELAKE(dev_priv)) {
> > +		enum intel_dpll_id id = pll->info->id;
> > +		i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> > +
> > +		val = I915_READ(enable_reg);
> > +		val |= PLL_ENABLE;
> > +		I915_WRITE(enable_reg, val);
> > +
> > +		/* TODO: wait times missing from the spec. */
> > +		if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
> > +					    PLL_LOCK, 5))
> > +			DRM_ERROR("PLL %d not locked\n", id);
> > +
> 
> I don't really see why this can't stay in the dpll mgr.
This was part of icl_pll_enable, which gets called for all hdmi, dp and mipi dsi.
But for mipi dsi we have couple of more things to do before we enable pll.

In order to keep it unchanged for other encoders , pll enable was moved here.
Another way could be have an encoder check in icl_pll_enable function and do those extra steps for mipi dsi and then enable the pll.
Please let me know what you think would be a better way to do.

Thanks,
Vandita 
> 
> >  		if (port >= PORT_C)
> >  			I915_WRITE(DDI_CLK_SEL(port),
> >  				   icl_pll_to_ddi_pll_sel(encoder, pll)); diff --git
> > a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index e6cac92..36ed155 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct
> intel_crtc_state *crtc_state,
> >  	return pll;
> >  }
> >
> > -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> >  {
> >  	switch (id) {
> >  	default:
> > @@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct drm_i915_private
> *dev_priv,
> >  	default:
> >  		MISSING_CASE(id);
> >  	}
> > -
> > -	/*
> > -	 * DVFS pre sequence would be here, but in our driver the cdclk code
> > -	 * paths should already be setting the appropriate voltage, hence we do
> > -	 * nothign here.
> > -	 */
> > -
> > -	val = I915_READ(enable_reg);
> > -	val |= PLL_ENABLE;
> > -	I915_WRITE(enable_reg, val);
> > -
> > -	if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK,
> > -				    1)) /* 600us actually. */
> > -		DRM_ERROR("PLL %d not locked\n", id);
> > -
> > -	/* DVFS post sequence would be here. See the comment above. */
> > +	/* Encoder specific PLL enable steps are added in encoder file */
> >  }
> >
> >  static void icl_pll_disable(struct drm_i915_private *dev_priv, diff
> > --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > index bf0de8a..9e89265 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > @@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct
> > drm_i915_private *dev_priv,  int icl_calc_dp_combo_pll_link(struct
> drm_i915_private *dev_priv,
> >  			       uint32_t pll_id);
> >  int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
> > -
> > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id);
> >  #endif /* _INTEL_DPLL_MGR_H_ */
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 3/3] drm/i915/icl: Calculate DPLL params for DSI

[Kulkarni, Vandita]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Friday, September 14, 2018 9:39 PM
> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Subject: Re: [Intel-gfx] [RFC 3/3] drm/i915/icl: Calculate DPLL params for DSI
> 
> On Fri, Sep 14, 2018 at 12:24:14PM +0530, Vandita Kulkarni wrote:
> > From: Madhav Chauhan <madhav.chauhan@intel.com>
> >
> > This patch calculate various DPLL dividers and parameters for DSI
> > encoder and adjust AFE clock for DSI. For DSI, 8x clock is AFE clock.
> >
> > v2: Extend haswell_crtc_compute_clock() for Gen11 DSI
> > v3: Rebase
> >
> > Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c  | 4 +++-
> > drivers/gpu/drm/i915/intel_dpll_mgr.c | 7 ++++++-
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 6928dcc..1a44c5e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9238,10 +9238,12 @@ void hsw_disable_pc8(struct drm_i915_private
> > *dev_priv)  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
> >  				      struct intel_crtc_state *crtc_state)  {
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	struct intel_atomic_state *state =
> >  		to_intel_atomic_state(crtc_state->base.state);
> >
> > -	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI)) {
> > +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) ||
> > +				 IS_ICELAKE(dev_priv)) {
> >  		struct intel_encoder *encoder =
> >  			intel_get_crtc_new_encoder(state, crtc_state);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index 36ed155..5175e44 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -22,6 +22,7 @@
> >   */
> >
> >  #include "intel_drv.h"
> > +#include "intel_dsi.h"
> >
> >  /**
> >   * DOC: Display PLLs
> > @@ -2532,7 +2533,11 @@ static bool icl_calc_dpll_state(struct
> intel_crtc_state *crtc_state,
> >  		ret = icl_calc_tbt_pll(dev_priv, clock, &pll_params);
> >  	else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv, &pll_params);
> > -	else
> > +	else if (encoder->type == INTEL_OUTPUT_DSI) {
> > +		struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> > +		ret = cnl_ddi_calculate_wrpll(intel_dsi->bitrate_khz/5,
> 
> Missing port_clock=whatever in compute config?
As per the spec I see that for mipi dsi on icl,
8x clock = AFE clock.
And 8x clock is calculated and stored already as pixel_clk * bpp / lane_count
Hence we are using already calculate 8x clock variable here and divide by/5 because later in the function it
gets multiplied by 5 for afe clock.

port_clock is assigned to pixel clock as I see. 
Please let me know if we have to use math around the port clock and pass that value, than passing 8x clock.

Thanks,
Vandita
> 
> > +					      dev_priv, &pll_params);
> > +	} else
> >  		ret = icl_calc_dp_combo_pll(dev_priv, clock, &pll_params);
> >
> >  	if (!ret)
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 3/3] drm/i915/icl: Calculate DPLL params for DSI

[Ville Syrjälä]

On Thu, Sep 20, 2018 at 08:49:52AM +0000, Kulkarni, Vandita wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Friday, September 14, 2018 9:39 PM
> > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> > Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> > Subject: Re: [Intel-gfx] [RFC 3/3] drm/i915/icl: Calculate DPLL params for DSI
> > 
> > On Fri, Sep 14, 2018 at 12:24:14PM +0530, Vandita Kulkarni wrote:
> > > From: Madhav Chauhan <madhav.chauhan@intel.com>
> > >
> > > This patch calculate various DPLL dividers and parameters for DSI
> > > encoder and adjust AFE clock for DSI. For DSI, 8x clock is AFE clock.
> > >
> > > v2: Extend haswell_crtc_compute_clock() for Gen11 DSI
> > > v3: Rebase
> > >
> > > Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> > > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c  | 4 +++-
> > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 7 ++++++-
> > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 6928dcc..1a44c5e 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -9238,10 +9238,12 @@ void hsw_disable_pc8(struct drm_i915_private
> > > *dev_priv)  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
> > >  				      struct intel_crtc_state *crtc_state)  {
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >  	struct intel_atomic_state *state =
> > >  		to_intel_atomic_state(crtc_state->base.state);
> > >
> > > -	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI)) {
> > > +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) ||
> > > +				 IS_ICELAKE(dev_priv)) {
> > >  		struct intel_encoder *encoder =
> > >  			intel_get_crtc_new_encoder(state, crtc_state);
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index 36ed155..5175e44 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -22,6 +22,7 @@
> > >   */
> > >
> > >  #include "intel_drv.h"
> > > +#include "intel_dsi.h"
> > >
> > >  /**
> > >   * DOC: Display PLLs
> > > @@ -2532,7 +2533,11 @@ static bool icl_calc_dpll_state(struct
> > intel_crtc_state *crtc_state,
> > >  		ret = icl_calc_tbt_pll(dev_priv, clock, &pll_params);
> > >  	else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv, &pll_params);
> > > -	else
> > > +	else if (encoder->type == INTEL_OUTPUT_DSI) {
> > > +		struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> > > +		ret = cnl_ddi_calculate_wrpll(intel_dsi->bitrate_khz/5,
> > 
> > Missing port_clock=whatever in compute config?
> As per the spec I see that for mipi dsi on icl,
> 8x clock = AFE clock.
> And 8x clock is calculated and stored already as pixel_clk * bpp / lane_count
> Hence we are using already calculate 8x clock variable here and divide by/5 because later in the function it
> gets multiplied by 5 for afe clock.
> 
> port_clock is assigned to pixel clock as I see. 

port_clock can basically be anything you want. It's really up to the
encoder to define it. For DP it's the freq (1.62ghz, 2.7ghz, etc.), for
HDMI it's the TMDS character rate, for SDVO it's the multiplied pixel
clock, etc.

I suppose that means that currently port_clock is the output from the
dpll in pretty much all cases. Sticking to that same rule will make DSI
look less special and thus easier to understand for everyone.

> Please let me know if we have to use math around the port clock and pass that value, than passing 8x clock.
> 
> Thanks,
> Vandita
> > 
> > > +					      dev_priv, &pll_params);
> > > +	} else
> > >  		ret = icl_calc_dp_combo_pll(dev_priv, clock, &pll_params);
> > >
> > >  	if (!ret)
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Ville Syrjälä]

On Wed, Sep 19, 2018 at 05:31:37PM +0000, Kulkarni, Vandita wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Friday, September 14, 2018 9:37 PM
> > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> > Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> > Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable
> > functionality
> > 
> > On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
> > > From: Madhav Chauhan <madhav.chauhan@intel.com>
> > >
> > > In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
> > > Most of the steps for enabling DPLL are common across DDI and DSI.
> > > This patch makes icl_dpll_enable() generic which will be used by all
> > > the encoders.
> > >
> > > Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> > > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++++
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
> > > drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
> > >  3 files changed, 15 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index cd01a09..2942a24 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct
> > intel_encoder *encoder,
> > >  	mutex_lock(&dev_priv->dpll_lock);
> > >
> > >  	if (IS_ICELAKE(dev_priv)) {
> > > +		enum intel_dpll_id id = pll->info->id;
> > > +		i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> > > +
> > > +		val = I915_READ(enable_reg);
> > > +		val |= PLL_ENABLE;
> > > +		I915_WRITE(enable_reg, val);
> > > +
> > > +		/* TODO: wait times missing from the spec. */
> > > +		if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
> > > +					    PLL_LOCK, 5))
> > > +			DRM_ERROR("PLL %d not locked\n", id);
> > > +
> > 
> > I don't really see why this can't stay in the dpll mgr.
> This was part of icl_pll_enable, which gets called for all hdmi, dp and mipi dsi.
> But for mipi dsi we have couple of more things to do before we enable pll.
> 
> In order to keep it unchanged for other encoders , pll enable was moved here.
> Another way could be have an encoder check in icl_pll_enable function and do those extra steps for mipi dsi and then enable the pll.
> Please let me know what you think would be a better way to do.

Hard to judge what is best without knowing what the differences are.

> 
> Thanks,
> Vandita 
> > 
> > >  		if (port >= PORT_C)
> > >  			I915_WRITE(DDI_CLK_SEL(port),
> > >  				   icl_pll_to_ddi_pll_sel(encoder, pll)); diff --git
> > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index e6cac92..36ed155 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct
> > intel_crtc_state *crtc_state,
> > >  	return pll;
> > >  }
> > >
> > > -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> > >  {
> > >  	switch (id) {
> > >  	default:
> > > @@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct drm_i915_private
> > *dev_priv,
> > >  	default:
> > >  		MISSING_CASE(id);
> > >  	}
> > > -
> > > -	/*
> > > -	 * DVFS pre sequence would be here, but in our driver the cdclk code
> > > -	 * paths should already be setting the appropriate voltage, hence we do
> > > -	 * nothign here.
> > > -	 */
> > > -
> > > -	val = I915_READ(enable_reg);
> > > -	val |= PLL_ENABLE;
> > > -	I915_WRITE(enable_reg, val);
> > > -
> > > -	if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK,
> > > -				    1)) /* 600us actually. */
> > > -		DRM_ERROR("PLL %d not locked\n", id);
> > > -
> > > -	/* DVFS post sequence would be here. See the comment above. */
> > > +	/* Encoder specific PLL enable steps are added in encoder file */
> > >  }
> > >
> > >  static void icl_pll_disable(struct drm_i915_private *dev_priv, diff
> > > --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > index bf0de8a..9e89265 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > @@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct
> > > drm_i915_private *dev_priv,  int icl_calc_dp_combo_pll_link(struct
> > drm_i915_private *dev_priv,
> > >  			       uint32_t pll_id);
> > >  int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
> > > -
> > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id);
> > >  #endif /* _INTEL_DPLL_MGR_H_ */
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Kulkarni, Vandita]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, September 26, 2018 7:57 PM
> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable
> functionality
> 
> On Wed, Sep 19, 2018 at 05:31:37PM +0000, Kulkarni, Vandita wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Sent: Friday, September 14, 2018 9:37 PM
> > > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
> > > <jani.nikula@intel.com>; Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> > > Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL
> > > DPLL enable functionality
> > >
> > > On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
> > > > From: Madhav Chauhan <madhav.chauhan@intel.com>
> > > >
> > > > In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
> > > > Most of the steps for enabling DPLL are common across DDI and DSI.
> > > > This patch makes icl_dpll_enable() generic which will be used by
> > > > all the encoders.
> > > >
> > > > Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> > > > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++++
> > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
> > > > drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
> > > >  3 files changed, 15 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index cd01a09..2942a24 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct
> > > intel_encoder *encoder,
> > > >  	mutex_lock(&dev_priv->dpll_lock);
> > > >
> > > >  	if (IS_ICELAKE(dev_priv)) {
> > > > +		enum intel_dpll_id id = pll->info->id;
> > > > +		i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> > > > +
> > > > +		val = I915_READ(enable_reg);
> > > > +		val |= PLL_ENABLE;
> > > > +		I915_WRITE(enable_reg, val);
> > > > +
> > > > +		/* TODO: wait times missing from the spec. */
> > > > +		if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
> > > > +					    PLL_LOCK, 5))
> > > > +			DRM_ERROR("PLL %d not locked\n", id);
> > > > +
> > >
> > > I don't really see why this can't stay in the dpll mgr.
> > This was part of icl_pll_enable, which gets called for all hdmi, dp and mipi dsi.
> > But for mipi dsi we have couple of more things to do before we enable pll.
> >
> > In order to keep it unchanged for other encoders , pll enable was moved here.
> > Another way could be have an encoder check in icl_pll_enable function and do
> those extra steps for mipi dsi and then enable the pll.
> > Please let me know what you think would be a better way to do.
> 
> Hard to judge what is best without knowing what the differences are.
As per the bspec,
The below two steps are the extra ones that we do before enabling the PLL
1. Configure both DSI_ESC_CLK_DIV and DPHY_ESC_CLK_DIV registers. Both registers must be programmed with the same value.
2. Read back both DPHY_ESC_CLK_DIV, ignoring the data value, this is to ensure write completed before the next step.

And these steps below
Configure DPCLKA_CFGCR0 to map the DPLL to the DDI/DSI.
Make sure that the DDI clocks are not gated at this point.  The DSI enabling sequence will gate the DDI clocks at a later point within the sequence.
are done before pll enable in case of pll enable sequence for mipi dsi.

Thanks
Vandita
> 
> >
> > Thanks,
> > Vandita
> > >
> > > >  		if (port >= PORT_C)
> > > >  			I915_WRITE(DDI_CLK_SEL(port),
> > > >  				   icl_pll_to_ddi_pll_sel(encoder, pll)); diff --git
> > > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > index e6cac92..36ed155 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > @@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct
> > > intel_crtc_state *crtc_state,
> > > >  	return pll;
> > > >  }
> > > >
> > > > -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> > > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> > > >  {
> > > >  	switch (id) {
> > > >  	default:
> > > > @@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct
> > > > drm_i915_private
> > > *dev_priv,
> > > >  	default:
> > > >  		MISSING_CASE(id);
> > > >  	}
> > > > -
> > > > -	/*
> > > > -	 * DVFS pre sequence would be here, but in our driver the cdclk code
> > > > -	 * paths should already be setting the appropriate voltage, hence we do
> > > > -	 * nothign here.
> > > > -	 */
> > > > -
> > > > -	val = I915_READ(enable_reg);
> > > > -	val |= PLL_ENABLE;
> > > > -	I915_WRITE(enable_reg, val);
> > > > -
> > > > -	if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK,
> > > > -				    1)) /* 600us actually. */
> > > > -		DRM_ERROR("PLL %d not locked\n", id);
> > > > -
> > > > -	/* DVFS post sequence would be here. See the comment above. */
> > > > +	/* Encoder specific PLL enable steps are added in encoder file
> > > > +*/
> > > >  }
> > > >
> > > >  static void icl_pll_disable(struct drm_i915_private *dev_priv,
> > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > > index bf0de8a..9e89265 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > > @@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct
> > > > drm_i915_private *dev_priv,  int icl_calc_dp_combo_pll_link(struct
> > > drm_i915_private *dev_priv,
> > > >  			       uint32_t pll_id);
> > > >  int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
> > > > -
> > > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id);
> > > >  #endif /* _INTEL_DPLL_MGR_H_ */
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> 
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 3/3] drm/i915/icl: Calculate DPLL params for DSI

[Kulkarni, Vandita]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, September 26, 2018 7:52 PM
> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Subject: Re: [Intel-gfx] [RFC 3/3] drm/i915/icl: Calculate DPLL params for DSI
> 
> On Thu, Sep 20, 2018 at 08:49:52AM +0000, Kulkarni, Vandita wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Sent: Friday, September 14, 2018 9:39 PM
> > > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
> > > <jani.nikula@intel.com>; Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> > > Subject: Re: [Intel-gfx] [RFC 3/3] drm/i915/icl: Calculate DPLL
> > > params for DSI
> > >
> > > On Fri, Sep 14, 2018 at 12:24:14PM +0530, Vandita Kulkarni wrote:
> > > > From: Madhav Chauhan <madhav.chauhan@intel.com>
> > > >
> > > > This patch calculate various DPLL dividers and parameters for DSI
> > > > encoder and adjust AFE clock for DSI. For DSI, 8x clock is AFE clock.
> > > >
> > > > v2: Extend haswell_crtc_compute_clock() for Gen11 DSI
> > > > v3: Rebase
> > > >
> > > > Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> > > > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c  | 4 +++-
> > > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 7 ++++++-
> > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 6928dcc..1a44c5e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -9238,10 +9238,12 @@ void hsw_disable_pc8(struct
> > > > drm_i915_private
> > > > *dev_priv)  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
> > > >  				      struct intel_crtc_state *crtc_state)  {
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > >  	struct intel_atomic_state *state =
> > > >  		to_intel_atomic_state(crtc_state->base.state);
> > > >
> > > > -	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI)) {
> > > > +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) ||
> > > > +				 IS_ICELAKE(dev_priv)) {
> > > >  		struct intel_encoder *encoder =
> > > >  			intel_get_crtc_new_encoder(state, crtc_state);
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > index 36ed155..5175e44 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > @@ -22,6 +22,7 @@
> > > >   */
> > > >
> > > >  #include "intel_drv.h"
> > > > +#include "intel_dsi.h"
> > > >
> > > >  /**
> > > >   * DOC: Display PLLs
> > > > @@ -2532,7 +2533,11 @@ static bool icl_calc_dpll_state(struct
> > > intel_crtc_state *crtc_state,
> > > >  		ret = icl_calc_tbt_pll(dev_priv, clock, &pll_params);
> > > >  	else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > > >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv, &pll_params);
> > > > -	else
> > > > +	else if (encoder->type == INTEL_OUTPUT_DSI) {
> > > > +		struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> > > > +		ret = cnl_ddi_calculate_wrpll(intel_dsi->bitrate_khz/5,
> > >
> > > Missing port_clock=whatever in compute config?
> > As per the spec I see that for mipi dsi on icl, 8x clock = AFE clock.
> > And 8x clock is calculated and stored already as pixel_clk * bpp /
> > lane_count Hence we are using already calculate 8x clock variable here
> > and divide by/5 because later in the function it gets multiplied by 5 for afe
> clock.
> >
> > port_clock is assigned to pixel clock as I see.
> 
> port_clock can basically be anything you want. It's really up to the encoder to
> define it. For DP it's the freq (1.62ghz, 2.7ghz, etc.), for HDMI it's the TMDS
> character rate, for SDVO it's the multiplied pixel clock, etc.
> 
> I suppose that means that currently port_clock is the output from the dpll in
> pretty much all cases. Sticking to that same rule will make DSI look less special
> and thus easier to understand for everyone.

Thank you, I will assign this bitrate_khz  (dsi_clk) to port_clock.

Regards,
Vandita
> 
> > Please let me know if we have to use math around the port clock and pass that
> value, than passing 8x clock.
> >
> > Thanks,
> > Vandita
> > >
> > > > +					      dev_priv, &pll_params);
> > > > +	} else
> > > >  		ret = icl_calc_dp_combo_pll(dev_priv, clock, &pll_params);
> > > >
> > > >  	if (!ret)
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> 
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 3/3] drm/i915/icl: Calculate DPLL params for DSI

[Chauhan, Madhav]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Kulkarni, Vandita
> Sent: Monday, October 1, 2018 5:00 PM
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Nikula, Jani <jani.nikula@intel.com>; intel-gfx@lists.freedesktop.org;
> Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Subject: Re: [Intel-gfx] [RFC 3/3] drm/i915/icl: Calculate DPLL params for DSI
> 
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Wednesday, September 26, 2018 7:52 PM
> > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
> > <jani.nikula@intel.com>; Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> > Subject: Re: [Intel-gfx] [RFC 3/3] drm/i915/icl: Calculate DPLL params
> > for DSI
> >
> > On Thu, Sep 20, 2018 at 08:49:52AM +0000, Kulkarni, Vandita wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Sent: Friday, September 14, 2018 9:39 PM
> > > > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> > > > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
> > > > <jani.nikula@intel.com>; Zanoni, Paulo R
> > > > <paulo.r.zanoni@intel.com>
> > > > Subject: Re: [Intel-gfx] [RFC 3/3] drm/i915/icl: Calculate DPLL
> > > > params for DSI
> > > >
> > > > On Fri, Sep 14, 2018 at 12:24:14PM +0530, Vandita Kulkarni wrote:
> > > > > From: Madhav Chauhan <madhav.chauhan@intel.com>
> > > > >
> > > > > This patch calculate various DPLL dividers and parameters for
> > > > > DSI encoder and adjust AFE clock for DSI. For DSI, 8x clock is AFE clock.
> > > > >
> > > > > v2: Extend haswell_crtc_compute_clock() for Gen11 DSI
> > > > > v3: Rebase
> > > > >
> > > > > Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> > > > > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c  | 4 +++-
> > > > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 7 ++++++-
> > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 6928dcc..1a44c5e 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -9238,10 +9238,12 @@ void hsw_disable_pc8(struct
> > > > > drm_i915_private
> > > > > *dev_priv)  static int haswell_crtc_compute_clock(struct intel_crtc
> *crtc,
> > > > >  				      struct intel_crtc_state *crtc_state)  {
> > > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > >  	struct intel_atomic_state *state =
> > > > >  		to_intel_atomic_state(crtc_state->base.state);
> > > > >
> > > > > -	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI)) {
> > > > > +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) ||
> > > > > +				 IS_ICELAKE(dev_priv)) {
> > > > >  		struct intel_encoder *encoder =
> > > > >  			intel_get_crtc_new_encoder(state, crtc_state);
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > index 36ed155..5175e44 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > @@ -22,6 +22,7 @@
> > > > >   */
> > > > >
> > > > >  #include "intel_drv.h"
> > > > > +#include "intel_dsi.h"
> > > > >
> > > > >  /**
> > > > >   * DOC: Display PLLs
> > > > > @@ -2532,7 +2533,11 @@ static bool icl_calc_dpll_state(struct
> > > > intel_crtc_state *crtc_state,
> > > > >  		ret = icl_calc_tbt_pll(dev_priv, clock, &pll_params);
> > > > >  	else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > > > >  		ret = cnl_ddi_calculate_wrpll(clock, dev_priv, &pll_params);
> > > > > -	else
> > > > > +	else if (encoder->type == INTEL_OUTPUT_DSI) {
> > > > > +		struct intel_dsi *intel_dsi =
> enc_to_intel_dsi(&encoder->base);
> > > > > +		ret = cnl_ddi_calculate_wrpll(intel_dsi-
> >bitrate_khz/5,
> > > >
> > > > Missing port_clock=whatever in compute config?
> > > As per the spec I see that for mipi dsi on icl, 8x clock = AFE clock.
> > > And 8x clock is calculated and stored already as pixel_clk * bpp /
> > > lane_count Hence we are using already calculate 8x clock variable
> > > here and divide by/5 because later in the function it gets
> > > multiplied by 5 for afe
> > clock.
> > >
> > > port_clock is assigned to pixel clock as I see.
> >
> > port_clock can basically be anything you want. It's really up to the
> > encoder to define it. For DP it's the freq (1.62ghz, 2.7ghz, etc.),
> > for HDMI it's the TMDS character rate, for SDVO it's the multiplied pixel
> clock, etc.
> >
> > I suppose that means that currently port_clock is the output from the
> > dpll in pretty much all cases. Sticking to that same rule will make
> > DSI look less special and thus easier to understand for everyone.
> 
> Thank you, I will assign this bitrate_khz  (dsi_clk) to port_clock.

Please make sure we are not breaking anything else by doing this as there will be assumption
In DSI code that this is the pixelclk.
And still you need to divide it by 5 before passing to caller,
for DSI AFE clock  = 8X clock or Bitrate
which is different for HDMI where AFE Clock = symbol_clock_freq * 5.

Regards,
Madhav

> 
> Regards,
> Vandita
> >
> > > Please let me know if we have to use math around the port clock and
> > > pass that
> > value, than passing 8x clock.
> > >
> > > Thanks,
> > > Vandita
> > > >
> > > > > +					      dev_priv, &pll_params);
> > > > > +	} else
> > > > >  		ret = icl_calc_dp_combo_pll(dev_priv, clock, &pll_params);
> > > > >
> > > > >  	if (!ret)
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel
> >
> > --
> > Ville Syrjälä
> > Intel
> _______________________________________________
> 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

Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Jani Nikula]

On Fri, 14 Sep 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
>> From: Madhav Chauhan <madhav.chauhan@intel.com>
>> 
>> In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
>> Most of the steps for enabling DPLL are common across DDI
>> and DSI. This patch makes icl_dpll_enable() generic which
>> will be used by all the encoders.
>> 
>> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++++
>>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
>>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
>>  3 files changed, 15 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index cd01a09..2942a24 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
>>  	mutex_lock(&dev_priv->dpll_lock);
>>  
>>  	if (IS_ICELAKE(dev_priv)) {
>> +		enum intel_dpll_id id = pll->info->id;
>> +		i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
>> +
>> +		val = I915_READ(enable_reg);
>> +		val |= PLL_ENABLE;
>> +		I915_WRITE(enable_reg, val);
>> +
>> +		/* TODO: wait times missing from the spec. */
>> +		if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
>> +					    PLL_LOCK, 5))
>> +			DRM_ERROR("PLL %d not locked\n", id);
>> +
>
> I don't really see why this can't stay in the dpll mgr.

Agreed, I think it should stay in DPLL manager.

The thing is, DPLL enabling for DSI requires encoder specific steps in
the middle of the sequence hidden in DPLL manager. It's not pretty to
add that in DPLL manager.

One approach might be to add encoder hooks to call from the right spot
in the DPLL manager, "mid_pll_enable". It's annoying because it would
have to happen in the middle of the platform specific DPLL manager
pll->info->funcs->enable hook. We'd have to call the hooks from platform
specific code, or split those hooks in two. The former is ugly because
it requires passing crtc to the pll enable hook. So I guess add a pll
post enable hook.

Below's some draft code to give an idea what I mean. You'd move the
above hunk to the post hook instead.

So then we'd add mid_pll_enable hooks and do the required magic in the
DSI mid pll hook.

Overall I'm starting to feel the appeal of driving modeset from
encoders, with library style helpers provided from intel_display.c,
instead of adding more and more encoder hooks to do stuff at specific
places. But I digress.

BR,
Jani.


diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index e6cac9225536..a4ca1b4a124c 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -191,6 +191,12 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
 
 	DRM_DEBUG_KMS("enabling %s\n", pll->info->name);
 	pll->info->funcs->enable(dev_priv, pll);
+
+	intel_encoders_mid_pll_enable(crtc); /* pipe_config, old_state? */
+
+	if (pll->info->funcs->post_enable)
+		pll->info->funcs->post_enable(dev_priv, pll);
+
 	pll->on = true;
 
 out:
@@ -3199,6 +3205,7 @@ static void icl_dump_hw_state(struct drm_i915_private *dev_priv,
 
 static const struct intel_shared_dpll_funcs icl_pll_funcs = {
 	.enable = icl_pll_enable,
+	.post_enable = icl_pll_post_enable,
 	.disable = icl_pll_disable,
 	.get_hw_state = icl_pll_get_hw_state,
 };
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index bf0de8a4dc63..eceeef3b8872 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -231,6 +231,16 @@ struct intel_shared_dpll_funcs {
 		       struct intel_shared_dpll *pll);
 
 	/**
+	 * @post_enable:
+	 *
+	 * Optional hook to call after encoder specific mid pll hooks have been
+	 * called from intel_enable_shared_dpll().
+	 */
+	void (*post_enable)(struct drm_i915_private *dev_priv,
+			    struct intel_shared_dpll *pll);
+
+
+	/**
 	 * @disable:
 	 *
 	 * Hook for disabling the pll, called from intel_disable_shared_dpll()



-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Enable ICL DSI PLL (rev2)

[Patchwork]

== Series Details ==

Series: Enable ICL DSI PLL (rev2)
URL   : https://patchwork.freedesktop.org/series/49683/
State : failure

== Summary ==

CALL    scripts/checksyscalls.sh
  DESCEND  objtool
  CHK     include/generated/compile.h
  CC [M]  drivers/gpu/drm/i915/intel_dpll_mgr.o
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function ‘intel_enable_shared_dpll’:
drivers/gpu/drm/i915/intel_dpll_mgr.c:196:2: error: implicit declaration of function ‘intel_encoders_mid_pll_enable’; did you mean ‘intel_power_domains_enable’? [-Werror=implicit-function-declaration]
  intel_encoders_mid_pll_enable(crtc); /* pipe_config, old_state? */
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  intel_power_domains_enable
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function ‘icl_calc_dpll_state’:
drivers/gpu/drm/i915/intel_dpll_mgr.c:2544:42: error: ‘struct intel_dsi’ has no member named ‘bitrate_khz’
   ret = cnl_ddi_calculate_wrpll(intel_dsi->bitrate_khz/5,
                                          ^~
drivers/gpu/drm/i915/intel_dpll_mgr.c: At top level:
drivers/gpu/drm/i915/intel_dpll_mgr.c:3213:17: error: ‘icl_pll_post_enable’ undeclared here (not in a function); did you mean ‘icl_pll_enable’?
  .post_enable = icl_pll_post_enable,
                 ^~~~~~~~~~~~~~~~~~~
                 icl_pll_enable
cc1: all warnings being treated as errors
scripts/Makefile.build:305: recipe for target 'drivers/gpu/drm/i915/intel_dpll_mgr.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_dpll_mgr.o] Error 1
scripts/Makefile.build:546: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:546: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:546: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1050: recipe for target 'drivers' failed
make: *** [drivers] Error 2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Jani Nikula]

On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> On Fri, 14 Sep 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
>>> From: Madhav Chauhan <madhav.chauhan@intel.com>
>>> 
>>> In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
>>> Most of the steps for enabling DPLL are common across DDI
>>> and DSI. This patch makes icl_dpll_enable() generic which
>>> will be used by all the encoders.
>>> 
>>> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
>>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++++
>>>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
>>>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
>>>  3 files changed, 15 insertions(+), 18 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>> index cd01a09..2942a24 100644
>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>> @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
>>>  	mutex_lock(&dev_priv->dpll_lock);
>>>  
>>>  	if (IS_ICELAKE(dev_priv)) {
>>> +		enum intel_dpll_id id = pll->info->id;
>>> +		i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
>>> +
>>> +		val = I915_READ(enable_reg);
>>> +		val |= PLL_ENABLE;
>>> +		I915_WRITE(enable_reg, val);
>>> +
>>> +		/* TODO: wait times missing from the spec. */
>>> +		if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
>>> +					    PLL_LOCK, 5))
>>> +			DRM_ERROR("PLL %d not locked\n", id);
>>> +
>>
>> I don't really see why this can't stay in the dpll mgr.
>
> Agreed, I think it should stay in DPLL manager.
>
> The thing is, DPLL enabling for DSI requires encoder specific steps in
> the middle of the sequence hidden in DPLL manager. It's not pretty to
> add that in DPLL manager.
>
> One approach might be to add encoder hooks to call from the right spot
> in the DPLL manager, "mid_pll_enable". It's annoying because it would
> have to happen in the middle of the platform specific DPLL manager
> pll->info->funcs->enable hook. We'd have to call the hooks from platform
> specific code, or split those hooks in two. The former is ugly because
> it requires passing crtc to the pll enable hook. So I guess add a pll
> post enable hook.
>
> Below's some draft code to give an idea what I mean. You'd move the
> above hunk to the post hook instead.
>
> So then we'd add mid_pll_enable hooks and do the required magic in the
> DSI mid pll hook.

PS. And even with this I'm not yet sure if we can do the overall DPLL
enabling at the right spot wrt bspec DSI mode set sequence. *cringe*.

BR,
Jani.

>
> Overall I'm starting to feel the appeal of driving modeset from
> encoders, with library style helpers provided from intel_display.c,
> instead of adding more and more encoder hooks to do stuff at specific
> places. But I digress.
>
> BR,
> Jani.
>
>
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index e6cac9225536..a4ca1b4a124c 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -191,6 +191,12 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
>  
>  	DRM_DEBUG_KMS("enabling %s\n", pll->info->name);
>  	pll->info->funcs->enable(dev_priv, pll);
> +
> +	intel_encoders_mid_pll_enable(crtc); /* pipe_config, old_state? */
> +
> +	if (pll->info->funcs->post_enable)
> +		pll->info->funcs->post_enable(dev_priv, pll);
> +
>  	pll->on = true;
>  
>  out:
> @@ -3199,6 +3205,7 @@ static void icl_dump_hw_state(struct drm_i915_private *dev_priv,
>  
>  static const struct intel_shared_dpll_funcs icl_pll_funcs = {
>  	.enable = icl_pll_enable,
> +	.post_enable = icl_pll_post_enable,
>  	.disable = icl_pll_disable,
>  	.get_hw_state = icl_pll_get_hw_state,
>  };
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index bf0de8a4dc63..eceeef3b8872 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -231,6 +231,16 @@ struct intel_shared_dpll_funcs {
>  		       struct intel_shared_dpll *pll);
>  
>  	/**
> +	 * @post_enable:
> +	 *
> +	 * Optional hook to call after encoder specific mid pll hooks have been
> +	 * called from intel_enable_shared_dpll().
> +	 */
> +	void (*post_enable)(struct drm_i915_private *dev_priv,
> +			    struct intel_shared_dpll *pll);
> +
> +
> +	/**
>  	 * @disable:
>  	 *
>  	 * Hook for disabling the pll, called from intel_disable_shared_dpll()

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Jani Nikula]

On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote:
>> On Fri, 14 Sep 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>>> On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
>>>> From: Madhav Chauhan <madhav.chauhan@intel.com>
>>>> 
>>>> In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
>>>> Most of the steps for enabling DPLL are common across DDI
>>>> and DSI. This patch makes icl_dpll_enable() generic which
>>>> will be used by all the encoders.
>>>> 
>>>> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
>>>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++++
>>>>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
>>>>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
>>>>  3 files changed, 15 insertions(+), 18 deletions(-)
>>>> 
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index cd01a09..2942a24 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
>>>>  	mutex_lock(&dev_priv->dpll_lock);
>>>>  
>>>>  	if (IS_ICELAKE(dev_priv)) {
>>>> +		enum intel_dpll_id id = pll->info->id;
>>>> +		i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
>>>> +
>>>> +		val = I915_READ(enable_reg);
>>>> +		val |= PLL_ENABLE;
>>>> +		I915_WRITE(enable_reg, val);
>>>> +
>>>> +		/* TODO: wait times missing from the spec. */
>>>> +		if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
>>>> +					    PLL_LOCK, 5))
>>>> +			DRM_ERROR("PLL %d not locked\n", id);
>>>> +
>>>
>>> I don't really see why this can't stay in the dpll mgr.
>>
>> Agreed, I think it should stay in DPLL manager.
>>
>> The thing is, DPLL enabling for DSI requires encoder specific steps in
>> the middle of the sequence hidden in DPLL manager. It's not pretty to
>> add that in DPLL manager.
>>
>> One approach might be to add encoder hooks to call from the right spot
>> in the DPLL manager, "mid_pll_enable". It's annoying because it would
>> have to happen in the middle of the platform specific DPLL manager
>> pll->info->funcs->enable hook. We'd have to call the hooks from platform
>> specific code, or split those hooks in two. The former is ugly because
>> it requires passing crtc to the pll enable hook. So I guess add a pll
>> post enable hook.
>>
>> Below's some draft code to give an idea what I mean. You'd move the
>> above hunk to the post hook instead.
>>
>> So then we'd add mid_pll_enable hooks and do the required magic in the
>> DSI mid pll hook.
>
> PS. And even with this I'm not yet sure if we can do the overall DPLL
> enabling at the right spot wrt bspec DSI mode set sequence. *cringe*.

Ville reminded me that we did have this idea of pushing pll enable calls
down to encoders on DDI platforms. This would help, of course.

BR,
Jani.


>
> BR,
> Jani.
>
>>
>> Overall I'm starting to feel the appeal of driving modeset from
>> encoders, with library style helpers provided from intel_display.c,
>> instead of adding more and more encoder hooks to do stuff at specific
>> places. But I digress.
>>
>> BR,
>> Jani.
>>
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> index e6cac9225536..a4ca1b4a124c 100644
>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> @@ -191,6 +191,12 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
>>  
>>  	DRM_DEBUG_KMS("enabling %s\n", pll->info->name);
>>  	pll->info->funcs->enable(dev_priv, pll);
>> +
>> +	intel_encoders_mid_pll_enable(crtc); /* pipe_config, old_state? */
>> +
>> +	if (pll->info->funcs->post_enable)
>> +		pll->info->funcs->post_enable(dev_priv, pll);
>> +
>>  	pll->on = true;
>>  
>>  out:
>> @@ -3199,6 +3205,7 @@ static void icl_dump_hw_state(struct drm_i915_private *dev_priv,
>>  
>>  static const struct intel_shared_dpll_funcs icl_pll_funcs = {
>>  	.enable = icl_pll_enable,
>> +	.post_enable = icl_pll_post_enable,
>>  	.disable = icl_pll_disable,
>>  	.get_hw_state = icl_pll_get_hw_state,
>>  };
>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> index bf0de8a4dc63..eceeef3b8872 100644
>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> @@ -231,6 +231,16 @@ struct intel_shared_dpll_funcs {
>>  		       struct intel_shared_dpll *pll);
>>  
>>  	/**
>> +	 * @post_enable:
>> +	 *
>> +	 * Optional hook to call after encoder specific mid pll hooks have been
>> +	 * called from intel_enable_shared_dpll().
>> +	 */
>> +	void (*post_enable)(struct drm_i915_private *dev_priv,
>> +			    struct intel_shared_dpll *pll);
>> +
>> +
>> +	/**
>>  	 * @disable:
>>  	 *
>>  	 * Hook for disabling the pll, called from intel_disable_shared_dpll()

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Kulkarni, Vandita]

> -----Original Message-----
> From: Nikula, Jani
> Sent: Wednesday, October 3, 2018 5:11 PM
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Kulkarni, Vandita
> <vandita.kulkarni@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R
> <paulo.r.zanoni@intel.com>
> Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable
> functionality
> 
> On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> > On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> >> On Fri, 14 Sep 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >>> On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
> >>>> From: Madhav Chauhan <madhav.chauhan@intel.com>
> >>>>
> >>>> In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
> >>>> Most of the steps for enabling DPLL are common across DDI and DSI.
> >>>> This patch makes icl_dpll_enable() generic which will be used by
> >>>> all the encoders.
> >>>>
> >>>> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> >>>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++++
> >>>>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
> >>>> drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
> >>>>  3 files changed, 15 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> >>>> b/drivers/gpu/drm/i915/intel_ddi.c
> >>>> index cd01a09..2942a24 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>> @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct
> intel_encoder *encoder,
> >>>>  	mutex_lock(&dev_priv->dpll_lock);
> >>>>
> >>>>  	if (IS_ICELAKE(dev_priv)) {
> >>>> +		enum intel_dpll_id id = pll->info->id;
> >>>> +		i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> >>>> +
> >>>> +		val = I915_READ(enable_reg);
> >>>> +		val |= PLL_ENABLE;
> >>>> +		I915_WRITE(enable_reg, val);
> >>>> +
> >>>> +		/* TODO: wait times missing from the spec. */
> >>>> +		if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
> >>>> +					    PLL_LOCK, 5))
> >>>> +			DRM_ERROR("PLL %d not locked\n", id);
> >>>> +
> >>>
> >>> I don't really see why this can't stay in the dpll mgr.
> >>
> >> Agreed, I think it should stay in DPLL manager.
> >>
> >> The thing is, DPLL enabling for DSI requires encoder specific steps
> >> in the middle of the sequence hidden in DPLL manager. It's not pretty
> >> to add that in DPLL manager.
> >>
> >> One approach might be to add encoder hooks to call from the right
> >> spot in the DPLL manager, "mid_pll_enable". It's annoying because it
> >> would have to happen in the middle of the platform specific DPLL
> >> manager
> >> pll->info->funcs->enable hook. We'd have to call the hooks from
> >> pll->info->funcs->platform
> >> specific code, or split those hooks in two. The former is ugly
> >> because it requires passing crtc to the pll enable hook. So I guess
> >> add a pll post enable hook.
> >>
> >> Below's some draft code to give an idea what I mean. You'd move the
> >> above hunk to the post hook instead.
> >>
> >> So then we'd add mid_pll_enable hooks and do the required magic in
> >> the DSI mid pll hook.

Thanks Jani, let me try this out.

Regards,
Vandita
> >
> > PS. And even with this I'm not yet sure if we can do the overall DPLL
> > enabling at the right spot wrt bspec DSI mode set sequence. *cringe*.
> 
> Ville reminded me that we did have this idea of pushing pll enable calls down to
> encoders on DDI platforms. This would help, of course.
> 
> BR,
> Jani.
> 
> 
> >
> > BR,
> > Jani.
> >
> >>
> >> Overall I'm starting to feel the appeal of driving modeset from
> >> encoders, with library style helpers provided from intel_display.c,
> >> instead of adding more and more encoder hooks to do stuff at specific
> >> places. But I digress.
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> index e6cac9225536..a4ca1b4a124c 100644
> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> @@ -191,6 +191,12 @@ void intel_enable_shared_dpll(struct intel_crtc
> >> *crtc)
> >>
> >>  	DRM_DEBUG_KMS("enabling %s\n", pll->info->name);
> >>  	pll->info->funcs->enable(dev_priv, pll);
> >> +
> >> +	intel_encoders_mid_pll_enable(crtc); /* pipe_config, old_state? */
> >> +
> >> +	if (pll->info->funcs->post_enable)
> >> +		pll->info->funcs->post_enable(dev_priv, pll);
> >> +
> >>  	pll->on = true;
> >>
> >>  out:
> >> @@ -3199,6 +3205,7 @@ static void icl_dump_hw_state(struct
> >> drm_i915_private *dev_priv,
> >>
> >>  static const struct intel_shared_dpll_funcs icl_pll_funcs = {
> >>  	.enable = icl_pll_enable,
> >> +	.post_enable = icl_pll_post_enable,
> >>  	.disable = icl_pll_disable,
> >>  	.get_hw_state = icl_pll_get_hw_state,  }; diff --git
> >> a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> >> b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> >> index bf0de8a4dc63..eceeef3b8872 100644
> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> >> @@ -231,6 +231,16 @@ struct intel_shared_dpll_funcs {
> >>  		       struct intel_shared_dpll *pll);
> >>
> >>  	/**
> >> +	 * @post_enable:
> >> +	 *
> >> +	 * Optional hook to call after encoder specific mid pll hooks have been
> >> +	 * called from intel_enable_shared_dpll().
> >> +	 */
> >> +	void (*post_enable)(struct drm_i915_private *dev_priv,
> >> +			    struct intel_shared_dpll *pll);
> >> +
> >> +
> >> +	/**
> >>  	 * @disable:
> >>  	 *
> >>  	 * Hook for disabling the pll, called from
> >> intel_disable_shared_dpll()
> 
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Jani Nikula]

On Thu, 04 Oct 2018, "Kulkarni, Vandita" <vandita.kulkarni@intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani
>> Sent: Wednesday, October 3, 2018 5:11 PM
>> To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Kulkarni, Vandita
>> <vandita.kulkarni@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R
>> <paulo.r.zanoni@intel.com>
>> Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable
>> functionality
>> 
>> On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote:
>> > On Wed, 03 Oct 2018, Jani Nikula <jani.nikula@intel.com> wrote:
>> >> On Fri, 14 Sep 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> >>> On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
>> >>>> From: Madhav Chauhan <madhav.chauhan@intel.com>
>> >>>>
>> >>>> In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
>> >>>> Most of the steps for enabling DPLL are common across DDI and DSI.
>> >>>> This patch makes icl_dpll_enable() generic which will be used by
>> >>>> all the encoders.
>> >>>>
>> >>>> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
>> >>>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
>> >>>> ---
>> >>>>  drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++++
>> >>>>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
>> >>>> drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
>> >>>>  3 files changed, 15 insertions(+), 18 deletions(-)
>> >>>>
>> >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> >>>> b/drivers/gpu/drm/i915/intel_ddi.c
>> >>>> index cd01a09..2942a24 100644
>> >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> >>>> @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct
>> intel_encoder *encoder,
>> >>>>  	mutex_lock(&dev_priv->dpll_lock);
>> >>>>
>> >>>>  	if (IS_ICELAKE(dev_priv)) {
>> >>>> +		enum intel_dpll_id id = pll->info->id;
>> >>>> +		i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
>> >>>> +
>> >>>> +		val = I915_READ(enable_reg);
>> >>>> +		val |= PLL_ENABLE;
>> >>>> +		I915_WRITE(enable_reg, val);
>> >>>> +
>> >>>> +		/* TODO: wait times missing from the spec. */
>> >>>> +		if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
>> >>>> +					    PLL_LOCK, 5))
>> >>>> +			DRM_ERROR("PLL %d not locked\n", id);
>> >>>> +
>> >>>
>> >>> I don't really see why this can't stay in the dpll mgr.
>> >>
>> >> Agreed, I think it should stay in DPLL manager.
>> >>
>> >> The thing is, DPLL enabling for DSI requires encoder specific steps
>> >> in the middle of the sequence hidden in DPLL manager. It's not pretty
>> >> to add that in DPLL manager.
>> >>
>> >> One approach might be to add encoder hooks to call from the right
>> >> spot in the DPLL manager, "mid_pll_enable". It's annoying because it
>> >> would have to happen in the middle of the platform specific DPLL
>> >> manager
>> >> pll->info->funcs->enable hook. We'd have to call the hooks from
>> >> pll->info->funcs->platform
>> >> specific code, or split those hooks in two. The former is ugly
>> >> because it requires passing crtc to the pll enable hook. So I guess
>> >> add a pll post enable hook.
>> >>
>> >> Below's some draft code to give an idea what I mean. You'd move the
>> >> above hunk to the post hook instead.
>> >>
>> >> So then we'd add mid_pll_enable hooks and do the required magic in
>> >> the DSI mid pll hook.
>
> Thanks Jani, let me try this out.

Like I said, I think Ville preferred pushing the pll enable calls down
to encoders on DDI platforms. In this case, we'd be able to better
control when and where the pll enable happens, and potentially split the
calls or add encoder specific parts into the dpll manager. (There
already exists some.)

BR,
Jani.


>
> Regards,
> Vandita
>> >
>> > PS. And even with this I'm not yet sure if we can do the overall DPLL
>> > enabling at the right spot wrt bspec DSI mode set sequence. *cringe*.
>> 
>> Ville reminded me that we did have this idea of pushing pll enable calls down to
>> encoders on DDI platforms. This would help, of course.
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > BR,
>> > Jani.
>> >
>> >>
>> >> Overall I'm starting to feel the appeal of driving modeset from
>> >> encoders, with library style helpers provided from intel_display.c,
>> >> instead of adding more and more encoder hooks to do stuff at specific
>> >> places. But I digress.
>> >>
>> >> BR,
>> >> Jani.
>> >>
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> >> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> >> index e6cac9225536..a4ca1b4a124c 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> >> @@ -191,6 +191,12 @@ void intel_enable_shared_dpll(struct intel_crtc
>> >> *crtc)
>> >>
>> >>  	DRM_DEBUG_KMS("enabling %s\n", pll->info->name);
>> >>  	pll->info->funcs->enable(dev_priv, pll);
>> >> +
>> >> +	intel_encoders_mid_pll_enable(crtc); /* pipe_config, old_state? */
>> >> +
>> >> +	if (pll->info->funcs->post_enable)
>> >> +		pll->info->funcs->post_enable(dev_priv, pll);
>> >> +
>> >>  	pll->on = true;
>> >>
>> >>  out:
>> >> @@ -3199,6 +3205,7 @@ static void icl_dump_hw_state(struct
>> >> drm_i915_private *dev_priv,
>> >>
>> >>  static const struct intel_shared_dpll_funcs icl_pll_funcs = {
>> >>  	.enable = icl_pll_enable,
>> >> +	.post_enable = icl_pll_post_enable,
>> >>  	.disable = icl_pll_disable,
>> >>  	.get_hw_state = icl_pll_get_hw_state,  }; diff --git
>> >> a/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> >> b/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> >> index bf0de8a4dc63..eceeef3b8872 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> >> @@ -231,6 +231,16 @@ struct intel_shared_dpll_funcs {
>> >>  		       struct intel_shared_dpll *pll);
>> >>
>> >>  	/**
>> >> +	 * @post_enable:
>> >> +	 *
>> >> +	 * Optional hook to call after encoder specific mid pll hooks have been
>> >> +	 * called from intel_enable_shared_dpll().
>> >> +	 */
>> >> +	void (*post_enable)(struct drm_i915_private *dev_priv,
>> >> +			    struct intel_shared_dpll *pll);
>> >> +
>> >> +
>> >> +	/**
>> >>  	 * @disable:
>> >>  	 *
>> >>  	 * Hook for disabling the pll, called from
>> >> intel_disable_shared_dpll()
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx