[Intel-gfx] [PATCH 7/8] drm/i915: extract pch_pll_set from ironlake_crtc_mode_set

[Paulo Zanoni]

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