Re: [Intel-gfx] [PATCH 3/9] drm/i915: fixup i8xx pll limits

2013-06-11 Thread Damien Lespiau
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.jakobs...@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.lesp...@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.vet...@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 = 35 },
   .vco = { .min = 93, .max = 140 },
 - .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 = 35 },
   .vco = { .min = 93, .max = 140 },
 - .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@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH 3/9] drm/i915: fixup i8xx pll limits

2013-05-21 Thread Daniel Vetter
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.jakobs...@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.

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.vet...@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 = 35 },
.vco = { .min = 93, .max = 140 },
-   .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 = 35 },
.vco = { .min = 93, .max = 140 },
-   .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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx