Re: [PATCH v3.6 20/22] drm/i915: Calculate haswell plane workaround, v4.

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, May 26, 2015 at 10:36:14AM +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.

The note about intel_crtc->atomic is out of date as of v3.  Otherwise, I
think this looks okay.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> 
> 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.
> Changes since v2:
>  - Move hsw_workaround_pipe back to crtc_state.
> Changes since v3:
>  - Return errors from haswell_mode_set_planes_workaround.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 109 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +
>  2 files changed, 80 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index de13c1c14c93..f6733a777590 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4867,42 +4867,15 @@ 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;
> +	struct intel_crtc_state *pipe_config =
> +		to_intel_crtc_state(crtc->state);
>  
>  	if (WARN_ON(intel_crtc->active))
>  		return;
> @@ -4979,7 +4952,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 = pipe_config->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)
> @@ -12169,6 +12146,71 @@ 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_state *crtc_state;
> +	struct intel_crtc *intel_crtc;
> +	struct drm_crtc *crtc;
> +	struct intel_crtc_state *first_crtc_state = NULL;
> +	struct intel_crtc_state *other_crtc_state = NULL;
> +	enum pipe first_pipe = INVALID_PIPE, enabled_pipe = INVALID_PIPE;
> +	int 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_state) {
> +			other_crtc_state = to_intel_crtc_state(crtc_state);
> +			break;
> +		} else {
> +			first_crtc_state = to_intel_crtc_state(crtc_state);
> +			first_pipe = intel_crtc->pipe;
> +		}
> +	}
> +
> +	/* No workaround needed? */
> +	if (!first_crtc_state)
> +		return 0;
> +
> +	/* w/a possibly needed, check how many crtc's are already enabled. */
> +	for_each_intel_crtc(state->dev, intel_crtc) {
> +		struct intel_crtc_state *pipe_config;
> +
> +		pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> +		if (IS_ERR(pipe_config))
> +			return PTR_ERR(pipe_config);
> +
> +		pipe_config->hsw_workaround_pipe = INVALID_PIPE;
> +
> +		if (!pipe_config->base.active ||
> +		    needs_modeset(&pipe_config->base))
> +			continue;
> +
> +		/* 2 or more enabled crtcs means no need for w/a */
> +		if (enabled_pipe != INVALID_PIPE)
> +			return 0;
> +
> +		enabled_pipe = intel_crtc->pipe;
> +	}
> +
> +	if (enabled_pipe != INVALID_PIPE)
> +		first_crtc_state->hsw_workaround_pipe = enabled_pipe;
> +	else if (other_crtc_state)
> +		other_crtc_state->hsw_workaround_pipe = first_pipe;
> +
> +	return 0;
> +}
> +
>  /* Code that should eventually be part of atomic_check() */
>  static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  {
> @@ -12192,7 +12234,10 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  	if (ret)
>  		return ret;
>  
> -	return 0;
> +	if (IS_HASWELL(dev))
> +		ret = haswell_mode_set_planes_workaround(state);
> +
> +	return ret;
>  }
>  
>  static int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 665e249ae8bf..d5f38e3d732b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -443,6 +443,9 @@ struct intel_crtc_state {
>  	int pbn;
>  
>  	struct intel_crtc_scaler_state scaler_state;
> +
> +	/* w/a for waiting 2 vblanks during crtc enable */
> +	enum pipe hsw_workaround_pipe;
>  };
>  
>  struct intel_pipe_wm {
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx