[Intel-gfx] [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Feb 28 16:56:48 UTC 2019
On Wed, Feb 27, 2019 at 05:32:57PM -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.
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at 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 | 42 +++++++++------------------
> 3 files changed, 24 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;
Hmm. I was wondering how we even manage to pass the state checker with
the current code. But apparently we don't have state checking for IPS.
I would suggest moving this into hsw_compute_ips_config() and then
adding the state checker (for HSW only though since BDW can't do the
readout).
>
> /* 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..f6d0b2aaffe2 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -280,11 +280,12 @@ 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_prepare(struct drm_i915_private *dev_priv, struct drm_crtc *crtc,
Just pass in the intel_crtc
> + bool enable)
> {
> struct drm_device *dev = &dev_priv->drm;
> - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
and then we don't have to have an ugly name for this.
Also pasing in dev_priv is redundant when you're already passing in the
crtc.
The function name isn't super descriptive. It makes me think we're
preparing for CRC capture, when in fact it just adds/removes the w/as.
> struct intel_crtc_state *pipe_config;
> struct drm_atomic_state *state;
> struct drm_modeset_acquire_ctx ctx;
> @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> state->acquire_ctx = &ctx;
>
> retry:
> - pipe_config = intel_atomic_get_crtc_state(state, crtc);
> + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> if (IS_ERR(pipe_config)) {
> ret = PTR_ERR(pipe_config);
> 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) && intel_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 +336,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 +349,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 +406,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 +417,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);
> }
> @@ -618,7 +605,9 @@ 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);
> + intel_crtc_crc_prepare(dev_priv, crtc, source != INTEL_PIPE_CRC_SOURCE_NONE);
> +
Aren't we potentially corrupting the last CRC(s) by turning off the w/as
before disbling CRC capture?
> + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
> if (ret != 0)
> goto out;
>
> @@ -629,9 +618,6 @@ 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;
> @@ -652,7 +638,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
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list