All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Do not re-read dpll registers
@ 2019-03-21 22:02 Lucas De Marchi
  2019-03-21 22:02 ` [PATCH 1/3] drm/i915/skl: use previous pll hw readout Lucas De Marchi
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Lucas De Marchi @ 2019-03-21 22:02 UTC (permalink / raw)
  To: intel-gfx

Instead of re-reading the registers we just read on the hw state
readout, use the values saved on intel_shared_dpll. Besides not doing
the MMIO, this helps on sharing code since we don't have to
differentiate e.g. ICL and CNL because they have different registers for
the same thing.

I'm a little hesitant wrt DSI. It seems we have completly different
implementations for ICL and gen <= 11. BXT has another hook so is not
affected, but I'm not sure if we have any other gens with DSI that share
the skl hooks.

Lucas De Marchi (3):
  drm/i915/skl: use previous pll hw readout
  drm/i915/cnl: use previous pll hw readout
  drm/i915/icl: use previous pll hw readout

 drivers/gpu/drm/i915/icl_dsi.c   |   5 +-
 drivers/gpu/drm/i915/intel_ddi.c | 139 +++++++++++++++----------------
 drivers/gpu/drm/i915/intel_drv.h |   2 +-
 3 files changed, 70 insertions(+), 76 deletions(-)

-- 
2.20.1

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 1/3] drm/i915/skl: use previous pll hw readout
  2019-03-21 22:02 [PATCH 0/3] Do not re-read dpll registers Lucas De Marchi
@ 2019-03-21 22:02 ` Lucas De Marchi
  2019-03-22 13:05   ` Ville Syrjälä
  2019-03-21 22:02 ` [PATCH 2/3] drm/i915/cnl: " Lucas De Marchi
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Lucas De Marchi @ 2019-03-21 22:02 UTC (permalink / raw)
  To: intel-gfx

By the time skl_ddi_clock_get() is called - and thus
skl_calc_wrpll_link() - 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 | 49 ++++++++++++++------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 933df3a57a8a..644541924208 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1240,24 +1240,15 @@ static int hsw_ddi_calc_wrpll_link(struct drm_i915_private *dev_priv,
 	return (refclk * n * 100) / (p * r);
 }
 
-static int skl_calc_wrpll_link(struct drm_i915_private *dev_priv,
-			       enum intel_dpll_id pll_id)
+static int skl_calc_wrpll_link(struct intel_dpll_hw_state *state)
 {
-	i915_reg_t cfgcr1_reg, cfgcr2_reg;
-	u32 cfgcr1_val, cfgcr2_val;
 	u32 p0, p1, p2, dco_freq;
 
-	cfgcr1_reg = DPLL_CFGCR1(pll_id);
-	cfgcr2_reg = DPLL_CFGCR2(pll_id);
+	p0 = state->cfgcr2 & DPLL_CFGCR2_PDIV_MASK;
+	p2 = state->cfgcr2 & DPLL_CFGCR2_KDIV_MASK;
 
-	cfgcr1_val = I915_READ(cfgcr1_reg);
-	cfgcr2_val = I915_READ(cfgcr2_reg);
-
-	p0 = cfgcr2_val & DPLL_CFGCR2_PDIV_MASK;
-	p2 = cfgcr2_val & DPLL_CFGCR2_KDIV_MASK;
-
-	if (cfgcr2_val &  DPLL_CFGCR2_QDIV_MODE(1))
-		p1 = (cfgcr2_val & DPLL_CFGCR2_QDIV_RATIO_MASK) >> 8;
+	if (state->cfgcr2 &  DPLL_CFGCR2_QDIV_MODE(1))
+		p1 = (state->cfgcr2 & DPLL_CFGCR2_QDIV_RATIO_MASK) >> 8;
 	else
 		p1 = 1;
 
@@ -1292,10 +1283,10 @@ static int skl_calc_wrpll_link(struct drm_i915_private *dev_priv,
 		break;
 	}
 
-	dco_freq = (cfgcr1_val & DPLL_CFGCR1_DCO_INTEGER_MASK) * 24 * 1000;
+	dco_freq = (state->cfgcr1 & DPLL_CFGCR1_DCO_INTEGER_MASK) * 24 * 1000;
 
-	dco_freq += (((cfgcr1_val & DPLL_CFGCR1_DCO_FRACTION_MASK) >> 9) * 24 *
-		1000) / 0x8000;
+	dco_freq += (((state->cfgcr1 & DPLL_CFGCR1_DCO_FRACTION_MASK) >> 9)
+		     * 24 * 1000) / 0x8000;
 
 	if (WARN_ON(p0 == 0 || p1 == 0 || p2 == 0))
 		return 0;
@@ -1546,20 +1537,23 @@ static void cnl_ddi_clock_get(struct intel_encoder *encoder,
 static void skl_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 dpll_ctl1;
-	enum intel_dpll_id pll_id;
 
-	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
-
-	dpll_ctl1 = I915_READ(DPLL_CTRL1);
+	/* For DDI ports we always use a shared PLL. */
+	if (WARN_ON(!pipe_config->shared_dpll))
+		goto end;
 
-	if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(pll_id)) {
-		link_clock = skl_calc_wrpll_link(dev_priv, pll_id);
+	/*
+	 * ctrl1 register is already shifted for each pll, just use 0 to get
+	 * the internal shift for each field
+	 */
+	state = &pipe_config->dpll_hw_state;
+	if (state->ctrl1 & DPLL_CTRL1_HDMI_MODE(0)) {
+		link_clock = skl_calc_wrpll_link(state);
 	} else {
-		link_clock = dpll_ctl1 & DPLL_CTRL1_LINK_RATE_MASK(pll_id);
-		link_clock >>= DPLL_CTRL1_LINK_RATE_SHIFT(pll_id);
+		link_clock = state->ctrl1 & DPLL_CTRL1_LINK_RATE_MASK(0);
+		link_clock >>= DPLL_CTRL1_LINK_RATE_SHIFT(0);
 
 		switch (link_clock) {
 		case DPLL_CTRL1_LINK_RATE_810:
@@ -1587,6 +1581,7 @@ static void skl_ddi_clock_get(struct intel_encoder *encoder,
 		link_clock *= 2;
 	}
 
+end:
 	pipe_config->port_clock = link_clock;
 
 	ddi_dotclock_get(pipe_config);
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 2/3] drm/i915/cnl: use previous pll hw readout
  2019-03-21 22:02 [PATCH 0/3] Do not re-read dpll registers Lucas De Marchi
  2019-03-21 22:02 ` [PATCH 1/3] drm/i915/skl: use previous pll hw readout Lucas De Marchi
@ 2019-03-21 22:02 ` Lucas De Marchi
  2019-03-22 13:09   ` Ville Syrjälä
  2019-03-21 22:02 ` [PATCH 3/3] drm/i915/icl: " Lucas De Marchi
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Lucas De Marchi @ 2019-03-21 22:02 UTC (permalink / raw)
  To: intel-gfx

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);
 }
 
+
 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;
+
 	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;
+	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

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 3/3] drm/i915/icl: use previous pll hw readout
  2019-03-21 22:02 [PATCH 0/3] Do not re-read dpll registers Lucas De Marchi
  2019-03-21 22:02 ` [PATCH 1/3] drm/i915/skl: use previous pll hw readout Lucas De Marchi
  2019-03-21 22:02 ` [PATCH 2/3] drm/i915/cnl: " Lucas De Marchi
@ 2019-03-21 22:02 ` Lucas De Marchi
  2019-03-22 13:09   ` Ville Syrjälä
  2019-03-22  1:37 ` ✗ Fi.CI.CHECKPATCH: warning for Do not re-read dpll registers Patchwork
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Lucas De Marchi @ 2019-03-21 22:02 UTC (permalink / raw)
  To: intel-gfx

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)
 {
-	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

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for Do not re-read dpll registers
  2019-03-21 22:02 [PATCH 0/3] Do not re-read dpll registers Lucas De Marchi
                   ` (2 preceding siblings ...)
  2019-03-21 22:02 ` [PATCH 3/3] drm/i915/icl: " Lucas De Marchi
@ 2019-03-22  1:37 ` Patchwork
  2019-03-22  2:05 ` ✓ Fi.CI.BAT: success " Patchwork
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2019-03-22  1:37 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

== Series Details ==

Series: Do not re-read dpll registers
URL   : https://patchwork.freedesktop.org/series/58382/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
b8b9a3a6bb07 drm/i915/skl: use previous pll hw readout
8b862f77d44e drm/i915/cnl: use previous pll hw readout
-:41: CHECK:LINE_SPACING: Please don't use multiple blank lines
#41: FILE: drivers/gpu/drm/i915/intel_ddi.c:1297:
 
+

total: 0 errors, 0 warnings, 1 checks, 127 lines checked
b807a4b76103 drm/i915/icl: use previous pll hw readout
-:22: CHECK:LINE_SPACING: Please don't use multiple blank lines
#22: FILE: drivers/gpu/drm/i915/intel_ddi.c:1375:
 
+

-:90: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#90: FILE: drivers/gpu/drm/i915/intel_ddi.c:1472:
+			link_clock = icl_calc_mg_pll_link(

total: 0 errors, 0 warnings, 2 checks, 75 lines checked

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* ✓ Fi.CI.BAT: success for Do not re-read dpll registers
  2019-03-21 22:02 [PATCH 0/3] Do not re-read dpll registers Lucas De Marchi
                   ` (3 preceding siblings ...)
  2019-03-22  1:37 ` ✗ Fi.CI.CHECKPATCH: warning for Do not re-read dpll registers Patchwork
@ 2019-03-22  2:05 ` Patchwork
  2019-03-22  9:25 ` [PATCH 0/3] " Jani Nikula
  2019-03-22 18:38 ` ✗ Fi.CI.IGT: failure for " Patchwork
  6 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2019-03-22  2:05 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

== Series Details ==

Series: Do not re-read dpll registers
URL   : https://patchwork.freedesktop.org/series/58382/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5791 -> Patchwork_12562
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58382/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12562 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_basic@basic-bsd2:
    - fi-kbl-7500u:       NOTRUN -> SKIP [fdo#109271] +9

  * igt@gem_exec_basic@readonly-bsd1:
    - fi-snb-2520m:       NOTRUN -> SKIP [fdo#109271] +57

  * igt@kms_busy@basic-flip-a:
    - fi-gdg-551:         PASS -> FAIL [fdo#103182] +1

  * igt@kms_busy@basic-flip-c:
    - fi-snb-2520m:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       NOTRUN -> DMESG-WARN [fdo#103841]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u3:          PASS -> FAIL [fdo#103167]

  * igt@kms_psr@primary_mmap_gtt:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +27

  * igt@runner@aborted:
    - fi-kbl-7500u:       NOTRUN -> FAIL [fdo#103841]

  
#### Possible fixes ####

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         INCOMPLETE [fdo#103927] / [fdo#109720] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720


Participating hosts (44 -> 36)
------------------------------

  Additional (2): fi-snb-2520m fi-kbl-7500u 
  Missing    (10): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus fi-byt-clapper fi-skl-6700k2 fi-kbl-r 


Build changes
-------------

    * Linux: CI_DRM_5791 -> Patchwork_12562

  CI_DRM_5791: 3b6d09692ea282a3487bdf972a068d312a67ca00 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4897: e12d69496a6bef09ac6c0f792b8d60a65cf5e0bf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12562: b807a4b761033c3e712118e5a8b26d932ed8e22b @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b807a4b76103 drm/i915/icl: use previous pll hw readout
8b862f77d44e drm/i915/cnl: use previous pll hw readout
b8b9a3a6bb07 drm/i915/skl: use previous pll hw readout

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12562/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] Do not re-read dpll registers
  2019-03-21 22:02 [PATCH 0/3] Do not re-read dpll registers Lucas De Marchi
                   ` (4 preceding siblings ...)
  2019-03-22  2:05 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2019-03-22  9:25 ` Jani Nikula
  2019-03-22 18:38 ` ✗ Fi.CI.IGT: failure for " Patchwork
  6 siblings, 0 replies; 14+ messages in thread
From: Jani Nikula @ 2019-03-22  9:25 UTC (permalink / raw)
  To: Lucas De Marchi, intel-gfx

On Thu, 21 Mar 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> Instead of re-reading the registers we just read on the hw state
> readout, use the values saved on intel_shared_dpll. Besides not doing
> the MMIO, this helps on sharing code since we don't have to
> differentiate e.g. ICL and CNL because they have different registers for
> the same thing.
>
> I'm a little hesitant wrt DSI. It seems we have completly different
> implementations for ICL and gen <= 11.

That's obviously a typo, ICL being gen 11 and introducing the new DSI
hardware and requiring a completely different implementation.

> BXT has another hook so is not affected, but I'm not sure if we have
> any other gens with DSI that share the skl hooks.

VLV/CHV/BXT/GLK all have their own DSI PLL stuff. ICL or gen 11 is the
divider.

BR,
Jani.





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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] drm/i915/skl: use previous pll hw readout
  2019-03-21 22:02 ` [PATCH 1/3] drm/i915/skl: use previous pll hw readout Lucas De Marchi
@ 2019-03-22 13:05   ` Ville Syrjälä
  0 siblings, 0 replies; 14+ messages in thread
From: Ville Syrjälä @ 2019-03-22 13:05 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

On Thu, Mar 21, 2019 at 03:02:55PM -0700, Lucas De Marchi wrote:
> By the time skl_ddi_clock_get() is called - and thus
> skl_calc_wrpll_link() - 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 | 49 ++++++++++++++------------------
>  1 file changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 933df3a57a8a..644541924208 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1240,24 +1240,15 @@ static int hsw_ddi_calc_wrpll_link(struct drm_i915_private *dev_priv,
>  	return (refclk * n * 100) / (p * r);
>  }
>  
> -static int skl_calc_wrpll_link(struct drm_i915_private *dev_priv,
> -			       enum intel_dpll_id pll_id)
> +static int skl_calc_wrpll_link(struct intel_dpll_hw_state *state)

const

And please call it pll_state or something like that. Bonus points for
unifying the naming convention elsewhere as well.

>  {
> -	i915_reg_t cfgcr1_reg, cfgcr2_reg;
> -	u32 cfgcr1_val, cfgcr2_val;
>  	u32 p0, p1, p2, dco_freq;
>  
> -	cfgcr1_reg = DPLL_CFGCR1(pll_id);
> -	cfgcr2_reg = DPLL_CFGCR2(pll_id);
> +	p0 = state->cfgcr2 & DPLL_CFGCR2_PDIV_MASK;
> +	p2 = state->cfgcr2 & DPLL_CFGCR2_KDIV_MASK;
>  
> -	cfgcr1_val = I915_READ(cfgcr1_reg);
> -	cfgcr2_val = I915_READ(cfgcr2_reg);
> -
> -	p0 = cfgcr2_val & DPLL_CFGCR2_PDIV_MASK;
> -	p2 = cfgcr2_val & DPLL_CFGCR2_KDIV_MASK;
> -
> -	if (cfgcr2_val &  DPLL_CFGCR2_QDIV_MODE(1))
> -		p1 = (cfgcr2_val & DPLL_CFGCR2_QDIV_RATIO_MASK) >> 8;
> +	if (state->cfgcr2 &  DPLL_CFGCR2_QDIV_MODE(1))
> +		p1 = (state->cfgcr2 & DPLL_CFGCR2_QDIV_RATIO_MASK) >> 8;
>  	else
>  		p1 = 1;
>  
> @@ -1292,10 +1283,10 @@ static int skl_calc_wrpll_link(struct drm_i915_private *dev_priv,
>  		break;
>  	}
>  
> -	dco_freq = (cfgcr1_val & DPLL_CFGCR1_DCO_INTEGER_MASK) * 24 * 1000;
> +	dco_freq = (state->cfgcr1 & DPLL_CFGCR1_DCO_INTEGER_MASK) * 24 * 1000;
>  
> -	dco_freq += (((cfgcr1_val & DPLL_CFGCR1_DCO_FRACTION_MASK) >> 9) * 24 *
> -		1000) / 0x8000;
> +	dco_freq += (((state->cfgcr1 & DPLL_CFGCR1_DCO_FRACTION_MASK) >> 9)
> +		     * 24 * 1000) / 0x8000;
>  
>  	if (WARN_ON(p0 == 0 || p1 == 0 || p2 == 0))
>  		return 0;
> @@ -1546,20 +1537,23 @@ static void cnl_ddi_clock_get(struct intel_encoder *encoder,
>  static void skl_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 dpll_ctl1;
> -	enum intel_dpll_id pll_id;
>  
> -	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
> -
> -	dpll_ctl1 = I915_READ(DPLL_CTRL1);
> +	/* For DDI ports we always use a shared PLL. */
> +	if (WARN_ON(!pipe_config->shared_dpll))
> +		goto end;

return 0 will do.

Or maybe eliminate these entirely. I don't see these checks as
particularly useful.

>  
> -	if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(pll_id)) {
> -		link_clock = skl_calc_wrpll_link(dev_priv, pll_id);
> +	/*
> +	 * ctrl1 register is already shifted for each pll, just use 0 to get
> +	 * the internal shift for each field
> +	 */
> +	state = &pipe_config->dpll_hw_state;
> +	if (state->ctrl1 & DPLL_CTRL1_HDMI_MODE(0)) {
> +		link_clock = skl_calc_wrpll_link(state);
>  	} else {
> -		link_clock = dpll_ctl1 & DPLL_CTRL1_LINK_RATE_MASK(pll_id);
> -		link_clock >>= DPLL_CTRL1_LINK_RATE_SHIFT(pll_id);
> +		link_clock = state->ctrl1 & DPLL_CTRL1_LINK_RATE_MASK(0);
> +		link_clock >>= DPLL_CTRL1_LINK_RATE_SHIFT(0);
>  
>  		switch (link_clock) {
>  		case DPLL_CTRL1_LINK_RATE_810:
> @@ -1587,6 +1581,7 @@ static void skl_ddi_clock_get(struct intel_encoder *encoder,
>  		link_clock *= 2;
>  	}
>  
> +end:
>  	pipe_config->port_clock = link_clock;
>  
>  	ddi_dotclock_get(pipe_config);
> -- 
> 2.20.1

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] drm/i915/cnl: use previous pll hw readout
  2019-03-21 22:02 ` [PATCH 2/3] drm/i915/cnl: " Lucas De Marchi
@ 2019-03-22 13:09   ` Ville Syrjälä
  0 siblings, 0 replies; 14+ messages in thread
From: Ville Syrjälä @ 2019-03-22 13:09 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] drm/i915/icl: use previous pll hw readout
  2019-03-21 22:02 ` [PATCH 3/3] drm/i915/icl: " Lucas De Marchi
@ 2019-03-22 13:09   ` Ville Syrjälä
  2019-03-22 19:50     ` Lucas De Marchi
  0 siblings, 1 reply; 14+ messages in thread
From: Ville Syrjälä @ 2019-03-22 13:09 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

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ä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread
* ✗ Fi.CI.IGT: failure for Do not re-read dpll registers
  2019-03-21 22:02 [PATCH 0/3] Do not re-read dpll registers Lucas De Marchi
                   ` (5 preceding siblings ...)
  2019-03-22  9:25 ` [PATCH 0/3] " Jani Nikula
@ 2019-03-22 18:38 ` Patchwork
  6 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2019-03-22 18:38 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

== Series Details ==

Series: Do not re-read dpll registers
URL   : https://patchwork.freedesktop.org/series/58382/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5791_full -> Patchwork_12562_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12562_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12562_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12562_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_blt@cold-min:
    - shard-iclb:         PASS -> INCOMPLETE +1

  
Known issues
------------

  Here are the changes found in Patchwork_12562_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_pwrite@big-cpu-fbr:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] +120

  * igt@i915_hangman@error-state-capture-bsd2:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109276]

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - shard-skl:          PASS -> INCOMPLETE [fdo#107807]

  * igt@i915_pm_rpm@system-suspend-execbuf:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#107807]

  * igt@i915_selftest@live_workarounds:
    - shard-iclb:         PASS -> DMESG-FAIL [fdo#108954]

  * igt@kms_atomic_transition@plane-all-modeset-transition:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927]

  * igt@kms_busy@extended-modeset-hang-newfb-render-a:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#110222]

  * igt@kms_busy@extended-modeset-hang-newfb-render-c:
    - shard-apl:          NOTRUN -> DMESG-WARN [fdo#110222]

  * igt@kms_busy@extended-modeset-hang-oldfb-render-d:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +9

  * igt@kms_busy@extended-modeset-hang-oldfb-render-f:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +3

  * igt@kms_busy@extended-pageflip-hang-newfb-render-c:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +6

  * igt@kms_cursor_legacy@cursor-vs-flip-toggle:
    - shard-iclb:         PASS -> FAIL [fdo#103355]

  * igt@kms_cursor_legacy@pipe-c-single-bo:
    - shard-kbl:          PASS -> DMESG-WARN [fdo#103558] / [fdo#105602] +19

  * igt@kms_fbcon_fbt@fbc:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#109593]

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-glk:          PASS -> FAIL [fdo#102887] / [fdo#105363]

  * igt@kms_flip_tiling@flip-x-tiled:
    - shard-iclb:         PASS -> FAIL [fdo#108303]

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-indfb-draw-pwrite:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +36

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-iclb:         PASS -> FAIL [fdo#105682] / [fdo#108040]

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-kbl:          PASS -> DMESG-WARN [fdo#103558] / [fdo#103841] / [fdo#105079] / [fdo#105602]

  * igt@kms_frontbuffer_tracking@fbc-tilingchange:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +6

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-rte:
    - shard-iclb:         PASS -> FAIL [fdo#105682] / [fdo#109247]

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-shrfb-draw-mmap-gtt:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +28

  * igt@kms_frontbuffer_tracking@fbcpsr-stridechange:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +8
    - shard-skl:          NOTRUN -> FAIL [fdo#105683]

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen:
    - shard-iclb:         PASS -> FAIL [fdo#109247] +22

  * igt@kms_frontbuffer_tracking@psr-2p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109280]

  * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145] +2

  * igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb:
    - shard-glk:          PASS -> FAIL [fdo#108145] +1

  * igt@kms_plane_alpha_blend@pipe-c-alpha-basic:
    - shard-skl:          NOTRUN -> FAIL [fdo#107815] / [fdo#108145]

  * igt@kms_plane_scaling@pipe-a-scaler-with-rotation:
    - shard-glk:          PASS -> SKIP [fdo#109271] / [fdo#109278] +1

  * igt@kms_psr@psr2_basic:
    - shard-iclb:         PASS -> SKIP [fdo#109441] +4

  * igt@kms_psr@sprite_mmap_gtt:
    - shard-iclb:         PASS -> FAIL [fdo#107383] / [fdo#110215] +2

  * igt@kms_rotation_crc@multiplane-rotation:
    - shard-kbl:          NOTRUN -> INCOMPLETE [fdo#103665]

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-kbl:          PASS -> DMESG-FAIL [fdo#105763]

  * igt@kms_universal_plane@universal-plane-gen9-features-pipe-d:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#107773]

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - shard-iclb:         PASS -> FAIL [fdo#104894]

  * igt@runner@aborted:
    - shard-kbl:          NOTRUN -> FAIL [fdo#109383]
    - shard-iclb:         NOTRUN -> FAIL [fdo#109593]

  
#### Possible fixes ####

  * igt@gem_tiled_pread_pwrite:
    - shard-iclb:         TIMEOUT [fdo#109673] -> PASS

  * igt@i915_pm_rpm@i2c:
    - shard-iclb:         DMESG-WARN [fdo#109982] -> PASS

  * igt@i915_pm_rpm@reg-read-ioctl:
    - shard-skl:          INCOMPLETE [fdo#107807] -> PASS

  * igt@i915_suspend@fence-restore-untiled:
    - shard-apl:          DMESG-WARN [fdo#108566] -> PASS +1

  * igt@kms_cursor_legacy@cursor-vs-flip-legacy:
    - shard-iclb:         FAIL [fdo#103355] -> PASS

  * igt@kms_flip@2x-modeset-vs-vblank-race-interruptible:
    - shard-glk:          FAIL [fdo#103060] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +7

  * igt@kms_frontbuffer_tracking@psr-rgb101010-draw-blt:
    - shard-iclb:         FAIL [fdo#109247] -> PASS +18

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          FAIL [fdo#107815] -> PASS

  * igt@kms_psr@psr2_dpms:
    - shard-iclb:         SKIP [fdo#109441] -> PASS +1

  * igt@kms_psr@sprite_mmap_cpu:
    - shard-iclb:         FAIL [fdo#107383] / [fdo#110215] -> PASS +1

  * igt@kms_vblank@pipe-a-ts-continuation-modeset-rpm:
    - shard-apl:          FAIL [fdo#104894] -> PASS

  
#### Warnings ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-iclb:         DMESG-WARN [fdo#108686] -> FAIL [fdo#108686]

  * igt@gem_wait@await-bsd2:
    - shard-iclb:         SKIP [fdo#109276] -> INCOMPLETE [fdo#107713]

  * igt@kms_content_protection@legacy:
    - shard-kbl:          FAIL [fdo#108597] / [fdo#108739] -> SKIP [fdo#105602] / [fdo#109271]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#105683]: https://bugs.freedesktop.org/show_bug.cgi?id=105683
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108303]: https://bugs.freedesktop.org/show_bug.cgi?id=108303
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108597]: https://bugs.freedesktop.org/show_bug.cgi?id=108597
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#108739]: https://bugs.freedesktop.org/show_bug.cgi?id=108739
  [fdo#108954]: https://bugs.freedesktop.org/show_bug.cgi?id=108954
  [fdo#109247]: https://bugs.freedesktop.org/show_bug.cgi?id=109247
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109383]: https://bugs.freedesktop.org/show_bug.cgi?id=109383
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109593]: https://bugs.freedesktop.org/show_bug.cgi?id=109593
  [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
  [fdo#109982]: https://bugs.freedesktop.org/show_bug.cgi?id=109982
  [fdo#110215]: https://bugs.freedesktop.org/show_bug.cgi?id=110215
  [fdo#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222


Participating hosts (10 -> 7)
------------------------------

  Missing    (3): pig-skl-6260u pig-hsw-4770r shard-hsw 


Build changes
-------------

    * Linux: CI_DRM_5791 -> Patchwork_12562

  CI_DRM_5791: 3b6d09692ea282a3487bdf972a068d312a67ca00 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4897: e12d69496a6bef09ac6c0f792b8d60a65cf5e0bf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12562: b807a4b761033c3e712118e5a8b26d932ed8e22b @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12562/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] drm/i915/icl: use previous pll hw readout
  2019-03-22 13:09   ` Ville Syrjälä
@ 2019-03-22 19:50     ` Lucas De Marchi
  2019-03-22 20:15       ` Ville Syrjälä
  2019-03-22 20:42       ` Lucas De Marchi
  0 siblings, 2 replies; 14+ messages in thread
From: Lucas De Marchi @ 2019-03-22 19:50 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

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?

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] drm/i915/icl: use previous pll hw readout
  2019-03-22 19:50     ` Lucas De Marchi
@ 2019-03-22 20:15       ` Ville Syrjälä
  2019-03-22 20:42       ` Lucas De Marchi
  1 sibling, 0 replies; 14+ messages in thread
From: Ville Syrjälä @ 2019-03-22 20:15 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] drm/i915/icl: use previous pll hw readout
  2019-03-22 19:50     ` Lucas De Marchi
  2019-03-22 20:15       ` Ville Syrjälä
@ 2019-03-22 20:42       ` Lucas De Marchi
  1 sibling, 0 replies; 14+ messages in thread
From: Lucas De Marchi @ 2019-03-22 20:42 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

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?

humn.. I should have checked the other patch again. I thought about
always passing the refclk via argument, but that will make it ugly. I
will add dev_priv back.

Lucas De Marchi

>
>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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-03-22 20:42 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-21 22:02 [PATCH 0/3] Do not re-read dpll registers Lucas De Marchi
2019-03-21 22:02 ` [PATCH 1/3] drm/i915/skl: use previous pll hw readout Lucas De Marchi
2019-03-22 13:05   ` Ville Syrjälä
2019-03-21 22:02 ` [PATCH 2/3] drm/i915/cnl: " Lucas De Marchi
2019-03-22 13:09   ` Ville Syrjälä
2019-03-21 22:02 ` [PATCH 3/3] drm/i915/icl: " Lucas De Marchi
2019-03-22 13:09   ` Ville Syrjälä
2019-03-22 19:50     ` Lucas De Marchi
2019-03-22 20:15       ` Ville Syrjälä
2019-03-22 20:42       ` Lucas De Marchi
2019-03-22  1:37 ` ✗ Fi.CI.CHECKPATCH: warning for Do not re-read dpll registers Patchwork
2019-03-22  2:05 ` ✓ Fi.CI.BAT: success " Patchwork
2019-03-22  9:25 ` [PATCH 0/3] " Jani Nikula
2019-03-22 18:38 ` ✗ Fi.CI.IGT: failure for " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.