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

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

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