Re: [PATCH] drm/i915/icl: pass cfgcr* register around instead of pll_id

[Lucas De Marchi <lucas.demarchi@xxxxxxxxx>]

On Mon, Mar 18, 2019 at 03:31:52PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 15, 2019 at 05:45:26PM -0700, Lucas De Marchi wrote:
> > The caller already knows what platform that is and what register should
> > be used. Instead of keep adding if/else chains on a leaf functions,
> > let the caller pass the register.
> >
> > We read cfgcr0 twice for CNL, but we were already doing that anyway.
> >
> > icl_calc_dp_combo_pll_link() is only used for ICL, but let's keep
> > consistency with cnl_calc_wrpll_link().
> >
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/icl_dsi.c        |  4 +++-
> >  drivers/gpu/drm/i915/intel_ddi.c      | 25 ++++++++++++++-----------
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c |  6 +++---
> >  drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h      |  2 +-
> >  5 files changed, 22 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
> > index beb30d9a855c..28f5da697693 100644
> > --- a/drivers/gpu/drm/i915/icl_dsi.c
> > +++ b/drivers/gpu/drm/i915/icl_dsi.c
> > @@ -1183,7 +1183,9 @@ static void gen11_dsi_get_config(struct intel_encoder *encoder,
> >
> >  	/* 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,
> > +						      ICL_DPLL_CFGCR0(pll_id),
> > +						      ICL_DPLL_CFGCR1(pll_id));
> >  	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 69aa0d148795..24675ef8b262 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1304,18 +1304,13 @@ static int skl_calc_wrpll_link(struct drm_i915_private *dev_priv,
> >  }
> >
> >  int cnl_calc_wrpll_link(struct drm_i915_private *dev_priv,
> > -			enum intel_dpll_id pll_id)
> > +			i915_reg_t cfgcr0_reg, i915_reg_t cfgcr1_reg)
> >  {
> >  	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));
> > -	}
> > +	cfgcr0 = I915_READ(cfgcr0_reg);
> > +	cfgcr1 = I915_READ(cfgcr1_reg);
>
> Don't we alredy have the dpll state read out at this point?

nops.

> I thought I changed at least some of these to not read it out
> directly.

What we have is that for ICL we only read it on the
cnl_calc_wrpll_link(). For CNL we read both on cnl_calc_wrpll_link() and
on the ddi_clock_get() function - at least cfgcr0, because we need it
to decide which function to call. 

If you prefer the read to be on the ddi_clock_get() what I could do is
to pass the register values.

Lucas De Marchi

> >  	p0 = cfgcr1 & DPLL_CFGCR1_PDIV_MASK;
> >  	p2 = cfgcr1 & DPLL_CFGCR1_KDIV_MASK;
> > @@ -1471,17 +1466,23 @@ 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);
> > +	i915_reg_t cfgcr0_reg, cfgcr1_reg;
> >  	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);
> > +	cfgcr0_reg = ICL_DPLL_CFGCR0(pll_id);
> > +	cfgcr1_reg = ICL_DPLL_CFGCR1(pll_id);
> > +
> >  	if (intel_port_is_combophy(dev_priv, port)) {
> >  		if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
> > -			link_clock = cnl_calc_wrpll_link(dev_priv, pll_id);
> > +			link_clock = cnl_calc_wrpll_link(dev_priv, cfgcr0_reg,
> > +							 cfgcr1_reg);
> >  		else
> >  			link_clock = icl_calc_dp_combo_pll_link(dev_priv,
> > -								pll_id);
> > +								cfgcr0_reg,
> > +								cfgcr1_reg);
> >  	} else {
> >  		if (pll_id == DPLL_ID_ICL_TBTPLL)
> >  			link_clock = icl_calc_tbt_pll_link(dev_priv, port);
> > @@ -1506,7 +1507,9 @@ static void cnl_ddi_clock_get(struct intel_encoder *encoder,
> >  	cfgcr0 = I915_READ(CNL_DPLL_CFGCR0(pll_id));
> >
> >  	if (cfgcr0 & DPLL_CFGCR0_HDMI_MODE) {
> > -		link_clock = cnl_calc_wrpll_link(dev_priv, pll_id);
> > +		link_clock = cnl_calc_wrpll_link(dev_priv,
> > +						 CNL_DPLL_CFGCR0(pll_id),
> > +						 CNL_DPLL_CFGCR1(pll_id));
> >  	} else {
> >  		link_clock = cfgcr0 & DPLL_CFGCR0_LINK_RATE_MASK;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index ca975213da2a..ed4024f4394b 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -2569,7 +2569,7 @@ static bool icl_calc_dpll_state(struct intel_crtc_state *crtc_state,
> >  }
> >
> >  int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
> > -			       u32 pll_id)
> > +			       i915_reg_t cfgcr0_reg, i915_reg_t cfgcr1_reg)
> >  {
> >  	u32 cfgcr0, cfgcr1;
> >  	u32 pdiv, kdiv, qdiv_mode, qdiv_ratio, dco_integer, dco_fraction;
> > @@ -2577,8 +2577,8 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
> >  	int index, n_entries, link_clock;
> >
> >  	/* Read back values from DPLL CFGCR registers */
> > -	cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(pll_id));
> > -	cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(pll_id));
> > +	cfgcr0 = I915_READ(cfgcr0_reg);
> > +	cfgcr1 = I915_READ(cfgcr1_reg);
> >
> >  	dco_integer = cfgcr0 & DPLL_CFGCR0_DCO_INTEGER_MASK;
> >  	dco_fraction = (cfgcr0 & DPLL_CFGCR0_DCO_FRACTION_MASK) >>
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > index 40e8391a92f2..0aa504e9bfbf 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > @@ -342,7 +342,7 @@ void intel_shared_dpll_init(struct drm_device *dev);
> >  void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv,
> >  			      struct intel_dpll_hw_state *hw_state);
> >  int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
> > -			       u32 pll_id);
> > +			       i915_reg_t cfgcr0_reg, i915_reg_t cfgcr1_reg);
> >  int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
> >  enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port);
> >  bool intel_dpll_is_combophy(enum intel_dpll_id id);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index d9f188ef21f4..9d0bf1914773 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);
> > +			i915_reg_t cgfcr0_reg, i915_reg_t cgfcr1_reg);
> >
> >  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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx