On Tue, 4 Sep 2012 21:32:28 +0200
Daniel Vetter <daniel.vetter at 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 at 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?
--
Jesse Barnes, Intel Open Source Technology Center
