[Intel-gfx] [PATCH 20/31] drm/i915: simplify the reduced clock handling for pch plls

[Daniel Vetter]

On Thu, Jun 13, 2013 at 1:26 PM, Damien Lespiau
<damien.lespiau at intel.com> wrote:
> On Wed, Jun 05, 2013 at 01:34:22PM +0200, Daniel Vetter wrote:
>> Just move the lowfreq_avail logic out of the register writing as a
>> prep step for the next patch, which will coalesce all the pch pll
>> enabling into one spot.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ecf0b1e..fc1b5f7 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5686,7 +5686,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>>               if (encoder->pre_pll_enable)
>>                       encoder->pre_pll_enable(encoder);
>>
>> -     intel_crtc->lowfreq_avail = false;
>> +     if (is_lvds && has_reduced_clock && i915_powersave)
>> +             intel_crtc->lowfreq_avail = true;
>
> is_lvds doesn't seem necessary as ironlake_compute_clocks() won't set
> has_reduced_clock to true if !is_lvds. Doesn't hurt either.

I want to move this all into encoder compute_config callbacks anyway,
so that we can neatly subsume eDP DRRS support, too. Until that's
fixed I don't care about a bit of fluff ...

>> +     else
>> +             intel_crtc->lowfreq_avail = false;
>>
>>       if (intel_crtc->config.has_pch_encoder) {
>>               pll = intel_crtc_to_shared_dpll(intel_crtc);
>> @@ -5704,12 +5707,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>>                */
>>               I915_WRITE(PCH_DPLL(pll->id), dpll);
>>
>> -             if (is_lvds && has_reduced_clock && i915_powersave) {
>> +             if (has_reduced_clock)
>>                       I915_WRITE(PCH_FP1(pll->id), fp2);
>
> Hum this is not quite the same condition? i915_powersave could be false
> and we don't want to take that branch? maybe reuse lowfreq_avail?
>
> Maybe compute_clocks() could check i915_powersave itself and set
> has_reduced_clock (or use_reduced_clock) correctly.

Well the entire lowfreq stuff is ripe for overhaul anyway, my plan is
to move it all into the pipe config.

For this case here of writing the FP1 register it doesn't matter if we
uncodntionally do it, since FP1 doesn't have any effect if we don't
enable the lowfreq mode. Which despite the appearance we currently
don't do at all on ilk+ ;-)

I should have mentioned this in the comment message. r-b if I fix that
while applying?
-Daniel

>
>> -                     intel_crtc->lowfreq_avail = true;
>> -             } else {
>> +             else
>>                       I915_WRITE(PCH_FP1(pll->id), fp);
>> -             }
>>       }
>>
>>       intel_set_pipe_timings(intel_crtc);
>> --
>> 1.7.11.7
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch