[PATCH 00/13] Update of the HDMI DPLL dividers computation

[PATCH 00/13] Update of the HDMI DPLL dividers computation

[Damien Lespiau]

As explained in the important patch of this series:

    The HW validation team came back from further testing with a slightly
    changed constraint on the deviation between the DCO frequency and the
    central frequency. Instead of +-4%, it's now +1%/-6%.
    
    Unfortunately, the previous algorithm didn't quite cope with these new
    constraints, the reason being that it wasn't thorough enough looking at
    the possible divider candidates.
    
    The new algorithm looks at all dividers, which is definitely a hammer
    approach (we could reduce further the set of dividers to good ones as a
    follow up, at the cost of a bit more complicated code). But, at least,
    we can now satisfy the +1%/+6% rule for all the "Well known" HDMI
    frequencies of my test set (373 entries).

tools/skl_compute_wrpll is what makes me a bit more confident that the code is
more correct than not. Which can help the review.

I found that we needed a full rewrite when testing a simpler solution based on
the current code: I couldn't light up one of the 1024x768 modes (65Mhz). Turned
out, the previous algorithm failed to respect the new constraints for:
6000000Hz, 39000000Hz, 57000000Hz, 61000000Hz, 65000000Hz, 65250000Hz,
65500000Hz, 66000000Hz, 71000000Hz, 72000000Hz, 76000000Hz. (From the list of
tested frequencies in the i-g-t test)

On top of this, I made a small refinement to the documented alrogithm to prefer
even dividers.

I believe it should be possible to retrain even further the list of potential
dividers we look at by choosing them around central_freq/afe_freq instead of
the full list of candidates, but it looks like it'd be more complicated code
for little gain.

-- 
Damien

Damien Lespiau (13):
  drm/i915/skl: Re-indent part of skl_ddi_calculate_wrpll()
  drm/i915/skl: Make sure to break when not finding suitable PLL
    dividers
  drm/i915/skl: Display the WRPLL frequency we couldn't accomodate when
    failing
  drm/i915/skl: Propagate the error if we fail to find a suitable DPLL
    divider
  drm/i915/skl: Use a more idomatic early return
  drm/i915/skl: Factor out computing the DPLL paramaters from the
    dividers
  drm/i915/skl: Remove unnecessary () used with div_u64()
  drm/i915/skl: Remove unnecessary () used with abs_diff()
  drm/i915/skl: Use MISSING_CASE() in skl_wrpll_params_populate()
  drm/i915: Correctly prefix HSW/BDW HDMI clock functions
  drm/i915/skl: Don't try to store the wrong central frequency
  drm/i915/skl: Replace the HDMI DPLL divider computation algorithm
  drm/i915/skl: Prefer even dividers for SKL DPLLs

 drivers/gpu/drm/i915/intel_ddi.c | 366 ++++++++++++++++++++++++---------------
 1 file changed, 223 insertions(+), 143 deletions(-)

-- 
2.1.0

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

[PATCH 01/13] drm/i915/skl: Re-indent part of skl_ddi_calculate_wrpll()

[Damien Lespiau]

A part of this function was indented with 2 tabs and 1 space instead of
just 2 tabs. We're going to touch that code, so start by re-indenting
it.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 64 ++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9c1e74a..2e24fa4 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1188,69 +1188,69 @@ found:
 	if (min_dco_index > 2) {
 		WARN(1, "No valid values found for the given pixel clock\n");
 	} else {
-		 wrpll_params->central_freq = dco_central_freq[min_dco_index];
+		wrpll_params->central_freq = dco_central_freq[min_dco_index];
 
-		 switch (dco_central_freq[min_dco_index]) {
-		 case 9600000000ULL:
+		switch (dco_central_freq[min_dco_index]) {
+		case 9600000000ULL:
 			wrpll_params->central_freq = 0;
 			break;
-		 case 9000000000ULL:
+		case 9000000000ULL:
 			wrpll_params->central_freq = 1;
 			break;
-		 case 8400000000ULL:
+		case 8400000000ULL:
 			wrpll_params->central_freq = 3;
-		 }
+		}
 
-		 switch (candidate_p0[min_dco_index]) {
-		 case 1:
+		switch (candidate_p0[min_dco_index]) {
+		case 1:
 			wrpll_params->pdiv = 0;
 			break;
-		 case 2:
+		case 2:
 			wrpll_params->pdiv = 1;
 			break;
-		 case 3:
+		case 3:
 			wrpll_params->pdiv = 2;
 			break;
-		 case 7:
+		case 7:
 			wrpll_params->pdiv = 4;
 			break;
-		 default:
+		default:
 			WARN(1, "Incorrect PDiv\n");
-		 }
+		}
 
-		 switch (candidate_p2[min_dco_index]) {
-		 case 5:
+		switch (candidate_p2[min_dco_index]) {
+		case 5:
 			wrpll_params->kdiv = 0;
 			break;
-		 case 2:
+		case 2:
 			wrpll_params->kdiv = 1;
 			break;
-		 case 3:
+		case 3:
 			wrpll_params->kdiv = 2;
 			break;
-		 case 1:
+		case 1:
 			wrpll_params->kdiv = 3;
 			break;
-		 default:
+		default:
 			WARN(1, "Incorrect KDiv\n");
-		 }
+		}
 
-		 wrpll_params->qdiv_ratio = candidate_p1[min_dco_index];
-		 wrpll_params->qdiv_mode =
+		wrpll_params->qdiv_ratio = candidate_p1[min_dco_index];
+		wrpll_params->qdiv_mode =
 			(wrpll_params->qdiv_ratio == 1) ? 0 : 1;
 
-		 dco_freq = candidate_p0[min_dco_index] *
-			 candidate_p1[min_dco_index] *
-			 candidate_p2[min_dco_index] * afe_clock;
+		dco_freq = candidate_p0[min_dco_index] *
+			candidate_p1[min_dco_index] *
+			candidate_p2[min_dco_index] * afe_clock;
 
 		/*
-		* Intermediate values are in Hz.
-		* Divide by MHz to match bsepc
-		*/
-		 wrpll_params->dco_integer = div_u64(dco_freq, (24 * MHz(1)));
-		 wrpll_params->dco_fraction =
-			 div_u64(((div_u64(dco_freq, 24) -
-				   wrpll_params->dco_integer * MHz(1)) * 0x8000), MHz(1));
+		 * Intermediate values are in Hz.
+		 * Divide by MHz to match bsepc
+		 */
+		wrpll_params->dco_integer = div_u64(dco_freq, (24 * MHz(1)));
+		wrpll_params->dco_fraction =
+			div_u64(((div_u64(dco_freq, 24) -
+				  wrpll_params->dco_integer * MHz(1)) * 0x8000), MHz(1));
 
 	}
 }
-- 
2.1.0

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

[PATCH 02/13] drm/i915/skl: Make sure to break when not finding suitable PLL dividers

[Damien Lespiau]

Right now, when finishing the cycle with odd dividers without finding a
suitable candidate, we end up in an infinite loop. Make sure to break in
that case.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 2e24fa4..da7aa0f 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1180,6 +1180,10 @@ found:
 		}
 
 		if (min_dco_index > 2 && dco_count == 2) {
+                        /* oh well, we tried... */
+                        if (retry_with_odd)
+                                break;
+
 			retry_with_odd = true;
 			dco_count = 0;
 		}
-- 
2.1.0

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

[PATCH 03/13] drm/i915/skl: Display the WRPLL frequency we couldn't accomodate when failing

[Damien Lespiau]

This helps debugging.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index da7aa0f..ff5eb05 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1190,7 +1190,8 @@ found:
 	}
 
 	if (min_dco_index > 2) {
-		WARN(1, "No valid values found for the given pixel clock\n");
+		WARN(1, "No valid parameters found for pixel clock: %dHz\n",
+		     clock);
 	} else {
 		wrpll_params->central_freq = dco_central_freq[min_dco_index];
 
-- 
2.1.0

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

[PATCH 04/13] drm/i915/skl: Propagate the error if we fail to find a suitable DPLL divider

[Damien Lespiau]

At the moment, even if we fail to find a suitable divider, we'll still
try to set the mode with bogus parameters.

Just fail the modeset if we can't generate the frequency.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ff5eb05..12592bf 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1111,7 +1111,7 @@ struct skl_wrpll_params {
 	uint32_t        central_freq;
 };
 
-static void
+static bool
 skl_ddi_calculate_wrpll(int clock /* in Hz */,
 			struct skl_wrpll_params *wrpll_params)
 {
@@ -1192,6 +1192,7 @@ found:
 	if (min_dco_index > 2) {
 		WARN(1, "No valid parameters found for pixel clock: %dHz\n",
 		     clock);
+		return false;
 	} else {
 		wrpll_params->central_freq = dco_central_freq[min_dco_index];
 
@@ -1258,6 +1259,8 @@ found:
 				  wrpll_params->dco_integer * MHz(1)) * 0x8000), MHz(1));
 
 	}
+
+	return true;
 }
 
 
@@ -1282,7 +1285,8 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc,
 
 		ctrl1 |= DPLL_CTRL1_HDMI_MODE(0);
 
-		skl_ddi_calculate_wrpll(clock * 1000, &wrpll_params);
+		if (!skl_ddi_calculate_wrpll(clock * 1000, &wrpll_params))
+			return false;
 
 		cfgcr1 = DPLL_CFGCR1_FREQ_ENABLE |
 			 DPLL_CFGCR1_DCO_FRACTION(wrpll_params.dco_fraction) |
-- 
2.1.0

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

[PATCH 05/13] drm/i915/skl: Use a more idomatic early return

[Damien Lespiau]

We can coalesce the WARN() condition with the WARN() itself and, as we
are returning early, we can de-intent the rest of the function.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 121 +++++++++++++++++++--------------------
 1 file changed, 59 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 12592bf..54ba03d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1189,76 +1189,73 @@ found:
 		}
 	}
 
-	if (min_dco_index > 2) {
-		WARN(1, "No valid parameters found for pixel clock: %dHz\n",
-		     clock);
+	if (WARN(min_dco_index > 2,
+		 "No valid parameters found for pixel clock: %dHz\n", clock))
 		return false;
-	} else {
-		wrpll_params->central_freq = dco_central_freq[min_dco_index];
 
-		switch (dco_central_freq[min_dco_index]) {
-		case 9600000000ULL:
-			wrpll_params->central_freq = 0;
-			break;
-		case 9000000000ULL:
-			wrpll_params->central_freq = 1;
-			break;
-		case 8400000000ULL:
-			wrpll_params->central_freq = 3;
-		}
+	wrpll_params->central_freq = dco_central_freq[min_dco_index];
 
-		switch (candidate_p0[min_dco_index]) {
-		case 1:
-			wrpll_params->pdiv = 0;
-			break;
-		case 2:
-			wrpll_params->pdiv = 1;
-			break;
-		case 3:
-			wrpll_params->pdiv = 2;
-			break;
-		case 7:
-			wrpll_params->pdiv = 4;
-			break;
-		default:
-			WARN(1, "Incorrect PDiv\n");
-		}
+	switch (dco_central_freq[min_dco_index]) {
+	case 9600000000ULL:
+		wrpll_params->central_freq = 0;
+		break;
+	case 9000000000ULL:
+		wrpll_params->central_freq = 1;
+		break;
+	case 8400000000ULL:
+		wrpll_params->central_freq = 3;
+	}
 
-		switch (candidate_p2[min_dco_index]) {
-		case 5:
-			wrpll_params->kdiv = 0;
-			break;
-		case 2:
-			wrpll_params->kdiv = 1;
-			break;
-		case 3:
-			wrpll_params->kdiv = 2;
-			break;
-		case 1:
-			wrpll_params->kdiv = 3;
-			break;
-		default:
-			WARN(1, "Incorrect KDiv\n");
-		}
+	switch (candidate_p0[min_dco_index]) {
+	case 1:
+		wrpll_params->pdiv = 0;
+		break;
+	case 2:
+		wrpll_params->pdiv = 1;
+		break;
+	case 3:
+		wrpll_params->pdiv = 2;
+		break;
+	case 7:
+		wrpll_params->pdiv = 4;
+		break;
+	default:
+		WARN(1, "Incorrect PDiv\n");
+	}
 
-		wrpll_params->qdiv_ratio = candidate_p1[min_dco_index];
-		wrpll_params->qdiv_mode =
-			(wrpll_params->qdiv_ratio == 1) ? 0 : 1;
+	switch (candidate_p2[min_dco_index]) {
+	case 5:
+		wrpll_params->kdiv = 0;
+		break;
+	case 2:
+		wrpll_params->kdiv = 1;
+		break;
+	case 3:
+		wrpll_params->kdiv = 2;
+		break;
+	case 1:
+		wrpll_params->kdiv = 3;
+		break;
+	default:
+		WARN(1, "Incorrect KDiv\n");
+	}
 
-		dco_freq = candidate_p0[min_dco_index] *
-			candidate_p1[min_dco_index] *
-			candidate_p2[min_dco_index] * afe_clock;
+	wrpll_params->qdiv_ratio = candidate_p1[min_dco_index];
+	wrpll_params->qdiv_mode =
+		(wrpll_params->qdiv_ratio == 1) ? 0 : 1;
 
-		/*
-		 * Intermediate values are in Hz.
-		 * Divide by MHz to match bsepc
-		 */
-		wrpll_params->dco_integer = div_u64(dco_freq, (24 * MHz(1)));
-		wrpll_params->dco_fraction =
-			div_u64(((div_u64(dco_freq, 24) -
-				  wrpll_params->dco_integer * MHz(1)) * 0x8000), MHz(1));
+	dco_freq = candidate_p0[min_dco_index] *
+		candidate_p1[min_dco_index] *
+		candidate_p2[min_dco_index] * afe_clock;
 
-	}
+	/*
+	 * Intermediate values are in Hz.
+	 * Divide by MHz to match bsepc
+	 */
+	wrpll_params->dco_integer = div_u64(dco_freq, (24 * MHz(1)));
+	wrpll_params->dco_fraction =
+		div_u64(((div_u64(dco_freq, 24) -
+			  wrpll_params->dco_integer * MHz(1)) * 0x8000), MHz(1));
 
 	return true;
 }
-- 
2.1.0

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

[PATCH 06/13] drm/i915/skl: Factor out computing the DPLL paramaters from the dividers

[Damien Lespiau]

This part doesn't depend on how we compute the DPLL dividers (p and
p0/p1/p2) and can be reused even if we change the algorithm to do so.
(something that is planned for a followup patch)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 139 +++++++++++++++++++++------------------
 1 file changed, 75 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 54ba03d..43b0b56 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1111,6 +1111,75 @@ struct skl_wrpll_params {
 	uint32_t        central_freq;
 };
 
+static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
+				      uint64_t afe_clock,
+				      uint64_t central_freq,
+				      uint32_t p0, uint32_t p1, uint32_t p2)
+{
+	uint64_t dco_freq;
+
+	params->central_freq = central_freq;
+
+	switch (central_freq) {
+	case 9600000000ULL:
+		params->central_freq = 0;
+		break;
+	case 9000000000ULL:
+		params->central_freq = 1;
+		break;
+	case 8400000000ULL:
+		params->central_freq = 3;
+	}
+
+	switch (p0) {
+	case 1:
+		params->pdiv = 0;
+		break;
+	case 2:
+		params->pdiv = 1;
+		break;
+	case 3:
+		params->pdiv = 2;
+		break;
+	case 7:
+		params->pdiv = 4;
+		break;
+	default:
+		WARN(1, "Incorrect PDiv\n");
+	}
+
+	switch (p2) {
+	case 5:
+		params->kdiv = 0;
+		break;
+	case 2:
+		params->kdiv = 1;
+		break;
+	case 3:
+		params->kdiv = 2;
+		break;
+	case 1:
+		params->kdiv = 3;
+		break;
+	default:
+		WARN(1, "Incorrect KDiv\n");
+	}
+
+	params->qdiv_ratio = p1;
+	params->qdiv_mode = (params->qdiv_ratio == 1) ? 0 : 1;
+
+	dco_freq = p0 * p1 * p2 * afe_clock;
+
+	/*
+	 * Intermediate values are in Hz.
+	 * Divide by MHz to match bsepc
+	 */
+	params->dco_integer = div_u64(dco_freq, (24 * MHz(1)));
+	params->dco_fraction =
+		div_u64(((div_u64(dco_freq, 24) -
+			  params->dco_integer * MHz(1)) * 0x8000), MHz(1));
+}
+
 static bool
 skl_ddi_calculate_wrpll(int clock /* in Hz */,
 			struct skl_wrpll_params *wrpll_params)
@@ -1130,7 +1199,6 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
 	uint32_t dco_central_freq_deviation[3];
 	uint32_t i, P1, k, dco_count;
 	bool retry_with_odd = false;
-	uint64_t dco_freq;
 
 	/* Determine P0, P1 or P2 */
 	for (dco_count = 0; dco_count < 3; dco_count++) {
@@ -1193,69 +1261,12 @@ found:
 		 "No valid parameters found for pixel clock: %dHz\n", clock))
 		return false;
 
-	wrpll_params->central_freq = dco_central_freq[min_dco_index];
-
-	switch (dco_central_freq[min_dco_index]) {
-	case 9600000000ULL:
-		wrpll_params->central_freq = 0;
-		break;
-	case 9000000000ULL:
-		wrpll_params->central_freq = 1;
-		break;
-	case 8400000000ULL:
-		wrpll_params->central_freq = 3;
-	}
-
-	switch (candidate_p0[min_dco_index]) {
-	case 1:
-		wrpll_params->pdiv = 0;
-		break;
-	case 2:
-		wrpll_params->pdiv = 1;
-		break;
-	case 3:
-		wrpll_params->pdiv = 2;
-		break;
-	case 7:
-		wrpll_params->pdiv = 4;
-		break;
-	default:
-		WARN(1, "Incorrect PDiv\n");
-	}
-
-	switch (candidate_p2[min_dco_index]) {
-	case 5:
-		wrpll_params->kdiv = 0;
-		break;
-	case 2:
-		wrpll_params->kdiv = 1;
-		break;
-	case 3:
-		wrpll_params->kdiv = 2;
-		break;
-	case 1:
-		wrpll_params->kdiv = 3;
-		break;
-	default:
-		WARN(1, "Incorrect KDiv\n");
-	}
-
-	wrpll_params->qdiv_ratio = candidate_p1[min_dco_index];
-	wrpll_params->qdiv_mode =
-		(wrpll_params->qdiv_ratio == 1) ? 0 : 1;
-
-	dco_freq = candidate_p0[min_dco_index] *
-		candidate_p1[min_dco_index] *
-		candidate_p2[min_dco_index] * afe_clock;
-
-	/*
-	 * Intermediate values are in Hz.
-	 * Divide by MHz to match bsepc
-	 */
-	wrpll_params->dco_integer = div_u64(dco_freq, (24 * MHz(1)));
-	wrpll_params->dco_fraction =
-		div_u64(((div_u64(dco_freq, 24) -
-			  wrpll_params->dco_integer * MHz(1)) * 0x8000), MHz(1));
+	skl_wrpll_params_populate(wrpll_params,
+				  afe_clock,
+				  dco_central_freq[min_dco_index],
+				  candidate_p0[min_dco_index],
+				  candidate_p1[min_dco_index],
+				  candidate_p2[min_dco_index]);
 
 	return true;
 }
-- 
2.1.0

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

[PATCH 07/13] drm/i915/skl: Remove unnecessary () used with div_u64()

[Damien Lespiau]

div_u64() can be either a inline function or a define, but in either
case it's safe to provide expressions as parameters without outer ()
around them.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 43b0b56..fd90771 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1174,10 +1174,10 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
 	 * Intermediate values are in Hz.
 	 * Divide by MHz to match bsepc
 	 */
-	params->dco_integer = div_u64(dco_freq, (24 * MHz(1)));
+	params->dco_integer = div_u64(dco_freq, 24 * MHz(1));
 	params->dco_fraction =
-		div_u64(((div_u64(dco_freq, 24) -
-			  params->dco_integer * MHz(1)) * 0x8000), MHz(1));
+		div_u64((div_u64(dco_freq, 24) -
+			 params->dco_integer * MHz(1)) * 0x8000, MHz(1));
 }
 
 static bool
-- 
2.1.0

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

[PATCH 08/13] drm/i915/skl: Remove unnecessary () used with abs_diff()

[Damien Lespiau]

abs_diff() properly protects its parameters, so no need for the outer ()
here.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index fd90771..b9d5d65 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1235,7 +1235,7 @@ found:
 		if (found) {
 			dco_central_freq_deviation[dco_count] =
 				div64_u64(10000 *
-					  abs_diff((candidate_p * afe_clock),
+					  abs_diff(candidate_p * afe_clock,
 						   dco_central_freq[dco_count]),
 					  dco_central_freq[dco_count]);
 
-- 
2.1.0

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

[PATCH 09/13] drm/i915/skl: Use MISSING_CASE() in skl_wrpll_params_populate()

[Damien Lespiau]

We now have a special macro for those cases.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index b9d5d65..ab327a1 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1145,7 +1145,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
 		params->pdiv = 4;
 		break;
 	default:
-		WARN(1, "Incorrect PDiv\n");
+		MISSING_CASE(p0);
 	}
 
 	switch (p2) {
@@ -1162,7 +1162,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
 		params->kdiv = 3;
 		break;
 	default:
-		WARN(1, "Incorrect KDiv\n");
+		MISSING_CASE(p2);
 	}
 
 	params->qdiv_ratio = p1;
-- 
2.1.0

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

[PATCH 10/13] drm/i915: Correctly prefix HSW/BDW HDMI clock functions

[Damien Lespiau]

Those functions were the only one in existence when they were
introduced. We now now they are only valid for HSW/BDW.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ab327a1..f3c5b49 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -624,11 +624,11 @@ intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
 	(void) (&__a == &__b);			\
 	__a > __b ? (__a - __b) : (__b - __a); })
 
-struct wrpll_rnp {
+struct hsw_wrpll_rnp {
 	unsigned p, n2, r2;
 };
 
-static unsigned wrpll_get_budget_for_freq(int clock)
+static unsigned hsw_wrpll_get_budget_for_freq(int clock)
 {
 	unsigned budget;
 
@@ -702,9 +702,9 @@ static unsigned wrpll_get_budget_for_freq(int clock)
 	return budget;
 }
 
-static void wrpll_update_rnp(uint64_t freq2k, unsigned budget,
-			     unsigned r2, unsigned n2, unsigned p,
-			     struct wrpll_rnp *best)
+static void hsw_wrpll_update_rnp(uint64_t freq2k, unsigned budget,
+				 unsigned r2, unsigned n2, unsigned p,
+				 struct hsw_wrpll_rnp *best)
 {
 	uint64_t a, b, c, d, diff, diff_best;
 
@@ -761,8 +761,7 @@ static void wrpll_update_rnp(uint64_t freq2k, unsigned budget,
 	/* Otherwise a < c && b >= d, do nothing */
 }
 
-static int intel_ddi_calc_wrpll_link(struct drm_i915_private *dev_priv,
-				     int reg)
+static int hsw_ddi_calc_wrpll_link(struct drm_i915_private *dev_priv, int reg)
 {
 	int refclk = LC_FREQ;
 	int n, p, r;
@@ -928,10 +927,10 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
 		link_clock = 270000;
 		break;
 	case PORT_CLK_SEL_WRPLL1:
-		link_clock = intel_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL1);
+		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL1);
 		break;
 	case PORT_CLK_SEL_WRPLL2:
-		link_clock = intel_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL2);
+		link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL2);
 		break;
 	case PORT_CLK_SEL_SPLL:
 		pll = I915_READ(SPLL_CTL) & SPLL_PLL_FREQ_MASK;
@@ -1010,12 +1009,12 @@ hsw_ddi_calculate_wrpll(int clock /* in Hz */,
 {
 	uint64_t freq2k;
 	unsigned p, n2, r2;
-	struct wrpll_rnp best = { 0, 0, 0 };
+	struct hsw_wrpll_rnp best = { 0, 0, 0 };
 	unsigned budget;
 
 	freq2k = clock / 100;
 
-	budget = wrpll_get_budget_for_freq(clock);
+	budget = hsw_wrpll_get_budget_for_freq(clock);
 
 	/* Special case handling for 540 pixel clock: bypass WR PLL entirely
 	 * and directly pass the LC PLL to it. */
@@ -1059,8 +1058,8 @@ hsw_ddi_calculate_wrpll(int clock /* in Hz */,
 		     n2++) {
 
 			for (p = P_MIN; p <= P_MAX; p += P_INC)
-				wrpll_update_rnp(freq2k, budget,
-						 r2, n2, p, &best);
+				hsw_wrpll_update_rnp(freq2k, budget,
+						     r2, n2, p, &best);
 		}
 	}
 
-- 
2.1.0

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

[PATCH 11/13] drm/i915/skl: Don't try to store the wrong central frequency

[Damien Lespiau]

The orignal code started by storing the actual central frequency (in Hz,
using a uint64_t) in a uint32_t which codes for the register value. That
can't be right.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index f3c5b49..a99fee1 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1117,8 +1117,6 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
 {
 	uint64_t dco_freq;
 
-	params->central_freq = central_freq;
-
 	switch (central_freq) {
 	case 9600000000ULL:
 		params->central_freq = 0;
-- 
2.1.0

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

[PATCH 12/13] drm/i915/skl: Replace the HDMI DPLL divider computation algorithm

[Damien Lespiau]

The HW validation team came back from further testing with a slightly
changed constraint on the deviation between the DCO frequency and the
central frequency. Instead of +-4%, it's now +1%/-6%.

Unfortunately, the previous algorithm didn't quite cope with these new
constraints, the reason being that it wasn't thorough enough looking at
the possible divider candidates.

The new algorithm looks at all dividers, which is definitely a hammer
approach (we could reduce further the set of dividers to good ones as a
follow up, at the cost of a bit more complicated code). But, at least,
we can now satisfy the +1%/+6% rule for all the "Well known" HDMI
frequencies of my test set (373 entries).

On that subject, the new code is quite extensively tested in
intel-gpu-tools (tools/skl_compute_wrpll).

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 207 +++++++++++++++++++++++++--------------
 1 file changed, 133 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index a99fee1..381a8c9 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1100,6 +1100,99 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
 	return true;
 }
 
+struct skl_wrpll_context {
+	uint32_t min_pdeviation;	/* record the minimum deviations to */
+	uint32_t min_ndeviation;	/* compare candidates */
+	uint64_t central_freq;		/* chosen central freq */
+	uint64_t dco_freq;		/* chosen dco freq */
+	unsigned int p;			/* chosen divider */
+};
+
+static void skl_wrpll_context_init(struct skl_wrpll_context *ctx)
+{
+	memset(ctx, 0, sizeof(*ctx));
+
+	/* DCO freq must be within +1%/-6%  of the DCO central freq */
+	ctx->min_pdeviation = 100;
+	ctx-> min_ndeviation = 600;
+}
+
+static void skl_wrpll_try_divider(struct skl_wrpll_context *ctx,
+				  uint64_t central_freq,
+				  uint64_t dco_freq,
+				  unsigned int divider)
+{
+	uint64_t deviation;
+
+	deviation = div64_u64(10000 * abs_diff(dco_freq, central_freq),
+			      central_freq);
+
+	/* positive deviation */
+	if (dco_freq >= central_freq) {
+		if (deviation < ctx->min_pdeviation) {
+			ctx->min_pdeviation = deviation;
+			ctx->central_freq = central_freq;
+			ctx->dco_freq = dco_freq;
+			ctx->p = divider;
+		}
+	/* negative deviation */
+	} else if (deviation < ctx->min_ndeviation) {
+		ctx->min_ndeviation = deviation;
+		ctx->central_freq = central_freq;
+		ctx->dco_freq = dco_freq;
+		ctx->p = divider;
+	}
+}
+
+static void skl_wrpll_get_multipliers(unsigned int p,
+				      unsigned int *p0 /* out */,
+				      unsigned int *p1 /* out */,
+				      unsigned int *p2 /* out */)
+{
+	/* even dividers */
+	if (p % 2 == 0) {
+		unsigned int half = p / 2;
+
+		if (half == 1 || half == 2 || half == 3 || half == 5) {
+			*p0 = 2;
+			*p1 = 1;
+			*p2 = half;
+		} else if (half % 2 == 0) {
+			*p0 = 2;
+			*p1 = half / 2;
+			*p2 = 2;
+		} else if (half % 3 == 0) {
+			*p0 = 3;
+			*p1 = half / 3;
+			*p2 = 2;
+		} else if (half % 7 == 0) {
+			*p0 = 7;
+			*p1 = half / 7;
+			*p2 = 2;
+		}
+	} else if (p == 3 || p == 9) {  /* 3, 5, 7, 9, 15, 21, 35 */
+		*p0 = 3;
+		*p1 = 1;
+		*p2 = p / 3;
+	} else if (p == 5 || p == 7) {
+		*p0 = p;
+		*p1 = 1;
+		*p2 = 1;
+	} else if (p == 15) {
+		*p0 = 3;
+		*p1 = 1;
+		*p2 = 5;
+	} else if (p == 21) {
+		*p0 = 7;
+		*p1 = 1;
+		*p2 = 3;
+	} else if (p == 35) {
+		*p0 = 7;
+		*p1 = 1;
+		*p2 = 5;
+	}
+}
+
 struct skl_wrpll_params {
 	uint32_t        dco_fraction;
 	uint32_t        dco_integer;
@@ -1185,90 +1278,56 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
 	uint64_t dco_central_freq[3] = {8400000000ULL,
 					9000000000ULL,
 					9600000000ULL};
-	uint32_t min_dco_deviation = 400;
-	uint32_t min_dco_index = 3;
-	uint32_t P0[4] = {1, 2, 3, 7};
-	uint32_t P2[4] = {1, 2, 3, 5};
-	bool found = false;
-	uint32_t candidate_p = 0;
-	uint32_t candidate_p0[3] = {0}, candidate_p1[3] = {0};
-	uint32_t candidate_p2[3] = {0};
-	uint32_t dco_central_freq_deviation[3];
-	uint32_t i, P1, k, dco_count;
-	bool retry_with_odd = false;
-
-	/* Determine P0, P1 or P2 */
-	for (dco_count = 0; dco_count < 3; dco_count++) {
-		found = false;
-		candidate_p =
-			div64_u64(dco_central_freq[dco_count], afe_clock);
-		if (retry_with_odd == false)
-			candidate_p = (candidate_p % 2 == 0 ?
-				candidate_p : candidate_p + 1);
-
-		for (P1 = 1; P1 < candidate_p; P1++) {
-			for (i = 0; i < 4; i++) {
-				if (!(P0[i] != 1 || P1 == 1))
-					continue;
-
-				for (k = 0; k < 4; k++) {
-					if (P1 != 1 && P2[k] != 2)
-						continue;
-
-					if (candidate_p == P0[i] * P1 * P2[k]) {
-						/* Found possible P0, P1, P2 */
-						found = true;
-						candidate_p0[dco_count] = P0[i];
-						candidate_p1[dco_count] = P1;
-						candidate_p2[dco_count] = P2[k];
-						goto found;
-					}
-
-				}
+	static const int even_dividers[] = {  4,  6,  8, 10, 12, 14, 16, 18, 20,
+					     24, 28, 30, 32, 36, 40, 42, 44,
+					     48, 52, 54, 56, 60, 64, 66, 68,
+					     70, 72, 76, 78, 80, 84, 88, 90,
+					     92, 96, 98 };
+	static const int odd_dividers[] = { 3, 5, 7, 9, 15, 21, 35 };
+	static const struct {
+		const int *list;
+		int n_dividers;
+	} dividers[] = {
+		{ even_dividers, ARRAY_SIZE(even_dividers) },
+		{ odd_dividers, ARRAY_SIZE(odd_dividers) },
+	};
+	struct skl_wrpll_context ctx;
+	unsigned int dco, d, i;
+	unsigned int p0, p1, p2;
+
+	skl_wrpll_context_init(&ctx);
+
+	for (dco = 0; dco < ARRAY_SIZE(dco_central_freq); dco++) {
+		for (d = 0; d < ARRAY_SIZE(dividers); d++) {
+			for (i = 0; i < dividers[d].n_dividers; i++) {
+				unsigned int p = dividers[d].list[i];
+				uint64_t dco_freq = p * afe_clock;
+
+				skl_wrpll_try_divider(&ctx,
+						      dco_central_freq[dco],
+						      dco_freq,
+						      p);
 			}
 		}
-
-found:
-		if (found) {
-			dco_central_freq_deviation[dco_count] =
-				div64_u64(10000 *
-					  abs_diff(candidate_p * afe_clock,
-						   dco_central_freq[dco_count]),
-					  dco_central_freq[dco_count]);
-
-			if (dco_central_freq_deviation[dco_count] <
-				min_dco_deviation) {
-				min_dco_deviation =
-					dco_central_freq_deviation[dco_count];
-				min_dco_index = dco_count;
-			}
-		}
-
-		if (min_dco_index > 2 && dco_count == 2) {
-                        /* oh well, we tried... */
-                        if (retry_with_odd)
-                                break;
-
-			retry_with_odd = true;
-			dco_count = 0;
-		}
 	}
 
-	if (WARN(min_dco_index > 2,
-		 "No valid parameters found for pixel clock: %dHz\n", clock))
+	if (!ctx.p) {
+		DRM_DEBUG_DRIVER("No valid divider found for %dHz\n", clock);
 		return false;
+	}
 
-	skl_wrpll_params_populate(wrpll_params,
-				  afe_clock,
-				  dco_central_freq[min_dco_index],
-				  candidate_p0[min_dco_index],
-				  candidate_p1[min_dco_index],
-				  candidate_p2[min_dco_index]);
+	/*
+	 * gcc incorrectly analyses that these can be used without being
+	 * initialized. To be fair, it's hard to guess.
+	 */
+	p0 = p1 = p2 = 0;
+	skl_wrpll_get_multipliers(ctx.p, &p0, &p1, &p2);
+	skl_wrpll_params_populate(wrpll_params, afe_clock, ctx.central_freq,
+				  p0, p1, p2);
 
 	return true;
 }
 
-
 static bool
 skl_ddi_pll_select(struct intel_crtc *intel_crtc,
 		   struct intel_crtc_state *crtc_state,
-- 
2.1.0

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

[PATCH 13/13] drm/i915/skl: Prefer even dividers for SKL DPLLs

[Damien Lespiau]

Currently, if an odd divider improves the deviation (minimizes it), we
take that divider. The recommendation is to prefer even dividers.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 381a8c9..54344c3 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1308,6 +1308,13 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
 						      dco_freq,
 						      p);
 			}
+
+			/*
+			 * If a solution is found with an even divider, prefer
+			 * this one.
+			 */
+			if (d == 0 && ctx.p)
+				break;
 		}
 	}
 
-- 
2.1.0

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

Re: [PATCH 01/13] drm/i915/skl: Re-indent part of skl_ddi_calculate_wrpll()

[Daniel Vetter]

On Thu, May 07, 2015 at 06:38:37PM +0100, Damien Lespiau wrote:
> A part of this function was indented with 2 tabs and 1 space instead of
> just 2 tabs. We're going to touch that code, so start by re-indenting
> it.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 64 ++++++++++++++++++++--------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9c1e74a..2e24fa4 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1188,69 +1188,69 @@ found:
>  	if (min_dco_index > 2) {
>  		WARN(1, "No valid values found for the given pixel clock\n");
>  	} else {
> -		 wrpll_params->central_freq = dco_central_freq[min_dco_index];
> +		wrpll_params->central_freq = dco_central_freq[min_dco_index];
>  
> -		 switch (dco_central_freq[min_dco_index]) {
> -		 case 9600000000ULL:
> +		switch (dco_central_freq[min_dco_index]) {
> +		case 9600000000ULL:
>  			wrpll_params->central_freq = 0;
>  			break;
> -		 case 9000000000ULL:
> +		case 9000000000ULL:
>  			wrpll_params->central_freq = 1;
>  			break;
> -		 case 8400000000ULL:
> +		case 8400000000ULL:
>  			wrpll_params->central_freq = 3;
> -		 }
> +		}
>  
> -		 switch (candidate_p0[min_dco_index]) {
> -		 case 1:
> +		switch (candidate_p0[min_dco_index]) {
> +		case 1:
>  			wrpll_params->pdiv = 0;
>  			break;
> -		 case 2:
> +		case 2:
>  			wrpll_params->pdiv = 1;
>  			break;
> -		 case 3:
> +		case 3:
>  			wrpll_params->pdiv = 2;
>  			break;
> -		 case 7:
> +		case 7:
>  			wrpll_params->pdiv = 4;
>  			break;
> -		 default:
> +		default:
>  			WARN(1, "Incorrect PDiv\n");
> -		 }
> +		}
>  
> -		 switch (candidate_p2[min_dco_index]) {
> -		 case 5:
> +		switch (candidate_p2[min_dco_index]) {
> +		case 5:
>  			wrpll_params->kdiv = 0;
>  			break;
> -		 case 2:
> +		case 2:
>  			wrpll_params->kdiv = 1;
>  			break;
> -		 case 3:
> +		case 3:
>  			wrpll_params->kdiv = 2;
>  			break;
> -		 case 1:
> +		case 1:
>  			wrpll_params->kdiv = 3;
>  			break;
> -		 default:
> +		default:
>  			WARN(1, "Incorrect KDiv\n");
> -		 }
> +		}
>  
> -		 wrpll_params->qdiv_ratio = candidate_p1[min_dco_index];
> -		 wrpll_params->qdiv_mode =
> +		wrpll_params->qdiv_ratio = candidate_p1[min_dco_index];
> +		wrpll_params->qdiv_mode =
>  			(wrpll_params->qdiv_ratio == 1) ? 0 : 1;
>  
> -		 dco_freq = candidate_p0[min_dco_index] *
> -			 candidate_p1[min_dco_index] *
> -			 candidate_p2[min_dco_index] * afe_clock;
> +		dco_freq = candidate_p0[min_dco_index] *
> +			candidate_p1[min_dco_index] *
> +			candidate_p2[min_dco_index] * afe_clock;
>  
>  		/*
> -		* Intermediate values are in Hz.
> -		* Divide by MHz to match bsepc
> -		*/
> -		 wrpll_params->dco_integer = div_u64(dco_freq, (24 * MHz(1)));
> -		 wrpll_params->dco_fraction =
> -			 div_u64(((div_u64(dco_freq, 24) -
> -				   wrpll_params->dco_integer * MHz(1)) * 0x8000), MHz(1));
> +		 * Intermediate values are in Hz.
> +		 * Divide by MHz to match bsepc
> +		 */
> +		wrpll_params->dco_integer = div_u64(dco_freq, (24 * MHz(1)));
> +		wrpll_params->dco_fraction =
> +			div_u64(((div_u64(dco_freq, 24) -
> +				  wrpll_params->dco_integer * MHz(1)) * 0x8000), MHz(1));
>  
>  	}
>  }
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/13] drm/i915/skl: Prefer even dividers for SKL DPLLs

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6348
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  342/342              342/342
BYT                 -1              228/228              227/228
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_userptr_blits@forked-sync-swapping-normal      PASS(2)      INIT(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/13] drm/i915/skl: Make sure to break when not finding suitable PLL dividers

[Paulo Zanoni]

2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> Right now, when finishing the cycle with odd dividers without finding a
> suitable candidate, we end up in an infinite loop. Make sure to break in
> that case.

Cc stable?

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2e24fa4..da7aa0f 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1180,6 +1180,10 @@ found:
>                 }
>
>                 if (min_dco_index > 2 && dco_count == 2) {
> +                        /* oh well, we tried... */
> +                        if (retry_with_odd)
> +                                break;
> +
>                         retry_with_odd = true;
>                         dco_count = 0;
>                 }
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 09/13] drm/i915/skl: Use MISSING_CASE() in skl_wrpll_params_populate()

[Paulo Zanoni]

2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> We now have a special macro for those cases.

I'm not sure if this patch is an improvement. Before it, we always
knew which "switch" statement was bad since we used to print either
"PDiv" or "KDiv". After the patch, it will not be possible to know
from which switch statement the error came from. Of course, there's
the advantage of at least knowing the value. I'd vote to either skip
this patch, or improve the MISSING_CASE macro to be able to account
for multiple uses on the same function. But I'm open to arugmentation
:)

>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index b9d5d65..ab327a1 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1145,7 +1145,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
>                 params->pdiv = 4;
>                 break;
>         default:
> -               WARN(1, "Incorrect PDiv\n");
> +               MISSING_CASE(p0);
>         }
>
>         switch (p2) {
> @@ -1162,7 +1162,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
>                 params->kdiv = 3;
>                 break;
>         default:
> -               WARN(1, "Incorrect KDiv\n");
> +               MISSING_CASE(p2);
>         }
>
>         params->qdiv_ratio = p1;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 08/13] drm/i915/skl: Remove unnecessary () used with abs_diff()

[Paulo Zanoni]

2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> abs_diff() properly protects its parameters, so no need for the outer ()
> here.

Patches 3, 4, 5, 6, 7 and 8:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index fd90771..b9d5d65 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1235,7 +1235,7 @@ found:
>                 if (found) {
>                         dco_central_freq_deviation[dco_count] =
>                                 div64_u64(10000 *
> -                                         abs_diff((candidate_p * afe_clock),
> +                                         abs_diff(candidate_p * afe_clock,
>                                                    dco_central_freq[dco_count]),
>                                           dco_central_freq[dco_count]);
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 10/13] drm/i915: Correctly prefix HSW/BDW HDMI clock functions

[Paulo Zanoni]

2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> Those functions were the only one in existence when they were
> introduced. We now now they are only valid for HSW/BDW.

s/now now/now know/

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ab327a1..f3c5b49 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -624,11 +624,11 @@ intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
>         (void) (&__a == &__b);                  \
>         __a > __b ? (__a - __b) : (__b - __a); })
>
> -struct wrpll_rnp {
> +struct hsw_wrpll_rnp {
>         unsigned p, n2, r2;
>  };
>
> -static unsigned wrpll_get_budget_for_freq(int clock)
> +static unsigned hsw_wrpll_get_budget_for_freq(int clock)
>  {
>         unsigned budget;
>
> @@ -702,9 +702,9 @@ static unsigned wrpll_get_budget_for_freq(int clock)
>         return budget;
>  }
>
> -static void wrpll_update_rnp(uint64_t freq2k, unsigned budget,
> -                            unsigned r2, unsigned n2, unsigned p,
> -                            struct wrpll_rnp *best)
> +static void hsw_wrpll_update_rnp(uint64_t freq2k, unsigned budget,
> +                                unsigned r2, unsigned n2, unsigned p,
> +                                struct hsw_wrpll_rnp *best)
>  {
>         uint64_t a, b, c, d, diff, diff_best;
>
> @@ -761,8 +761,7 @@ static void wrpll_update_rnp(uint64_t freq2k, unsigned budget,
>         /* Otherwise a < c && b >= d, do nothing */
>  }
>
> -static int intel_ddi_calc_wrpll_link(struct drm_i915_private *dev_priv,
> -                                    int reg)
> +static int hsw_ddi_calc_wrpll_link(struct drm_i915_private *dev_priv, int reg)
>  {
>         int refclk = LC_FREQ;
>         int n, p, r;
> @@ -928,10 +927,10 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
>                 link_clock = 270000;
>                 break;
>         case PORT_CLK_SEL_WRPLL1:
> -               link_clock = intel_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL1);
> +               link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL1);
>                 break;
>         case PORT_CLK_SEL_WRPLL2:
> -               link_clock = intel_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL2);
> +               link_clock = hsw_ddi_calc_wrpll_link(dev_priv, WRPLL_CTL2);
>                 break;
>         case PORT_CLK_SEL_SPLL:
>                 pll = I915_READ(SPLL_CTL) & SPLL_PLL_FREQ_MASK;
> @@ -1010,12 +1009,12 @@ hsw_ddi_calculate_wrpll(int clock /* in Hz */,
>  {
>         uint64_t freq2k;
>         unsigned p, n2, r2;
> -       struct wrpll_rnp best = { 0, 0, 0 };
> +       struct hsw_wrpll_rnp best = { 0, 0, 0 };
>         unsigned budget;
>
>         freq2k = clock / 100;
>
> -       budget = wrpll_get_budget_for_freq(clock);
> +       budget = hsw_wrpll_get_budget_for_freq(clock);
>
>         /* Special case handling for 540 pixel clock: bypass WR PLL entirely
>          * and directly pass the LC PLL to it. */
> @@ -1059,8 +1058,8 @@ hsw_ddi_calculate_wrpll(int clock /* in Hz */,
>                      n2++) {
>
>                         for (p = P_MIN; p <= P_MAX; p += P_INC)
> -                               wrpll_update_rnp(freq2k, budget,
> -                                                r2, n2, p, &best);
> +                               hsw_wrpll_update_rnp(freq2k, budget,
> +                                                    r2, n2, p, &best);
>                 }
>         }
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 11/13] drm/i915/skl: Don't try to store the wrong central frequency

[Paulo Zanoni]

2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> The orignal code started by storing the actual central frequency (in Hz,
> using a uint64_t) in a uint32_t which codes for the register value. That
> can't be right.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index f3c5b49..a99fee1 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1117,8 +1117,6 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
>  {
>         uint64_t dco_freq;
>
> -       params->central_freq = central_freq;
> -
>         switch (central_freq) {

While at it, since it's related, can I ask you to add a default case
to this switch statement? You know, just to be sure... It can be in a
separate patch. And I guess the discussion of patch 9 will define how
that default case will look like.

With or without it: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>         case 9600000000ULL:
>                 params->central_freq = 0;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 12/13] drm/i915/skl: Replace the HDMI DPLL divider computation algorithm

[Paulo Zanoni]

2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> The HW validation team came back from further testing with a slightly
> changed constraint on the deviation between the DCO frequency and the
> central frequency. Instead of +-4%, it's now +1%/-6%.
>
> Unfortunately, the previous algorithm didn't quite cope with these new
> constraints, the reason being that it wasn't thorough enough looking at
> the possible divider candidates.
>
> The new algorithm looks at all dividers, which is definitely a hammer
> approach (we could reduce further the set of dividers to good ones as a
> follow up, at the cost of a bit more complicated code). But, at least,
> we can now satisfy the +1%/+6% rule for all the "Well known" HDMI
> frequencies of my test set (373 entries).
>
> On that subject, the new code is quite extensively tested in
> intel-gpu-tools (tools/skl_compute_wrpll).
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 207 +++++++++++++++++++++++++--------------
>  1 file changed, 133 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index a99fee1..381a8c9 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1100,6 +1100,99 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>         return true;
>  }
>
> +struct skl_wrpll_context {
> +       uint32_t min_pdeviation;        /* record the minimum deviations to */
> +       uint32_t min_ndeviation;        /* compare candidates */
> +       uint64_t central_freq;          /* chosen central freq */
> +       uint64_t dco_freq;              /* chosen dco freq */
> +       unsigned int p;                 /* chosen divider */
> +};
> +
> +static void skl_wrpll_context_init(struct skl_wrpll_context *ctx)
> +{
> +       memset(ctx, 0, sizeof(*ctx));
> +
> +       /* DCO freq must be within +1%/-6%  of the DCO central freq */
> +       ctx->min_pdeviation = 100;
> +       ctx-> min_ndeviation = 600;

Since this is called only once, you could just have initialized the
struct by the usual way...

Also, there's a white space on the "ctx-> min_ndeviation" part.

> +}
> +
> +static void skl_wrpll_try_divider(struct skl_wrpll_context *ctx,
> +                                 uint64_t central_freq,
> +                                 uint64_t dco_freq,
> +                                 unsigned int divider)
> +{
> +       uint64_t deviation;
> +
> +       deviation = div64_u64(10000 * abs_diff(dco_freq, central_freq),
> +                             central_freq);
> +
> +       /* positive deviation */
> +       if (dco_freq >= central_freq) {
> +               if (deviation < ctx->min_pdeviation) {
> +                       ctx->min_pdeviation = deviation;
> +                       ctx->central_freq = central_freq;
> +                       ctx->dco_freq = dco_freq;
> +                       ctx->p = divider;
> +               }
> +       /* negative deviation */
> +       } else if (deviation < ctx->min_ndeviation) {
> +               ctx->min_ndeviation = deviation;
> +               ctx->central_freq = central_freq;
> +               ctx->dco_freq = dco_freq;
> +               ctx->p = divider;
> +       }
> +}
> +
> +static void skl_wrpll_get_multipliers(unsigned int p,
> +                                     unsigned int *p0 /* out */,
> +                                     unsigned int *p1 /* out */,
> +                                     unsigned int *p2 /* out */)
> +{
> +       /* even dividers */
> +       if (p % 2 == 0) {
> +               unsigned int half = p / 2;
> +
> +               if (half == 1 || half == 2 || half == 3 || half == 5) {
> +                       *p0 = 2;
> +                       *p1 = 1;
> +                       *p2 = half;
> +               } else if (half % 2 == 0) {
> +                       *p0 = 2;
> +                       *p1 = half / 2;
> +                       *p2 = 2;
> +               } else if (half % 3 == 0) {
> +                       *p0 = 3;
> +                       *p1 = half / 3;
> +                       *p2 = 2;
> +               } else if (half % 7 == 0) {
> +                       *p0 = 7;
> +                       *p1 = half / 7;
> +                       *p2 = 2;
> +               }
> +       } else if (p == 3 || p == 9) {  /* 3, 5, 7, 9, 15, 21, 35 */
> +               *p0 = 3;
> +               *p1 = 1;
> +               *p2 = p / 3;
> +       } else if (p == 5 || p == 7) {
> +               *p0 = p;
> +               *p1 = 1;
> +               *p2 = 1;
> +       } else if (p == 15) {
> +               *p0 = 3;
> +               *p1 = 1;
> +               *p2 = 5;
> +       } else if (p == 21) {
> +               *p0 = 7;
> +               *p1 = 1;
> +               *p2 = 3;
> +       } else if (p == 35) {
> +               *p0 = 7;
> +               *p1 = 1;
> +               *p2 = 5;
> +       }
> +}
> +
>  struct skl_wrpll_params {
>         uint32_t        dco_fraction;
>         uint32_t        dco_integer;
> @@ -1185,90 +1278,56 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
>         uint64_t dco_central_freq[3] = {8400000000ULL,
>                                         9000000000ULL,
>                                         9600000000ULL};
> -       uint32_t min_dco_deviation = 400;
> -       uint32_t min_dco_index = 3;
> -       uint32_t P0[4] = {1, 2, 3, 7};
> -       uint32_t P2[4] = {1, 2, 3, 5};
> -       bool found = false;
> -       uint32_t candidate_p = 0;
> -       uint32_t candidate_p0[3] = {0}, candidate_p1[3] = {0};
> -       uint32_t candidate_p2[3] = {0};
> -       uint32_t dco_central_freq_deviation[3];
> -       uint32_t i, P1, k, dco_count;
> -       bool retry_with_odd = false;
> -
> -       /* Determine P0, P1 or P2 */
> -       for (dco_count = 0; dco_count < 3; dco_count++) {
> -               found = false;
> -               candidate_p =
> -                       div64_u64(dco_central_freq[dco_count], afe_clock);
> -               if (retry_with_odd == false)
> -                       candidate_p = (candidate_p % 2 == 0 ?
> -                               candidate_p : candidate_p + 1);
> -
> -               for (P1 = 1; P1 < candidate_p; P1++) {
> -                       for (i = 0; i < 4; i++) {
> -                               if (!(P0[i] != 1 || P1 == 1))
> -                                       continue;
> -
> -                               for (k = 0; k < 4; k++) {
> -                                       if (P1 != 1 && P2[k] != 2)
> -                                               continue;
> -
> -                                       if (candidate_p == P0[i] * P1 * P2[k]) {
> -                                               /* Found possible P0, P1, P2 */
> -                                               found = true;
> -                                               candidate_p0[dco_count] = P0[i];
> -                                               candidate_p1[dco_count] = P1;
> -                                               candidate_p2[dco_count] = P2[k];
> -                                               goto found;
> -                                       }
> -
> -                               }
> +       static const int even_dividers[] = {  4,  6,  8, 10, 12, 14, 16, 18, 20,
> +                                            24, 28, 30, 32, 36, 40, 42, 44,
> +                                            48, 52, 54, 56, 60, 64, 66, 68,
> +                                            70, 72, 76, 78, 80, 84, 88, 90,
> +                                            92, 96, 98 };
> +       static const int odd_dividers[] = { 3, 5, 7, 9, 15, 21, 35 };
> +       static const struct {
> +               const int *list;
> +               int n_dividers;
> +       } dividers[] = {
> +               { even_dividers, ARRAY_SIZE(even_dividers) },
> +               { odd_dividers, ARRAY_SIZE(odd_dividers) },
> +       };
> +       struct skl_wrpll_context ctx;
> +       unsigned int dco, d, i;
> +       unsigned int p0, p1, p2;
> +
> +       skl_wrpll_context_init(&ctx);
> +
> +       for (dco = 0; dco < ARRAY_SIZE(dco_central_freq); dco++) {
> +               for (d = 0; d < ARRAY_SIZE(dividers); d++) {

From what I can tell by reading the documentation, it really seems
that the two lines above should be inverted. First we do everything
for the even (including all DCOs), and only then for the odd. It's
easier to realize this when you look at the part of the doc that says,
in order: "next candidate divider \n next DCO central frequency \n
next divider parity". Anyway, this is not exactly a problem right now
since we never break the loop, but you change this on the next patch,
so it's probably best to discuss this here.

> +                       for (i = 0; i < dividers[d].n_dividers; i++) {
> +                               unsigned int p = dividers[d].list[i];
> +                               uint64_t dco_freq = p * afe_clock;
> +
> +                               skl_wrpll_try_divider(&ctx,
> +                                                     dco_central_freq[dco],
> +                                                     dco_freq,
> +                                                     p);
>                         }
>                 }
> -
> -found:
> -               if (found) {
> -                       dco_central_freq_deviation[dco_count] =
> -                               div64_u64(10000 *
> -                                         abs_diff(candidate_p * afe_clock,
> -                                                  dco_central_freq[dco_count]),
> -                                         dco_central_freq[dco_count]);
> -
> -                       if (dco_central_freq_deviation[dco_count] <
> -                               min_dco_deviation) {
> -                               min_dco_deviation =
> -                                       dco_central_freq_deviation[dco_count];
> -                               min_dco_index = dco_count;
> -                       }
> -               }
> -
> -               if (min_dco_index > 2 && dco_count == 2) {
> -                        /* oh well, we tried... */
> -                        if (retry_with_odd)
> -                                break;
> -
> -                       retry_with_odd = true;
> -                       dco_count = 0;
> -               }
>         }
>
> -       if (WARN(min_dco_index > 2,
> -                "No valid parameters found for pixel clock: %dHz\n", clock))
> +       if (!ctx.p) {
> +               DRM_DEBUG_DRIVER("No valid divider found for %dHz\n", clock);
>                 return false;
> +       }
>
> -       skl_wrpll_params_populate(wrpll_params,
> -                                 afe_clock,
> -                                 dco_central_freq[min_dco_index],
> -                                 candidate_p0[min_dco_index],
> -                                 candidate_p1[min_dco_index],
> -                                 candidate_p2[min_dco_index]);
> +       /*
> +        * gcc incorrectly analyses that these can be used without being
> +        * initialized. To be fair, it's hard to guess.

Why didn't you just write the mathematical proof as a comment? I hope
it's not too big to fit on the 80 column margin limit.

Jokes aside, the only "blocker" would be the DCO first vs odd/even
first. I'd like to hear your interpretation on this.

> +        */
> +       p0 = p1 = p2 = 0;
> +       skl_wrpll_get_multipliers(ctx.p, &p0, &p1, &p2);
> +       skl_wrpll_params_populate(wrpll_params, afe_clock, ctx.central_freq,
> +                                 p0, p1, p2);
>
>         return true;
>  }
>
> -
>  static bool
>  skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>                    struct intel_crtc_state *crtc_state,
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 13/13] drm/i915/skl: Prefer even dividers for SKL DPLLs

[Paulo Zanoni]

2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> Currently, if an odd divider improves the deviation (minimizes it), we
> take that divider. The recommendation is to prefer even dividers.

The doc says "It is preferred to get as close to the DCO central
frequency as possible, but using an even divider value takes
precedence.", but I'm wondering if they meant "prefer even over odd in
case they have the same deviation" or just "even divider is preferred
as long as it's on the deviation threshold, even if there's an odd
divider with minimal/no deviation". I see you implement the last
option - if you don't count the possible bug mentioned on my review of
patch 12.

Assuming the loop order will be fixed on patch 12, and assuming you
are correctly interpreting the spec, then your patch does what it
says, so: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>.

>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 381a8c9..54344c3 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1308,6 +1308,13 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
>                                                       dco_freq,
>                                                       p);
>                         }
> +
> +                       /*
> +                        * If a solution is found with an even divider, prefer
> +                        * this one.
> +                        */
> +                       if (d == 0 && ctx.p)
> +                               break;
>                 }
>         }
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 12/13] drm/i915/skl: Replace the HDMI DPLL divider computation algorithm

[Paulo Zanoni]

2015-05-27 18:28 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>:
> 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
>> The HW validation team came back from further testing with a slightly
>> changed constraint on the deviation between the DCO frequency and the
>> central frequency. Instead of +-4%, it's now +1%/-6%.
>>
>> Unfortunately, the previous algorithm didn't quite cope with these new
>> constraints, the reason being that it wasn't thorough enough looking at
>> the possible divider candidates.
>>
>> The new algorithm looks at all dividers, which is definitely a hammer
>> approach (we could reduce further the set of dividers to good ones as a
>> follow up, at the cost of a bit more complicated code). But, at least,
>> we can now satisfy the +1%/+6% rule for all the "Well known" HDMI
>> frequencies of my test set (373 entries).
>>
>> On that subject, the new code is quite extensively tested in
>> intel-gpu-tools (tools/skl_compute_wrpll).
>>
>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 207 +++++++++++++++++++++++++--------------
>>  1 file changed, 133 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index a99fee1..381a8c9 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1100,6 +1100,99 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>>         return true;
>>  }
>>
>> +struct skl_wrpll_context {
>> +       uint32_t min_pdeviation;        /* record the minimum deviations to */
>> +       uint32_t min_ndeviation;        /* compare candidates */
>> +       uint64_t central_freq;          /* chosen central freq */
>> +       uint64_t dco_freq;              /* chosen dco freq */
>> +       unsigned int p;                 /* chosen divider */
>> +};
>> +
>> +static void skl_wrpll_context_init(struct skl_wrpll_context *ctx)
>> +{
>> +       memset(ctx, 0, sizeof(*ctx));
>> +
>> +       /* DCO freq must be within +1%/-6%  of the DCO central freq */
>> +       ctx->min_pdeviation = 100;
>> +       ctx-> min_ndeviation = 600;
>
> Since this is called only once, you could just have initialized the
> struct by the usual way...
>
> Also, there's a white space on the "ctx-> min_ndeviation" part.
>
>> +}
>> +
>> +static void skl_wrpll_try_divider(struct skl_wrpll_context *ctx,
>> +                                 uint64_t central_freq,
>> +                                 uint64_t dco_freq,
>> +                                 unsigned int divider)
>> +{
>> +       uint64_t deviation;
>> +
>> +       deviation = div64_u64(10000 * abs_diff(dco_freq, central_freq),
>> +                             central_freq);
>> +
>> +       /* positive deviation */
>> +       if (dco_freq >= central_freq) {
>> +               if (deviation < ctx->min_pdeviation) {
>> +                       ctx->min_pdeviation = deviation;
>> +                       ctx->central_freq = central_freq;
>> +                       ctx->dco_freq = dco_freq;
>> +                       ctx->p = divider;
>> +               }
>> +       /* negative deviation */
>> +       } else if (deviation < ctx->min_ndeviation) {
>> +               ctx->min_ndeviation = deviation;
>> +               ctx->central_freq = central_freq;
>> +               ctx->dco_freq = dco_freq;
>> +               ctx->p = divider;
>> +       }

Some additional comments:

Don't we want to break the loop in case we reach a point where any of
the deviations is zero?

Also notice that, due to the way we loop over the variables at the
main func, we will always pick the last deviation that was "improved"
(positive or negative), as opposed to the very minimal deviation. In
other words, if the last thing our algorithm did was move the
ndeviation from 600 to 599, we will pick this, even if, in previous
iterations, we moved the pdeviation from 100 to 1. Is this really what
we want? Maybe we want to compare min_pdeviation against
min_ndeviation before picking central_freq, dco_freq and p?

Also, if we loop over the *odd* dividers before looping over the
*even* divers, and change the comparisons to "<=" instead of just "<",
and don't "break the loop in case we reach 0 variation" for the odd
dividers, then our algorithm will give preference to even dividers in
case we find the same deviation on both odd and even dividers (in
other words, this will give you the implementation of the alternative
interpretation of the spec, which we're discussing on patch 13).

I know you probably don't have any of these answers, so I won't block
the patch review on the problems on this email. But I'd at least like
to know your opinions here.Maybe we could send some additional
questions to the spec writers.

>> +}
>> +
>> +static void skl_wrpll_get_multipliers(unsigned int p,
>> +                                     unsigned int *p0 /* out */,
>> +                                     unsigned int *p1 /* out */,
>> +                                     unsigned int *p2 /* out */)
>> +{
>> +       /* even dividers */
>> +       if (p % 2 == 0) {
>> +               unsigned int half = p / 2;
>> +
>> +               if (half == 1 || half == 2 || half == 3 || half == 5) {
>> +                       *p0 = 2;
>> +                       *p1 = 1;
>> +                       *p2 = half;
>> +               } else if (half % 2 == 0) {
>> +                       *p0 = 2;
>> +                       *p1 = half / 2;
>> +                       *p2 = 2;
>> +               } else if (half % 3 == 0) {
>> +                       *p0 = 3;
>> +                       *p1 = half / 3;
>> +                       *p2 = 2;
>> +               } else if (half % 7 == 0) {
>> +                       *p0 = 7;
>> +                       *p1 = half / 7;
>> +                       *p2 = 2;
>> +               }
>> +       } else if (p == 3 || p == 9) {  /* 3, 5, 7, 9, 15, 21, 35 */
>> +               *p0 = 3;
>> +               *p1 = 1;
>> +               *p2 = p / 3;
>> +       } else if (p == 5 || p == 7) {
>> +               *p0 = p;
>> +               *p1 = 1;
>> +               *p2 = 1;
>> +       } else if (p == 15) {
>> +               *p0 = 3;
>> +               *p1 = 1;
>> +               *p2 = 5;
>> +       } else if (p == 21) {
>> +               *p0 = 7;
>> +               *p1 = 1;
>> +               *p2 = 3;
>> +       } else if (p == 35) {
>> +               *p0 = 7;
>> +               *p1 = 1;
>> +               *p2 = 5;
>> +       }
>> +}
>> +
>>  struct skl_wrpll_params {
>>         uint32_t        dco_fraction;
>>         uint32_t        dco_integer;
>> @@ -1185,90 +1278,56 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
>>         uint64_t dco_central_freq[3] = {8400000000ULL,
>>                                         9000000000ULL,
>>                                         9600000000ULL};
>> -       uint32_t min_dco_deviation = 400;
>> -       uint32_t min_dco_index = 3;
>> -       uint32_t P0[4] = {1, 2, 3, 7};
>> -       uint32_t P2[4] = {1, 2, 3, 5};
>> -       bool found = false;
>> -       uint32_t candidate_p = 0;
>> -       uint32_t candidate_p0[3] = {0}, candidate_p1[3] = {0};
>> -       uint32_t candidate_p2[3] = {0};
>> -       uint32_t dco_central_freq_deviation[3];
>> -       uint32_t i, P1, k, dco_count;
>> -       bool retry_with_odd = false;
>> -
>> -       /* Determine P0, P1 or P2 */
>> -       for (dco_count = 0; dco_count < 3; dco_count++) {
>> -               found = false;
>> -               candidate_p =
>> -                       div64_u64(dco_central_freq[dco_count], afe_clock);
>> -               if (retry_with_odd == false)
>> -                       candidate_p = (candidate_p % 2 == 0 ?
>> -                               candidate_p : candidate_p + 1);
>> -
>> -               for (P1 = 1; P1 < candidate_p; P1++) {
>> -                       for (i = 0; i < 4; i++) {
>> -                               if (!(P0[i] != 1 || P1 == 1))
>> -                                       continue;
>> -
>> -                               for (k = 0; k < 4; k++) {
>> -                                       if (P1 != 1 && P2[k] != 2)
>> -                                               continue;
>> -
>> -                                       if (candidate_p == P0[i] * P1 * P2[k]) {
>> -                                               /* Found possible P0, P1, P2 */
>> -                                               found = true;
>> -                                               candidate_p0[dco_count] = P0[i];
>> -                                               candidate_p1[dco_count] = P1;
>> -                                               candidate_p2[dco_count] = P2[k];
>> -                                               goto found;
>> -                                       }
>> -
>> -                               }
>> +       static const int even_dividers[] = {  4,  6,  8, 10, 12, 14, 16, 18, 20,
>> +                                            24, 28, 30, 32, 36, 40, 42, 44,
>> +                                            48, 52, 54, 56, 60, 64, 66, 68,
>> +                                            70, 72, 76, 78, 80, 84, 88, 90,
>> +                                            92, 96, 98 };
>> +       static const int odd_dividers[] = { 3, 5, 7, 9, 15, 21, 35 };
>> +       static const struct {
>> +               const int *list;
>> +               int n_dividers;
>> +       } dividers[] = {
>> +               { even_dividers, ARRAY_SIZE(even_dividers) },
>> +               { odd_dividers, ARRAY_SIZE(odd_dividers) },
>> +       };
>> +       struct skl_wrpll_context ctx;
>> +       unsigned int dco, d, i;
>> +       unsigned int p0, p1, p2;
>> +
>> +       skl_wrpll_context_init(&ctx);
>> +
>> +       for (dco = 0; dco < ARRAY_SIZE(dco_central_freq); dco++) {
>> +               for (d = 0; d < ARRAY_SIZE(dividers); d++) {
>
> From what I can tell by reading the documentation, it really seems
> that the two lines above should be inverted. First we do everything
> for the even (including all DCOs), and only then for the odd. It's
> easier to realize this when you look at the part of the doc that says,
> in order: "next candidate divider \n next DCO central frequency \n
> next divider parity". Anyway, this is not exactly a problem right now
> since we never break the loop, but you change this on the next patch,
> so it's probably best to discuss this here.
>
>> +                       for (i = 0; i < dividers[d].n_dividers; i++) {
>> +                               unsigned int p = dividers[d].list[i];
>> +                               uint64_t dco_freq = p * afe_clock;
>> +
>> +                               skl_wrpll_try_divider(&ctx,
>> +                                                     dco_central_freq[dco],
>> +                                                     dco_freq,
>> +                                                     p);
>>                         }
>>                 }
>> -
>> -found:
>> -               if (found) {
>> -                       dco_central_freq_deviation[dco_count] =
>> -                               div64_u64(10000 *
>> -                                         abs_diff(candidate_p * afe_clock,
>> -                                                  dco_central_freq[dco_count]),
>> -                                         dco_central_freq[dco_count]);
>> -
>> -                       if (dco_central_freq_deviation[dco_count] <
>> -                               min_dco_deviation) {
>> -                               min_dco_deviation =
>> -                                       dco_central_freq_deviation[dco_count];
>> -                               min_dco_index = dco_count;
>> -                       }
>> -               }
>> -
>> -               if (min_dco_index > 2 && dco_count == 2) {
>> -                        /* oh well, we tried... */
>> -                        if (retry_with_odd)
>> -                                break;
>> -
>> -                       retry_with_odd = true;
>> -                       dco_count = 0;
>> -               }
>>         }
>>
>> -       if (WARN(min_dco_index > 2,
>> -                "No valid parameters found for pixel clock: %dHz\n", clock))
>> +       if (!ctx.p) {
>> +               DRM_DEBUG_DRIVER("No valid divider found for %dHz\n", clock);
>>                 return false;
>> +       }
>>
>> -       skl_wrpll_params_populate(wrpll_params,
>> -                                 afe_clock,
>> -                                 dco_central_freq[min_dco_index],
>> -                                 candidate_p0[min_dco_index],
>> -                                 candidate_p1[min_dco_index],
>> -                                 candidate_p2[min_dco_index]);
>> +       /*
>> +        * gcc incorrectly analyses that these can be used without being
>> +        * initialized. To be fair, it's hard to guess.
>
> Why didn't you just write the mathematical proof as a comment? I hope
> it's not too big to fit on the 80 column margin limit.
>
> Jokes aside, the only "blocker" would be the DCO first vs odd/even
> first. I'd like to hear your interpretation on this.
>
>> +        */
>> +       p0 = p1 = p2 = 0;
>> +       skl_wrpll_get_multipliers(ctx.p, &p0, &p1, &p2);
>> +       skl_wrpll_params_populate(wrpll_params, afe_clock, ctx.central_freq,
>> +                                 p0, p1, p2);
>>
>>         return true;
>>  }
>>
>> -
>>  static bool
>>  skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>>                    struct intel_crtc_state *crtc_state,
>> --
>> 2.1.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni



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

Re: [PATCH 13/13] drm/i915/skl: Prefer even dividers for SKL DPLLs

[Paulo Zanoni]

2015-05-27 18:39 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>:
> 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
>> Currently, if an odd divider improves the deviation (minimizes it), we
>> take that divider. The recommendation is to prefer even dividers.
>
> The doc says "It is preferred to get as close to the DCO central
> frequency as possible, but using an even divider value takes
> precedence.", but I'm wondering if they meant "prefer even over odd in
> case they have the same deviation" or just "even divider is preferred
> as long as it's on the deviation threshold, even if there's an odd
> divider with minimal/no deviation". I see you implement the last
> option - if you don't count the possible bug mentioned on my review of
> patch 12.
>
> Assuming the loop order will be fixed on patch 12, and assuming you
> are correctly interpreting the spec, then your patch does what it
> says, so: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>.

I kept looking at the spec, and the section that describes the 4 steps
for the algorithm totally clarifies what's the correct interpretation.
Please see step 4. An "even" divider with any acceptable deviation is
preferred over any possible "odd" divider, it doesn't matter what is
the best deviation of the "odd" dividers.

Considering this, I think we really should invert the loops as I
suggested on patch 12, and then we should modify this patch so that it
breaks the loop only after we've also iterated over all DCOs. I guess
these changes are a requirement for the R-B tags on patches 12 and 13
as long as you don't have counter arguments.

We should still consider breaking the loop earlier in case we reach 0
deviation, and we should still consider comparing the pdeviation with
the ndeviation before picking central_freq, dco_freq and divider.
These things are not requirements for the R-B tags, but, as I said,
i'd like to see your opinion.

>
>>
>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 381a8c9..54344c3 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1308,6 +1308,13 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
>>                                                       dco_freq,
>>                                                       p);
>>                         }
>> +
>> +                       /*
>> +                        * If a solution is found with an even divider, prefer
>> +                        * this one.
>> +                        */
>> +                       if (d == 0 && ctx.p)
>> +                               break;
>>                 }
>>         }
>>
>> --
>> 2.1.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni



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

Re: [PATCH 02/13] drm/i915/skl: Make sure to break when not finding suitable PLL dividers

[Jani Nikula]

On Wed, 27 May 2015, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
>> Right now, when finishing the cycle with odd dividers without finding a
>> suitable candidate, we end up in an infinite loop. Make sure to break in
>> that case.
>
> Cc stable?

No, fixes for platforms that require the preliminary_hw_support flag are
neither for stable nor the current development kernel.

BR,
Jani.


>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
>>
>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 2e24fa4..da7aa0f 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1180,6 +1180,10 @@ found:
>>                 }
>>
>>                 if (min_dco_index > 2 && dco_count == 2) {
>> +                        /* oh well, we tried... */
>> +                        if (retry_with_odd)
>> +                                break;
>> +
>>                         retry_with_odd = true;
>>                         dco_count = 0;
>>                 }
>> --
>> 2.1.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 02/13] drm/i915/skl: Make sure to break when not finding suitable PLL dividers

[Daniel Vetter]

On Thu, May 07, 2015 at 06:38:38PM +0100, Damien Lespiau wrote:
> Right now, when finishing the cycle with odd dividers without finding a
> suitable candidate, we end up in an infinite loop. Make sure to break in
> that case.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2e24fa4..da7aa0f 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1180,6 +1180,10 @@ found:
>  		}
>  
>  		if (min_dco_index > 2 && dco_count == 2) {
> +                        /* oh well, we tried... */
> +                        if (retry_with_odd)
> +                                break;

Shouldn't we have a return value somewhere here and then indicate to
userspace that things seriously went wrong? The error code handling is
almost there already to pass it all back up to haswell_crtc_compute_clock.
-Daniel

> +
>  			retry_with_odd = true;
>  			dco_count = 0;
>  		}
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/13] drm/i915/skl: Display the WRPLL frequency we couldn't accomodate when failing

[Daniel Vetter]

On Thu, May 07, 2015 at 06:38:39PM +0100, Damien Lespiau wrote:
> This helps debugging.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index da7aa0f..ff5eb05 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1190,7 +1190,8 @@ found:
>  	}
>  
>  	if (min_dco_index > 2) {
> -		WARN(1, "No valid values found for the given pixel clock\n");
> +		WARN(1, "No valid parameters found for pixel clock: %dHz\n",
> +		     clock);

Same complaint here, this should return false and get forwarded eventually
to haswell_crtc_compute_clock. I'll leave patch 2&3 for now since they
need to be reworked anyway.
-Daniel

>  	} else {
>  		wrpll_params->central_freq = dco_central_freq[min_dco_index];
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/13] drm/i915/skl: Use MISSING_CASE() in skl_wrpll_params_populate()

[Daniel Vetter]

On Wed, May 27, 2015 at 03:40:32PM -0300, Paulo Zanoni wrote:
> 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> > We now have a special macro for those cases.
> 
> I'm not sure if this patch is an improvement. Before it, we always
> knew which "switch" statement was bad since we used to print either
> "PDiv" or "KDiv". After the patch, it will not be possible to know
> from which switch statement the error came from. Of course, there's
> the advantage of at least knowing the value. I'd vote to either skip
> this patch, or improve the MISSING_CASE macro to be able to account
> for multiple uses on the same function. But I'm open to arugmentation
> :)

MISSING_CASE is a WARN, which also prints the line number. Not enough?
-Daniel

> 
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index b9d5d65..ab327a1 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1145,7 +1145,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
> >                 params->pdiv = 4;
> >                 break;
> >         default:
> > -               WARN(1, "Incorrect PDiv\n");
> > +               MISSING_CASE(p0);
> >         }
> >
> >         switch (p2) {
> > @@ -1162,7 +1162,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
> >                 params->kdiv = 3;
> >                 break;
> >         default:
> > -               WARN(1, "Incorrect KDiv\n");
> > +               MISSING_CASE(p2);
> >         }
> >
> >         params->qdiv_ratio = p1;
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/13] drm/i915/skl: Don't try to store the wrong central frequency

[Daniel Vetter]

On Wed, May 27, 2015 at 04:58:46PM -0300, Paulo Zanoni wrote:
> 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> > The orignal code started by storing the actual central frequency (in Hz,
> > using a uint64_t) in a uint32_t which codes for the register value. That
> > can't be right.
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index f3c5b49..a99fee1 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1117,8 +1117,6 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
> >  {
> >         uint64_t dco_freq;
> >
> > -       params->central_freq = central_freq;
> > -
> >         switch (central_freq) {
> 
> While at it, since it's related, can I ask you to add a default case
> to this switch statement? You know, just to be sure... It can be in a
> separate patch. And I guess the discussion of patch 9 will define how
> that default case will look like.
> 
> With or without it: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Merged the reviewed patches to dinq, thanks.
-Daniel

> 
> >         case 9600000000ULL:
> >                 params->central_freq = 0;
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/13] drm/i915/skl: Make sure to break when not finding suitable PLL dividers

[Paulo Zanoni]

2015-05-28 4:45 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, May 07, 2015 at 06:38:38PM +0100, Damien Lespiau wrote:
>> Right now, when finishing the cycle with odd dividers without finding a
>> suitable candidate, we end up in an infinite loop. Make sure to break in
>> that case.
>>
>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 2e24fa4..da7aa0f 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1180,6 +1180,10 @@ found:
>>               }
>>
>>               if (min_dco_index > 2 && dco_count == 2) {
>> +                        /* oh well, we tried... */
>> +                        if (retry_with_odd)
>> +                                break;
>
> Shouldn't we have a return value somewhere here and then indicate to
> userspace that things seriously went wrong? The error code handling is
> almost there already to pass it all back up to haswell_crtc_compute_clock.

I guess you want what's on patch 4?

> -Daniel
>
>> +
>>                       retry_with_odd = true;
>>                       dco_count = 0;
>>               }
>> --
>> 2.1.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 09/13] drm/i915/skl: Use MISSING_CASE() in skl_wrpll_params_populate()

[Paulo Zanoni]

2015-05-28 4:51 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, May 27, 2015 at 03:40:32PM -0300, Paulo Zanoni wrote:
>> 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
>> > We now have a special macro for those cases.
>>
>> I'm not sure if this patch is an improvement. Before it, we always
>> knew which "switch" statement was bad since we used to print either
>> "PDiv" or "KDiv". After the patch, it will not be possible to know
>> from which switch statement the error came from. Of course, there's
>> the advantage of at least knowing the value. I'd vote to either skip
>> this patch, or improve the MISSING_CASE macro to be able to account
>> for multiple uses on the same function. But I'm open to arugmentation
>> :)
>
> MISSING_CASE is a WARN, which also prints the line number. Not enough?

Line numbers are not very useful unless you're absolutely sure which
tree/commit someone is running. And it still takes a lot of work to
checkout the correct tree/commit and discover which of the WARNs is on
that specific line.

> -Daniel
>
>>
>> >
>> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_ddi.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> > index b9d5d65..ab327a1 100644
>> > --- a/drivers/gpu/drm/i915/intel_ddi.c
>> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> > @@ -1145,7 +1145,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
>> >                 params->pdiv = 4;
>> >                 break;
>> >         default:
>> > -               WARN(1, "Incorrect PDiv\n");
>> > +               MISSING_CASE(p0);
>> >         }
>> >
>> >         switch (p2) {
>> > @@ -1162,7 +1162,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
>> >                 params->kdiv = 3;
>> >                 break;
>> >         default:
>> > -               WARN(1, "Incorrect KDiv\n");
>> > +               MISSING_CASE(p2);
>> >         }
>> >
>> >         params->qdiv_ratio = p1;
>> > --
>> > 2.1.0
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



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

Re: [PATCH 09/13] drm/i915/skl: Use MISSING_CASE() in skl_wrpll_params_populate()

[Damien Lespiau]

On Thu, May 28, 2015 at 11:06:57AM -0300, Paulo Zanoni wrote:
> 2015-05-28 4:51 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Wed, May 27, 2015 at 03:40:32PM -0300, Paulo Zanoni wrote:
> >> 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> >> > We now have a special macro for those cases.
> >>
> >> I'm not sure if this patch is an improvement. Before it, we always
> >> knew which "switch" statement was bad since we used to print either
> >> "PDiv" or "KDiv". After the patch, it will not be possible to know
> >> from which switch statement the error came from. Of course, there's
> >> the advantage of at least knowing the value. I'd vote to either skip
> >> this patch, or improve the MISSING_CASE macro to be able to account
> >> for multiple uses on the same function. But I'm open to arugmentation
> >> :)
> >
> > MISSING_CASE is a WARN, which also prints the line number. Not enough?
> 
> Line numbers are not very useful unless you're absolutely sure which
> tree/commit someone is running. And it still takes a lot of work to
> checkout the correct tree/commit and discover which of the WARNs is on
> that specific line.

Life is too short, let's drop this patch.

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

Re: [PATCH 12/13] drm/i915/skl: Replace the HDMI DPLL divider computation algorithm

[Damien Lespiau]

On Wed, May 27, 2015 at 06:28:06PM -0300, Paulo Zanoni wrote:
> > @@ -1185,90 +1278,56 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
> >         uint64_t dco_central_freq[3] = {8400000000ULL,
> >                                         9000000000ULL,
> >                                         9600000000ULL};
> > +       static const int even_dividers[] = {  4,  6,  8, 10, 12, 14, 16, 18, 20,
> > +                                            24, 28, 30, 32, 36, 40, 42, 44,
> > +                                            48, 52, 54, 56, 60, 64, 66, 68,
> > +                                            70, 72, 76, 78, 80, 84, 88, 90,
> > +                                            92, 96, 98 };
> > +       static const int odd_dividers[] = { 3, 5, 7, 9, 15, 21, 35 };
> > +       static const struct {
> > +               const int *list;
> > +               int n_dividers;
> > +       } dividers[] = {
> > +               { even_dividers, ARRAY_SIZE(even_dividers) },
> > +               { odd_dividers, ARRAY_SIZE(odd_dividers) },
> > +       };
> > +       struct skl_wrpll_context ctx;
> > +       unsigned int dco, d, i;
> > +       unsigned int p0, p1, p2;
> > +
> > +       skl_wrpll_context_init(&ctx);
> > +
> > +       for (dco = 0; dco < ARRAY_SIZE(dco_central_freq); dco++) {
> > +               for (d = 0; d < ARRAY_SIZE(dividers); d++) {
> 
> From what I can tell by reading the documentation, it really seems
> that the two lines above should be inverted. First we do everything
> for the even (including all DCOs), and only then for the odd. It's
> easier to realize this when you look at the part of the doc that says,
> in order: "next candidate divider \n next DCO central frequency \n
> next divider parity". Anyway, this is not exactly a problem right now
> since we never break the loop, but you change this on the next patch,
> so it's probably best to discuss this here.

You're absolutely right, those 2 lines need to be inverted.

> > +       /*
> > +        * gcc incorrectly analyses that these can be used without being
> > +        * initialized. To be fair, it's hard to guess.
> 
> Why didn't you just write the mathematical proof as a comment? I hope
> it's not too big to fit on the 80 column margin limit.
> 
> Jokes aside, the only "blocker" would be the DCO first vs odd/even
> first. I'd like to hear your interpretation on this.
> 
> > +        */
> > +       p0 = p1 = p2 = 0;
> > +       skl_wrpll_get_multipliers(ctx.p, &p0, &p1, &p2);
> > +       skl_wrpll_params_populate(wrpll_params, afe_clock, ctx.central_freq,
> > +                                 p0, p1, p2);

The "proof" is in tools/skl_compute_wrpll.c, in the test_multipliers()
function that unit-tests skl_wrpll_get_multipliers().

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

Re: [PATCH 12/13] drm/i915/skl: Replace the HDMI DPLL divider computation algorithm

[Damien Lespiau]

On Wed, May 27, 2015 at 06:51:13PM -0300, Paulo Zanoni wrote:
> >> +static void skl_wrpll_try_divider(struct skl_wrpll_context *ctx,
> >> +                                 uint64_t central_freq,
> >> +                                 uint64_t dco_freq,
> >> +                                 unsigned int divider)
> >> +{
> >> +       uint64_t deviation;
> >> +
> >> +       deviation = div64_u64(10000 * abs_diff(dco_freq, central_freq),
> >> +                             central_freq);
> >> +
> >> +       /* positive deviation */
> >> +       if (dco_freq >= central_freq) {
> >> +               if (deviation < ctx->min_pdeviation) {
> >> +                       ctx->min_pdeviation = deviation;
> >> +                       ctx->central_freq = central_freq;
> >> +                       ctx->dco_freq = dco_freq;
> >> +                       ctx->p = divider;
> >> +               }
> >> +       /* negative deviation */
> >> +       } else if (deviation < ctx->min_ndeviation) {
> >> +               ctx->min_ndeviation = deviation;
> >> +               ctx->central_freq = central_freq;
> >> +               ctx->dco_freq = dco_freq;
> >> +               ctx->p = divider;
> >> +       }
> 
> Some additional comments:
> 
> Don't we want to break the loop in case we reach a point where any of
> the deviations is zero?

We could, haven't done it in the v2 of the patch, could be done as a
follow up.

> Also notice that, due to the way we loop over the variables at the
> main func, we will always pick the last deviation that was "improved"
> (positive or negative), as opposed to the very minimal deviation. In
> other words, if the last thing our algorithm did was move the
> ndeviation from 600 to 599, we will pick this, even if, in previous
> iterations, we moved the pdeviation from 100 to 1. Is this really what
> we want? Maybe we want to compare min_pdeviation against
> min_ndeviation before picking central_freq, dco_freq and p?

That seems to be a (pretty big!) deficiency of the documented algorithm,
that's definitely something improving the average deviation in the i-g-t
test (average deviation 206.52 Vs 194.47)

> Also, if we loop over the *odd* dividers before looping over the
> *even* divers, and change the comparisons to "<=" instead of just "<",
> and don't "break the loop in case we reach 0 variation" for the odd
> dividers, then our algorithm will give preference to even dividers in
> case we find the same deviation on both odd and even dividers (in
> other words, this will give you the implementation of the alternative
> interpretation of the spec, which we're discussing on patch 13).

But then it would not use an even divider that has a slightly worse
deviation. My reading of the specs is:

  - If there's an even divider with the deviation fitting in the +1%/-6%
    constraint, choose it. (if there are several such even dividers,
    choose the one minimizing the deviation)
  - If we can't find an even divider within +1%/-6%, settle for an odd
    divider that satisfies those constraints.

> I know you probably don't have any of these answers, so I won't block
> the patch review on the problems on this email. But I'd at least like
> to know your opinions here.Maybe we could send some additional
> questions to the spec writers.

I do have some answers :) at least assuming the interpreation above is
the correct one. The two big suggestions you have make quite a
difference in the 2 metrics I gather in the i-g-t test:

v1 of this series:
  even/odd dividers: 338/35
  average deviation: 206.52

v2 of this series:
  even/odd dividers: 363/10
  average deviation: 194.47

It's also easier to follow the changes as diffs in the corresponding
i-g-t series than the v2 of the kernel patches.

testdisplay still cycles through the modes on the HDMI displays I have.

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

[PATCH 12/13 v2] drm/i915/skl: Replace the HDMI DPLL divider computation algorithm

[Damien Lespiau]

The HW validation team came back from further testing with a slightly
changed constraint on the deviation between the DCO frequency and the
central frequency. Instead of +-4%, it's now +1%/-6%.

Unfortunately, the previous algorithm didn't quite cope with these new
constraints, the reason being that it wasn't thorough enough looking at
the possible divider candidates.

The new algorithm looks at all dividers, which is definitely a hammer
approach (we could reduce further the set of dividers to good ones as a
follow up, at the cost of a bit more complicated code). But, at least,
we can now satisfy the +1%/+6% rule for all the "Well known" HDMI
frequencies of my test set (373 entries).

On that subject, the new code is quite extensively tested in
intel-gpu-tools (tools/skl_compute_wrpll).

v2: Fix cycling between central frequencies and dividers (Paulo)
    Properly choose the minimal deviation between postive and negative
    candidates (Paulo).

    On the 373 test frequencies, v2 computes better dividers than v1 (ie
    more even dividers and lower deviation on average):

    v1: average deviation: 206.52
    v2: average deviation: 194.47

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 211 +++++++++++++++++++++++++--------------
 1 file changed, 137 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 31b29e8..6e964ef 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1104,6 +1104,103 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
 	return true;
 }
 
+struct skl_wrpll_context {
+	uint64_t min_deviation;		/* current minimal deviation */
+	uint64_t central_freq;		/* chosen central freq */
+	uint64_t dco_freq;		/* chosen dco freq */
+	unsigned int p;			/* chosen divider */
+};
+
+static void skl_wrpll_context_init(struct skl_wrpll_context *ctx)
+{
+	memset(ctx, 0, sizeof(*ctx));
+
+	ctx->min_deviation = U64_MAX;
+}
+
+/* DCO freq must be within +1%/-6%  of the DCO central freq */
+#define SKL_DCO_MAX_PDEVIATION	100
+#define SKL_DCO_MAX_NDEVIATION	600
+
+static void skl_wrpll_try_divider(struct skl_wrpll_context *ctx,
+				  uint64_t central_freq,
+				  uint64_t dco_freq,
+				  unsigned int divider)
+{
+	uint64_t deviation;
+
+	deviation = div64_u64(10000 * abs_diff(dco_freq, central_freq),
+			      central_freq);
+
+	/* positive deviation */
+	if (dco_freq >= central_freq) {
+		if (deviation < SKL_DCO_MAX_PDEVIATION &&
+		    deviation < ctx->min_deviation) {
+			ctx->min_deviation = deviation;
+			ctx->central_freq = central_freq;
+			ctx->dco_freq = dco_freq;
+			ctx->p = divider;
+		}
+	/* negative deviation */
+	} else if (deviation < SKL_DCO_MAX_NDEVIATION &&
+		   deviation < ctx->min_deviation) {
+		ctx->min_deviation = deviation;
+		ctx->central_freq = central_freq;
+		ctx->dco_freq = dco_freq;
+		ctx->p = divider;
+	}
+
+}
+
+static void skl_wrpll_get_multipliers(unsigned int p,
+				      unsigned int *p0 /* out */,
+				      unsigned int *p1 /* out */,
+				      unsigned int *p2 /* out */)
+{
+	/* even dividers */
+	if (p % 2 == 0) {
+		unsigned int half = p / 2;
+
+		if (half == 1 || half == 2 || half == 3 || half == 5) {
+			*p0 = 2;
+			*p1 = 1;
+			*p2 = half;
+		} else if (half % 2 == 0) {
+			*p0 = 2;
+			*p1 = half / 2;
+			*p2 = 2;
+		} else if (half % 3 == 0) {
+			*p0 = 3;
+			*p1 = half / 3;
+			*p2 = 2;
+		} else if (half % 7 == 0) {
+			*p0 = 7;
+			*p1 = half / 7;
+			*p2 = 2;
+		}
+	} else if (p == 3 || p == 9) {  /* 3, 5, 7, 9, 15, 21, 35 */
+		*p0 = 3;
+		*p1 = 1;
+		*p2 = p / 3;
+	} else if (p == 5 || p == 7) {
+		*p0 = p;
+		*p1 = 1;
+		*p2 = 1;
+	} else if (p == 15) {
+		*p0 = 3;
+		*p1 = 1;
+		*p2 = 5;
+	} else if (p == 21) {
+		*p0 = 7;
+		*p1 = 1;
+		*p2 = 3;
+	} else if (p == 35) {
+		*p0 = 7;
+		*p1 = 1;
+		*p2 = 5;
+	}
+}
+
 struct skl_wrpll_params {
 	uint32_t        dco_fraction;
 	uint32_t        dco_integer;
@@ -1189,90 +1286,56 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
 	uint64_t dco_central_freq[3] = {8400000000ULL,
 					9000000000ULL,
 					9600000000ULL};
-	uint32_t min_dco_deviation = 400;
-	uint32_t min_dco_index = 3;
-	uint32_t P0[4] = {1, 2, 3, 7};
-	uint32_t P2[4] = {1, 2, 3, 5};
-	bool found = false;
-	uint32_t candidate_p = 0;
-	uint32_t candidate_p0[3] = {0}, candidate_p1[3] = {0};
-	uint32_t candidate_p2[3] = {0};
-	uint32_t dco_central_freq_deviation[3];
-	uint32_t i, P1, k, dco_count;
-	bool retry_with_odd = false;
-
-	/* Determine P0, P1 or P2 */
-	for (dco_count = 0; dco_count < 3; dco_count++) {
-		found = false;
-		candidate_p =
-			div64_u64(dco_central_freq[dco_count], afe_clock);
-		if (retry_with_odd == false)
-			candidate_p = (candidate_p % 2 == 0 ?
-				candidate_p : candidate_p + 1);
-
-		for (P1 = 1; P1 < candidate_p; P1++) {
-			for (i = 0; i < 4; i++) {
-				if (!(P0[i] != 1 || P1 == 1))
-					continue;
-
-				for (k = 0; k < 4; k++) {
-					if (P1 != 1 && P2[k] != 2)
-						continue;
-
-					if (candidate_p == P0[i] * P1 * P2[k]) {
-						/* Found possible P0, P1, P2 */
-						found = true;
-						candidate_p0[dco_count] = P0[i];
-						candidate_p1[dco_count] = P1;
-						candidate_p2[dco_count] = P2[k];
-						goto found;
-					}
-
-				}
-			}
-		}
-
-found:
-		if (found) {
-			dco_central_freq_deviation[dco_count] =
-				div64_u64(10000 *
-					  abs_diff(candidate_p * afe_clock,
-						   dco_central_freq[dco_count]),
-					  dco_central_freq[dco_count]);
-
-			if (dco_central_freq_deviation[dco_count] <
-				min_dco_deviation) {
-				min_dco_deviation =
-					dco_central_freq_deviation[dco_count];
-				min_dco_index = dco_count;
+	static const int even_dividers[] = {  4,  6,  8, 10, 12, 14, 16, 18, 20,
+					     24, 28, 30, 32, 36, 40, 42, 44,
+					     48, 52, 54, 56, 60, 64, 66, 68,
+					     70, 72, 76, 78, 80, 84, 88, 90,
+					     92, 96, 98 };
+	static const int odd_dividers[] = { 3, 5, 7, 9, 15, 21, 35 };
+	static const struct {
+		const int *list;
+		int n_dividers;
+	} dividers[] = {
+		{ even_dividers, ARRAY_SIZE(even_dividers) },
+		{ odd_dividers, ARRAY_SIZE(odd_dividers) },
+	};
+	struct skl_wrpll_context ctx;
+	unsigned int dco, d, i;
+	unsigned int p0, p1, p2;
+
+	skl_wrpll_context_init(&ctx);
+
+	for (d = 0; d < ARRAY_SIZE(dividers); d++) {
+		for (dco = 0; dco < ARRAY_SIZE(dco_central_freq); dco++) {
+			for (i = 0; i < dividers[d].n_dividers; i++) {
+				unsigned int p = dividers[d].list[i];
+				uint64_t dco_freq = p * afe_clock;
+
+				skl_wrpll_try_divider(&ctx,
+						      dco_central_freq[dco],
+						      dco_freq,
+						      p);
 			}
 		}
-
-		if (min_dco_index > 2 && dco_count == 2) {
-                        /* oh well, we tried... */
-                        if (retry_with_odd)
-                                break;
-
-			retry_with_odd = true;
-			dco_count = 0;
-		}
 	}
 
-	if (WARN(min_dco_index > 2,
-		 "No valid parameters found for pixel clock: %dHz\n", clock))
+	if (!ctx.p) {
+		DRM_DEBUG_DRIVER("No valid divider found for %dHz\n", clock);
 		return false;
+	}
 
-	skl_wrpll_params_populate(wrpll_params,
-				  afe_clock,
-				  dco_central_freq[min_dco_index],
-				  candidate_p0[min_dco_index],
-				  candidate_p1[min_dco_index],
-				  candidate_p2[min_dco_index]);
+	/*
+	 * gcc incorrectly analyses that these can be used without being
+	 * initialized. To be fair, it's hard to guess.
+	 */
+	p0 = p1 = p2 = 0;
+	skl_wrpll_get_multipliers(ctx.p, &p0, &p1, &p2);
+	skl_wrpll_params_populate(wrpll_params, afe_clock, ctx.central_freq,
+				  p0, p1, p2);
 
 	return true;
 }
 
-
 static bool
 skl_ddi_pll_select(struct intel_crtc *intel_crtc,
 		   struct intel_crtc_state *crtc_state,
-- 
2.1.0

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

Re: [PATCH 13/13] drm/i915/skl: Prefer even dividers for SKL DPLLs

[Damien Lespiau]

On Wed, May 27, 2015 at 07:08:38PM -0300, Paulo Zanoni wrote:
> 2015-05-27 18:39 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>:
> > 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> >> Currently, if an odd divider improves the deviation (minimizes it), we
> >> take that divider. The recommendation is to prefer even dividers.
> >
> > The doc says "It is preferred to get as close to the DCO central
> > frequency as possible, but using an even divider value takes
> > precedence.", but I'm wondering if they meant "prefer even over odd in
> > case they have the same deviation" or just "even divider is preferred
> > as long as it's on the deviation threshold, even if there's an odd
> > divider with minimal/no deviation". I see you implement the last
> > option - if you don't count the possible bug mentioned on my review of
> > patch 12.
> >
> > Assuming the loop order will be fixed on patch 12, and assuming you
> > are correctly interpreting the spec, then your patch does what it
> > says, so: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>.
> 
> I kept looking at the spec, and the section that describes the 4 steps
> for the algorithm totally clarifies what's the correct interpretation.
> Please see step 4. An "even" divider with any acceptable deviation is
> preferred over any possible "odd" divider, it doesn't matter what is
> the best deviation of the "odd" dividers.

Right, too bad I read that last, but that's indeed what I think and well
and answered a previous comment with that already.

> Considering this, I think we really should invert the loops as I
> suggested on patch 12, and then we should modify this patch so that it
> breaks the loop only after we've also iterated over all DCOs. I guess
> these changes are a requirement for the R-B tags on patches 12 and 13
> as long as you don't have counter arguments.

Yup, agreed.

> We should still consider breaking the loop earlier in case we reach 0
> deviation, and we should still consider comparing the pdeviation with
> the ndeviation before picking central_freq, dco_freq and divider.
> These things are not requirements for the R-B tags, but, as I said,
> i'd like to see your opinion.

With v2, the break on deviation 0 would just be a "fast path" because we
can't improve on that. Still worth doing as a follow up (I like having
real diffs rather than v2/v3 with all the changes squashed).

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

[PATCH 13/13 v2] drm/i915/skl: Prefer even dividers for SKL DPLLs

[Damien Lespiau]

Currently, if an odd divider improves the deviation (minimizes it), we
take that divider. The recommendation is to prefer even dividers.

v2: Move the check at the right place after having inverted the two for
    loops in the previous patch.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 6e964ef..f6b3ccc 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1317,6 +1317,13 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
 						      p);
 			}
 		}
+
+		/*
+		 * If a solution is found with an even divider, prefer
+		 * this one.
+		 */
+		if (d == 0 && ctx.p)
+			break;
 	}
 
 	if (!ctx.p) {
-- 
2.1.0

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

Re: [PATCH 13/13 v2] drm/i915/skl: Prefer even dividers for SKL DPLLs

[Paulo Zanoni]

2015-06-25 12:19 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> Currently, if an odd divider improves the deviation (minimizes it), we
> take that divider. The recommendation is to prefer even dividers.
>
> v2: Move the check at the right place after having inverted the two for
>     loops in the previous patch.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 6e964ef..f6b3ccc 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1317,6 +1317,13 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
>                                                       p);
>                         }
>                 }
> +
> +               /*
> +                * If a solution is found with an even divider, prefer
> +                * this one.
> +                */
> +               if (d == 0 && ctx.p)
> +                       break;
>         }
>
>         if (!ctx.p) {
> --
> 2.1.0
>



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

Re: [PATCH 12/13 v2] drm/i915/skl: Replace the HDMI DPLL divider computation algorithm

[Paulo Zanoni]

2015-06-25 12:15 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> The HW validation team came back from further testing with a slightly
> changed constraint on the deviation between the DCO frequency and the
> central frequency. Instead of +-4%, it's now +1%/-6%.
>
> Unfortunately, the previous algorithm didn't quite cope with these new
> constraints, the reason being that it wasn't thorough enough looking at
> the possible divider candidates.
>
> The new algorithm looks at all dividers, which is definitely a hammer
> approach (we could reduce further the set of dividers to good ones as a
> follow up, at the cost of a bit more complicated code). But, at least,
> we can now satisfy the +1%/+6% rule for all the "Well known" HDMI
> frequencies of my test set (373 entries).
>
> On that subject, the new code is quite extensively tested in
> intel-gpu-tools (tools/skl_compute_wrpll).
>
> v2: Fix cycling between central frequencies and dividers (Paulo)
>     Properly choose the minimal deviation between postive and negative
>     candidates (Paulo).
>
>     On the 373 test frequencies, v2 computes better dividers than v1 (ie
>     more even dividers and lower deviation on average):
>
>     v1: average deviation: 206.52
>     v2: average deviation: 194.47
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 211 +++++++++++++++++++++++++--------------
>  1 file changed, 137 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 31b29e8..6e964ef 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1104,6 +1104,103 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>         return true;
>  }
>
> +struct skl_wrpll_context {
> +       uint64_t min_deviation;         /* current minimal deviation */
> +       uint64_t central_freq;          /* chosen central freq */
> +       uint64_t dco_freq;              /* chosen dco freq */
> +       unsigned int p;                 /* chosen divider */
> +};
> +
> +static void skl_wrpll_context_init(struct skl_wrpll_context *ctx)
> +{
> +       memset(ctx, 0, sizeof(*ctx));
> +
> +       ctx->min_deviation = U64_MAX;
> +}
> +
> +/* DCO freq must be within +1%/-6%  of the DCO central freq */
> +#define SKL_DCO_MAX_PDEVIATION 100
> +#define SKL_DCO_MAX_NDEVIATION 600
> +
> +static void skl_wrpll_try_divider(struct skl_wrpll_context *ctx,
> +                                 uint64_t central_freq,
> +                                 uint64_t dco_freq,
> +                                 unsigned int divider)
> +{
> +       uint64_t deviation;
> +
> +       deviation = div64_u64(10000 * abs_diff(dco_freq, central_freq),
> +                             central_freq);
> +
> +       /* positive deviation */
> +       if (dco_freq >= central_freq) {
> +               if (deviation < SKL_DCO_MAX_PDEVIATION &&
> +                   deviation < ctx->min_deviation) {
> +                       ctx->min_deviation = deviation;
> +                       ctx->central_freq = central_freq;
> +                       ctx->dco_freq = dco_freq;
> +                       ctx->p = divider;
> +               }
> +       /* negative deviation */
> +       } else if (deviation < SKL_DCO_MAX_NDEVIATION &&
> +                  deviation < ctx->min_deviation) {
> +               ctx->min_deviation = deviation;
> +               ctx->central_freq = central_freq;
> +               ctx->dco_freq = dco_freq;
> +               ctx->p = divider;
> +       }
> +
> +}
> +
> +static void skl_wrpll_get_multipliers(unsigned int p,
> +                                     unsigned int *p0 /* out */,
> +                                     unsigned int *p1 /* out */,
> +                                     unsigned int *p2 /* out */)
> +{
> +       /* even dividers */
> +       if (p % 2 == 0) {
> +               unsigned int half = p / 2;
> +
> +               if (half == 1 || half == 2 || half == 3 || half == 5) {
> +                       *p0 = 2;
> +                       *p1 = 1;
> +                       *p2 = half;
> +               } else if (half % 2 == 0) {
> +                       *p0 = 2;
> +                       *p1 = half / 2;
> +                       *p2 = 2;
> +               } else if (half % 3 == 0) {
> +                       *p0 = 3;
> +                       *p1 = half / 3;
> +                       *p2 = 2;
> +               } else if (half % 7 == 0) {
> +                       *p0 = 7;
> +                       *p1 = half / 7;
> +                       *p2 = 2;
> +               }
> +       } else if (p == 3 || p == 9) {  /* 3, 5, 7, 9, 15, 21, 35 */
> +               *p0 = 3;
> +               *p1 = 1;
> +               *p2 = p / 3;
> +       } else if (p == 5 || p == 7) {
> +               *p0 = p;
> +               *p1 = 1;
> +               *p2 = 1;
> +       } else if (p == 15) {
> +               *p0 = 3;
> +               *p1 = 1;
> +               *p2 = 5;
> +       } else if (p == 21) {
> +               *p0 = 7;
> +               *p1 = 1;
> +               *p2 = 3;
> +       } else if (p == 35) {
> +               *p0 = 7;
> +               *p1 = 1;
> +               *p2 = 5;
> +       }
> +}
> +
>  struct skl_wrpll_params {
>         uint32_t        dco_fraction;
>         uint32_t        dco_integer;
> @@ -1189,90 +1286,56 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
>         uint64_t dco_central_freq[3] = {8400000000ULL,
>                                         9000000000ULL,
>                                         9600000000ULL};
> -       uint32_t min_dco_deviation = 400;
> -       uint32_t min_dco_index = 3;
> -       uint32_t P0[4] = {1, 2, 3, 7};
> -       uint32_t P2[4] = {1, 2, 3, 5};
> -       bool found = false;
> -       uint32_t candidate_p = 0;
> -       uint32_t candidate_p0[3] = {0}, candidate_p1[3] = {0};
> -       uint32_t candidate_p2[3] = {0};
> -       uint32_t dco_central_freq_deviation[3];
> -       uint32_t i, P1, k, dco_count;
> -       bool retry_with_odd = false;
> -
> -       /* Determine P0, P1 or P2 */
> -       for (dco_count = 0; dco_count < 3; dco_count++) {
> -               found = false;
> -               candidate_p =
> -                       div64_u64(dco_central_freq[dco_count], afe_clock);
> -               if (retry_with_odd == false)
> -                       candidate_p = (candidate_p % 2 == 0 ?
> -                               candidate_p : candidate_p + 1);
> -
> -               for (P1 = 1; P1 < candidate_p; P1++) {
> -                       for (i = 0; i < 4; i++) {
> -                               if (!(P0[i] != 1 || P1 == 1))
> -                                       continue;
> -
> -                               for (k = 0; k < 4; k++) {
> -                                       if (P1 != 1 && P2[k] != 2)
> -                                               continue;
> -
> -                                       if (candidate_p == P0[i] * P1 * P2[k]) {
> -                                               /* Found possible P0, P1, P2 */
> -                                               found = true;
> -                                               candidate_p0[dco_count] = P0[i];
> -                                               candidate_p1[dco_count] = P1;
> -                                               candidate_p2[dco_count] = P2[k];
> -                                               goto found;
> -                                       }
> -
> -                               }
> -                       }
> -               }
> -
> -found:
> -               if (found) {
> -                       dco_central_freq_deviation[dco_count] =
> -                               div64_u64(10000 *
> -                                         abs_diff(candidate_p * afe_clock,
> -                                                  dco_central_freq[dco_count]),
> -                                         dco_central_freq[dco_count]);
> -
> -                       if (dco_central_freq_deviation[dco_count] <
> -                               min_dco_deviation) {
> -                               min_dco_deviation =
> -                                       dco_central_freq_deviation[dco_count];
> -                               min_dco_index = dco_count;
> +       static const int even_dividers[] = {  4,  6,  8, 10, 12, 14, 16, 18, 20,
> +                                            24, 28, 30, 32, 36, 40, 42, 44,
> +                                            48, 52, 54, 56, 60, 64, 66, 68,
> +                                            70, 72, 76, 78, 80, 84, 88, 90,
> +                                            92, 96, 98 };
> +       static const int odd_dividers[] = { 3, 5, 7, 9, 15, 21, 35 };
> +       static const struct {
> +               const int *list;
> +               int n_dividers;
> +       } dividers[] = {
> +               { even_dividers, ARRAY_SIZE(even_dividers) },
> +               { odd_dividers, ARRAY_SIZE(odd_dividers) },
> +       };
> +       struct skl_wrpll_context ctx;
> +       unsigned int dco, d, i;
> +       unsigned int p0, p1, p2;
> +
> +       skl_wrpll_context_init(&ctx);
> +
> +       for (d = 0; d < ARRAY_SIZE(dividers); d++) {
> +               for (dco = 0; dco < ARRAY_SIZE(dco_central_freq); dco++) {
> +                       for (i = 0; i < dividers[d].n_dividers; i++) {
> +                               unsigned int p = dividers[d].list[i];
> +                               uint64_t dco_freq = p * afe_clock;
> +
> +                               skl_wrpll_try_divider(&ctx,
> +                                                     dco_central_freq[dco],
> +                                                     dco_freq,
> +                                                     p);
>                         }
>                 }
> -
> -               if (min_dco_index > 2 && dco_count == 2) {
> -                        /* oh well, we tried... */
> -                        if (retry_with_odd)
> -                                break;
> -
> -                       retry_with_odd = true;
> -                       dco_count = 0;
> -               }
>         }
>
> -       if (WARN(min_dco_index > 2,
> -                "No valid parameters found for pixel clock: %dHz\n", clock))
> +       if (!ctx.p) {
> +               DRM_DEBUG_DRIVER("No valid divider found for %dHz\n", clock);
>                 return false;
> +       }
>
> -       skl_wrpll_params_populate(wrpll_params,
> -                                 afe_clock,
> -                                 dco_central_freq[min_dco_index],
> -                                 candidate_p0[min_dco_index],
> -                                 candidate_p1[min_dco_index],
> -                                 candidate_p2[min_dco_index]);
> +       /*
> +        * gcc incorrectly analyses that these can be used without being
> +        * initialized. To be fair, it's hard to guess.
> +        */
> +       p0 = p1 = p2 = 0;
> +       skl_wrpll_get_multipliers(ctx.p, &p0, &p1, &p2);
> +       skl_wrpll_params_populate(wrpll_params, afe_clock, ctx.central_freq,
> +                                 p0, p1, p2);
>
>         return true;
>  }
>
> -
>  static bool
>  skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>                    struct intel_crtc_state *crtc_state,
> --
> 2.1.0
>



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

Re: [PATCH 13/13 v2] drm/i915/skl: Prefer even dividers for SKL DPLLs

[Daniel Vetter]

On Fri, Jun 26, 2015 at 02:08:30PM -0300, Paulo Zanoni wrote:
> 2015-06-25 12:19 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> > Currently, if an odd divider improves the deviation (minimizes it), we
> > take that divider. The recommendation is to prefer even dividers.
> >
> > v2: Move the check at the right place after having inverted the two for
> >     loops in the previous patch.
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Last two patches merged, thanks.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 6e964ef..f6b3ccc 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1317,6 +1317,13 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
> >                                                       p);
> >                         }
> >                 }
> > +
> > +               /*
> > +                * If a solution is found with an even divider, prefer
> > +                * this one.
> > +                */
> > +               if (d == 0 && ctx.p)
> > +                       break;
> >         }
> >
> >         if (!ctx.p) {
> > --
> > 2.1.0
> >
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx