2015-05-27 18:28 GMT-03:00 Paulo Zanoni <przanoni@xxxxxxxxx>:
> 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@xxxxxxxxx>:
>> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
