Message-Id: <b9dded$hh72bl@orsmga002.jf.intel.com>
Date:	Thu, 30 Dec 2010 22:53:00 +0000
From:	Chris Wilson <chris@...is-wilson.co.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Jesse Barnes <jbarnes@...tuousgeek.org>
Cc:	Dave Airlie <airlied@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [git pull] i915 fixes

On Thu, 30 Dec 2010 14:29:34 -0800, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> On Thu, Dec 30, 2010 at 2:01 PM, Jesse Barnes <jbarnes@...tuousgeek.org> wrote:
> >
> > Did you want to disable SSC entirely on gen5+ just so we can get all
> > the known machines working?  It's possible it could add some wifi or
> > sound interference, but that's better than not having a display (most
> > of the time).
> 
> It would be sad to possibly add more electrical noise to machines that
> already work with it, but at the same time I do think that "working
> screen" tends to be a lot more important than "try to avoid RF noise".
> 
> Also, are you sure it's really the fact that we enable spread-spectrum
> that causes this? The code is really confused, and seems to mix up
> "lvds_use_ssc" not just with the enabling of SSC, but it also with how
> impacts the reference clock itself.
> 
> And it impacts the reference clock in really odd ways, that look buggy
> and confusing, where the tests are repeated in multiple places: first
> to set the reference frequency, and then later to set the bits that
> choose the reference input frequency.
> 
> In particular, look at how 'refclk' is calculated in
> intel_crtc_mode_set(), vs how we actually set the input frequency
> later in the function. The two don't actually *match*. That sounds
> bogus to me - since it means that the pll values have been calculated
> for a reference clock that isn't actually used. No?
> 
> Look at the code for the "!is_lvds" case, for example. It uses
> "IS_GEN2()" to determine what refclk to use, but then when setting the
> PLL_REF_INPUT_xyz bits, we actually take "is_tv" into account - which
> the code didn't when it calculated refclk. That strikes me as odd. No?
> 
> Now, that shouldn't matter for the LVDS case, but I'm wondering
> whether something similar is going on where the conditionals just
> don't match up, and we end up calculating the plls for a different
> frequency than the one we actually end up _using_.
> 
> There's also this very odd special refclock magic for ironlake
> limiting that only happens for ssc_freq == 100 when ssc is enabled.
> Maybe the problem is in the limiting tables, and the ssc frequency
> change just ends up then picking the "wrong" limiter table? So even if
> the frequency is correct, and the pll calculations are using that
> correct frequency, the 120-vs-100Mhz frequency change ended up
> switching the tables around?

Switching those tables around was the reason why the one-line change had
any impact at all, and I hoped that it was the leniency in our pll finder
that enabled us to bring up any SSC-enabled panel. I also made one pass at
removing the confusion surrounding refclk, which wasn't -fixes material.
Now I have several more concerns about the code.

However, switching the SSC refclk back to 100MHz we do end up choosing the
same PLL clocks as the BIOS does on the U160, but modesetting still fails.
So the discrepancy is not likely to be in the limit tables themselves.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists