[Intel-gfx] [PATCH] drm/i915: Compute WR PLL dividers dynamically

[Daniel Vetter]

On Wed, May 08, 2013 at 09:13:54PM +0200, Daniel Vetter wrote:
> On Wed, May 08, 2013 at 01:07:23PM -0300, Paulo Zanoni wrote:
> > Hi
> > 
> > 2013/5/8 Damien Lespiau <damien.lespiau at intel.com>:
> > > Up to now, we were using a static table to match the clock frequency
> > > with a (r2,n2,p) triplet. Despite this table being big, it's by no mean
> > > comprehensive and we had to fall back to the closest frequency when the
> > > requested TMDS clock wasn't in the table.
> > >
> > > This patch computes (r2,n2,p) dynamically and get rid of The Big Table.
> > >
> > 
> > There are 2 minor optional bikesheds below. I "reviewed" your patch,
> > it looks correct, but I have to admit I didn't really check the math
> > too hard (and there's no definitive documentation we can check your
> > math against...).
> > 
> > So the real work I did was testing. I discovered I don't have any
> > monitors with modes that are not on the big wrpll_tmds_clock_table, so
> > I couldn't test that case. In one of my monitors I tested all the
> > modes and they all work. In another monitor I tested some modes, they
> > work, and I also checked whether the p/n/r values match the table and
> > they do match. I also checked on the monitor's menu if it thinks its
> > frequency is the correct one. All looks correct.
> > 
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Tested-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> 
> Queued for -next, thanks for the patch.
>  
> > I also looked briefly to your i-g-t patches. They look fine, but my
> > concern is that the code inside the Kernel will get out-of-sync with
> > the code in i-g-t, so we won't really be able to catch regressions.
> > OTOH, I do have an idea for a different i-g-t test: you read our
> > registers (WRPLL_1, WRPLL_2, Link M) in order to check which
> > frequency/r/n/p we're using inside the Kernel, then you look at the
> > old wrpll_tmds_clock_table and check if the values used on the Kernel
> > match the values on the table. Maybe this should be incorporated at
> > testdisplay. Maybe my idea is just a bad idea and should be ignored.
> > Feel free to do whatever you prefer for the i-g-t patches.
> 
> Yeah, I'm not too sure about what we should do with the i-g-t. Commit it
> as-is certainly won't hurt, but I'm not sure whether it can provide
> additional value now that the conversion is done.

Oops, blows up on 32bit machines. I guess the igt will be useful once more
to check that the 32bit version is solid, too. Dropped from dinq for now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch