Re: [PATCH v4 5/9] drm/i915/crc: Make IPS workaround generic

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Mar 01, 2019 at 05:34:52PM -0800, José Roberto de Souza wrote:
> Other features like PSR2 also needs to be disabled while getting CRC
> so lets rename ips_force_disable to crc_enabled, drop all this checks
> for pipe A and HSW and BDW and make it generic and
> hsw_compute_ips_config() will take care of all the checks removed
> from here.
> 
> v2: Renaming and parameter changes to the functions that prepares the
> commit (Ville)
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

nice clean-up.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>



> ---
>  drivers/gpu/drm/i915/intel_display.c  | 10 ++++--
>  drivers/gpu/drm/i915/intel_drv.h      |  3 +-
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 47 +++++++++++----------------
>  3 files changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 816e8f124b3b..328967c642b3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *crtc_state)
>  	if (!hsw_crtc_state_ips_capable(crtc_state))
>  		return false;
>  
> -	if (crtc_state->ips_force_disable)
> +	/*
> +	 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> +	 * enabled and disabled dynamically based on package C states,
> +	 * user space can't make reliable use of the CRCs, so let's just
> +	 * completely disable it.
> +	 */
> +	if (crtc_state->crc_enabled)
>  		return false;
>  
>  	/* IPS should be fine as long as at least one plane is enabled. */
> @@ -11684,7 +11690,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	saved_state->shared_dpll = crtc_state->shared_dpll;
>  	saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
>  	saved_state->pch_pfit.force_thru = crtc_state->pch_pfit.force_thru;
> -	saved_state->ips_force_disable = crtc_state->ips_force_disable;
> +	saved_state->crc_enabled = crtc_state->crc_enabled;
>  	if (IS_G4X(dev_priv) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		saved_state->wm = crtc_state->wm;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5412373e2f98..2be64529e4a2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -999,7 +999,8 @@ struct intel_crtc_state {
>  	struct intel_link_m_n fdi_m_n;
>  
>  	bool ips_enabled;
> -	bool ips_force_disable;
> +
> +	bool crc_enabled;
>  
>  	bool enable_fbc;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 53d4ec68d3c4..af64597c5c6e 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -280,15 +280,15 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
>  	return 0;
>  }
>  
> -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> -			      bool enable)
> +static void
> +intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool enable)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct drm_device *dev = &dev_priv->drm;
> -	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
>  	struct intel_crtc_state *pipe_config;
>  	struct drm_atomic_state *state;
>  	struct drm_modeset_acquire_ctx ctx;
> -	int ret = 0;
> +	int ret;
>  
>  	drm_modeset_acquire_init(&ctx, 0);
>  
> @@ -307,17 +307,9 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>  		goto put_state;
>  	}
>  
> -	if (HAS_IPS(dev_priv)) {
> -		/*
> -		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> -		 * enabled and disabled dynamically based on package C states,
> -		 * user space can't make reliable use of the CRCs, so let's just
> -		 * completely disable it.
> -		 */
> -		pipe_config->ips_force_disable = enable;
> -	}
> +	pipe_config->crc_enabled = enable;
>  
> -	if (IS_HASWELL(dev_priv)) {
> +	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
>  		pipe_config->pch_pfit.force_thru = enable;
>  		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
>  		    pipe_config->pch_pfit.enabled != enable)
> @@ -343,8 +335,7 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  				enum pipe pipe,
>  				enum intel_pipe_crc_source *source,
> -				u32 *val,
> -				bool set_wa)
> +				u32 *val)
>  {
>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
>  		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
> @@ -357,10 +348,6 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> -		if (set_wa && (IS_HASWELL(dev_priv) ||
> -		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> -			hsw_pipe_A_crc_wa(dev_priv, true);
> -
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_NONE:
> @@ -418,8 +405,7 @@ static int skl_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  
>  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  			       enum pipe pipe,
> -			       enum intel_pipe_crc_source *source, u32 *val,
> -			       bool set_wa)
> +			       enum intel_pipe_crc_source *source, u32 *val)
>  {
>  	if (IS_GEN(dev_priv, 2))
>  		return i8xx_pipe_crc_ctl_reg(source, val);
> @@ -430,7 +416,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  	else if (IS_GEN_RANGE(dev_priv, 5, 6))
>  		return ilk_pipe_crc_ctl_reg(source, val);
>  	else if (INTEL_GEN(dev_priv) < 9)
> -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
> +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
>  	else
>  		return skl_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
>  }
> @@ -605,6 +591,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
>  	intel_wakeref_t wakeref;
>  	u32 val = 0; /* shut up gcc */
>  	int ret = 0;
> +	bool enable;
>  
>  	if (display_crc_ctl_parse_source(source_name, &source) < 0) {
>  		DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
> @@ -618,7 +605,11 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
>  		return -EIO;
>  	}
>  
> -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
> +	enable = source != INTEL_PIPE_CRC_SOURCE_NONE;
> +	if (enable)
> +		intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), enable);
> +
> +	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
>  	if (ret != 0)
>  		goto out;
>  
> @@ -629,14 +620,14 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
>  	if (!source) {
>  		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  			vlv_undo_pipe_scramble_reset(dev_priv, crtc->index);
> -		else if ((IS_HASWELL(dev_priv) ||
> -			  IS_BROADWELL(dev_priv)) && crtc->index == PIPE_A)
> -			hsw_pipe_A_crc_wa(dev_priv, false);
>  	}
>  
>  	pipe_crc->skipped = 0;
>  
>  out:
> +	if (!enable)
> +		intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), enable);
> +
>  	intel_display_power_put(dev_priv, power_domain, wakeref);
>  
>  	return ret;
> @@ -652,7 +643,7 @@ void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
>  	if (!crtc->crc.opened)
>  		return;
>  
> -	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0)
> +	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val) < 0)
>  		return;
>  
>  	/* Don't need pipe_crc->lock here, IRQs are not generated. */
> -- 
> 2.21.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