[Intel-gfx] [PATCH 32/42] drm/i915: Calculate haswell plane workaround.

[Daniel Vetter]

On Mon, May 11, 2015 at 04:25:08PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 109 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  2 files changed, 81 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7bc78b49f9f4..7a79659dca00 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4848,35 +4848,6 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
>  	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
>  }
>  
> -/*
> - * This implements the workaround described in the "notes" section of the mode
> - * set sequence documentation. When going from no pipes or single pipe to
> - * multiple pipes, and planes are enabled after the pipe, we need to wait at
> - * least 2 vblanks on the first pipe before enabling planes on the second pipe.
> - */
> -static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->base.dev;
> -	struct intel_crtc *crtc_it, *other_active_crtc = NULL;
> -
> -	/* We want to get the other_active_crtc only if there's only 1 other
> -	 * active crtc. */
> -	for_each_intel_crtc(dev, crtc_it) {
> -		if (!crtc_it->active || crtc_it == crtc)
> -			continue;
> -
> -		if (other_active_crtc)
> -			return;
> -
> -		other_active_crtc = crtc_it;
> -	}
> -	if (!other_active_crtc)
> -		return;
> -
> -	intel_wait_for_vblank(dev, other_active_crtc->pipe);
> -	intel_wait_for_vblank(dev, other_active_crtc->pipe);
> -}
> -
>  static void haswell_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -4967,7 +4938,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  
>  	/* If we change the relative order between pipe/planes enabling, we need
>  	 * to change the workaround. */
> -	haswell_mode_set_planes_workaround(intel_crtc);
> +	if (pipe_config->hsw_workaround_pipe != INVALID_PIPE) {
> +		intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
> +		intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
> +	}
>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> @@ -12147,6 +12121,74 @@ done:
>  	return ret;
>  }
>  
> +/*
> + * This implements the workaround described in the "notes" section of the mode
> + * set sequence documentation. When going from no pipes or single pipe to
> + * multiple pipes, and planes are enabled after the pipe, we need to wait at
> + * least 2 vblanks on the first pipe before enabling planes on the second pipe.
> + */
> +static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	struct intel_crtc_state *first_crtc_state = NULL, *other_crtc_state = NULL;
> +	struct intel_crtc *intel_crtc, *first_crtc = NULL, *enabled_crtc;
> +	int enabled_crtcs = 0, ret, i;
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(crtc_state);
> +
> +		pipe_config->hsw_workaround_pipe = INVALID_PIPE;

This kind of state resetting should be done in duplicate_state.

> +
> +		if (!crtc_state->active)
> +			continue;
> +
> +		if (!needs_modeset(crtc_state)) {
> +			enabled_crtcs++;
> +			enabled_crtc = to_intel_crtc(crtc);
> +			continue;
> +		}
> +
> +		if (first_crtc) {
> +			other_crtc_state = pipe_config;
> +			break;
> +		}
> +		first_crtc = to_intel_crtc(crtc);
> +		first_crtc_state = pipe_config;
> +	}
> +
> +	/* No workaround needed? */
> +	if (!first_crtc || enabled_crtcs > 1)
> +		return 0;
> +
> +	for_each_intel_crtc(state->dev, intel_crtc) {
> +		if (state->crtcs[drm_crtc_index(&intel_crtc->base)])
> +			continue;
> +
> +		ret = drm_modeset_lock(&intel_crtc->base.mutex,
> +				       state->acquire_ctx);
> +		if (ret)
> +			return ret;
> +
> +		if (!intel_crtc->base.state->active)
> +			continue;

Imo just unconditionally acquire the crtc state, there' shouldn't be any
harm in that (as long as we leave mode/active/planes_changed untouched).

> +
> +		/* 2 enabled crtcs means no need for w/a */
> +		if (++enabled_crtcs >= 2)
> +			return 0;
> +
> +		enabled_crtc = intel_crtc;
> +	}
> +
> +	if (enabled_crtcs == 1)
> +		first_crtc_state->hsw_workaround_pipe = enabled_crtc->pipe;

first_crtc_state could be miscomputed if 1 crtc is already on and you add
another one. Then first_crtc will point at the other crtc since that's the
only one at first in the atomic set. Imo a simpler algo would be:

1. Check whether your activating any crtc at all.
2. If so add all crtc states.
3. Same loop as the old one, just using for_each_crtc_in_state.

There's other cases with shared resources where we need to grab all crtc
locks already anyway (shared dpll), trying to be clever doesn't seem
beneficial. And if this indeed becomes a problem then we need a global
state ww mutex and use that (instead of crtc locks) for this book-keeping:
First enable crtc would set global_state->hsw_wa_pipe, 2nd and later would
clear it. We might need this eventually (I've heard rumours about people
not liking stalls when unplugging external screens that much), but let's
not overcomplicate things while we do this conversion.
-Daniel

> +	else if (other_crtc_state)
> +		other_crtc_state->hsw_workaround_pipe = first_crtc->pipe;
> +
> +	return 0;
> +}
> +
>  /* Code that should eventually be part of atomic_check() */
>  static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  {
> @@ -12158,6 +12200,13 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  		return -EINVAL;
>  	}
>  
> +	if (IS_HASWELL(state->dev)) {
> +		ret = haswell_mode_set_planes_workaround(state);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/*
>  	 * See if the config requires any additional preparation, e.g.
>  	 * to adjust global state with pipes off.  We need to do this
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index eb87f82b5aff..6c2ba5dbcd79 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -437,6 +437,8 @@ struct intel_crtc_state {
>  	int pbn;
>  
>  	struct intel_crtc_scaler_state scaler_state;
> +
> +	enum pipe hsw_workaround_pipe;
>  };
>  
>  struct intel_pipe_wm {
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch