Em Qua, 2016-12-21 às 11:31 +0200, ville.syrjala@xxxxxxxxxxxxxxx
escreveu:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Oneshot disabling of IPS when CRC capturing is started is
> insufficient.
> IPS may get re-enabled by any plane update, and hence tests that keep
> CRC capturing on across plane updates will start to see inconsistent
> results as soon as IPS kicks back in. Add a new knob into the crtc
> state
> to make sure IPS stays disabled as long as CRC capturing is enabled.
>
> Forcing a modeset is the easiest way to handle this since that's
> already
> how we do the panel fitter workaround. It's a little heavy handed
> just
> for IPS, but seeing as we might already do the panel fitter
> workaround
> I think it's better to follow that. We migth want to optimize both
> cases
> later if someone gets too upset by the extra delay from the modeset.
>
Please add a "Testcase:" tag listing the IGTs this patch unbreaks.
Maybe also "Bugzilla:" if there's any.
> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 5 +++-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pipe_crc.c | 43 +++++++++++++++++++----
> ------------
> 3 files changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index ef5dde5ab1cf..1ce479614f52 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7189,6 +7189,7 @@ static void hsw_compute_ips_config(struct
> intel_crtc *crtc,
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> pipe_config->ips_enabled = i915.enable_ips &&
> + !pipe_config->ips_force_disable &&
> hsw_crtc_supports_ips(crtc) &&
> pipe_config_supports_ips(dev_priv, pipe_config);
> }
> @@ -12958,7 +12959,7 @@ clear_intel_crtc_state(struct
> intel_crtc_state *crtc_state)
> struct intel_crtc_scaler_state scaler_state;
> struct intel_dpll_hw_state dpll_hw_state;
> struct intel_shared_dpll *shared_dpll;
> - bool force_thru;
> + bool force_thru, ips_force_disable;
>
> /* FIXME: before the switch to atomic started, a new
> pipe_config was
> * kzalloc'd. Code that depends on any field being zero
> should be
> @@ -12970,6 +12971,7 @@ clear_intel_crtc_state(struct
> intel_crtc_state *crtc_state)
> shared_dpll = crtc_state->shared_dpll;
> dpll_hw_state = crtc_state->dpll_hw_state;
> force_thru = crtc_state->pch_pfit.force_thru;
> + ips_force_disable = crtc_state->ips_force_disable;
>
> memset(crtc_state, 0, sizeof *crtc_state);
>
> @@ -12978,6 +12980,7 @@ clear_intel_crtc_state(struct
> intel_crtc_state *crtc_state)
> crtc_state->shared_dpll = shared_dpll;
> crtc_state->dpll_hw_state = dpll_hw_state;
> crtc_state->pch_pfit.force_thru = force_thru;
> + crtc_state->ips_force_disable = ips_force_disable;
> }
>
> static int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 025e4c8b3e63..cadba9b92cc9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -651,6 +651,7 @@ struct intel_crtc_state {
> struct intel_link_m_n fdi_m_n;
>
> bool ips_enabled;
> + bool ips_force_disable;
>
> bool enable_fbc;
>
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index ef0c0e195164..708cf1d419d4 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -547,8 +547,8 @@ static int ilk_pipe_crc_ctl_reg(enum
> intel_pipe_crc_source *source,
> return 0;
> }
>
> -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private
> *dev_priv,
> - bool enable)
> +static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> + bool enable)
Now we apply more than one WA. So maybe: hsw_pipe_a_crc_workarounds()
or just hsw_pipe_crc_workarounds() (or apply_workarounds). Just "was"
is confusing since it looks like a verb is missing. Also, that capital
A is not exactly following our coding style.
> {
> struct drm_device *dev = &dev_priv->drm;
> struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> PIPE_A);
> @@ -570,11 +570,23 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct
> drm_i915_private *dev_priv,
> goto out;
> }
>
> - pipe_config->pch_pfit.force_thru = enable;
> - if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> - pipe_config->pch_pfit.enabled != enable)
> + /*
> + * 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;
> + if (pipe_config->ips_force_disable != enable)
The fix mentioned in your other email makes sense, but I find it easier
to read "if (pipe_config->ips_enable == pipe_config-
>ips_force_disable)".
> pipe_config->base.connectors_changed = true;
>
> + if (IS_HASWELL(dev_priv)) {
> + pipe_config->pch_pfit.force_thru = enable;
> + if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> + pipe_config->pch_pfit.enabled != enable)
> + pipe_config->base.connectors_changed = true;
> + }
> +
> ret = drm_atomic_commit(state);
> out:
> WARN(ret, "Toggling workaround to %i returns %i\n", enable,
> ret);
> @@ -598,8 +610,9 @@ 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_PF:
> - if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> - hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
> + if ((IS_HASWELL(dev_priv) ||
> + IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> + hsw_pipe_A_crc_wa(dev_priv, true);
I know this problem is not caused by your patch, but in case you want:
I wonder if there's a better place to call this. Maybe outside this
function. Calling it here sounds like an unrelated side-effect of a
function that's supposed to just return a value for a register.
>
> *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
> break;
> @@ -618,7 +631,6 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
> enum intel_pipe_crc_source source)
> {
> struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> pipe);
> enum intel_display_power_domain power_domain;
> u32 val = 0; /* shut up gcc */
> int ret;
> @@ -665,14 +677,6 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
> goto out;
> }
>
> - /*
> - * 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.
> - */
> - hsw_disable_ips(crtc);
> -
> spin_lock_irq(&pipe_crc->lock);
> kfree(pipe_crc->entries);
> pipe_crc->entries = entries;
> @@ -713,10 +717,9 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
> g4x_undo_pipe_scramble_reset(dev_priv,
> pipe);
> else if (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv))
> vlv_undo_pipe_scramble_reset(dev_priv,
> pipe);
> - else if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> - hsw_trans_edp_pipe_A_crc_wa(dev_priv,
> false);
> -
> - hsw_enable_ips(crtc);
> + else if ((IS_HASWELL(dev_priv) ||
> + IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
Moving the gen+pipe checks to inside the function would deduplicate
them and, IMHO, make the code slightly easier to read.
Now here's a question that maybe I never asked myself before: if we
have two pipes enabled (A and B), and our system is properly configured
for power management so we're reaching the deep PC states (depending on
your machine, all you need to do is to run powertop --auto-tune), does
pipe B also change its CRCs when IPS is enabled on pipe A? The reason I
ask this is because IPS enables deeper PC states, but maybe it's the
deeper PC states that causes different CRC calculation, not IPS, and
then this would also affect the CRCs for non-IPS pipes (AKA pipe B). It
would be great if you could check this, because I don't really remember
if I checked for this when I wrote this code. Of course, the fix for
this would be a separate patch since it's not the problem you're
touching here. But then, maybe we could do something else to prevent
deep PC states instead of disabling IPS.
Most of (all?) my suggestions above are bikesheds, so with or without
them:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> + hsw_pipe_A_crc_wa(dev_priv, false);
> }
>
> ret = 0;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
