On Thu, Jun 13, 2013 at 01:35:44PM +0200, Daniel Vetter wrote: > 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? yes! Reviewed-by: Damien Lespiau <damien.lespiau at intel.com> -- Damien > -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