| Message ID | 1346787148-2564-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, 4 Sep 2012 21:32:28 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > With this change we can (finally!) rip out a few of the temporary hacks > and clean up a few other things: > - Kill intel_crtc_prepare_encoders, now unused. > - Kill the hacks in the crtc_disable/enable functions to always call the > encoder callbacks, we now always call the crtc functions with the right > encoder -> crtc links. > - Also push down the crtc->enable, encoder and connector dpms state > updates. Unfortunately we can't add a WARN in the crtc_disable > callbacks to ensure that the crtc is always still enabled when > disabling an output pipe - the crtc sanitizer of the hw readout path > can hit this when it needs to disable an active pipe without any > enabled outputs. > - Only call crtc->disable if the pipe is already enabled - again avoids > running afoul of the new WARN. > > v2: Copy&paste our own version of crtc_in_use, too. > > v3: We need to update the dpms an encoder->connectors_active states, > too. > > v4: I've forgotten to kill the unconditional encoder->disable calls in > the crtc_disable functions. > > v5: Rip out leftover debug printk. > > v6: Properly clear intel_encoder->connectors_active. This wasn't > properly cleared when disabling an encoder because it was no longer on > the new connector list, but the crtc was still enabled (i.e. switching > the encoder of an active crtc). Reported by Jani Nikula. > > v7: Don't clobber the encoder->connectors_active state of untouched > encoders. Since X likes to first disable all outputs with dpms off > before setting a new framebuffer, this hit a few warnings. Reported by > Paulo Zanoni. > > v8: Kill the now stale comment warning that intel_crtc->active is not > always updated at the right times. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 108 +++++++++++++++++++++++------------ > drivers/gpu/drm/i915/intel_drv.h | 3 - > 2 files changed, 70 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2c2b4873..c3ab86b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3221,10 +3221,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > WARN_ON(!crtc->enabled); > > - /* XXX: For compatability with the crtc helper code, call the encoder's > - * enable function unconditionally for now. */ > if (intel_crtc->active) > - goto encoders; > + return; > > intel_crtc->active = true; > intel_update_watermarks(dev); > @@ -3272,7 +3270,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > intel_crtc_update_cursor(crtc, true); > > -encoders: > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->enable(encoder); > > @@ -3290,14 +3287,13 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > int plane = intel_crtc->plane; > u32 reg, temp; > > - /* XXX: For compatability with the crtc helper code, call the encoder's > - * disable function unconditionally for now. */ > - for_each_encoder_on_crtc(dev, crtc, encoder) > - encoder->disable(encoder); > > if (!intel_crtc->active) > return; > > + for_each_encoder_on_crtc(dev, crtc, encoder) > + encoder->disable(encoder); > + > intel_crtc_wait_for_pending_flips(crtc); > drm_vblank_off(dev, pipe); > intel_crtc_update_cursor(crtc, false); > @@ -3399,10 +3395,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) > > WARN_ON(!crtc->enabled); > > - /* XXX: For compatability with the crtc helper code, call the encoder's > - * enable function unconditionally for now. */ > if (intel_crtc->active) > - goto encoders; > + return; > > intel_crtc->active = true; > intel_update_watermarks(dev); > @@ -3418,7 +3412,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) > intel_crtc_dpms_overlay(intel_crtc, true); > intel_crtc_update_cursor(crtc, true); > > -encoders: > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->enable(encoder); > } > @@ -3432,14 +3425,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > int pipe = intel_crtc->pipe; > int plane = intel_crtc->plane; > > - /* XXX: For compatability with the crtc helper code, call the encoder's > - * disable function unconditionally for now. */ > - for_each_encoder_on_crtc(dev, crtc, encoder) > - encoder->disable(encoder); > > if (!intel_crtc->active) > return; > > + 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); > @@ -6631,18 +6623,6 @@ static bool intel_encoder_crtc_ok(struct drm_encoder *encoder, > return false; > } > > -static void > -intel_crtc_prepare_encoders(struct drm_device *dev) > -{ > - struct intel_encoder *encoder; > - > - list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) { > - /* Disable unused encoders */ > - if (encoder->base.crtc == NULL) > - encoder->disable(encoder); > - } > -} > - > /** > * intel_modeset_update_staged_output_state > * > @@ -6822,6 +6802,60 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes, > *prepare_pipes &= ~(*disable_pipes); > } > > +static bool intel_crtc_in_use(struct drm_crtc *crtc) > +{ > + struct drm_encoder *encoder; > + struct drm_device *dev = crtc->dev; > + > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) > + if (encoder->crtc == crtc) > + return true; > + > + return false; > +} > + > +static void > +intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes) > +{ > + struct intel_encoder *intel_encoder; > + struct intel_crtc *intel_crtc; > + struct drm_connector *connector; > + > + list_for_each_entry(intel_encoder, &dev->mode_config.encoder_list, > + base.head) { > + if (!intel_encoder->base.crtc) > + continue; > + > + intel_crtc = to_intel_crtc(intel_encoder->base.crtc); > + > + if (prepare_pipes & (1 << intel_crtc->pipe)) > + intel_encoder->connectors_active = false; > + } > + > + intel_modeset_commit_output_state(dev); > + > + /* Update computed state. */ > + list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, > + base.head) { > + intel_crtc->base.enabled = intel_crtc_in_use(&intel_crtc->base); > + } > + > + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > + if (!connector->encoder || !connector->encoder->crtc) > + continue; > + > + intel_crtc = to_intel_crtc(connector->encoder->crtc); > + > + if (prepare_pipes & (1 << intel_crtc->pipe)) { > + connector->dpms = DRM_MODE_DPMS_ON; > + > + intel_encoder = to_intel_encoder(connector->encoder); > + intel_encoder->connectors_active = true; > + } > + } > + > +} > + > #define for_each_intel_crtc_masked(dev, mask, intel_crtc) \ > list_for_each_entry((intel_crtc), \ > &(dev)->mode_config.crtc_list, \ > @@ -6850,12 +6884,6 @@ bool intel_set_mode(struct drm_crtc *crtc, > for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc) > intel_crtc_disable(&intel_crtc->base); > > - intel_modeset_commit_output_state(dev); > - > - list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, > - base.head) > - intel_crtc->base.enabled = drm_helper_crtc_in_use(crtc); > - > saved_hwmode = crtc->hwmode; > saved_mode = crtc->mode; > > @@ -6870,12 +6898,12 @@ bool intel_set_mode(struct drm_crtc *crtc, > if (IS_ERR(adjusted_mode)) { > return false; > } > - > - intel_crtc_prepare_encoders(dev); > } > > - for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) > - dev_priv->display.crtc_disable(&intel_crtc->base); > + for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) { > + if (intel_crtc->base.enabled) > + dev_priv->display.crtc_disable(&intel_crtc->base); > + } > > if (modeset_pipes) { > crtc->mode = *mode; > @@ -6883,6 +6911,10 @@ bool intel_set_mode(struct drm_crtc *crtc, > crtc->y = y; > } > > + /* Only after disabling all output pipelines that will be changed can we > + * update the the output configuration. */ > + intel_modeset_update_state(dev, prepare_pipes); > + > /* Set up the DPLL and any encoders state that needs to adjust or depend > * on the DPLL. > */ > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 1282bf0..3e6feae 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -184,9 +184,6 @@ struct intel_crtc { > * Whether the crtc and the connected output pipeline is active. Implies > * that crtc->enabled is set, i.e. the current mode configuration has > * some outputs connected to this crtc. > - * > - * Atm crtc->enabled is unconditionally updated _before_ the hw state is > - * changed, hence we can only check this when enabling the crtc. > */ > bool active; > bool primary_disabled; /* is the crtc obscured by a plane? */ The variables have me confused a little... I would have expected update_state to take modeset_pipes rather than prepare_pipes. Could you use either? Or will that not catch cases where we updated a pipe that was already on?
On Wed, Sep 5, 2012 at 8:28 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > The variables have me confused a little... I would have expected > update_state to take modeset_pipes rather than prepare_pipes. Could > you use either? Or will that not catch cases where we updated a pipe > that was already on? The abstract idea for these masks was the following: Any pipe that changes anything goes into prepare_pipes. For any pipe that also changes the mode, it goes in addition into the modeset_pipes mask, so the later is a subset of prepare pipes. The idea here was to avoid the modeset step where not necessary (e.g. when disabling the 2nd output of a cloned crtc we only need to disable/enable, not change anything with the mode or clocks). But after some in-depth discussion with Paulo Zanoni I think we'll move large parts of the mode_set step into the enable function (at least for hsw due to funky ordering requirements), so I think this disdinction doesn't make sense. The disable mask just contains those pipes that get fully disable (and which then also get removed from the prepares/modset masks). Hence I pass the prepares mask into update_states, not just the modeset mask. -Daniel
On Wed, 5 Sep 2012 21:48:52 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Wed, Sep 5, 2012 at 8:28 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > > The variables have me confused a little... I would have expected > > update_state to take modeset_pipes rather than prepare_pipes. Could > > you use either? Or will that not catch cases where we updated a pipe > > that was already on? > > The abstract idea for these masks was the following: Any pipe that > changes anything goes into prepare_pipes. For any pipe that also > changes the mode, it goes in addition into the modeset_pipes mask, so > the later is a subset of prepare pipes. The idea here was to avoid the > modeset step where not necessary (e.g. when disabling the 2nd output > of a cloned crtc we only need to disable/enable, not change anything > with the mode or clocks). But after some in-depth discussion with > Paulo Zanoni I think we'll move large parts of the mode_set step into > the enable function (at least for hsw due to funky ordering > requirements), so I think this disdinction doesn't make sense. > > The disable mask just contains those pipes that get fully disable (and > which then also get removed from the prepares/modset masks). > > Hence I pass the prepares mask into update_states, not just the modeset mask. Ok, that makes some sense. Hopefully we can preserve the full mode set vs simple update behavior even after the refactoring for HSW.
On Wed, 5 Sep 2012 12:50:26 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Wed, 5 Sep 2012 21:48:52 +0200 > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > On Wed, Sep 5, 2012 at 8:28 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > > > > The variables have me confused a little... I would have expected > > > update_state to take modeset_pipes rather than prepare_pipes. Could > > > you use either? Or will that not catch cases where we updated a pipe > > > that was already on? > > > > The abstract idea for these masks was the following: Any pipe that > > changes anything goes into prepare_pipes. For any pipe that also > > changes the mode, it goes in addition into the modeset_pipes mask, so > > the later is a subset of prepare pipes. The idea here was to avoid the > > modeset step where not necessary (e.g. when disabling the 2nd output > > of a cloned crtc we only need to disable/enable, not change anything > > with the mode or clocks). But after some in-depth discussion with > > Paulo Zanoni I think we'll move large parts of the mode_set step into > > the enable function (at least for hsw due to funky ordering > > requirements), so I think this disdinction doesn't make sense. > > > > The disable mask just contains those pipes that get fully disable (and > > which then also get removed from the prepares/modset masks). > > > > Hence I pass the prepares mask into update_states, not just the modeset mask. > > Ok, that makes some sense. Hopefully we can preserve the full mode set > vs simple update behavior even after the refactoring for HSW. > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2c2b4873..c3ab86b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3221,10 +3221,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) WARN_ON(!crtc->enabled); - /* XXX: For compatability with the crtc helper code, call the encoder's - * enable function unconditionally for now. */ if (intel_crtc->active) - goto encoders; + return; intel_crtc->active = true; intel_update_watermarks(dev); @@ -3272,7 +3270,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) intel_crtc_update_cursor(crtc, true); -encoders: for_each_encoder_on_crtc(dev, crtc, encoder) encoder->enable(encoder); @@ -3290,14 +3287,13 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) int plane = intel_crtc->plane; u32 reg, temp; - /* XXX: For compatability with the crtc helper code, call the encoder's - * disable function unconditionally for now. */ - for_each_encoder_on_crtc(dev, crtc, encoder) - encoder->disable(encoder); if (!intel_crtc->active) return; + for_each_encoder_on_crtc(dev, crtc, encoder) + encoder->disable(encoder); + intel_crtc_wait_for_pending_flips(crtc); drm_vblank_off(dev, pipe); intel_crtc_update_cursor(crtc, false); @@ -3399,10 +3395,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) WARN_ON(!crtc->enabled); - /* XXX: For compatability with the crtc helper code, call the encoder's - * enable function unconditionally for now. */ if (intel_crtc->active) - goto encoders; + return; intel_crtc->active = true; intel_update_watermarks(dev); @@ -3418,7 +3412,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) intel_crtc_dpms_overlay(intel_crtc, true); intel_crtc_update_cursor(crtc, true); -encoders: for_each_encoder_on_crtc(dev, crtc, encoder) encoder->enable(encoder); } @@ -3432,14 +3425,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) int pipe = intel_crtc->pipe; int plane = intel_crtc->plane; - /* XXX: For compatability with the crtc helper code, call the encoder's - * disable function unconditionally for now. */ - for_each_encoder_on_crtc(dev, crtc, encoder) - encoder->disable(encoder); if (!intel_crtc->active) return; + 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); @@ -6631,18 +6623,6 @@ static bool intel_encoder_crtc_ok(struct drm_encoder *encoder, return false; } -static void -intel_crtc_prepare_encoders(struct drm_device *dev) -{ - struct intel_encoder *encoder; - - list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) { - /* Disable unused encoders */ - if (encoder->base.crtc == NULL) - encoder->disable(encoder); - } -} - /** * intel_modeset_update_staged_output_state * @@ -6822,6 +6802,60 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes, *prepare_pipes &= ~(*disable_pipes); } +static bool intel_crtc_in_use(struct drm_crtc *crtc) +{ + struct drm_encoder *encoder; + struct drm_device *dev = crtc->dev; + + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) + if (encoder->crtc == crtc) + return true; + + return false; +} + +static void +intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes) +{ + struct intel_encoder *intel_encoder; + struct intel_crtc *intel_crtc; + struct drm_connector *connector; + + list_for_each_entry(intel_encoder, &dev->mode_config.encoder_list, + base.head) { + if (!intel_encoder->base.crtc) + continue; + + intel_crtc = to_intel_crtc(intel_encoder->base.crtc); + + if (prepare_pipes & (1 << intel_crtc->pipe)) + intel_encoder->connectors_active = false; + } + + intel_modeset_commit_output_state(dev); + + /* Update computed state. */ + list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, + base.head) { + intel_crtc->base.enabled = intel_crtc_in_use(&intel_crtc->base); + } + + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + if (!connector->encoder || !connector->encoder->crtc) + continue; + + intel_crtc = to_intel_crtc(connector->encoder->crtc); + + if (prepare_pipes & (1 << intel_crtc->pipe)) { + connector->dpms = DRM_MODE_DPMS_ON; + + intel_encoder = to_intel_encoder(connector->encoder); + intel_encoder->connectors_active = true; + } + } + +} + #define for_each_intel_crtc_masked(dev, mask, intel_crtc) \ list_for_each_entry((intel_crtc), \ &(dev)->mode_config.crtc_list, \ @@ -6850,12 +6884,6 @@ bool intel_set_mode(struct drm_crtc *crtc, for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc) intel_crtc_disable(&intel_crtc->base); - intel_modeset_commit_output_state(dev); - - list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, - base.head) - intel_crtc->base.enabled = drm_helper_crtc_in_use(crtc); - saved_hwmode = crtc->hwmode; saved_mode = crtc->mode; @@ -6870,12 +6898,12 @@ bool intel_set_mode(struct drm_crtc *crtc, if (IS_ERR(adjusted_mode)) { return false; } - - intel_crtc_prepare_encoders(dev); } - for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) - dev_priv->display.crtc_disable(&intel_crtc->base); + for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) { + if (intel_crtc->base.enabled) + dev_priv->display.crtc_disable(&intel_crtc->base); + } if (modeset_pipes) { crtc->mode = *mode; @@ -6883,6 +6911,10 @@ bool intel_set_mode(struct drm_crtc *crtc, crtc->y = y; } + /* Only after disabling all output pipelines that will be changed can we + * update the the output configuration. */ + intel_modeset_update_state(dev, prepare_pipes); + /* Set up the DPLL and any encoders state that needs to adjust or depend * on the DPLL. */ diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1282bf0..3e6feae 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -184,9 +184,6 @@ struct intel_crtc { * Whether the crtc and the connected output pipeline is active. Implies * that crtc->enabled is set, i.e. the current mode configuration has * some outputs connected to this crtc. - * - * Atm crtc->enabled is unconditionally updated _before_ the hw state is - * changed, hence we can only check this when enabling the crtc. */ bool active; bool primary_disabled; /* is the crtc obscured by a plane? */
With this change we can (finally!) rip out a few of the temporary hacks and clean up a few other things: - Kill intel_crtc_prepare_encoders, now unused. - Kill the hacks in the crtc_disable/enable functions to always call the encoder callbacks, we now always call the crtc functions with the right encoder -> crtc links. - Also push down the crtc->enable, encoder and connector dpms state updates. Unfortunately we can't add a WARN in the crtc_disable callbacks to ensure that the crtc is always still enabled when disabling an output pipe - the crtc sanitizer of the hw readout path can hit this when it needs to disable an active pipe without any enabled outputs. - Only call crtc->disable if the pipe is already enabled - again avoids running afoul of the new WARN. v2: Copy&paste our own version of crtc_in_use, too. v3: We need to update the dpms an encoder->connectors_active states, too. v4: I've forgotten to kill the unconditional encoder->disable calls in the crtc_disable functions. v5: Rip out leftover debug printk. v6: Properly clear intel_encoder->connectors_active. This wasn't properly cleared when disabling an encoder because it was no longer on the new connector list, but the crtc was still enabled (i.e. switching the encoder of an active crtc). Reported by Jani Nikula. v7: Don't clobber the encoder->connectors_active state of untouched encoders. Since X likes to first disable all outputs with dpms off before setting a new framebuffer, this hit a few warnings. Reported by Paulo Zanoni. v8: Kill the now stale comment warning that intel_crtc->active is not always updated at the right times. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 108 +++++++++++++++++++++++------------ drivers/gpu/drm/i915/intel_drv.h | 3 - 2 files changed, 70 insertions(+), 41 deletions(-)