| Message ID | 1434616228-28358-4-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 18, 2015 at 10:30:23AM +0200, Daniel Vetter wrote: > With the new DRRS code it kinda sticks out, and we never managed to > get this to work well enough without causing issues. Time to wave > goodbye. > > I've decided to keep the logic for programming the reduced clocks > intact, but everything else is gone. If anyone ever wants to resurrect > this we need to redo it all anyway on top of the frontbuffer tracking. Can you nuke just the intel_mark_busy side? Keeping the mode finder intact would be useful as the intrepid reader need only then implement the intel_frontbuffer callbacks and have the harder part of matching appropriate modes and switching routines ready to plug in. (Those latter ones I expect to be tweaked over time, and so the reader's first step of reverting this commit would conflict in such a way as to dissuade them.) -Chris
On Thu, Jun 18, 2015 at 10:00:51AM +0100, Chris Wilson wrote: > On Thu, Jun 18, 2015 at 10:30:23AM +0200, Daniel Vetter wrote: > > With the new DRRS code it kinda sticks out, and we never managed to > > get this to work well enough without causing issues. Time to wave > > goodbye. > > > > I've decided to keep the logic for programming the reduced clocks > > intact, but everything else is gone. If anyone ever wants to resurrect > > this we need to redo it all anyway on top of the frontbuffer tracking. > > Can you nuke just the intel_mark_busy side? Keeping the mode finder > intact would be useful as the intrepid reader need only then implement > the intel_frontbuffer callbacks and have the harder part of matching > appropriate modes and switching routines ready to plug in. (Those latter > ones I expect to be tweaked over time, and so the reader's first step of > reverting this commit would conflict in such a way as to dissuade them.) Well I was also somewhat annoyed by the dev_priv->lvds_* stuff and figured getting rid of that is good - it really should be stored somewhere in intel_lvds or in the pipe_config. Also given that no one ever really bothered to fix this up since gen5 (where the bit to change frequency moved around iirc) I don't think anyone will ever resurrect this. Hence the much more eager delete. -Daniel
On Thu, Jun 18, 2015 at 11:23:17AM +0200, Daniel Vetter wrote: > On Thu, Jun 18, 2015 at 10:00:51AM +0100, Chris Wilson wrote: > > On Thu, Jun 18, 2015 at 10:30:23AM +0200, Daniel Vetter wrote: > > > With the new DRRS code it kinda sticks out, and we never managed to > > > get this to work well enough without causing issues. Time to wave > > > goodbye. > > > > > > I've decided to keep the logic for programming the reduced clocks > > > intact, but everything else is gone. If anyone ever wants to resurrect > > > this we need to redo it all anyway on top of the frontbuffer tracking. > > > > Can you nuke just the intel_mark_busy side? Keeping the mode finder > > intact would be useful as the intrepid reader need only then implement > > the intel_frontbuffer callbacks and have the harder part of matching > > appropriate modes and switching routines ready to plug in. (Those latter > > ones I expect to be tweaked over time, and so the reader's first step of > > reverting this commit would conflict in such a way as to dissuade them.) > > Well I was also somewhat annoyed by the dev_priv->lvds_* stuff and figured > getting rid of that is good - it really should be stored somewhere in > intel_lvds or in the pipe_config. Also given that no one ever really > bothered to fix this up since gen5 (where the bit to change frequency > moved around iirc) I don't think anyone will ever resurrect this. Hence > the much more eager delete. *shrug* we know that people do try to use it, I was just thinking of a way that we could make it easier for them to refresh the code. (Ideally, I would prefer that they wouldn't have to and the new framework provided the impetus needed to solidify LVDS reclocking. All that was lacking was the vblank task to do the reclocking on the refresh to hide any flicker on transition. That and so few manufacturers supplied dual-mode panels.) -Chris
On Thu, Jun 18, 2015 at 10:30:36AM +0100, Chris Wilson wrote: > On Thu, Jun 18, 2015 at 11:23:17AM +0200, Daniel Vetter wrote: > > On Thu, Jun 18, 2015 at 10:00:51AM +0100, Chris Wilson wrote: > > > On Thu, Jun 18, 2015 at 10:30:23AM +0200, Daniel Vetter wrote: > > > > With the new DRRS code it kinda sticks out, and we never managed to > > > > get this to work well enough without causing issues. Time to wave > > > > goodbye. > > > > > > > > I've decided to keep the logic for programming the reduced clocks > > > > intact, but everything else is gone. If anyone ever wants to resurrect > > > > this we need to redo it all anyway on top of the frontbuffer tracking. > > > > > > Can you nuke just the intel_mark_busy side? Keeping the mode finder > > > intact would be useful as the intrepid reader need only then implement > > > the intel_frontbuffer callbacks and have the harder part of matching > > > appropriate modes and switching routines ready to plug in. (Those latter > > > ones I expect to be tweaked over time, and so the reader's first step of > > > reverting this commit would conflict in such a way as to dissuade them.) > > > > Well I was also somewhat annoyed by the dev_priv->lvds_* stuff and figured > > getting rid of that is good - it really should be stored somewhere in > > intel_lvds or in the pipe_config. Also given that no one ever really > > bothered to fix this up since gen5 (where the bit to change frequency > > moved around iirc) I don't think anyone will ever resurrect this. Hence > > the much more eager delete. > > *shrug* we know that people do try to use it, I was just thinking of a > way that we could make it easier for them to refresh the code. (Ideally, > I would prefer that they wouldn't have to and the new framework provided > the impetus needed to solidify LVDS reclocking. All that was lacking was > the vblank task to do the reclocking on the refresh to hide any flicker > on transition. That and so few manufacturers supplied dual-mode panels.) Not sure users trying to use it is a good argument - they enthusiastically try it on gen5+ too (where we don't frob the right pipe bits to make the frequency switch) and on machines without lvds panels. I expect a dummy i915.save_me_some_power option would get enabled too ;-) Btw for the vblank work I think the crucial bit is that we want seamless DRRS (which edp drrs checks in the vbt) and lvds drrs never did check that. At least that might explain why it flickered often badly. Given that ack or nack? -Daniel
On Tue, Jun 23, 2015 at 11:18:05PM +0200, Daniel Vetter wrote: > On Thu, Jun 18, 2015 at 10:30:36AM +0100, Chris Wilson wrote: > > On Thu, Jun 18, 2015 at 11:23:17AM +0200, Daniel Vetter wrote: > > > On Thu, Jun 18, 2015 at 10:00:51AM +0100, Chris Wilson wrote: > > > > On Thu, Jun 18, 2015 at 10:30:23AM +0200, Daniel Vetter wrote: > > > > > With the new DRRS code it kinda sticks out, and we never managed to > > > > > get this to work well enough without causing issues. Time to wave > > > > > goodbye. > > > > > > > > > > I've decided to keep the logic for programming the reduced clocks > > > > > intact, but everything else is gone. If anyone ever wants to resurrect > > > > > this we need to redo it all anyway on top of the frontbuffer tracking. > > > > > > > > Can you nuke just the intel_mark_busy side? Keeping the mode finder > > > > intact would be useful as the intrepid reader need only then implement > > > > the intel_frontbuffer callbacks and have the harder part of matching > > > > appropriate modes and switching routines ready to plug in. (Those latter > > > > ones I expect to be tweaked over time, and so the reader's first step of > > > > reverting this commit would conflict in such a way as to dissuade them.) > > > > > > Well I was also somewhat annoyed by the dev_priv->lvds_* stuff and figured > > > getting rid of that is good - it really should be stored somewhere in > > > intel_lvds or in the pipe_config. Also given that no one ever really > > > bothered to fix this up since gen5 (where the bit to change frequency > > > moved around iirc) I don't think anyone will ever resurrect this. Hence > > > the much more eager delete. > > > > *shrug* we know that people do try to use it, I was just thinking of a > > way that we could make it easier for them to refresh the code. (Ideally, > > I would prefer that they wouldn't have to and the new framework provided > > the impetus needed to solidify LVDS reclocking. All that was lacking was > > the vblank task to do the reclocking on the refresh to hide any flicker > > on transition. That and so few manufacturers supplied dual-mode panels.) > > Not sure users trying to use it is a good argument - they enthusiastically > try it on gen5+ too (where we don't frob the right pipe bits to make the > frequency switch) and on machines without lvds panels. I expect a dummy > i915.save_me_some_power option would get enabled too ;-) > > Btw for the vblank work I think the crucial bit is that we want seamless > DRRS (which edp drrs checks in the vbt) and lvds drrs never did check > that. At least that might explain why it flickered often badly. > > Given that ack or nack? Given that it has been buggy forever, and removes some interloping code, ack. I'm just disappointed. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 491ef0cfcb0b..b34ff2ba2cbc 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1793,9 +1793,6 @@ struct drm_i915_private { /* Reclocking support */ bool render_reclock_avail; - bool lvds_downclock_avail; - /* indicates the reduced downclock for LVDS*/ - int lvds_downclock; struct i915_frontbuffer_tracking fb_tracking; @@ -2548,7 +2545,6 @@ struct i915_params { int modeset; int panel_ignore_lid; int semaphores; - unsigned int lvds_downclock; int lvds_channel_mode; int panel_use_ssc; int vbt_sdvo_panel_type; diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 8ac5a1b29ac0..566826d98c97 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -28,7 +28,6 @@ struct i915_params i915 __read_mostly = { .modeset = -1, .panel_ignore_lid = 1, .semaphores = -1, - .lvds_downclock = 0, .lvds_channel_mode = 0, .panel_use_ssc = -1, .vbt_sdvo_panel_type = -1, @@ -84,11 +83,6 @@ MODULE_PARM_DESC(enable_fbc, "Enable frame buffer compression for power savings " "(default: -1 (use per-chip default))"); -module_param_named(lvds_downclock, i915.lvds_downclock, int, 0400); -MODULE_PARM_DESC(lvds_downclock, - "Use panel (LVDS/eDP) downclocking for power savings " - "(default: false)"); - module_param_named(lvds_channel_mode, i915.lvds_channel_mode, int, 0600); MODULE_PARM_DESC(lvds_channel_mode, "Specify LVDS channel mode " diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 198fc3c3291b..2ff9eb00fdec 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -122,42 +122,6 @@ fill_detail_timing_data(struct drm_display_mode *panel_fixed_mode, drm_mode_set_name(panel_fixed_mode); } -static bool -lvds_dvo_timing_equal_size(const struct lvds_dvo_timing *a, - const struct lvds_dvo_timing *b) -{ - if (a->hactive_hi != b->hactive_hi || - a->hactive_lo != b->hactive_lo) - return false; - - if (a->hsync_off_hi != b->hsync_off_hi || - a->hsync_off_lo != b->hsync_off_lo) - return false; - - if (a->hsync_pulse_width != b->hsync_pulse_width) - return false; - - if (a->hblank_hi != b->hblank_hi || - a->hblank_lo != b->hblank_lo) - return false; - - if (a->vactive_hi != b->vactive_hi || - a->vactive_lo != b->vactive_lo) - return false; - - if (a->vsync_off != b->vsync_off) - return false; - - if (a->vsync_pulse_width != b->vsync_pulse_width) - return false; - - if (a->vblank_hi != b->vblank_hi || - a->vblank_lo != b->vblank_lo) - return false; - - return true; -} - static const struct lvds_dvo_timing * get_lvds_dvo_timing(const struct bdb_lvds_lfp_data *lvds_lfp_data, const struct bdb_lvds_lfp_data_ptrs *lvds_lfp_data_ptrs, @@ -213,7 +177,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv, const struct lvds_dvo_timing *panel_dvo_timing; const struct lvds_fp_timing *fp_timing; struct drm_display_mode *panel_fixed_mode; - int i, downclock, drrs_mode; + int drrs_mode; lvds_options = find_section(bdb, BDB_LVDS_OPTIONS); if (!lvds_options) @@ -272,30 +236,6 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv, DRM_DEBUG_KMS("Found panel mode in BIOS VBT tables:\n"); drm_mode_debug_printmodeline(panel_fixed_mode); - /* - * Iterate over the LVDS panel timing info to find the lowest clock - * for the native resolution. - */ - downclock = panel_dvo_timing->clock; - for (i = 0; i < 16; i++) { - const struct lvds_dvo_timing *dvo_timing; - - dvo_timing = get_lvds_dvo_timing(lvds_lfp_data, - lvds_lfp_data_ptrs, - i); - if (lvds_dvo_timing_equal_size(dvo_timing, panel_dvo_timing) && - dvo_timing->clock < downclock) - downclock = dvo_timing->clock; - } - - if (downclock < panel_dvo_timing->clock && i915.lvds_downclock) { - dev_priv->lvds_downclock_avail = 1; - dev_priv->lvds_downclock = downclock * 10; - DRM_DEBUG_KMS("LVDS downclock is found in VBT. " - "Normal Clock %dKHz, downclock %dKHz\n", - panel_fixed_mode->clock, 10*downclock); - } - fp_timing = get_lvds_fp_timing(bdb, lvds_lfp_data, lvds_lfp_data_ptrs, lvds_options->panel_type); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3f4891782cf6..45a93c87d48e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7827,9 +7827,9 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc, struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; int refclk, num_connectors = 0; - intel_clock_t clock, reduced_clock; - bool ok, has_reduced_clock = false; - bool is_lvds = false, is_dsi = false; + intel_clock_t clock; + bool ok; + bool is_dsi = false; struct intel_encoder *encoder; const intel_limit_t *limit; struct drm_atomic_state *state = crtc_state->base.state; @@ -7847,9 +7847,6 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc, encoder = to_intel_encoder(connector_state->best_encoder); switch (encoder->type) { - case INTEL_OUTPUT_LVDS: - is_lvds = true; - break; case INTEL_OUTPUT_DSI: is_dsi = true; break; @@ -7881,19 +7878,6 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc, return -EINVAL; } - if (is_lvds && dev_priv->lvds_downclock_avail) { - /* - * Ensure we match the reduced clock's P to the target - * clock. If the clocks don't match, we can't switch - * the display clock by using the FP0/FP1. In such case - * we will disable the LVDS downclock feature. - */ - has_reduced_clock = - dev_priv->display.find_dpll(limit, crtc_state, - dev_priv->lvds_downclock, - refclk, &clock, - &reduced_clock); - } /* Compat-code for transition, will disappear. */ crtc_state->dpll.n = clock.n; crtc_state->dpll.m1 = clock.m1; @@ -7903,16 +7887,14 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc, } if (IS_GEN2(dev)) { - i8xx_update_pll(crtc, crtc_state, - has_reduced_clock ? &reduced_clock : NULL, + i8xx_update_pll(crtc, crtc_state, NULL, num_connectors); } else if (IS_CHERRYVIEW(dev)) { chv_update_pll(crtc, crtc_state); } else if (IS_VALLEYVIEW(dev)) { vlv_update_pll(crtc, crtc_state); } else { - i9xx_update_pll(crtc, crtc_state, - has_reduced_clock ? &reduced_clock : NULL, + i9xx_update_pll(crtc, crtc_state, NULL, num_connectors); } @@ -8724,9 +8706,7 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc, struct drm_i915_private *dev_priv = dev->dev_private; int refclk; const intel_limit_t *limit; - bool ret, is_lvds = false; - - is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS); + bool ret; refclk = ironlake_get_refclk(crtc_state); @@ -8742,20 +8722,6 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc, if (!ret) return false; - if (is_lvds && dev_priv->lvds_downclock_avail) { - /* - * Ensure we match the reduced clock's P to the target clock. - * If the clocks don't match, we can't switch the display clock - * by using the FP0/FP1. In such case we will disable the LVDS - * downclock feature. - */ - *has_reduced_clock = - dev_priv->display.find_dpll(limit, crtc_state, - dev_priv->lvds_downclock, - refclk, clock, - reduced_clock); - } - return true; } @@ -10730,42 +10696,6 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev, return mode; } -static void intel_decrease_pllclock(struct drm_crtc *crtc) -{ - struct drm_device *dev = crtc->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - - if (!HAS_GMCH_DISPLAY(dev)) - return; - - if (!dev_priv->lvds_downclock_avail) - return; - - /* - * Since this is called by a timer, we should never get here in - * the manual case. - */ - if (!HAS_PIPE_CXSR(dev) && intel_crtc->lowfreq_avail) { - int pipe = intel_crtc->pipe; - int dpll_reg = DPLL(pipe); - int dpll; - - DRM_DEBUG_DRIVER("downclocking LVDS\n"); - - assert_panel_unlocked(dev_priv, pipe); - - dpll = I915_READ(dpll_reg); - dpll |= DISPLAY_RATE_SELECT_FPA1; - I915_WRITE(dpll_reg, dpll); - intel_wait_for_vblank(dev, pipe); - dpll = I915_READ(dpll_reg); - if (!(dpll & DISPLAY_RATE_SELECT_FPA1)) - DRM_DEBUG_DRIVER("failed to downclock LVDS!\n"); - } - -} - void intel_mark_busy(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -10783,20 +10713,12 @@ void intel_mark_busy(struct drm_device *dev) void intel_mark_idle(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - struct drm_crtc *crtc; if (!dev_priv->mm.busy) return; dev_priv->mm.busy = false; - for_each_crtc(dev, crtc) { - if (!crtc->primary->fb) - continue; - - intel_decrease_pllclock(crtc); - } - if (INTEL_INFO(dev)->gen >= 6) gen6_rps_idle(dev->dev_private); diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c index 899b6112bbe0..3c4862c8629d 100644 --- a/drivers/gpu/drm/i915/intel_frontbuffer.c +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c @@ -65,59 +65,6 @@ #include "intel_drv.h" #include "i915_drv.h" -static void intel_increase_pllclock(struct drm_device *dev, - enum pipe pipe) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - int dpll_reg = DPLL(pipe); - int dpll; - - if (!HAS_GMCH_DISPLAY(dev)) - return; - - if (!dev_priv->lvds_downclock_avail) - return; - - dpll = I915_READ(dpll_reg); - if (!HAS_PIPE_CXSR(dev) && (dpll & DISPLAY_RATE_SELECT_FPA1)) { - DRM_DEBUG_DRIVER("upclocking LVDS\n"); - - assert_panel_unlocked(dev_priv, pipe); - - dpll &= ~DISPLAY_RATE_SELECT_FPA1; - I915_WRITE(dpll_reg, dpll); - intel_wait_for_vblank(dev, pipe); - - dpll = I915_READ(dpll_reg); - if (dpll & DISPLAY_RATE_SELECT_FPA1) - DRM_DEBUG_DRIVER("failed to upclock LVDS!\n"); - } -} - -/** - * intel_mark_fb_busy - mark given planes as busy - * @dev: DRM device - * @frontbuffer_bits: bits for the affected planes - * @ring: optional ring for asynchronous commands - * - * This function gets called every time the screen contents change. It can be - * used to keep e.g. the update rate at the nominal refresh rate with DRRS. - */ -static void intel_mark_fb_busy(struct drm_device *dev, - unsigned frontbuffer_bits, - struct intel_engine_cs *ring) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - enum pipe pipe; - - for_each_pipe(dev_priv, pipe) { - if (!(frontbuffer_bits & INTEL_FRONTBUFFER_ALL_MASK(pipe))) - continue; - - intel_increase_pllclock(dev, pipe); - } -} - /** * intel_fb_obj_invalidate - invalidate frontbuffer object * @obj: GEM object to invalidate @@ -151,8 +98,6 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj, mutex_unlock(&dev_priv->fb_tracking.lock); } - intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring); - intel_psr_invalidate(dev, obj->frontbuffer_bits); intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits); intel_fbc_invalidate(dev_priv, obj->frontbuffer_bits, origin); @@ -182,8 +127,6 @@ void intel_frontbuffer_flush(struct drm_device *dev, if (!frontbuffer_bits) return; - intel_mark_fb_busy(dev, frontbuffer_bits, NULL); - intel_edp_drrs_flush(dev, frontbuffer_bits); intel_psr_flush(dev, frontbuffer_bits); intel_fbc_flush(dev_priv, frontbuffer_bits); diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 161ab26f81fb..61dff9e8ba32 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -1068,24 +1068,8 @@ void intel_lvds_init(struct drm_device *dev) drm_mode_debug_printmodeline(scan); fixed_mode = drm_mode_duplicate(dev, scan); - if (fixed_mode) { - downclock_mode = - intel_find_panel_downclock(dev, - fixed_mode, connector); - if (downclock_mode != NULL && - i915.lvds_downclock) { - /* We found the downclock for LVDS. */ - dev_priv->lvds_downclock_avail = true; - dev_priv->lvds_downclock = - downclock_mode->clock; - DRM_DEBUG_KMS("LVDS downclock is found" - " in EDID. Normal clock %dKhz, " - "downclock %dKhz\n", - fixed_mode->clock, - dev_priv->lvds_downclock); - } + if (fixed_mode) goto out; - } } }
With the new DRRS code it kinda sticks out, and we never managed to get this to work well enough without causing issues. Time to wave goodbye. I've decided to keep the logic for programming the reduced clocks intact, but everything else is gone. If anyone ever wants to resurrect this we need to redo it all anyway on top of the frontbuffer tracking. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 4 -- drivers/gpu/drm/i915/i915_params.c | 6 --- drivers/gpu/drm/i915/intel_bios.c | 62 +--------------------- drivers/gpu/drm/i915/intel_display.c | 90 +++----------------------------- drivers/gpu/drm/i915/intel_frontbuffer.c | 57 -------------------- drivers/gpu/drm/i915/intel_lvds.c | 18 +------ 6 files changed, 8 insertions(+), 229 deletions(-)