Re: [PATCH v2 07/10] drm/i915: Try to make sure cxsr is disabled around plane enable/disable

From: Matt Roper <matthew.d.roper@intel.com>
To: ville.syrjala@linux.intel.com
Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH v2 07/10] drm/i915: Try to make sure cxsr is disabled around plane enable/disable
Date: Wed, 1 Jul 2015 13:38:35 -0700	[thread overview]
Message-ID: <20150701203835.GP5645@intel.com> (raw)
In-Reply-To: <1435778018-9412-1-git-send-email-ville.syrjala@linux.intel.com>

On Wed, Jul 01, 2015 at 10:13:38PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> CxSR (or maxfifo on VLV/CHV) blocks somne changes to the plane control
> register (enable bit at least, not quite sure about the rest). So in
> order to have the plane enable/disable when we want we need to first
> kick the hardware out of cxsr.
> 
> Unfortunateloy this requires some extra vblank waits. For the CxSR
> enable after the plane update we should eventually use an async
> vblank worker, but since we don't have that just do sync vblank
> waits. For the disable case we have no choice but to do it
> synchronously.
> 
> v2: Don't add a spurious intel_pre_plane_update() to crtc disable
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
> Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> Paulo noticed some frontbuffer_bits WARNs from this patch, and sure enough
> I accidentally added another intel_pre_plane_update() to the crtc disable loop.
> I failed to notice because I had commented out the frontbuffer_bits WARNs earlier
> from my tree since they were too noisy.

I can confirm that your v2 fixes the warning spam caused by v1.

Regarding the v2 fix:
Tested-by: Matt Roper <matthew.d.roper@intel.com>

> 
>  drivers/gpu/drm/i915/intel_display.c | 34 +++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  drivers/gpu/drm/i915/intel_pm.c      | 11 ++++-------
>  3 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d67b5f1..defc4ce 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4716,6 +4716,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>  
>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
> +	if (atomic->disable_cxsr)
> +		crtc->wm.cxsr_allowed = true;
> +
>  	if (crtc->atomic.update_wm_post)
>  		intel_update_watermarks(&crtc->base);
>  
> @@ -4765,6 +4768,11 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>  
>  	if (atomic->pre_disable_primary)
>  		intel_pre_disable_primary(&crtc->base);
> +
> +	if (atomic->disable_cxsr) {
> +		crtc->wm.cxsr_allowed = false;
> +		intel_set_memory_cxsr(dev_priv, false);
> +	}
>  }
>  
>  static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
> @@ -11646,12 +11654,26 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  			 plane->base.id, was_visible, visible,
>  			 turn_off, turn_on, mode_changed);
>  
> -	if (turn_on)
> +	if (turn_on) {
>  		intel_crtc->atomic.update_wm_pre = true;
> -	else if (turn_off)
> +		/* must disable cxsr around plane enable/disable */
> +		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +			intel_crtc->atomic.disable_cxsr = true;
> +			/* to potentially re-enable cxsr */
> +			intel_crtc->atomic.wait_vblank = true;
> +			intel_crtc->atomic.update_wm_post = true;
> +		}
> +	} else if (turn_off) {
>  		intel_crtc->atomic.update_wm_post = true;
> -	else if (intel_wm_need_update(plane, plane_state))
> +		/* must disable cxsr around plane enable/disable */
> +		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +			if (is_crtc_enabled)
> +				intel_crtc->atomic.wait_vblank = true;
> +			intel_crtc->atomic.disable_cxsr = true;
> +		}
> +	} else if (intel_wm_need_update(plane, plane_state)) {
>  		intel_crtc->atomic.update_wm_pre = true;
> +	}
>  
>  	if (visible)
>  		intel_crtc->atomic.fb_bits |=
> @@ -11808,8 +11830,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  	if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES)
>  		intel_crtc_check_initial_planes(crtc, crtc_state);
>  
> -	if (mode_changed)
> -		intel_crtc->atomic.update_wm_post = !crtc_state->active;
> +	if (mode_changed && !crtc_state->active)
> +		intel_crtc->atomic.update_wm_post = true;
>  
>  	if (mode_changed && crtc_state->enable &&
>  	    dev_priv->display.crtc_compute_clock &&
> @@ -14089,6 +14111,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	intel_crtc->cursor_cntl = ~0;
>  	intel_crtc->cursor_size = ~0;
>  
> +	intel_crtc->wm.cxsr_allowed = true;
> +
>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f26a680..4e8d13e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -507,6 +507,7 @@ struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool wait_for_flips;
>  	bool disable_fbc;
> +	bool disable_cxsr;
>  	bool pre_disable_primary;
>  	bool update_wm_pre, update_wm_post;
>  	unsigned disabled_planes;
> @@ -565,6 +566,8 @@ struct intel_crtc {
>  		struct intel_pipe_wm active;
>  		/* SKL wm values currently in use */
>  		struct skl_pipe_wm skl_active;
> +		/* allow CxSR on this pipe */
> +		bool cxsr_allowed;
>  	} wm;
>  
>  	int scanline_offset;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c7c90ce..b65817d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -335,6 +335,7 @@ void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
>  	if (IS_VALLEYVIEW(dev)) {
>  		I915_WRITE(FW_BLC_SELF_VLV, enable ? FW_CSPWRDWNEN : 0);
>  		POSTING_READ(FW_BLC_SELF_VLV);
> +		dev_priv->wm.vlv.cxsr = enable;
>  	} else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
>  		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
>  		POSTING_READ(FW_BLC_SELF);
> @@ -1116,7 +1117,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>  
>  	memset(wm_state, 0, sizeof(*wm_state));
>  
> -	wm_state->cxsr = crtc->pipe != PIPE_C;
> +	wm_state->cxsr = crtc->pipe != PIPE_C && crtc->wm.cxsr_allowed;
>  	if (IS_CHERRYVIEW(dev))
>  		wm_state->num_levels = CHV_WM_NUM_LEVELS;
>  	else
> @@ -1369,10 +1370,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>  	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_PM5)
>  		chv_set_memory_pm5(dev_priv, false);
>  
> -	if (!wm.cxsr && dev_priv->wm.vlv.cxsr) {
> +	if (!wm.cxsr && dev_priv->wm.vlv.cxsr)
>  		intel_set_memory_cxsr(dev_priv, false);
> -		intel_wait_for_vblank(dev, pipe);
> -	}
>  
>  	/* FIXME should be part of crtc atomic commit */
>  	vlv_pipe_set_fifo_size(intel_crtc);
> @@ -1385,10 +1384,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>  		      wm.pipe[pipe].sprite[0], wm.pipe[pipe].sprite[1],
>  		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
>  
> -	if (wm.cxsr && !dev_priv->wm.vlv.cxsr) {
> -		intel_wait_for_vblank(dev, pipe);
> +	if (wm.cxsr && !dev_priv->wm.vlv.cxsr)
>  		intel_set_memory_cxsr(dev_priv, true);
> -	}
>  
>  	if (wm.level >= VLV_WM_LEVEL_PM5 &&
>  	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
> -- 
> 2.3.6
> 
> _______________________________________________
> 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