[Intel-gfx] [PATCH v2 15/17] drm/i915: Calculate haswell plane workaround, v2.

[Daniel Vetter]

On Wed, May 13, 2015 at 10:23:45PM +0200, Maarten Lankhorst wrote:
> This needs to be a global check because at the time of crtc checking
> not all modesets have to be calculated yet. Use intel_crtc->atomic
> because there's no reason to keep it in state.

Hm, intel_crtc->atomic needs to be moved into intel_crtc_state eventually,
so I don't really understand your reasoning here. Yes it's not really a
static state, but we carry all the other state transition hints like
->need_modeset or ->active_changed around in state objects too. Imo it's
the right place for this stuff, the only tricky part is that we must clear
them all in the relevant duplicate_state hooks.

Anyway nothing that blocks this one, just something to keep in mind for
the road ahead.
-Daniel

> 
> Changes since v1:
>  - Use intel_crtc->atomic as a place to put hsw_workaround_pipe.
>  - Make sure quirk only applies to haswell.
>  - Use first loop to iterate over newly enabled crtc's only.
>    This increases readability.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 100 ++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h     |   3 ++
>  2 files changed, 72 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1d3b0233f070..9316b0be4f5b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4897,42 +4897,13 @@ 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;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
> -	int pipe = intel_crtc->pipe;
> +	int pipe = intel_crtc->pipe, hsw_workaround_pipe;
>  
>  	if (WARN_ON(intel_crtc->active))
>  		return;
> @@ -5009,7 +4980,11 @@ 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);
> +	hsw_workaround_pipe = intel_crtc->atomic.hsw_workaround_pipe;
> +	if (IS_HASWELL(dev) && hsw_workaround_pipe != INVALID_PIPE) {
> +		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> +		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> +	}
>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> @@ -12099,6 +12074,66 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
>  	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 *intel_crtc, *enabled_crtc;
> +	struct intel_crtc *first_crtc = NULL, *other_crtc = NULL;
> +	int enabled_crtcs = 0, i;
> +
> +	/* look at all crtc's that are going to be enabled in during modeset */
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!crtc_state->active || !needs_modeset(crtc_state))
> +			continue;
> +
> +		if (first_crtc) {
> +			other_crtc = intel_crtc;
> +			break;
> +		} else {
> +			first_crtc = intel_crtc;
> +		}
> +	}
> +
> +	/* No workaround needed? */
> +	if (!first_crtc)
> +		return 0;
> +
> +	/* w/a possibly needed, check how many crtc's are already enabled. */
> +	for_each_intel_crtc(state->dev, intel_crtc) {
> +		intel_crtc->atomic.hsw_workaround_pipe = INVALID_PIPE;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state,
> +						       &intel_crtc->base);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +
> +		if (!crtc_state->active || needs_modeset(crtc_state))
> +			continue;
> +
> +		/* 2 or more enabled crtcs means no need for w/a */
> +		if (++enabled_crtcs >= 2)
> +			return 0;
> +
> +		enabled_crtc = intel_crtc;
> +	}
> +
> +	if (enabled_crtcs == 1)
> +		first_crtc->atomic.hsw_workaround_pipe = enabled_crtc->pipe;
> +	else if (other_crtc)
> +		other_crtc->atomic.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)
>  {
> @@ -12122,6 +12157,9 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  	if (ret)
>  		return ret;
>  
> +	if (IS_HASWELL(dev))
> +		haswell_mode_set_planes_workaround(state);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 65f5f3d41b5a..1aa10a9445de 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -490,6 +490,9 @@ struct intel_crtc_atomic_commit {
>  	bool update_fbc;
>  	bool post_enable_primary;
>  	unsigned update_sprite_watermarks;
> +
> +	/* Operations to perform during modeset */
> +	enum pipe hsw_workaround_pipe;
>  };
>  
>  struct intel_crtc {
> -- 
> 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