Re: [PATCH 2/3] drm/i915/cnl: use previous pll hw readout

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Mar 21, 2019 at 03:02:56PM -0700, Lucas De Marchi wrote:
> By the time cnl_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.
> 
> This also affects the code for ICL since it partially reuses the CNL
> code. However the more intricate part on ICL is left for another patch.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/icl_dsi.c   |  5 ++-
>  drivers/gpu/drm/i915/intel_ddi.c | 53 +++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h |  2 +-
>  3 files changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
> index beb30d9a855c..c7dcd783d986 100644
> --- a/drivers/gpu/drm/i915/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/icl_dsi.c
> @@ -1179,11 +1179,10 @@ static void gen11_dsi_get_config(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> -	u32 pll_id;
>  
>  	/* FIXME: adapt icl_ddi_clock_get() for DSI and use that? */
> -	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
> -	pipe_config->port_clock = cnl_calc_wrpll_link(dev_priv, pll_id);
> +	pipe_config->port_clock =
> +		cnl_calc_wrpll_link(dev_priv, &pipe_config->dpll_hw_state);
>  	pipe_config->base.adjusted_mode.crtc_clock = intel_dsi->pclk;
>  	pipe_config->output_types |= BIT(INTEL_OUTPUT_DSI);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 644541924208..fe52af9fa4aa 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1294,25 +1294,17 @@ static int skl_calc_wrpll_link(struct intel_dpll_hw_state *state)
>  	return dco_freq / (p0 * p1 * p2 * 5);
>  }
>  
> +

Stray newline.

>  int cnl_calc_wrpll_link(struct drm_i915_private *dev_priv,
> -			enum intel_dpll_id pll_id)
> +			struct intel_dpll_hw_state *state)
>  {
> -	u32 cfgcr0, cfgcr1;
>  	u32 p0, p1, p2, dco_freq, ref_clock;
>  
> -	if (INTEL_GEN(dev_priv) >= 11) {
> -		cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(pll_id));
> -		cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(pll_id));
> -	} else {
> -		cfgcr0 = I915_READ(CNL_DPLL_CFGCR0(pll_id));
> -		cfgcr1 = I915_READ(CNL_DPLL_CFGCR1(pll_id));
> -	}
> -
> -	p0 = cfgcr1 & DPLL_CFGCR1_PDIV_MASK;
> -	p2 = cfgcr1 & DPLL_CFGCR1_KDIV_MASK;
> +	p0 = state->cfgcr1 & DPLL_CFGCR1_PDIV_MASK;
> +	p2 = state->cfgcr1 & DPLL_CFGCR1_KDIV_MASK;
>  
> -	if (cfgcr1 & DPLL_CFGCR1_QDIV_MODE(1))
> -		p1 = (cfgcr1 & DPLL_CFGCR1_QDIV_RATIO_MASK) >>
> +	if (state->cfgcr1 & DPLL_CFGCR1_QDIV_MODE(1))
> +		p1 = (state->cfgcr1 & DPLL_CFGCR1_QDIV_RATIO_MASK) >>
>  			DPLL_CFGCR1_QDIV_RATIO_SHIFT;
>  	else
>  		p1 = 1;
> @@ -1347,9 +1339,9 @@ int cnl_calc_wrpll_link(struct drm_i915_private *dev_priv,
>  
>  	ref_clock = cnl_hdmi_pll_ref_clock(dev_priv);
>  
> -	dco_freq = (cfgcr0 & DPLL_CFGCR0_DCO_INTEGER_MASK) * ref_clock;
> +	dco_freq = (state->cfgcr0 & DPLL_CFGCR0_DCO_INTEGER_MASK) * ref_clock;
>  
> -	dco_freq += (((cfgcr0 & DPLL_CFGCR0_DCO_FRACTION_MASK) >>
> +	dco_freq += (((state->cfgcr0 & DPLL_CFGCR0_DCO_FRACTION_MASK) >>
>  		      DPLL_CFGCR0_DCO_FRACTION_SHIFT) * ref_clock) / 0x8000;
>  
>  	if (WARN_ON(p0 == 0 || p1 == 0 || p2 == 0))
> @@ -1462,13 +1454,21 @@ static void icl_ddi_clock_get(struct intel_encoder *encoder,
>  			      struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	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))
> +		goto end;
> +
> +	state = &pipe_config->dpll_hw_state;

No point in assigning this late.

> +
>  	if (intel_port_is_combophy(dev_priv, port)) {
> -		link_clock = cnl_calc_wrpll_link(dev_priv, pll_id);
> +		link_clock = cnl_calc_wrpll_link(dev_priv, state);
>  	} else {
>  		if (pll_id == DPLL_ID_ICL_TBTPLL)
>  			link_clock = icl_calc_tbt_pll_link(dev_priv, port);
> @@ -1476,7 +1476,9 @@ static void icl_ddi_clock_get(struct intel_encoder *encoder,
>  			link_clock = icl_calc_mg_pll_link(dev_priv, port);
>  	}
>  
> +end:
>  	pipe_config->port_clock = link_clock;
> +
>  	ddi_dotclock_get(pipe_config);
>  }
>  
> @@ -1484,18 +1486,18 @@ static void cnl_ddi_clock_get(struct intel_encoder *encoder,
>  			      struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_dpll_hw_state *state;
>  	int link_clock = 0;
> -	u32 cfgcr0;
> -	enum intel_dpll_id pll_id;
> -
> -	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
>  
> -	cfgcr0 = I915_READ(CNL_DPLL_CFGCR0(pll_id));
> +	/* For DDI ports we always use a shared PLL. */
> +	if (WARN_ON(!pipe_config->shared_dpll))
> +		goto end;
>  
> -	if (cfgcr0 & DPLL_CFGCR0_HDMI_MODE) {
> -		link_clock = cnl_calc_wrpll_link(dev_priv, pll_id);
> +	state = &pipe_config->dpll_hw_state;

ditto

> +	if (state->cfgcr0 & DPLL_CFGCR0_HDMI_MODE) {
> +		link_clock = cnl_calc_wrpll_link(dev_priv, state);
>  	} else {
> -		link_clock = cfgcr0 & DPLL_CFGCR0_LINK_RATE_MASK;
> +		link_clock = state->cfgcr0 & DPLL_CFGCR0_LINK_RATE_MASK;
>  
>  		switch (link_clock) {
>  		case DPLL_CFGCR0_LINK_RATE_810:
> @@ -1529,6 +1531,7 @@ static void cnl_ddi_clock_get(struct intel_encoder *encoder,
>  		link_clock *= 2;
>  	}
>  
> +end:
>  	pipe_config->port_clock = link_clock;
>  
>  	ddi_dotclock_get(pipe_config);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4d7ae579fc92..65ab0f90ce6a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1659,7 +1659,7 @@ int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder,
>  				     bool enable);
>  void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder);
>  int cnl_calc_wrpll_link(struct drm_i915_private *dev_priv,
> -			enum intel_dpll_id pll_id);
> +			struct intel_dpll_hw_state *state);
>  
>  unsigned int intel_fb_align_height(const struct drm_framebuffer *fb,
>  				   int color_plane, unsigned int height);
> -- 
> 2.20.1

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