[Intel-gfx] [PATCH] drm/i915: Keep IPS disabled during CRC collection
Lofstedt, Marta
marta.lofstedt at intel.com
Thu Aug 10 12:01:45 UTC 2017
Tested-by: Marta Lofstedt <marta.lofstedt at intel.com>
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> Sent: Thursday, August 10, 2017 12:55 PM
> To: intel-gfx at lists.freedesktop.org
> Cc: Lofstedt, Marta <marta.lofstedt at intel.com>; Maarten Lankhorst
> <maarten.lankhorst at linux.intel.com>
> Subject: [PATCH] drm/i915: Keep IPS disabled during CRC collection
>
> We need to look at crtc->state->active for the legacy crc to match the drm crc
> api. Even though the drm api explictily checks it, there's no harm in doing the
> same there so keep it for both.
>
> Introduce a intel_crtc->ips_disabled flag for CRC, which is protected by crtc-
> >mutex and set it to true while collecting. This way CRC will not interfere with
> ips, regardless of visibility.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101664
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++--------------
> drivers/gpu/drm/i915/intel_drv.h | 3 ++
> drivers/gpu/drm/i915/intel_pipe_crc.c | 58
> +++++++++++++++++++++++++++--------
> 3 files changed, 71 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a8775ed817d1..88dfb4bd8db0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -117,7 +117,8 @@ static void ironlake_pfit_disable(struct intel_crtc
> *crtc, bool force); static void ironlake_pfit_enable(struct intel_crtc *crtc);
> static void intel_modeset_setup_hw_state(struct drm_device *dev,
>
> struct drm_modeset_acquire_ctx *ctx); -static void
> intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
> +static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc,
> +
> struct intel_crtc_state *pipe_config);
>
> struct intel_limit {
> struct {
> @@ -2723,7 +2724,8 @@ intel_find_initial_plane_obj(struct intel_crtc
> *intel_crtc,
> intel_set_plane_visible(to_intel_crtc_state(crtc_state),
>
> to_intel_plane_state(plane_state),
> false);
> - intel_pre_disable_primary_noatomic(&intel_crtc->base);
> + intel_pre_disable_primary_noatomic(&intel_crtc->base,
> +
> to_intel_crtc_state(crtc_state));
> trace_intel_disable_plane(primary, intel_crtc);
> intel_plane->disable_plane(intel_plane, intel_crtc);
>
> @@ -4668,9 +4670,6 @@ void hsw_enable_ips(struct intel_crtc *crtc)
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> - if (!crtc->config->ips_enabled)
> - return;
> -
> /*
> * We can only enable IPS after we enable a plane and wait for
> a vblank
> * This function is called from post_plane_update, which is run
> after @@ -4706,9 +4705,6 @@ void hsw_disable_ips(struct intel_crtc *crtc)
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> - if (!crtc->config->ips_enabled)
> - return;
> -
> assert_plane_enabled(dev_priv, crtc->plane);
> if (IS_BROADWELL(dev_priv)) {
> mutex_lock(&dev_priv->rps.hw_lock);
> @@ -4754,7 +4750,7 @@ static void intel_crtc_dpms_overlay_disable(struct
> intel_crtc *intel_crtc)
> * completely hide the primary plane.
> */
> static void
> -intel_post_enable_primary(struct drm_crtc *crtc)
> +intel_post_enable_primary(struct drm_crtc *crtc, struct
> +intel_crtc_state *pipe_config)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev); @@ -4767,7
> +4763,8 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> * when going from primary only to sprite only and vice
> * versa.
> */
> - hsw_enable_ips(intel_crtc);
> + if (pipe_config->ips_enabled && !intel_crtc->ips_disabled)
> + hsw_enable_ips(intel_crtc);
>
> /*
> * Gen2 reports pipe underruns whenever all planes are
> disabled.
> @@ -4786,7 +4783,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>
> /* FIXME move all this to pre_plane_update() with proper state tracking */
> static void -intel_pre_disable_primary(struct drm_crtc *crtc)
> +intel_pre_disable_primary(struct drm_crtc *crtc, struct
> +intel_crtc_state *pipe_config)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev); @@ -
> 4808,19 +4805,20 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
> * when going from primary only to sprite only and vice
> * versa.
> */
> - hsw_disable_ips(intel_crtc);
> + if (pipe_config->ips_enabled)
> + hsw_disable_ips(intel_crtc);
> }
>
> /* FIXME get rid of this and use pre_plane_update */ static void -
> intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc, struct
> +intel_crtc_state *pipe_config)
> {
> 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);
> + intel_pre_disable_primary(crtc, pipe_config);
>
> /*
> * Vblank time updates from the shadow to live plane control
> register @@ -4858,7 +4856,7 @@ static void intel_post_plane_update(struct
> intel_crtc_state *old_crtc_state)
> if (new_primary_state->visible &&
> (needs_modeset(&pipe_config->base) ||
> !old_primary_state->visible))
> - intel_post_enable_primary(&crtc-
> >base);
> + intel_post_enable_primary(&crtc-
> >base, pipe_config);
> }
> }
>
> @@ -4885,7 +4883,7 @@ static void intel_pre_plane_update(struct
> intel_crtc_state *old_crtc_state,
>
> if (old_primary_state->visible &&
> (modeset || !new_primary_state->visible))
> - intel_pre_disable_primary(&crtc-
> >base);
> + intel_pre_disable_primary(&crtc-
> >base, pipe_config);
> }
>
> /*
> @@ -5700,13 +5698,6 @@ static void intel_crtc_disable_noatomic(struct
> drm_crtc *crtc,
> if (!intel_crtc->active)
> return;
>
> - if (crtc->primary->state->visible) {
> - intel_pre_disable_primary_noatomic(crtc);
> -
> - intel_crtc_disable_planes(crtc, 1 <<
> drm_plane_index(crtc->primary));
> - crtc->primary->state->visible = false;
> - }
> -
> state = drm_atomic_state_alloc(crtc->dev);
> if (!state) {
> DRM_DEBUG_KMS("failed to disable
> [CRTC:%d:%s], out of memory", @@ -5722,6 +5713,14 @@ static void
> intel_crtc_disable_noatomic(struct drm_crtc *crtc,
>
> WARN_ON(IS_ERR(crtc_state) || ret);
>
> + if (crtc->primary->state->visible) {
> + intel_pre_disable_primary_noatomic(crtc,
> crtc_state);
> +
> + intel_crtc_disable_planes(crtc, 1 <<
> drm_plane_index(crtc->primary));
> + crtc->primary->state->visible = false;
> + }
> +
> +
> dev_priv->display.crtc_disable(crtc_state, state);
>
> drm_atomic_state_put(state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index a07d06fbc4b4..872b1fce1d0e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -828,6 +828,9 @@ struct intel_crtc {
> bool cpu_fifo_underrun_disabled;
> bool pch_fifo_underrun_disabled;
>
> + /* Whether ips is force-disabled for collecting CRC */
> + bool ips_disabled;
> +
> /* per-pipe watermark state */
> struct {
> /* watermarks currently being used */ diff --git
> a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 8fbd2bd0877f..fc3682c4c055 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -601,6 +601,33 @@ static int get_new_crc_ctl_reg(struct
> drm_i915_private *dev_priv,
> return ivb_pipe_crc_ctl_reg(dev_priv, pipe,
> source, val); }
>
> +static int toggle_ips_and_test_active(struct intel_crtc *crtc,
> +
> bool has_source)
> +{
> + bool has_ips = false;
> + int ret;
> +
> +
> + ret = drm_modeset_lock_interruptible(&crtc->base.mutex,
> NULL);
> + if (ret)
> + return ret;
> +
> + if (to_intel_crtc_state(crtc->base.state)->ips_enabled &&
> + crtc->base.state->plane_mask & BIT(drm_plane_index(crtc-
> >base.primary)))
> + has_ips = crtc->base.primary->state->visible;
> +
> + if (!crtc->base.state->active && has_source) {
> + DRM_DEBUG_KMS("Trying to capture CRC while
> pipe is off\n");
> + ret = -EIO;
> + }
> +
> + crtc->ips_disabled = has_source;
> +
> + drm_modeset_unlock(&crtc->base.mutex);
> +
> + return ret ?: has_ips;
> +}
> +
> static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> enum pipe pipe,
> enum intel_pipe_crc_source
> source) @@ -610,6 +637,7 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
> enum intel_display_power_domain power_domain;
> u32 val = 0; /* shut up gcc */
> int ret;
> + int has_ips;
>
> if (pipe_crc->source == source)
> return 0;
> @@ -618,11 +646,12 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
> if (pipe_crc->source && source)
> return -EINVAL;
>
> + has_ips = toggle_ips_and_test_active(crtc, source);
> + if (has_ips < 0)
> + return has_ips;
> +
> power_domain = POWER_DOMAIN_PIPE(pipe);
> - if (!intel_display_power_get_if_enabled(dev_priv,
> power_domain)) {
> - DRM_DEBUG_KMS("Trying to capture CRC while
> pipe is off\n");
> - return -EIO;
> - }
> + intel_display_power_get(dev_priv, power_domain);
>
> ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
> if (ret != 0)
> @@ -649,7 +678,8 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
> * user space can't make reliable use of the
> CRCs, so let's just
> * completely disable it.
> */
> - hsw_disable_ips(crtc);
> + if (has_ips)
> + hsw_disable_ips(crtc);
>
> spin_lock_irq(&pipe_crc->lock);
> kfree(pipe_crc->entries);
> @@ -694,7 +724,8 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
> else if (IS_HASWELL(dev_priv) && pipe ==
> PIPE_A)
>
> hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
>
> - hsw_enable_ips(crtc);
> + if (has_ips)
> + hsw_enable_ips(crtc);
> }
>
> ret = 0;
> @@ -919,23 +950,25 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc,
> const char *source_name,
> enum intel_pipe_crc_source source;
> u32 val = 0; /* shut up gcc */
> int ret = 0;
> + int has_ips;
>
> if (display_crc_ctl_parse_source(source_name, &source) < 0) {
> DRM_DEBUG_DRIVER("unknown source %s\n",
> source_name);
> return -EINVAL;
> }
>
> + has_ips = toggle_ips_and_test_active(intel_crtc, source);
> + if (has_ips < 0)
> + return has_ips;
> +
> power_domain = POWER_DOMAIN_PIPE(crtc->index);
> - if (!intel_display_power_get_if_enabled(dev_priv,
> power_domain)) {
> - DRM_DEBUG_KMS("Trying to capture CRC while
> pipe is off\n");
> - return -EIO;
> - }
> + intel_display_power_get(dev_priv, power_domain);
>
> ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source,
> &val);
> if (ret != 0)
> goto out;
>
> - if (source) {
> + if (source && has_ips) {
> /*
> * When IPS gets enabled, the pipe CRC
> changes. Since IPS gets
> * enabled and disabled dynamically based on
> package C states, @@ -956,7 +989,8 @@ int intel_crtc_set_crc_source(struct
> drm_crtc *crtc, const char *source_name,
> else if (IS_HASWELL(dev_priv) && crtc->index
> == PIPE_A)
>
> hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
>
> - hsw_enable_ips(intel_crtc);
> + if (has_ips)
> + hsw_enable_ips(intel_crtc);
> }
>
> pipe_crc->skipped = 0;
> --
> 2.11.0
More information about the Intel-gfx
mailing list