| Message ID | 1370432073-27634-21-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. > + 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. > - 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jun 13, 2013 at 1:26 PM, Damien Lespiau <damien.lespiau@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@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@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
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@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@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@intel.com>
On Thu, Jun 13, 2013 at 01:32:03PM +0100, Damien Lespiau wrote: > 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@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@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! Added ... > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> ... and queued for -next, thanks for the review. -Daniel
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; + 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); - intel_crtc->lowfreq_avail = true; - } else { + else I915_WRITE(PCH_FP1(pll->id), fp); - } } intel_set_pipe_timings(intel_crtc);
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@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)