| Message ID | 20190316004526.12065-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/icl: pass cfgcr* register around instead of pll_id | expand |
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@intel.com> > --- > 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? I thought I changed at least some of these to not read it out directly. > > 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
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@intel.com> >> --- >> 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
On Mon, Mar 18, 2019 at 11:40:34AM -0700, Lucas De Marchi wrote: > 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@intel.com> > >> --- > >> 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. We must have it since bxt is already using it. Either that or bxt is broken.
On Mon, Mar 18, 2019 at 08:53:23PM +0200, Ville Syrjälä wrote: >On Mon, Mar 18, 2019 at 11:40:34AM -0700, Lucas De Marchi wrote: >> 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@intel.com> >> >> --- >> >> 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. > >We must have it since bxt is already using it. Either that or bxt is >broken. humn... I need to double check it then. bxt is the only one not reading the registers and instead using what's saved on dpll_hw_state - in fact hsw, skl, cnl, icl - all read wrpll/spll/cfgcr* registers that they need :-o Lucas De Marchi > >-- >Ville Syrjälä >Intel
On Mon, Mar 18, 2019 at 04:33:51PM -0700, Lucas De Marchi wrote: > On Mon, Mar 18, 2019 at 08:53:23PM +0200, Ville Syrjälä wrote: > >On Mon, Mar 18, 2019 at 11:40:34AM -0700, Lucas De Marchi wrote: > >> 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@intel.com> > >> >> --- > >> >> 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. > > > >We must have it since bxt is already using it. Either that or bxt is > >broken. > > oh.. I think you forgot to push > https://patchwork.freedesktop.org/series/56354/ Ah yes. Now pushed. Thanks for reviewing it. > > I guess it will also make my life easier here. > > Lucas De Marchi > > > > >-- > >Ville Syrjälä > >Intel
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); 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);
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@intel.com> --- 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(-)