| Message ID | 1379526320-7607-3-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/9/18 <ville.syrjala@linux.intel.com>: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Refactor the plane enabling/disabling into helper functions and move > the calls to happen as the first thing during .crtc_disable, and the > last thing during .crtc_enable. > > Those are the two clear points where we are sure that the pipe is > actually running regardless of the encoder type or hardware generation. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Just some clarification since we're moving the discussion to the list: I asked Ville to send this patch because I'm investigating a FIFO underrun on Haswell and depending on how the plane enabling is done we may need a different workaround implementation, so since Ville planned to change the plane enabling order, I concluded it's probably better to change it now and apply the workaround for "planes enabled after the pipe is running". Now some discussion: while I do think it's nice to unify code for all Gens, I fear this will introduce regressions on Gen 2/3/4/5/6/7. The changes seem correct: previous gens do support enabling/disabling planes after the pipe, but the problem is that at least on Haswell we have different workarounds that depend on the order we do stuff, and I would imagine that previous Gens probably have similar problems. Quoting Arthur: "but if you change this around you might be exposing new problems and missing workarounds". On top of this, add the fact that any gen-agnostic Display patch has a chance of 49% to add a regression on a random Gen. So I'm not very confident to give a R-B for this patch knowing we didn't test all possible gens yet :) Now the good news: I did extract a Haswell-only version of this patch and tested it on my machine, and now the FIFO underrun is just gone without even requiring us to implement the workaround (I have a theory why: when we enable the second pipe, the first one was already enabled more than 2 vblanks ago). So perhaps we could try to merge the Haswell-only patch first (since it's tested and it solves a real bug) and then later you can port the other Gens to use the same code paths? Thanks, Paulo > --- > drivers/gpu/drm/i915/intel_display.c | 134 +++++++++++++++-------------------- > 1 file changed, 59 insertions(+), 75 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 6cd0c1b..e365476 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3355,6 +3355,51 @@ static void hsw_disable_ips(struct intel_crtc *crtc) > intel_wait_for_vblank(dev, crtc->pipe); > } > > +static void intel_crtc_enable_planes(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); > + int pipe = intel_crtc->pipe; > + int plane = intel_crtc->plane; > + > + intel_enable_plane(dev_priv, plane, pipe); > + intel_enable_planes(crtc); > + /* The fixup needs to happen before cursor is enabled */ > + if (IS_G4X(dev)) > + g4x_fixup_plane(dev_priv, pipe); > + intel_crtc_update_cursor(crtc, true); > + intel_crtc_dpms_overlay(intel_crtc, true); > + > + hsw_enable_ips(intel_crtc); > + > + mutex_lock(&dev->struct_mutex); > + intel_update_fbc(dev); > + mutex_unlock(&dev->struct_mutex); > +} > + > +static void intel_crtc_disable_planes(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); > + int pipe = intel_crtc->pipe; > + int plane = intel_crtc->plane; > + > + intel_crtc_wait_for_pending_flips(crtc); > + drm_vblank_off(dev, pipe); > + > + if (dev_priv->fbc.plane == plane) > + intel_disable_fbc(dev); > + > + hsw_disable_ips(intel_crtc); > + > + intel_crtc_dpms_overlay(intel_crtc, false); > + intel_crtc_update_cursor(crtc, false); > + intel_disable_planes(crtc); > + intel_disable_plane(dev_priv, plane, pipe); > +} > + > static void ironlake_crtc_enable(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > @@ -3362,7 +3407,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_encoder *encoder; > int pipe = intel_crtc->pipe; > - int plane = intel_crtc->plane; > > WARN_ON(!crtc->enabled); > > @@ -3399,23 +3443,18 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > intel_update_watermarks(crtc); > intel_enable_pipe(dev_priv, pipe, > intel_crtc->config.has_pch_encoder, false); > - intel_enable_plane(dev_priv, plane, pipe); > - intel_enable_planes(crtc); > - intel_crtc_update_cursor(crtc, true); > > if (intel_crtc->config.has_pch_encoder) > ironlake_pch_enable(crtc); > > - mutex_lock(&dev->struct_mutex); > - intel_update_fbc(dev); > - mutex_unlock(&dev->struct_mutex); > - > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->enable(encoder); > > if (HAS_PCH_CPT(dev)) > cpt_verify_modeset(dev, intel_crtc->pipe); > > + intel_crtc_enable_planes(crtc); > + > /* > * There seems to be a race in PCH platform hw (at least on some > * outputs) where an enabled pipe still completes any pageflip right > @@ -3440,7 +3479,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_encoder *encoder; > int pipe = intel_crtc->pipe; > - int plane = intel_crtc->plane; > > WARN_ON(!crtc->enabled); > > @@ -3476,24 +3514,17 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) > intel_update_watermarks(crtc); > intel_enable_pipe(dev_priv, pipe, > intel_crtc->config.has_pch_encoder, false); > - intel_enable_plane(dev_priv, plane, pipe); > - intel_enable_planes(crtc); > - intel_crtc_update_cursor(crtc, true); > - > - hsw_enable_ips(intel_crtc); > > if (intel_crtc->config.has_pch_encoder) > lpt_pch_enable(crtc); > > - mutex_lock(&dev->struct_mutex); > - intel_update_fbc(dev); > - mutex_unlock(&dev->struct_mutex); > - > for_each_encoder_on_crtc(dev, crtc, encoder) { > encoder->enable(encoder); > intel_opregion_notify_encoder(encoder, true); > } > > + intel_crtc_enable_planes(crtc); > + > /* > * There seems to be a race in PCH platform hw (at least on some > * outputs) where an enabled pipe still completes any pageflip right > @@ -3527,26 +3558,16 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_encoder *encoder; > int pipe = intel_crtc->pipe; > - int plane = intel_crtc->plane; > u32 reg, temp; > > - > if (!intel_crtc->active) > return; > > + intel_crtc_disable_planes(crtc); > + > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->disable(encoder); > > - intel_crtc_wait_for_pending_flips(crtc); > - drm_vblank_off(dev, pipe); > - > - if (dev_priv->fbc.plane == plane) > - intel_disable_fbc(dev); > - > - intel_crtc_update_cursor(crtc, false); > - intel_disable_planes(crtc); > - intel_disable_plane(dev_priv, plane, pipe); > - > if (intel_crtc->config.has_pch_encoder) > intel_set_pch_fifo_underrun_reporting(dev, pipe, false); > > @@ -3600,30 +3621,18 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_encoder *encoder; > int pipe = intel_crtc->pipe; > - int plane = intel_crtc->plane; > enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder; > > if (!intel_crtc->active) > return; > > + intel_crtc_disable_planes(crtc); > + > for_each_encoder_on_crtc(dev, crtc, encoder) { > intel_opregion_notify_encoder(encoder, false); > encoder->disable(encoder); > } > > - intel_crtc_wait_for_pending_flips(crtc); > - drm_vblank_off(dev, pipe); > - > - /* FBC must be disabled before disabling the plane on HSW. */ > - if (dev_priv->fbc.plane == plane) > - intel_disable_fbc(dev); > - > - hsw_disable_ips(intel_crtc); > - > - intel_crtc_update_cursor(crtc, false); > - intel_disable_planes(crtc); > - intel_disable_plane(dev_priv, plane, pipe); > - > if (intel_crtc->config.has_pch_encoder) > intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, false); > intel_disable_pipe(dev_priv, pipe); > @@ -3694,7 +3703,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_encoder *encoder; > int pipe = intel_crtc->pipe; > - int plane = intel_crtc->plane; > bool is_dsi; > > WARN_ON(!crtc->enabled); > @@ -3723,14 +3731,11 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) > > intel_update_watermarks(crtc); > intel_enable_pipe(dev_priv, pipe, false, is_dsi); > - intel_enable_plane(dev_priv, plane, pipe); > - intel_enable_planes(crtc); > - intel_crtc_update_cursor(crtc, true); > - > - intel_update_fbc(dev); > > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->enable(encoder); > + > + intel_crtc_enable_planes(crtc); > } > > static void i9xx_crtc_enable(struct drm_crtc *crtc) > @@ -3740,7 +3745,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_encoder *encoder; > int pipe = intel_crtc->pipe; > - int plane = intel_crtc->plane; > > WARN_ON(!crtc->enabled); > > @@ -3761,20 +3765,11 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) > > intel_update_watermarks(crtc); > intel_enable_pipe(dev_priv, pipe, false, false); > - intel_enable_plane(dev_priv, plane, pipe); > - intel_enable_planes(crtc); > - /* The fixup needs to happen before cursor is enabled */ > - if (IS_G4X(dev)) > - g4x_fixup_plane(dev_priv, pipe); > - intel_crtc_update_cursor(crtc, true); > - > - /* Give the overlay scaler a chance to enable if it's on this pipe */ > - intel_crtc_dpms_overlay(intel_crtc, true); > - > - intel_update_fbc(dev); > > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->enable(encoder); > + > + intel_crtc_enable_planes(crtc); > } > > static void i9xx_pfit_disable(struct intel_crtc *crtc) > @@ -3799,26 +3794,15 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_encoder *encoder; > int pipe = intel_crtc->pipe; > - int plane = intel_crtc->plane; > > if (!intel_crtc->active) > return; > > + intel_crtc_disable_planes(crtc); > + > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->disable(encoder); > > - /* Give the overlay scaler a chance to disable if it's on this pipe */ > - intel_crtc_wait_for_pending_flips(crtc); > - drm_vblank_off(dev, pipe); > - > - if (dev_priv->fbc.plane == plane) > - intel_disable_fbc(dev); > - > - intel_crtc_dpms_overlay(intel_crtc, false); > - intel_crtc_update_cursor(crtc, false); > - intel_disable_planes(crtc); > - intel_disable_plane(dev_priv, plane, pipe); > - > intel_disable_pipe(dev_priv, pipe); > > i9xx_pfit_disable(intel_crtc); > -- > 1.8.1.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6cd0c1b..e365476 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3355,6 +3355,51 @@ static void hsw_disable_ips(struct intel_crtc *crtc) intel_wait_for_vblank(dev, crtc->pipe); } +static void intel_crtc_enable_planes(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); + int pipe = intel_crtc->pipe; + int plane = intel_crtc->plane; + + intel_enable_plane(dev_priv, plane, pipe); + intel_enable_planes(crtc); + /* The fixup needs to happen before cursor is enabled */ + if (IS_G4X(dev)) + g4x_fixup_plane(dev_priv, pipe); + intel_crtc_update_cursor(crtc, true); + intel_crtc_dpms_overlay(intel_crtc, true); + + hsw_enable_ips(intel_crtc); + + mutex_lock(&dev->struct_mutex); + intel_update_fbc(dev); + mutex_unlock(&dev->struct_mutex); +} + +static void intel_crtc_disable_planes(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); + int pipe = intel_crtc->pipe; + int plane = intel_crtc->plane; + + intel_crtc_wait_for_pending_flips(crtc); + drm_vblank_off(dev, pipe); + + if (dev_priv->fbc.plane == plane) + intel_disable_fbc(dev); + + hsw_disable_ips(intel_crtc); + + intel_crtc_dpms_overlay(intel_crtc, false); + intel_crtc_update_cursor(crtc, false); + intel_disable_planes(crtc); + intel_disable_plane(dev_priv, plane, pipe); +} + static void ironlake_crtc_enable(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; @@ -3362,7 +3407,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_encoder *encoder; int pipe = intel_crtc->pipe; - int plane = intel_crtc->plane; WARN_ON(!crtc->enabled); @@ -3399,23 +3443,18 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, intel_crtc->config.has_pch_encoder, false); - intel_enable_plane(dev_priv, plane, pipe); - intel_enable_planes(crtc); - intel_crtc_update_cursor(crtc, true); if (intel_crtc->config.has_pch_encoder) ironlake_pch_enable(crtc); - mutex_lock(&dev->struct_mutex); - intel_update_fbc(dev); - mutex_unlock(&dev->struct_mutex); - for_each_encoder_on_crtc(dev, crtc, encoder) encoder->enable(encoder); if (HAS_PCH_CPT(dev)) cpt_verify_modeset(dev, intel_crtc->pipe); + intel_crtc_enable_planes(crtc); + /* * There seems to be a race in PCH platform hw (at least on some * outputs) where an enabled pipe still completes any pageflip right @@ -3440,7 +3479,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_encoder *encoder; int pipe = intel_crtc->pipe; - int plane = intel_crtc->plane; WARN_ON(!crtc->enabled); @@ -3476,24 +3514,17 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, intel_crtc->config.has_pch_encoder, false); - intel_enable_plane(dev_priv, plane, pipe); - intel_enable_planes(crtc); - intel_crtc_update_cursor(crtc, true); - - hsw_enable_ips(intel_crtc); if (intel_crtc->config.has_pch_encoder) lpt_pch_enable(crtc); - mutex_lock(&dev->struct_mutex); - intel_update_fbc(dev); - mutex_unlock(&dev->struct_mutex); - for_each_encoder_on_crtc(dev, crtc, encoder) { encoder->enable(encoder); intel_opregion_notify_encoder(encoder, true); } + intel_crtc_enable_planes(crtc); + /* * There seems to be a race in PCH platform hw (at least on some * outputs) where an enabled pipe still completes any pageflip right @@ -3527,26 +3558,16 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_encoder *encoder; int pipe = intel_crtc->pipe; - int plane = intel_crtc->plane; u32 reg, temp; - if (!intel_crtc->active) return; + intel_crtc_disable_planes(crtc); + for_each_encoder_on_crtc(dev, crtc, encoder) encoder->disable(encoder); - intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe); - - if (dev_priv->fbc.plane == plane) - intel_disable_fbc(dev); - - intel_crtc_update_cursor(crtc, false); - intel_disable_planes(crtc); - intel_disable_plane(dev_priv, plane, pipe); - if (intel_crtc->config.has_pch_encoder) intel_set_pch_fifo_underrun_reporting(dev, pipe, false); @@ -3600,30 +3621,18 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_encoder *encoder; int pipe = intel_crtc->pipe; - int plane = intel_crtc->plane; enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder; if (!intel_crtc->active) return; + intel_crtc_disable_planes(crtc); + for_each_encoder_on_crtc(dev, crtc, encoder) { intel_opregion_notify_encoder(encoder, false); encoder->disable(encoder); } - intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe); - - /* FBC must be disabled before disabling the plane on HSW. */ - if (dev_priv->fbc.plane == plane) - intel_disable_fbc(dev); - - hsw_disable_ips(intel_crtc); - - intel_crtc_update_cursor(crtc, false); - intel_disable_planes(crtc); - intel_disable_plane(dev_priv, plane, pipe); - if (intel_crtc->config.has_pch_encoder) intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, false); intel_disable_pipe(dev_priv, pipe); @@ -3694,7 +3703,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_encoder *encoder; int pipe = intel_crtc->pipe; - int plane = intel_crtc->plane; bool is_dsi; WARN_ON(!crtc->enabled); @@ -3723,14 +3731,11 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, false, is_dsi); - intel_enable_plane(dev_priv, plane, pipe); - intel_enable_planes(crtc); - intel_crtc_update_cursor(crtc, true); - - intel_update_fbc(dev); for_each_encoder_on_crtc(dev, crtc, encoder) encoder->enable(encoder); + + intel_crtc_enable_planes(crtc); } static void i9xx_crtc_enable(struct drm_crtc *crtc) @@ -3740,7 +3745,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_encoder *encoder; int pipe = intel_crtc->pipe; - int plane = intel_crtc->plane; WARN_ON(!crtc->enabled); @@ -3761,20 +3765,11 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, false, false); - intel_enable_plane(dev_priv, plane, pipe); - intel_enable_planes(crtc); - /* The fixup needs to happen before cursor is enabled */ - if (IS_G4X(dev)) - g4x_fixup_plane(dev_priv, pipe); - intel_crtc_update_cursor(crtc, true); - - /* Give the overlay scaler a chance to enable if it's on this pipe */ - intel_crtc_dpms_overlay(intel_crtc, true); - - intel_update_fbc(dev); for_each_encoder_on_crtc(dev, crtc, encoder) encoder->enable(encoder); + + intel_crtc_enable_planes(crtc); } static void i9xx_pfit_disable(struct intel_crtc *crtc) @@ -3799,26 +3794,15 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_encoder *encoder; int pipe = intel_crtc->pipe; - int plane = intel_crtc->plane; if (!intel_crtc->active) return; + intel_crtc_disable_planes(crtc); + for_each_encoder_on_crtc(dev, crtc, encoder) encoder->disable(encoder); - /* Give the overlay scaler a chance to disable if it's on this pipe */ - intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe); - - if (dev_priv->fbc.plane == plane) - intel_disable_fbc(dev); - - intel_crtc_dpms_overlay(intel_crtc, false); - intel_crtc_update_cursor(crtc, false); - intel_disable_planes(crtc); - intel_disable_plane(dev_priv, plane, pipe); - intel_disable_pipe(dev_priv, pipe); i9xx_pfit_disable(intel_crtc);