Hi
2012/9/12 Daniel Vetter <daniel at ffwll.ch>:
> On Wed, Sep 12, 2012 at 10:06:35AM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
[ ... ]
>> + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>
> Since the plan is to move all the hsw_crtc_stuff out into it's own
> function I'd prefer a !HAS_PCH_LPT check here.
I don't agree with the LPT check. Let me give some more details about
my plan so you can understand why I did this.
My plan was to add even more (HAS_PCH_IBX || HAS_PCH_CPT) checks to
other places of this function and copy them all to the
haswell_crtc_mode_set version since I'm still not sure we'll never
ever have HSW with something older than LPT. After forking the Haswell
version, the plan was to add a WARN(!(HAS_PCH_IBX || HAS_PCH_CPT)) to
ironlake_crtc_mode_set and then remove the checks form it (leaving the
checks on haswell_crtc_mode_set untouched and prepared for the 42 new
PCHs they plan to ship after LPT). This code under the check was made
specifically for IBX/CPT (and PPT) and only tested on it, so I guess
the correct check is check for IBX/CPT.
So, may I keep the IBX/CPT check? Should I change the plan instead?
>> - intel_crtc->lowfreq_avail = false;
>> - if (intel_crtc->pch_pll) {
>> - if (is_lvds && has_reduced_clock && i915_powersave) {
>> - I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp2);
>> - intel_crtc->lowfreq_avail = true;
>> - } else {
>> - I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp);
>> - }
>> - }
>> -
>
> Again, you're moving registers around - see the comment about that the
> lvds pin pairs need to be enabled _before_ we fire up the pll.
Oh, thanks! Let's hope that after the rework it's harder to make such
mistakes :)
> Yeah, this
> is a mess and I think we should move the actual pll enabling to the
> crtc_enable stage (and the also move the lvds pin pair/port enabling in
> there). So
> - the actuall pll enabling needs to stay here.
> - maybe call the function update_pch_pll instead of set_pch_pll. This is
> more in line with Jesse's similar refactoring of i9xx_crtc_mode_set,
> where he called the split-out pll functions update_pll, too.
Ok, will do.
--
Paulo Zanoni
