On Wed, Jul 01, 2015 at 10:13:38PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Reviewed-by: Clint Taylor <Clinton.A.Taylor@xxxxxxxxx>
> Tested-by: Clint Taylor <Clinton.A.Taylor@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
