[PATCH 0/3] drm/i915: Enable fastboot, v2!

[PATCH 0/3] drm/i915: Enable fastboot, v2!

[Maarten Lankhorst]

Small fixes for IPS, and then we flip the switch! :-)

Maarten Lankhorst (3):
  drm/i915: Make ips_enabled a property depending on whether IPS is
    enabled.
  drm/i915: Enable IPS for sprite plane
  drm/i915: Re-enable fastboot by default

 drivers/gpu/drm/i915/i915_params.h    |  2 +-
 drivers/gpu/drm/i915/intel_display.c  | 76 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
 3 files changed, 36 insertions(+), 44 deletions(-)

-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled.

[Maarten Lankhorst]

ips_enabled was used as a variable of whether IPS can be enabled or not,
but should be used to test whether IPS is actually enabled.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  | 75 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
 2 files changed, 35 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b3dec1e6640..8283e80597bd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (!crtc->config->ips_enabled)
+	if (!crtc_state->ips_enabled)
 		return;
 
 	/*
@@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 
-	/*
-	 * FIXME IPS should be fine as long as one plane is
-	 * enabled, but in practice it seems to have problems
-	 * when going from primary only to sprite only and vice
-	 * versa.
-	 */
-	hsw_enable_ips(new_crtc_state);
-
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
 	 * So don't enable underrun reporting before at least some planes
@@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
 	intel_check_pch_fifo_underruns(dev_priv);
 }
 
-/* FIXME move all this to pre_plane_update() with proper state tracking */
+/* FIXME get rid of this and use pre_plane_update */
 static void
-intel_pre_disable_primary(struct drm_crtc *crtc,
-			  const struct intel_crtc_state *old_crtc_state)
+intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
 
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
-	 * So diasble underrun reporting before all the planes get disabled.
-	 * FIXME: Need to fix the logic to work when we turn off all planes
-	 * but leave the pipe running.
+	 * So disable underrun reporting before all the planes get disabled.
 	 */
 	if (IS_GEN2(dev_priv))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
 
-	/*
-	 * FIXME IPS should be fine as long as one plane is
-	 * enabled, but in practice it seems to have problems
-	 * when going from primary only to sprite only and vice
-	 * versa.
-	 */
-	hsw_disable_ips(old_crtc_state);
-}
-
-/* FIXME get rid of this and use pre_plane_update */
-static void
-intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;
-
-	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
+	hsw_disable_ips(to_intel_crtc_state(crtc->state));
 
 	/*
 	 * Vblank time updates from the shadow to live plane control register
@@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	if (pipe_config->update_wm_post && pipe_config->base.active)
 		intel_update_watermarks(crtc);
 
+	if (!old_crtc_state->ips_enabled ||
+	    needs_modeset(&old_crtc_state->base) ||
+	    pipe_config->update_pipe)
+		hsw_enable_ips(pipe_config);
+
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
@@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 	struct intel_atomic_state *old_intel_state =
 		to_intel_atomic_state(old_state);
 
+	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
+		hsw_disable_ips(old_crtc_state);
+
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			intel_atomic_get_new_plane_state(old_intel_state,
@@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 			to_intel_plane_state(old_pri_state);
 
 		intel_fbc_pre_update(crtc, pipe_config, primary_state);
-
-		if (old_primary_state->base.visible &&
+		/*
+		 * Gen2 reports pipe underruns whenever all planes are disabled.
+		 * So disable underrun reporting before all the planes get disabled.
+		 */
+		if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
 		    (modeset || !primary_state->base.visible))
-			intel_pre_disable_primary(&crtc->base, old_crtc_state);
+			intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
 	}
 
 	/*
@@ -6231,12 +6213,25 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
 static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
 				     struct intel_crtc_state *pipe_config)
 {
+	u8 visible_planes;
+
 	if (pipe_config->ips_force_disable)
 		return false;
 
 	if (pipe_config->pipe_bpp > 24)
 		return false;
 
+	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
+
+	/*
+	 * FIXME IPS should be fine as long as one plane is
+	 * enabled, but in practice it seems to have problems
+	 * when going from primary only to sprite only and vice
+	 * versa.
+	 */
+	if (visible_planes != BIT(PLANE_PRIMARY))
+		return false;
+
 	/* HSW can handle pixel rate up to cdclk? */
 	if (IS_HASWELL(dev_priv))
 		return true;
@@ -6378,9 +6373,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 
 	intel_crtc_compute_pixel_rate(pipe_config);
 
-	if (HAS_IPS(dev_priv))
-		hsw_compute_ips_config(crtc, pipe_config);
-
 	if (pipe_config->has_pch_encoder)
 		return ironlake_fdi_compute_config(crtc, pipe_config);
 
@@ -10488,6 +10480,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 							 pipe_config);
 	}
 
+	if (HAS_IPS(dev_priv))
+		hsw_compute_ips_config(intel_crtc, pipe_config);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 61641d479b93..1f5cd572a7ff 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 		 * completely disable it.
 		 */
 		pipe_config->ips_force_disable = enable;
-		if (pipe_config->ips_enabled == enable)
-			pipe_config->base.connectors_changed = true;
 	}
 
 	if (IS_HASWELL(dev_priv)) {
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Enable IPS for sprite plane

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8283e80597bd..38a1cdb3dbb2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 		intel_fbc_post_update(crtc);
 
 		if (primary_state->base.visible &&
-		    (needs_modeset(&pipe_config->base) ||
+		    (pipe_config->disable_cxsr ||
 		     !old_primary_state->base.visible))
 			intel_post_enable_primary(&crtc->base, pipe_config);
 	}
@@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 	struct intel_atomic_state *old_intel_state =
 		to_intel_atomic_state(old_state);
 
-	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
+	if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)
 		hsw_disable_ips(old_crtc_state);
 
 	if (old_pri_state) {
@@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
 	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
 
 	/*
-	 * FIXME IPS should be fine as long as one plane is
-	 * enabled, but in practice it seems to have problems
-	 * when going from primary only to sprite only and vice
-	 * versa.
+	 * IPS should be fine as long as one plane is enabled, but
+	 * temporarily disable it when when going from primary only
+	 * to sprite only and vice versa.
 	 */
-	if (visible_planes != BIT(PLANE_PRIMARY))
+	if (hweight32(visible_planes) != 1)
 		return false;
 
 	/* HSW can handle pixel rate up to cdclk? */
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[(resend) PATCH 3/3] drm/i915: Re-enable fastboot by default

[Maarten Lankhorst]

This fix was originally reverted because it left a chromebook pixel
black, and no immediate fix was available. This has been fixed in the
meantime.

Rather than trying to remove the parameter, set it to default to true
for now, so we can always back out if required.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Testcase: kms_panel_fitting
---
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c7292268ed43..b99cb58801e6 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -57,7 +57,7 @@
 	param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT)) \
 	param(bool, enable_cmd_parser, true) \
 	param(bool, enable_hangcheck, true) \
-	param(bool, fastboot, false) \
+	param(bool, fastboot, true) \
 	param(bool, prefault_disable, false) \
 	param(bool, load_detect_test, false) \
 	param(bool, force_reset_modeset_test, false) \
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled.

[Ville Syrjälä]

On Fri, Nov 17, 2017 at 04:37:54PM +0100, Maarten Lankhorst wrote:
> ips_enabled was used as a variable of whether IPS can be enabled or not,
> but should be used to test whether IPS is actually enabled.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  | 75 ++++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
>  2 files changed, 35 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3b3dec1e6640..8283e80597bd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> -	if (!crtc->config->ips_enabled)
> +	if (!crtc_state->ips_enabled)
>  		return;
>  
>  	/*
> @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
>  
> -	/*
> -	 * FIXME IPS should be fine as long as one plane is
> -	 * enabled, but in practice it seems to have problems
> -	 * when going from primary only to sprite only and vice
> -	 * versa.
> -	 */
> -	hsw_enable_ips(new_crtc_state);
> -
>  	/*
>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
>  	 * So don't enable underrun reporting before at least some planes
> @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
>  	intel_check_pch_fifo_underruns(dev_priv);
>  }
>  
> -/* FIXME move all this to pre_plane_update() with proper state tracking */
> +/* FIXME get rid of this and use pre_plane_update */
>  static void
> -intel_pre_disable_primary(struct drm_crtc *crtc,
> -			  const struct intel_crtc_state *old_crtc_state)
> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
>  
>  	/*
>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
> -	 * So diasble underrun reporting before all the planes get disabled.
> -	 * FIXME: Need to fix the logic to work when we turn off all planes
> -	 * but leave the pipe running.
> +	 * So disable underrun reporting before all the planes get disabled.
>  	 */
>  	if (IS_GEN2(dev_priv))
>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>  
> -	/*
> -	 * FIXME IPS should be fine as long as one plane is
> -	 * enabled, but in practice it seems to have problems
> -	 * when going from primary only to sprite only and vice
> -	 * versa.
> -	 */
> -	hsw_disable_ips(old_crtc_state);
> -}
> -
> -/* FIXME get rid of this and use pre_plane_update */
> -static void
> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int pipe = intel_crtc->pipe;
> -
> -	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
> +	hsw_disable_ips(to_intel_crtc_state(crtc->state));

BTW thinking about the lack of readout on BDW. In case the BIOS ever
enables IPS, I think we should sanitize ips_enabled to true on BDW.
Or maybe just have the redaout code always set ips_enabled=true on BDW?
That way the first modeset will try to turn it off regardless of whether it
was enabled or not.

>  
>  	/*
>  	 * Vblank time updates from the shadow to live plane control register
> @@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  	if (pipe_config->update_wm_post && pipe_config->base.active)
>  		intel_update_watermarks(crtc);
>  
> +	if (!old_crtc_state->ips_enabled ||
> +	    needs_modeset(&old_crtc_state->base) ||

Shoudldn't that be needs_modeset(new_crtc_state)?

> +	    pipe_config->update_pipe)

Why do we have to check that?

> +		hsw_enable_ips(pipe_config);
> +
>  	if (old_pri_state) {
>  		struct intel_plane_state *primary_state =
>  			intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
> @@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>  	struct intel_atomic_state *old_intel_state =
>  		to_intel_atomic_state(old_state);
>  
> +	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
> +		hsw_disable_ips(old_crtc_state);
> +
>  	if (old_pri_state) {
>  		struct intel_plane_state *primary_state =
>  			intel_atomic_get_new_plane_state(old_intel_state,
> @@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>  			to_intel_plane_state(old_pri_state);
>  
>  		intel_fbc_pre_update(crtc, pipe_config, primary_state);
> -
> -		if (old_primary_state->base.visible &&
> +		/*
> +		 * Gen2 reports pipe underruns whenever all planes are disabled.
> +		 * So disable underrun reporting before all the planes get disabled.
> +		 */
> +		if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
>  		    (modeset || !primary_state->base.visible))
> -			intel_pre_disable_primary(&crtc->base, old_crtc_state);
> +			intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
>  	}
>  
>  	/*
> @@ -6231,12 +6213,25 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
>  static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
>  				     struct intel_crtc_state *pipe_config)
>  {
> +	u8 visible_planes;
> +
>  	if (pipe_config->ips_force_disable)
>  		return false;
>  
>  	if (pipe_config->pipe_bpp > 24)
>  		return false;
>  
> +	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
> +
> +	/*
> +	 * FIXME IPS should be fine as long as one plane is
> +	 * enabled, but in practice it seems to have problems
> +	 * when going from primary only to sprite only and vice
> +	 * versa.
> +	 */
> +	if (visible_planes != BIT(PLANE_PRIMARY))
> +		return false;

I think that should be '(active_planes & PRIMARY) == 0'. As in we don't
care what the other planes are doing, IPS just tracks the primary state
(for the time being).

What's missing from this patch is changing the cdclk vs. ips workaround
to not assume that cdclk can be increased to accomodate ips.

> +
>  	/* HSW can handle pixel rate up to cdclk? */
>  	if (IS_HASWELL(dev_priv))
>  		return true;
> @@ -6378,9 +6373,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  
>  	intel_crtc_compute_pixel_rate(pipe_config);
>  
> -	if (HAS_IPS(dev_priv))
> -		hsw_compute_ips_config(crtc, pipe_config);
> -
>  	if (pipe_config->has_pch_encoder)
>  		return ironlake_fdi_compute_config(crtc, pipe_config);
>  
> @@ -10488,6 +10480,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  							 pipe_config);
>  	}
>  
> +	if (HAS_IPS(dev_priv))
> +		hsw_compute_ips_config(intel_crtc, pipe_config);
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 61641d479b93..1f5cd572a7ff 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>  		 * completely disable it.
>  		 */
>  		pipe_config->ips_force_disable = enable;
> -		if (pipe_config->ips_enabled == enable)
> -			pipe_config->base.connectors_changed = true;
>  	}
>  
>  	if (IS_HASWELL(dev_priv)) {
> -- 
> 2.15.0
> 
> _______________________________________________
> 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

Re: [PATCH 2/3] drm/i915: Enable IPS for sprite plane

[Ville Syrjälä]

On Fri, Nov 17, 2017 at 04:37:55PM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8283e80597bd..38a1cdb3dbb2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  		intel_fbc_post_update(crtc);
>  
>  		if (primary_state->base.visible &&
> -		    (needs_modeset(&pipe_config->base) ||
> +		    (pipe_config->disable_cxsr ||
>  		     !old_primary_state->base.visible))
>  			intel_post_enable_primary(&crtc->base, pipe_config);
>  	}
> @@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>  	struct intel_atomic_state *old_intel_state =
>  		to_intel_atomic_state(old_state);
>  
> -	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
> +	if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)

What does IPS have to do with cxsr?

>  		hsw_disable_ips(old_crtc_state);
>  
>  	if (old_pri_state) {
> @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
>  	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
>  
>  	/*
> -	 * FIXME IPS should be fine as long as one plane is
> -	 * enabled, but in practice it seems to have problems
> -	 * when going from primary only to sprite only and vice
> -	 * versa.
> +	 * IPS should be fine as long as one plane is enabled, but
> +	 * temporarily disable it when when going from primary only
> +	 * to sprite only and vice versa.
>  	 */
> -	if (visible_planes != BIT(PLANE_PRIMARY))
> +	if (hweight32(visible_planes) != 1)
>  		return false;

That should just be
if (active_planes == 0)
	return false;

assuming we have no problems with the toggling between
primary only and sprite only.

I can't recall how the cursor affecrs IPS. But I think IPS should 
work as long as any plane (including the cursor) is enabled.

>  
>  	/* HSW can handle pixel rate up to cdclk? */
> -- 
> 2.15.0
> 
> _______________________________________________
> 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

✓ Fi.CI.BAT: success for drm/i915: Enable fastboot, v2!

[Patchwork]

== Series Details ==

Series: drm/i915: Enable fastboot, v2!
URL   : https://patchwork.freedesktop.org/series/34010/
State : success

== Summary ==

Series 34010v1 drm/i915: Enable fastboot, v2!
https://patchwork.freedesktop.org/api/1.0/series/34010/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                fail       -> PASS       (fi-kbl-7500u) fdo#103163

fdo#103163 https://bugs.freedesktop.org/show_bug.cgi?id=103163

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:445s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:457s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:390s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:539s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:278s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:508s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:511s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:499s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:490s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:426s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:271s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:543s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:431s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:440s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:424s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:462s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:487s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:541s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:475s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:533s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:578s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:455s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:541s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:561s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:521s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:504s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:458s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:561s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:419s
Blacklisted hosts:
fi-cfl-s2        total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:615s
fi-glk-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:494s

ed17259c4b681ddc178c14368f1a3db512016989 drm-tip: 2017y-11m-17d-12h-03m-24s UTC integration manifest
f8d72017605a drm/i915: Re-enable fastboot by default
d8f81e71499f drm/i915: Enable IPS for sprite plane
faa673850459 drm/i915: Make ips_enabled a property depending on whether IPS is enabled.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7175/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] drm/i915: Enable fastboot, v2!

[Rodrigo Vivi]

On Fri, Nov 17, 2017 at 03:37:53PM +0000, Maarten Lankhorst wrote:
> Small fixes for IPS, and then we flip the switch! :-)
> 
> Maarten Lankhorst (3):
>   drm/i915: Make ips_enabled a property depending on whether IPS is
>     enabled.
>   drm/i915: Enable IPS for sprite plane
>   drm/i915: Re-enable fastboot by default

Just a heads up to DK that this will for sure impact PSR.

PSR is enabled when we enable DDI. With fast boot that path
is usually not executed any longer so PSR will never get enabled.

Not sure about FBC and DRRS though...

Thanks,
Rodrigo.

> 
>  drivers/gpu/drm/i915/i915_params.h    |  2 +-
>  drivers/gpu/drm/i915/intel_display.c  | 76 ++++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
>  3 files changed, 36 insertions(+), 44 deletions(-)
> 
> -- 
> 2.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Enable fastboot, v2!

[Patchwork]

== Series Details ==

Series: drm/i915: Enable fastboot, v2!
URL   : https://patchwork.freedesktop.org/series/34010/
State : success

== Summary ==

Test drv_selftest:
        Subgroup mock_sanitycheck:
                pass       -> DMESG-WARN (shard-hsw) fdo#103719
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
                fail       -> PASS       (shard-snb) fdo#101623
        Subgroup fbc-1p-primscrn-pri-shrfb-draw-mmap-gtt:
                skip       -> PASS       (shard-hsw) fdo#103167
Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252
Test kms_busy:
        Subgroup extended-modeset-hang-newfb-with-reset-render-a:
                skip       -> PASS       (shard-hsw) fdo#102249
Test kms_atomic_transition:
        Subgroup 1x-modeset-transitions-fencing:
                skip       -> PASS       (shard-hsw)
Test kms_plane:
        Subgroup plane-position-hole-pipe-a-planes:
                skip       -> PASS       (shard-hsw)
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912
Test drv_module_reload:
        Subgroup basic-reload-inject:
                dmesg-warn -> PASS       (shard-hsw) fdo#102707

fdo#103719 https://bugs.freedesktop.org/show_bug.cgi?id=103719
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102249 https://bugs.freedesktop.org/show_bug.cgi?id=102249
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707

shard-hsw        total:2585 pass:1473 dwarn:2   dfail:1   fail:10  skip:1099 time:9448s
shard-snb        total:2585 pass:1261 dwarn:1   dfail:1   fail:11  skip:1311 time:8002s
Blacklisted hosts:
shard-apl        total:2563 pass:1600 dwarn:2   dfail:0   fail:23  skip:936 time:12973s
shard-kbl        total:2492 pass:1659 dwarn:9   dfail:1   fail:21  skip:801 time:10337s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7175/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled.

[Maarten Lankhorst]

Op 17-11-17 om 16:52 schreef Ville Syrjälä:
> On Fri, Nov 17, 2017 at 04:37:54PM +0100, Maarten Lankhorst wrote:
>> ips_enabled was used as a variable of whether IPS can be enabled or not,
>> but should be used to test whether IPS is actually enabled.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c  | 75 ++++++++++++++++-------------------
>>  drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
>>  2 files changed, 35 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3b3dec1e6640..8283e80597bd 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
>>  	struct drm_device *dev = crtc->base.dev;
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>  
>> -	if (!crtc->config->ips_enabled)
>> +	if (!crtc_state->ips_enabled)
>>  		return;
>>  
>>  	/*
>> @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  	int pipe = intel_crtc->pipe;
>>  
>> -	/*
>> -	 * FIXME IPS should be fine as long as one plane is
>> -	 * enabled, but in practice it seems to have problems
>> -	 * when going from primary only to sprite only and vice
>> -	 * versa.
>> -	 */
>> -	hsw_enable_ips(new_crtc_state);
>> -
>>  	/*
>>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
>>  	 * So don't enable underrun reporting before at least some planes
>> @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
>>  	intel_check_pch_fifo_underruns(dev_priv);
>>  }
>>  
>> -/* FIXME move all this to pre_plane_update() with proper state tracking */
>> +/* FIXME get rid of this and use pre_plane_update */
>>  static void
>> -intel_pre_disable_primary(struct drm_crtc *crtc,
>> -			  const struct intel_crtc_state *old_crtc_state)
>> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
>>  
>>  	/*
>>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
>> -	 * So diasble underrun reporting before all the planes get disabled.
>> -	 * FIXME: Need to fix the logic to work when we turn off all planes
>> -	 * but leave the pipe running.
>> +	 * So disable underrun reporting before all the planes get disabled.
>>  	 */
>>  	if (IS_GEN2(dev_priv))
>>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>>  
>> -	/*
>> -	 * FIXME IPS should be fine as long as one plane is
>> -	 * enabled, but in practice it seems to have problems
>> -	 * when going from primary only to sprite only and vice
>> -	 * versa.
>> -	 */
>> -	hsw_disable_ips(old_crtc_state);
>> -}
>> -
>> -/* FIXME get rid of this and use pre_plane_update */
>> -static void
>> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>> -{
>> -	struct drm_device *dev = crtc->dev;
>> -	struct drm_i915_private *dev_priv = to_i915(dev);
>> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	int pipe = intel_crtc->pipe;
>> -
>> -	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
>> +	hsw_disable_ips(to_intel_crtc_state(crtc->state));
> BTW thinking about the lack of readout on BDW. In case the BIOS ever
> enables IPS, I think we should sanitize ips_enabled to true on BDW.
> Or maybe just have the redaout code always set ips_enabled=true on BDW?
> That way the first modeset will try to turn it off regardless of whether it
> was enabled or not.
Yes, good idea. :)
>>  
>>  	/*
>>  	 * Vblank time updates from the shadow to live plane control register
>> @@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>>  	if (pipe_config->update_wm_post && pipe_config->base.active)
>>  		intel_update_watermarks(crtc);
>>  
>> +	if (!old_crtc_state->ips_enabled ||
>> +	    needs_modeset(&old_crtc_state->base) ||
> Shoudldn't that be needs_modeset(new_crtc_state)?
Yes, oops..
>> +	    pipe_config->update_pipe)
> Why do we have to check that?
In case we take over the hw state from bios..

If we default ips_enabled to true on BDW and read out on HSW again, then this
check will make IPS work, regardless of bios setting..

>> +		hsw_enable_ips(pipe_config);
>> +
>>  	if (old_pri_state) {
>>  		struct intel_plane_state *primary_state =
>>  			intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
>> @@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>>  	struct intel_atomic_state *old_intel_state =
>>  		to_intel_atomic_state(old_state);
>>  
>> +	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
>> +		hsw_disable_ips(old_crtc_state);
>> +
>>  	if (old_pri_state) {
>>  		struct intel_plane_state *primary_state =
>>  			intel_atomic_get_new_plane_state(old_intel_state,
>> @@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>>  			to_intel_plane_state(old_pri_state);
>>  
>>  		intel_fbc_pre_update(crtc, pipe_config, primary_state);
>> -
>> -		if (old_primary_state->base.visible &&
>> +		/*
>> +		 * Gen2 reports pipe underruns whenever all planes are disabled.
>> +		 * So disable underrun reporting before all the planes get disabled.
>> +		 */
>> +		if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
>>  		    (modeset || !primary_state->base.visible))
>> -			intel_pre_disable_primary(&crtc->base, old_crtc_state);
>> +			intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
>>  	}
>>  
>>  	/*
>> @@ -6231,12 +6213,25 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
>>  static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
>>  				     struct intel_crtc_state *pipe_config)
>>  {
>> +	u8 visible_planes;
>> +
>>  	if (pipe_config->ips_force_disable)
>>  		return false;
>>  
>>  	if (pipe_config->pipe_bpp > 24)
>>  		return false;
>>  
>> +	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
>> +
>> +	/*
>> +	 * FIXME IPS should be fine as long as one plane is
>> +	 * enabled, but in practice it seems to have problems
>> +	 * when going from primary only to sprite only and vice
>> +	 * versa.
>> +	 */
>> +	if (visible_planes != BIT(PLANE_PRIMARY))
>> +		return false;
> I think that should be '(active_planes & PRIMARY) == 0'. As in we don't
> care what the other planes are doing, IPS just tracks the primary state
> (for the time being).
Oops, faultily assumed from the comment that IPS only worked with the primary plane
or sprite plane, not both. I'll correct it.
> What's missing from this patch is changing the cdclk vs. ips workaround
> to not assume that cdclk can be increased to accomodate ips.
Hm true, I missed that..

Will send a new version in a bit.
>> +
>>  	/* HSW can handle pixel rate up to cdclk? */
>>  	if (IS_HASWELL(dev_priv))
>>  		return true;
>> @@ -6378,9 +6373,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>  
>>  	intel_crtc_compute_pixel_rate(pipe_config);
>>  
>> -	if (HAS_IPS(dev_priv))
>> -		hsw_compute_ips_config(crtc, pipe_config);
>> -
>>  	if (pipe_config->has_pch_encoder)
>>  		return ironlake_fdi_compute_config(crtc, pipe_config);
>>  
>> @@ -10488,6 +10480,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>>  							 pipe_config);
>>  	}
>>  
>> +	if (HAS_IPS(dev_priv))
>> +		hsw_compute_ips_config(intel_crtc, pipe_config);
>> +
>>  	return ret;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
>> index 61641d479b93..1f5cd572a7ff 100644
>> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
>> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
>> @@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>>  		 * completely disable it.
>>  		 */
>>  		pipe_config->ips_force_disable = enable;
>> -		if (pipe_config->ips_enabled == enable)
>> -			pipe_config->base.connectors_changed = true;
>>  	}
>>  
>>  	if (IS_HASWELL(dev_priv)) {
>> -- 
>> 2.15.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled.

[Ville Syrjälä]

On Mon, Nov 20, 2017 at 11:48:22AM +0100, Maarten Lankhorst wrote:
> Op 17-11-17 om 16:52 schreef Ville Syrjälä:
> > On Fri, Nov 17, 2017 at 04:37:54PM +0100, Maarten Lankhorst wrote:
> >> ips_enabled was used as a variable of whether IPS can be enabled or not,
> >> but should be used to test whether IPS is actually enabled.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c  | 75 ++++++++++++++++-------------------
> >>  drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
> >>  2 files changed, 35 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 3b3dec1e6640..8283e80597bd 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
> >>  	struct drm_device *dev = crtc->base.dev;
> >>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >>  
> >> -	if (!crtc->config->ips_enabled)
> >> +	if (!crtc_state->ips_enabled)
> >>  		return;
> >>  
> >>  	/*
> >> @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
> >>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>  	int pipe = intel_crtc->pipe;
> >>  
> >> -	/*
> >> -	 * FIXME IPS should be fine as long as one plane is
> >> -	 * enabled, but in practice it seems to have problems
> >> -	 * when going from primary only to sprite only and vice
> >> -	 * versa.
> >> -	 */
> >> -	hsw_enable_ips(new_crtc_state);
> >> -
> >>  	/*
> >>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
> >>  	 * So don't enable underrun reporting before at least some planes
> >> @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
> >>  	intel_check_pch_fifo_underruns(dev_priv);
> >>  }
> >>  
> >> -/* FIXME move all this to pre_plane_update() with proper state tracking */
> >> +/* FIXME get rid of this and use pre_plane_update */
> >>  static void
> >> -intel_pre_disable_primary(struct drm_crtc *crtc,
> >> -			  const struct intel_crtc_state *old_crtc_state)
> >> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> >>  {
> >>  	struct drm_device *dev = crtc->dev;
> >>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >> @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
> >>  
> >>  	/*
> >>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
> >> -	 * So diasble underrun reporting before all the planes get disabled.
> >> -	 * FIXME: Need to fix the logic to work when we turn off all planes
> >> -	 * but leave the pipe running.
> >> +	 * So disable underrun reporting before all the planes get disabled.
> >>  	 */
> >>  	if (IS_GEN2(dev_priv))
> >>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> >>  
> >> -	/*
> >> -	 * FIXME IPS should be fine as long as one plane is
> >> -	 * enabled, but in practice it seems to have problems
> >> -	 * when going from primary only to sprite only and vice
> >> -	 * versa.
> >> -	 */
> >> -	hsw_disable_ips(old_crtc_state);
> >> -}
> >> -
> >> -/* FIXME get rid of this and use pre_plane_update */
> >> -static void
> >> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> >> -{
> >> -	struct drm_device *dev = crtc->dev;
> >> -	struct drm_i915_private *dev_priv = to_i915(dev);
> >> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> -	int pipe = intel_crtc->pipe;
> >> -
> >> -	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
> >> +	hsw_disable_ips(to_intel_crtc_state(crtc->state));
> > BTW thinking about the lack of readout on BDW. In case the BIOS ever
> > enables IPS, I think we should sanitize ips_enabled to true on BDW.
> > Or maybe just have the redaout code always set ips_enabled=true on BDW?
> > That way the first modeset will try to turn it off regardless of whether it
> > was enabled or not.
> Yes, good idea. :)
> >>  
> >>  	/*
> >>  	 * Vblank time updates from the shadow to live plane control register
> >> @@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> >>  	if (pipe_config->update_wm_post && pipe_config->base.active)
> >>  		intel_update_watermarks(crtc);
> >>  
> >> +	if (!old_crtc_state->ips_enabled ||
> >> +	    needs_modeset(&old_crtc_state->base) ||
> > Shoudldn't that be needs_modeset(new_crtc_state)?
> Yes, oops..
> >> +	    pipe_config->update_pipe)
> > Why do we have to check that?
> In case we take over the hw state from bios..
> 
> If we default ips_enabled to true on BDW and read out on HSW again, then this
> check will make IPS work, regardless of bios setting..

Why aren't we just checking something like
if ((!old_state->ips_enabled || needs_modeset()) && new_state->ips_enabled) ?

IPS state may have to change whenever plane visibility changes, so I don't
see any reson to involve ->update_pipe in these decision.


> 
> >> +		hsw_enable_ips(pipe_config);
> >> +
> >>  	if (old_pri_state) {
> >>  		struct intel_plane_state *primary_state =
> >>  			intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
> >> @@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> >>  	struct intel_atomic_state *old_intel_state =
> >>  		to_intel_atomic_state(old_state);
> >>  
> >> +	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
> >> +		hsw_disable_ips(old_crtc_state);
> >> +
> >>  	if (old_pri_state) {
> >>  		struct intel_plane_state *primary_state =
> >>  			intel_atomic_get_new_plane_state(old_intel_state,
> >> @@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> >>  			to_intel_plane_state(old_pri_state);
> >>  
> >>  		intel_fbc_pre_update(crtc, pipe_config, primary_state);
> >> -
> >> -		if (old_primary_state->base.visible &&
> >> +		/*
> >> +		 * Gen2 reports pipe underruns whenever all planes are disabled.
> >> +		 * So disable underrun reporting before all the planes get disabled.
> >> +		 */
> >> +		if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
> >>  		    (modeset || !primary_state->base.visible))
> >> -			intel_pre_disable_primary(&crtc->base, old_crtc_state);
> >> +			intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
> >>  	}
> >>  
> >>  	/*
> >> @@ -6231,12 +6213,25 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> >>  static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> >>  				     struct intel_crtc_state *pipe_config)
> >>  {
> >> +	u8 visible_planes;
> >> +
> >>  	if (pipe_config->ips_force_disable)
> >>  		return false;
> >>  
> >>  	if (pipe_config->pipe_bpp > 24)
> >>  		return false;
> >>  
> >> +	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
> >> +
> >> +	/*
> >> +	 * FIXME IPS should be fine as long as one plane is
> >> +	 * enabled, but in practice it seems to have problems
> >> +	 * when going from primary only to sprite only and vice
> >> +	 * versa.
> >> +	 */
> >> +	if (visible_planes != BIT(PLANE_PRIMARY))
> >> +		return false;
> > I think that should be '(active_planes & PRIMARY) == 0'. As in we don't
> > care what the other planes are doing, IPS just tracks the primary state
> > (for the time being).
> Oops, faultily assumed from the comment that IPS only worked with the primary plane
> or sprite plane, not both. I'll correct it.
> > What's missing from this patch is changing the cdclk vs. ips workaround
> > to not assume that cdclk can be increased to accomodate ips.
> Hm true, I missed that..
> 
> Will send a new version in a bit.
> >> +
> >>  	/* HSW can handle pixel rate up to cdclk? */
> >>  	if (IS_HASWELL(dev_priv))
> >>  		return true;
> >> @@ -6378,9 +6373,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
> >>  
> >>  	intel_crtc_compute_pixel_rate(pipe_config);
> >>  
> >> -	if (HAS_IPS(dev_priv))
> >> -		hsw_compute_ips_config(crtc, pipe_config);
> >> -
> >>  	if (pipe_config->has_pch_encoder)
> >>  		return ironlake_fdi_compute_config(crtc, pipe_config);
> >>  
> >> @@ -10488,6 +10480,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >>  							 pipe_config);
> >>  	}
> >>  
> >> +	if (HAS_IPS(dev_priv))
> >> +		hsw_compute_ips_config(intel_crtc, pipe_config);
> >> +
> >>  	return ret;
> >>  }
> >>  
> >> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> >> index 61641d479b93..1f5cd572a7ff 100644
> >> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> >> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> >> @@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> >>  		 * completely disable it.
> >>  		 */
> >>  		pipe_config->ips_force_disable = enable;
> >> -		if (pipe_config->ips_enabled == enable)
> >> -			pipe_config->base.connectors_changed = true;
> >>  	}
> >>  
> >>  	if (IS_HASWELL(dev_priv)) {
> >> -- 
> >> 2.15.0
> >>
> >> _______________________________________________
> >> 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

Re: [PATCH 2/3] drm/i915: Enable IPS for sprite plane

[Maarten Lankhorst]

Op 17-11-17 om 16:55 schreef Ville Syrjälä:
> On Fri, Nov 17, 2017 at 04:37:55PM +0100, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8283e80597bd..38a1cdb3dbb2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>>  		intel_fbc_post_update(crtc);
>>  
>>  		if (primary_state->base.visible &&
>> -		    (needs_modeset(&pipe_config->base) ||
>> +		    (pipe_config->disable_cxsr ||
>>  		     !old_primary_state->base.visible))
>>  			intel_post_enable_primary(&crtc->base, pipe_config);
>>  	}
>> @@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>>  	struct intel_atomic_state *old_intel_state =
>>  		to_intel_atomic_state(old_state);
>>  
>> -	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
>> +	if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)
> What does IPS have to do with cxsr?
Nothing, laziness. disable cxsr is set when planes get disabled.
>>  		hsw_disable_ips(old_crtc_state);
>>  
>>  	if (old_pri_state) {
>> @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
>>  	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
>>  
>>  	/*
>> -	 * FIXME IPS should be fine as long as one plane is
>> -	 * enabled, but in practice it seems to have problems
>> -	 * when going from primary only to sprite only and vice
>> -	 * versa.
>> +	 * IPS should be fine as long as one plane is enabled, but
>> +	 * temporarily disable it when when going from primary only
>> +	 * to sprite only and vice versa.
>>  	 */
>> -	if (visible_planes != BIT(PLANE_PRIMARY))
>> +	if (hweight32(visible_planes) != 1)
>>  		return false;
> That should just be
> if (active_planes == 0)
> 	return false;
>
> assuming we have no problems with the toggling between
> primary only and sprite only.
>
> I can't recall how the cursor affecrs IPS. But I think IPS should 
> work as long as any plane (including the cursor) is enabled.
But cursor visibility can change without the full atomic commit, so it's too unreliable to take into
account with the calculations.

What happens when it doesn't work well? Would it be caught by any tests?

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Enable IPS for sprite plane

[Ville Syrjälä]

On Mon, Nov 20, 2017 at 12:12:40PM +0100, Maarten Lankhorst wrote:
> Op 17-11-17 om 16:55 schreef Ville Syrjälä:
> > On Fri, Nov 17, 2017 at 04:37:55PM +0100, Maarten Lankhorst wrote:
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 13 ++++++-------
> >>  1 file changed, 6 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 8283e80597bd..38a1cdb3dbb2 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> >>  		intel_fbc_post_update(crtc);
> >>  
> >>  		if (primary_state->base.visible &&
> >> -		    (needs_modeset(&pipe_config->base) ||
> >> +		    (pipe_config->disable_cxsr ||
> >>  		     !old_primary_state->base.visible))
> >>  			intel_post_enable_primary(&crtc->base, pipe_config);
> >>  	}
> >> @@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> >>  	struct intel_atomic_state *old_intel_state =
> >>  		to_intel_atomic_state(old_state);
> >>  
> >> -	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
> >> +	if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)
> > What does IPS have to do with cxsr?
> Nothing, laziness. disable cxsr is set when planes get disabled.
> >>  		hsw_disable_ips(old_crtc_state);
> >>  
> >>  	if (old_pri_state) {
> >> @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> >>  	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
> >>  
> >>  	/*
> >> -	 * FIXME IPS should be fine as long as one plane is
> >> -	 * enabled, but in practice it seems to have problems
> >> -	 * when going from primary only to sprite only and vice
> >> -	 * versa.
> >> +	 * IPS should be fine as long as one plane is enabled, but
> >> +	 * temporarily disable it when when going from primary only
> >> +	 * to sprite only and vice versa.
> >>  	 */
> >> -	if (visible_planes != BIT(PLANE_PRIMARY))
> >> +	if (hweight32(visible_planes) != 1)
> >>  		return false;
> > That should just be
> > if (active_planes == 0)
> > 	return false;
> >
> > assuming we have no problems with the toggling between
> > primary only and sprite only.
> >
> > I can't recall how the cursor affecrs IPS. But I think IPS should 
> > work as long as any plane (including the cursor) is enabled.
> But cursor visibility can change without the full atomic commit, so it's too unreliable to take into
> account with the calculations.

Hmm. I guess we can ignore the cursor then. It's generally pretty
small, so probably no huge benefit from using IPS with cursor only
anyway.

> 
> What happens when it doesn't work well? Would it be caught by any tests?

I think it should be caught by CRCs, if we have any that capture
CRCs across plane enable/disable.

Except, there is a problem on ilk-ivb at least, maybe it went all
the way to bdw. IIRC when I last tried a test like that the CRC for
a black primary plane didn't match the CRC for no planes. And I think I
tried to eliminate gamma/csc as the source of the discrepancy without
success. Also IIRC there was always a single frame with an even stranger
CRC just after turning off the last plane.

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

Re: [PATCH 1/3] drm/i915: Make ips_enabled a property depending on whether IPS is enabled.

[Maarten Lankhorst]

Op 20-11-17 om 11:58 schreef Ville Syrjälä:
> On Mon, Nov 20, 2017 at 11:48:22AM +0100, Maarten Lankhorst wrote:
>> Op 17-11-17 om 16:52 schreef Ville Syrjälä:
>>> On Fri, Nov 17, 2017 at 04:37:54PM +0100, Maarten Lankhorst wrote:
>>>> ips_enabled was used as a variable of whether IPS can be enabled or not,
>>>> but should be used to test whether IPS is actually enabled.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c  | 75 ++++++++++++++++-------------------
>>>>  drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
>>>>  2 files changed, 35 insertions(+), 42 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 3b3dec1e6640..8283e80597bd 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
>>>>  	struct drm_device *dev = crtc->base.dev;
>>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>>  
>>>> -	if (!crtc->config->ips_enabled)
>>>> +	if (!crtc_state->ips_enabled)
>>>>  		return;
>>>>  
>>>>  	/*
>>>> @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
>>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>>  	int pipe = intel_crtc->pipe;
>>>>  
>>>> -	/*
>>>> -	 * FIXME IPS should be fine as long as one plane is
>>>> -	 * enabled, but in practice it seems to have problems
>>>> -	 * when going from primary only to sprite only and vice
>>>> -	 * versa.
>>>> -	 */
>>>> -	hsw_enable_ips(new_crtc_state);
>>>> -
>>>>  	/*
>>>>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
>>>>  	 * So don't enable underrun reporting before at least some planes
>>>> @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
>>>>  	intel_check_pch_fifo_underruns(dev_priv);
>>>>  }
>>>>  
>>>> -/* FIXME move all this to pre_plane_update() with proper state tracking */
>>>> +/* FIXME get rid of this and use pre_plane_update */
>>>>  static void
>>>> -intel_pre_disable_primary(struct drm_crtc *crtc,
>>>> -			  const struct intel_crtc_state *old_crtc_state)
>>>> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>>>>  {
>>>>  	struct drm_device *dev = crtc->dev;
>>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>> @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
>>>>  
>>>>  	/*
>>>>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
>>>> -	 * So diasble underrun reporting before all the planes get disabled.
>>>> -	 * FIXME: Need to fix the logic to work when we turn off all planes
>>>> -	 * but leave the pipe running.
>>>> +	 * So disable underrun reporting before all the planes get disabled.
>>>>  	 */
>>>>  	if (IS_GEN2(dev_priv))
>>>>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>>>>  
>>>> -	/*
>>>> -	 * FIXME IPS should be fine as long as one plane is
>>>> -	 * enabled, but in practice it seems to have problems
>>>> -	 * when going from primary only to sprite only and vice
>>>> -	 * versa.
>>>> -	 */
>>>> -	hsw_disable_ips(old_crtc_state);
>>>> -}
>>>> -
>>>> -/* FIXME get rid of this and use pre_plane_update */
>>>> -static void
>>>> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>>>> -{
>>>> -	struct drm_device *dev = crtc->dev;
>>>> -	struct drm_i915_private *dev_priv = to_i915(dev);
>>>> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>> -	int pipe = intel_crtc->pipe;
>>>> -
>>>> -	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
>>>> +	hsw_disable_ips(to_intel_crtc_state(crtc->state));
>>> BTW thinking about the lack of readout on BDW. In case the BIOS ever
>>> enables IPS, I think we should sanitize ips_enabled to true on BDW.
>>> Or maybe just have the redaout code always set ips_enabled=true on BDW?
>>> That way the first modeset will try to turn it off regardless of whether it
>>> was enabled or not.
>> Yes, good idea. :)
>>>>  
>>>>  	/*
>>>>  	 * Vblank time updates from the shadow to live plane control register
>>>> @@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>>>>  	if (pipe_config->update_wm_post && pipe_config->base.active)
>>>>  		intel_update_watermarks(crtc);
>>>>  
>>>> +	if (!old_crtc_state->ips_enabled ||
>>>> +	    needs_modeset(&old_crtc_state->base) ||
>>> Shoudldn't that be needs_modeset(new_crtc_state)?
>> Yes, oops..
>>>> +	    pipe_config->update_pipe)
>>> Why do we have to check that?
>> In case we take over the hw state from bios..
>>
>> If we default ips_enabled to true on BDW and read out on HSW again, then this
>> check will make IPS work, regardless of bios setting..
> Why aren't we just checking something like
> if ((!old_state->ips_enabled || needs_modeset()) && new_state->ips_enabled) ?
>
> IPS state may have to change whenever plane visibility changes, so I don't
> see any reson to involve ->update_pipe in these decision.
This is what we do, except the new_state->ips_enabled check is in hsw_enable_ips. We also allow update_pipe for broadwell, in which case we don't know the hw state.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx