Re: [PATCH] drm/i915: push commit_output_state past the crtc/encoder preparing

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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?

-- 
Jesse Barnes, Intel Open Source Technology Center