Re: [PATCH 3/3] drm/i915: Clean up skip_intermediate_wm handling

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 27, 2017 at 09:59:47AM +0200, Maarten Lankhorst wrote:
> Originally we implemented skip_intermediate_wm to fix up garbage
> watermarks for ILK watermarks that are insane, but it makes sense
> to extend this to all platforms with 2-style watermark programming.

I don't particularly like skip_intermediate_wm. g4x/vlv/chv watermarks
get sanitized in a different way, so this is not needed unless something
is really seriously fscked up. So I think I'd rather change ILK+ to
sanitize the same way and kill skip_intermediate_wm.

If the current watermarks are total garbage (which I doubt), then I
think the safest solution would be to force a modeset at that point
rather than hoping that just smashing in new watermarks is going to save
the day. Running with bad watermarks could even mean that the pipe has
already died (at least on some platforms).

> 
> We don't know what the previous contents of the watermarks are, and they
> could be something entirely garbage when the pipe is off, so computing
> them should only be based on the new state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++----------
>  drivers/gpu/drm/i915/intel_pm.c      |  9 ++++++---
>  2 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a369b35d044d..09313af4d64d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3571,8 +3571,7 @@ __intel_display_resume(struct drm_device *dev,
>  	}
>  
>  	/* ignore any reset values/BIOS leftovers in the WM registers */
> -	if (!HAS_GMCH_DISPLAY(to_i915(dev)))
> -		to_intel_atomic_state(state)->skip_intermediate_wm = true;
> +	to_intel_atomic_state(state)->skip_intermediate_wm = true;
>  
>  	ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
>  
> @@ -10379,7 +10378,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc_state);
> -	struct drm_atomic_state *state = crtc_state->state;
>  	int ret;
>  	bool mode_changed = needs_modeset(crtc_state);
>  
> @@ -10416,8 +10414,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  		}
>  	}
>  
> -	if (dev_priv->display.compute_intermediate_wm &&
> -	    !to_intel_atomic_state(state)->skip_intermediate_wm) {
> +	if (dev_priv->display.compute_intermediate_wm) {
>  		if (WARN_ON(!dev_priv->display.compute_pipe_wm))
>  			return 0;
>  
> @@ -10433,9 +10430,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
>  			return ret;
>  		}
> -	} else if (dev_priv->display.compute_intermediate_wm) {
> -		if (HAS_PCH_SPLIT(dev_priv) && INTEL_GEN(dev_priv) < 9)
> -			pipe_config->wm.ilk.intermediate = pipe_config->wm.ilk.optimal;
>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 9) {
> @@ -14416,8 +14410,7 @@ static void sanitize_watermarks(struct drm_device *dev)
>  	 * intermediate watermarks (since we don't trust the current
>  	 * watermarks).
>  	 */
> -	if (!HAS_GMCH_DISPLAY(dev_priv))
> -		intel_state->skip_intermediate_wm = true;
> +	intel_state->skip_intermediate_wm = true;
>  
>  	ret = intel_atomic_check(dev, state);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a9e7683d5a0d..346c30a222cd 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1420,7 +1420,8 @@ static int g4x_compute_intermediate_wm(struct drm_device *dev,
>  	const struct g4x_wm_state *active = &old_crtc_state->wm.g4x.optimal;
>  	enum plane_id plane_id;
>  
> -	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
> +	if (!new_crtc_state->base.active || intel_state->skip_intermediate_wm ||
> +	    drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
>  		*intermediate = *optimal;
>  
>  		intermediate->cxsr = false;
> @@ -2053,7 +2054,8 @@ static int vlv_compute_intermediate_wm(struct drm_device *dev,
>  	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
>  	int level;
>  
> -	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
> +	if (!new_crtc_state->base.active || intel_state->skip_intermediate_wm ||
> +	    drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
>  		*intermediate = *optimal;
>  
>  		intermediate->cxsr = false;
> @@ -3162,7 +3164,8 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
>  	 * and after the vblank.
>  	 */
>  	*a = newstate->wm.ilk.optimal;
> -	if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
> +	if (!newstate->base.active || intel_state->skip_intermediate_wm ||
> +	    drm_atomic_crtc_needs_modeset(&newstate->base))
>  		return 0;
>  
>  	a->pipe_enabled |= b->pipe_enabled;
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx