diff mbox series

[3/3] drm/i915/icl: use previous pll hw readout

Message ID 20190321220257.32638-4-lucas.demarchi@intel.com (mailing list archive)
State New, archived
Headers show
Series Do not re-read dpll registers | expand

Commit Message

Lucas De Marchi March 21, 2019, 10:02 p.m. UTC 
By the time icl_ddi_clock_get() is called we've just got the hw state
from the pll registers. We don't need to read them again: we can rather
reuse what was cached in the dpll_hw_state.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 39 +++++++++++++++-----------------
 1 file changed, 18 insertions(+), 21 deletions(-)

Comments

Ville Syrjälä March 22, 2019, 1:09 p.m. UTC | #1 
On Thu, Mar 21, 2019 at 03:02:57PM -0700, Lucas De Marchi wrote:
> By the time icl_ddi_clock_get() is called we've just got the hw state
> from the pll registers. We don't need to read them again: we can rather
> reuse what was cached in the dpll_hw_state.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 39 +++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index fe52af9fa4aa..0ea5a97dfe9d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1372,26 +1372,20 @@ static int icl_calc_tbt_pll_link(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static int icl_calc_mg_pll_link(struct drm_i915_private *dev_priv,
> -				enum port port)
> +
> +static int icl_calc_mg_pll_link(struct intel_dpll_hw_state *state, u32 refclk)

Inconsistent with the cnl variant.

>  {
> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> -	u32 mg_pll_div0, mg_clktop_hsclkctl;
> -	u32 m1, m2_int, m2_frac, div1, div2, refclk;
> +	u32 m1, m2_int, m2_frac, div1, div2;
>  	u64 tmp;
>  
> -	refclk = dev_priv->cdclk.hw.ref;
> -
> -	mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
> -	mg_clktop_hsclkctl = I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
> -
> -	m1 = I915_READ(MG_PLL_DIV1(tc_port)) & MG_PLL_DIV1_FBPREDIV_MASK;
> -	m2_int = mg_pll_div0 & MG_PLL_DIV0_FBDIV_INT_MASK;
> -	m2_frac = (mg_pll_div0 & MG_PLL_DIV0_FRACNEN_H) ?
> -		  (mg_pll_div0 & MG_PLL_DIV0_FBDIV_FRAC_MASK) >>
> +	m1 = state->mg_pll_div1 & MG_PLL_DIV1_FBPREDIV_MASK;
> +	m2_int = state->mg_pll_div0 & MG_PLL_DIV0_FBDIV_INT_MASK;
> +	m2_frac = (state->mg_pll_div0 & MG_PLL_DIV0_FRACNEN_H) ?
> +		  (state->mg_pll_div0 & MG_PLL_DIV0_FBDIV_FRAC_MASK) >>
>  		  MG_PLL_DIV0_FBDIV_FRAC_SHIFT : 0;
>  
> -	switch (mg_clktop_hsclkctl & MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK) {
> +	switch (state->mg_clktop2_hsclkctl &
> +		MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK) {
>  	case MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_2:
>  		div1 = 2;
>  		break;
> @@ -1405,12 +1399,14 @@ static int icl_calc_mg_pll_link(struct drm_i915_private *dev_priv,
>  		div1 = 7;
>  		break;
>  	default:
> -		MISSING_CASE(mg_clktop_hsclkctl);
> +		MISSING_CASE(state->mg_clktop2_hsclkctl);
>  		return 0;
>  	}
>  
> -	div2 = (mg_clktop_hsclkctl & MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK) >>
> +	div2 = (state->mg_clktop2_hsclkctl &
> +		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK) >>
>  		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_SHIFT;
> +
>  	/* div2 value of 0 is same as 1 means no div */
>  	if (div2 == 0)
>  		div2 = 1;
> @@ -1457,9 +1453,6 @@ static void icl_ddi_clock_get(struct intel_encoder *encoder,
>  	struct intel_dpll_hw_state *state;
>  	enum port port = encoder->port;
>  	int link_clock = 0;
> -	u32 pll_id;
> -
> -	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
>  
>  	/* For DDI ports we always use a shared PLL. */
>  	if (WARN_ON(!pipe_config->shared_dpll))
> @@ -1470,10 +1463,14 @@ static void icl_ddi_clock_get(struct intel_encoder *encoder,
>  	if (intel_port_is_combophy(dev_priv, port)) {
>  		link_clock = cnl_calc_wrpll_link(dev_priv, state);
>  	} else {
> +		enum intel_dpll_id pll_id = intel_get_shared_dpll_id(dev_priv,
> +				pipe_config->shared_dpll);
> +
>  		if (pll_id == DPLL_ID_ICL_TBTPLL)
>  			link_clock = icl_calc_tbt_pll_link(dev_priv, port);
>  		else
> -			link_clock = icl_calc_mg_pll_link(dev_priv, port);
> +			link_clock = icl_calc_mg_pll_link(
> +					state, dev_priv->cdclk.hw.ref);
>  	}
>  
>  end:
> -- 
> 2.20.1
Ville Syrjälä March 22, 2019, 8:15 p.m. UTC | #2 
On Fri, Mar 22, 2019 at 12:50:07PM -0700, Lucas De Marchi wrote:
> On Fri, Mar 22, 2019 at 03:09:59PM +0200, Ville Syrjälä wrote:
> >On Thu, Mar 21, 2019 at 03:02:57PM -0700, Lucas De Marchi wrote:
> >> By the time icl_ddi_clock_get() is called we've just got the hw state
> >> from the pll registers. We don't need to read them again: we can rather
> >> reuse what was cached in the dpll_hw_state.
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_ddi.c | 39 +++++++++++++++-----------------
> >>  1 file changed, 18 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index fe52af9fa4aa..0ea5a97dfe9d 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -1372,26 +1372,20 @@ static int icl_calc_tbt_pll_link(struct drm_i915_private *dev_priv,
> >>  	}
> >>  }
> >>
> >> -static int icl_calc_mg_pll_link(struct drm_i915_private *dev_priv,
> >> -				enum port port)
> >> +
> >> +static int icl_calc_mg_pll_link(struct intel_dpll_hw_state *state, u32 refclk)
> >
> >Inconsistent with the cnl variant.
> 
> 
> see below
> 
> >
> >>  {
> >> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> >> -	u32 mg_pll_div0, mg_clktop_hsclkctl;
> >> -	u32 m1, m2_int, m2_frac, div1, div2, refclk;
> >> +	u32 m1, m2_int, m2_frac, div1, div2;
> >>  	u64 tmp;
> >>
> >> -	refclk = dev_priv->cdclk.hw.ref;
> 
> because this one needs the refclk. If i don't add refclk to the
> arguments I need to leave dev_priv there and it will still not be
> consistent. What do you suggest?

Didn't cnl pass in the dev_priv also for this exact same reason?

> 
> Lucas De Marchi
> 
> >> -
> >> -	mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
> >> -	mg_clktop_hsclkctl = I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
> >> -
> >> -	m1 = I915_READ(MG_PLL_DIV1(tc_port)) & MG_PLL_DIV1_FBPREDIV_MASK;
> >> -	m2_int = mg_pll_div0 & MG_PLL_DIV0_FBDIV_INT_MASK;
> >> -	m2_frac = (mg_pll_div0 & MG_PLL_DIV0_FRACNEN_H) ?
> >> -		  (mg_pll_div0 & MG_PLL_DIV0_FBDIV_FRAC_MASK) >>
> >> +	m1 = state->mg_pll_div1 & MG_PLL_DIV1_FBPREDIV_MASK;
> >> +	m2_int = state->mg_pll_div0 & MG_PLL_DIV0_FBDIV_INT_MASK;
> >> +	m2_frac = (state->mg_pll_div0 & MG_PLL_DIV0_FRACNEN_H) ?
> >> +		  (state->mg_pll_div0 & MG_PLL_DIV0_FBDIV_FRAC_MASK) >>
> >>  		  MG_PLL_DIV0_FBDIV_FRAC_SHIFT : 0;
> >>
> >> -	switch (mg_clktop_hsclkctl & MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK) {
> >> +	switch (state->mg_clktop2_hsclkctl &
> >> +		MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK) {
> >>  	case MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_2:
> >>  		div1 = 2;
> >>  		break;
> >> @@ -1405,12 +1399,14 @@ static int icl_calc_mg_pll_link(struct drm_i915_private *dev_priv,
> >>  		div1 = 7;
> >>  		break;
> >>  	default:
> >> -		MISSING_CASE(mg_clktop_hsclkctl);
> >> +		MISSING_CASE(state->mg_clktop2_hsclkctl);
> >>  		return 0;
> >>  	}
> >>
> >> -	div2 = (mg_clktop_hsclkctl & MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK) >>
> >> +	div2 = (state->mg_clktop2_hsclkctl &
> >> +		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK) >>
> >>  		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_SHIFT;
> >> +
> >>  	/* div2 value of 0 is same as 1 means no div */
> >>  	if (div2 == 0)
> >>  		div2 = 1;
> >> @@ -1457,9 +1453,6 @@ static void icl_ddi_clock_get(struct intel_encoder *encoder,
> >>  	struct intel_dpll_hw_state *state;
> >>  	enum port port = encoder->port;
> >>  	int link_clock = 0;
> >> -	u32 pll_id;
> >> -
> >> -	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
> >>
> >>  	/* For DDI ports we always use a shared PLL. */
> >>  	if (WARN_ON(!pipe_config->shared_dpll))
> >> @@ -1470,10 +1463,14 @@ static void icl_ddi_clock_get(struct intel_encoder *encoder,
> >>  	if (intel_port_is_combophy(dev_priv, port)) {
> >>  		link_clock = cnl_calc_wrpll_link(dev_priv, state);
> >>  	} else {
> >> +		enum intel_dpll_id pll_id = intel_get_shared_dpll_id(dev_priv,
> >> +				pipe_config->shared_dpll);
> >> +
> >>  		if (pll_id == DPLL_ID_ICL_TBTPLL)
> >>  			link_clock = icl_calc_tbt_pll_link(dev_priv, port);
> >>  		else
> >> -			link_clock = icl_calc_mg_pll_link(dev_priv, port);
> >> +			link_clock = icl_calc_mg_pll_link(
> >> +					state, dev_priv->cdclk.hw.ref);
> >>  	}
> >>
> >>  end:
> >> --
> >> 2.20.1
> >
> >-- 
> >Ville Syrjälä
> >Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index fe52af9fa4aa..0ea5a97dfe9d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1372,26 +1372,20 @@  static int icl_calc_tbt_pll_link(struct drm_i915_private *dev_priv,
 	}
 }
 
-static int icl_calc_mg_pll_link(struct drm_i915_private *dev_priv,
-				enum port port)
+
+static int icl_calc_mg_pll_link(struct intel_dpll_hw_state *state, u32 refclk)
 {
-	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
-	u32 mg_pll_div0, mg_clktop_hsclkctl;
-	u32 m1, m2_int, m2_frac, div1, div2, refclk;
+	u32 m1, m2_int, m2_frac, div1, div2;
 	u64 tmp;
 
-	refclk = dev_priv->cdclk.hw.ref;
-
-	mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
-	mg_clktop_hsclkctl = I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
-
-	m1 = I915_READ(MG_PLL_DIV1(tc_port)) & MG_PLL_DIV1_FBPREDIV_MASK;
-	m2_int = mg_pll_div0 & MG_PLL_DIV0_FBDIV_INT_MASK;
-	m2_frac = (mg_pll_div0 & MG_PLL_DIV0_FRACNEN_H) ?
-		  (mg_pll_div0 & MG_PLL_DIV0_FBDIV_FRAC_MASK) >>
+	m1 = state->mg_pll_div1 & MG_PLL_DIV1_FBPREDIV_MASK;
+	m2_int = state->mg_pll_div0 & MG_PLL_DIV0_FBDIV_INT_MASK;
+	m2_frac = (state->mg_pll_div0 & MG_PLL_DIV0_FRACNEN_H) ?
+		  (state->mg_pll_div0 & MG_PLL_DIV0_FBDIV_FRAC_MASK) >>
 		  MG_PLL_DIV0_FBDIV_FRAC_SHIFT : 0;
 
-	switch (mg_clktop_hsclkctl & MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK) {
+	switch (state->mg_clktop2_hsclkctl &
+		MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK) {
 	case MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_2:
 		div1 = 2;
 		break;
@@ -1405,12 +1399,14 @@  static int icl_calc_mg_pll_link(struct drm_i915_private *dev_priv,
 		div1 = 7;
 		break;
 	default:
-		MISSING_CASE(mg_clktop_hsclkctl);
+		MISSING_CASE(state->mg_clktop2_hsclkctl);
 		return 0;
 	}
 
-	div2 = (mg_clktop_hsclkctl & MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK) >>
+	div2 = (state->mg_clktop2_hsclkctl &
+		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK) >>
 		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_SHIFT;
+
 	/* div2 value of 0 is same as 1 means no div */
 	if (div2 == 0)
 		div2 = 1;
@@ -1457,9 +1453,6 @@  static void icl_ddi_clock_get(struct intel_encoder *encoder,
 	struct intel_dpll_hw_state *state;
 	enum port port = encoder->port;
 	int link_clock = 0;
-	u32 pll_id;
-
-	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
 
 	/* For DDI ports we always use a shared PLL. */
 	if (WARN_ON(!pipe_config->shared_dpll))
@@ -1470,10 +1463,14 @@  static void icl_ddi_clock_get(struct intel_encoder *encoder,
 	if (intel_port_is_combophy(dev_priv, port)) {
 		link_clock = cnl_calc_wrpll_link(dev_priv, state);
 	} else {
+		enum intel_dpll_id pll_id = intel_get_shared_dpll_id(dev_priv,
+				pipe_config->shared_dpll);
+
 		if (pll_id == DPLL_ID_ICL_TBTPLL)
 			link_clock = icl_calc_tbt_pll_link(dev_priv, port);
 		else
-			link_clock = icl_calc_mg_pll_link(dev_priv, port);
+			link_clock = icl_calc_mg_pll_link(
+					state, dev_priv->cdclk.hw.ref);
 	}
 
 end: