Re: [PATCH 37/49] drm/i915: factor out vlv_PLL_is_optimal

Jesse Barnes
2015-03-19T20:31:58Z

On 03/17/2015 02:40 AM, Imre Deak wrote:
> Factor out the logic to decide whether the newly calculated dividers are
> better than the best found so far. Do this for clarity and to prepare
> for the upcoming BXT helper needing the same.
>
> No functional change.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++++++++++++++----------
> 1 file changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 95ce0a8..7feb047 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -776,6 +776,33 @@ g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> return found;
> }
>
> +/*
> + * Check if the calculated PLL configuration is more optimal compared to the
> + * best configuration and error found so far. Return the calculated error.
> + */
> +static bool vlv_PLL_is_optimal(struct drm_device *dev, int target_freq,
> + const intel_clock_t *calculated_clock,
> + const intel_clock_t *best_clock,
> + unsigned int best_error_ppm,
> + unsigned int *error_ppm)
> +{
> + *error_ppm = div_u64(1000000ULL *
> + abs(target_freq - calculated_clock->dot),
> + target_freq);
> + /*
> + * Prefer a better P value over a better (smaller) error if the error
> + * is small. Ensure this preference for future configurations too by
> + * setting the error to 0.
> + */
> + if (*error_ppm < 100 && calculated_clock->p > best_clock->p) {
> + *error_ppm = 0;
> +
> + return true;
> + }
> +
> + return *error_ppm + 10 < best_error_ppm;
> +}
> +
> static bool
> vlv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> int target, int refclk, intel_clock_t *match_clock,
> @@ -800,7 +827,7 @@ vlv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> clock.p = clock.p1 * clock.p2;
> /* based on hardware requirement, prefer bigger m1,m2 values */
> for (clock.m1 = limit->m1.min; clock.m1 <= limit->m1.max; clock.m1++) {
> - unsigned int ppm, diff;
> + unsigned int ppm;
>
> clock.m2 = DIV_ROUND_CLOSEST(target * clock.p * clock.n,
> refclk * clock.m1);
> @@ -811,20 +838,15 @@ vlv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
> &clock))
> continue;
>
> - diff = abs(clock.dot - target);
> - ppm = div_u64(1000000ULL * diff, target);
> + if (!vlv_PLL_is_optimal(dev, target,
> + &clock,
> + best_clock,
> + bestppm, &ppm))
> + continue;
>
> - if (ppm < 100 && clock.p > best_clock->p) {
> - bestppm = 0;
> - *best_clock = clock;
> - found = true;
> - }
> -
> - if (bestppm >= 10 && ppm < bestppm - 10) {
> - bestppm = ppm;
> - *best_clock = clock;
> - found = true;
> - }
> + *best_clock = clock;
> + bestppm = ppm;
> + found = true;
> }
> }
> }
>

Did a double take because of the logic reversal, but I think it's correct.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>