On Tue, May 21, 2013 at 09:54:53PM +0200, Daniel Vetter wrote:
> So apparently the pll limits in the docs are the real values, but the
> formula actually seems to consistenly use register values. See
>
> commit 4f7dfb6788dd022446847fbbfbe45e13bedb5be2
> Author: Patrik Jakobsson <patrik.r.jakobsson at gmail.com>
> Date: Wed Feb 13 22:20:22 2013 +0100
>
> drm/i915: Set i9xx sdvo clock limits according to specifications
>
> On gen2 the situation is a bit more messy: We reuse the code for
> m1/m2/n computation and register frobbing from gen3, so those limits
> need to be in register values (and so need to substract 2 from the doc
> limits).
>
> But gen2 also stores p1/p2 with 2/1 substracted in the register! For
> those though i8xx_update_pll does the adjustments, but the clock
> computation code assumes it works like on gen3. So for divisor limits
> we must _not_ adjust them to the register values.
I think this deserves a comment for the next one looking at that code.
Also the documented computations for p are:
gen2: p = (p1 + 2) * 2^(p2 + 1)
gen3: p = p1 * p2
which lead me to believe we weren't computing the dot clock correctly on
gen2 (but in fact it's fine as p1/p2 are never expressed as register
values, which are not a direct deduction (say on gen 3, p2 = 10 is coded
with 0, p2 = 5 is coded by 1))
So IIUC, we have n, m1, m2 that we express in the "register domain"
while m, p1, p2, p are in the "clock formula domain", for lack of a
better wording. And so the limits follow that.
Otherwise, the patch looks good:
Reviewed-by: Damien Lespiau <damien.lespiau at intel.com>
--
Damien
>
> v2: Extract the common i8xx m/n limits into a single define. This was
> motivated by the mess for the g4x limits where they've all been off in
> different directions, apparently to fix specific bugs ...
>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_display.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 959c2ae..ea8eb0c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -84,13 +84,16 @@ intel_fdi_link_freq(struct drm_device *dev)
> return 27;
> }
>
> +#define I8XX_DPLL_M_N_LIMITS \
> + .n = { .min = 1, .max = 14 }, \
> + .m = { .min = 96, .max = 140 }, \
> + .m1 = { .min = 16, .max = 24 }, \
> + .m2 = { .min = 4, .max = 14 },
> +
> static const intel_limit_t intel_limits_i8xx_dvo = {
> .dot = { .min = 25000, .max = 350000 },
> .vco = { .min = 930000, .max = 1400000 },
> - .n = { .min = 3, .max = 16 },
> - .m = { .min = 96, .max = 140 },
> - .m1 = { .min = 18, .max = 26 },
> - .m2 = { .min = 6, .max = 16 },
> + I8XX_DPLL_M_N_LIMITS
> .p = { .min = 4, .max = 128 },
> .p1 = { .min = 2, .max = 33 },
> .p2 = { .dot_limit = 165000,
> @@ -100,10 +103,7 @@ static const intel_limit_t intel_limits_i8xx_dvo = {
> static const intel_limit_t intel_limits_i8xx_lvds = {
> .dot = { .min = 25000, .max = 350000 },
> .vco = { .min = 930000, .max = 1400000 },
> - .n = { .min = 3, .max = 16 },
> - .m = { .min = 96, .max = 140 },
> - .m1 = { .min = 18, .max = 26 },
> - .m2 = { .min = 6, .max = 16 },
> + I8XX_DPLL_M_N_LIMITS
> .p = { .min = 4, .max = 128 },
> .p1 = { .min = 1, .max = 6 },
> .p2 = { .dot_limit = 165000,
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
