[Intel-gfx] [PATCH 12/13] drm/i915/skl: Replace the HDMI DPLL divider computation algorithm
Paulo Zanoni
przanoni at gmail.com
Wed May 27 14:28:06 PDT 2015
2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
More information about the Intel-gfx
mailing list