Re: [PATCH] drm/i915: Keep IPS disabled during CRC collection

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Tested-by: Marta Lofstedt <marta.lofstedt@xxxxxxxxx>


> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx]
> Sent: Thursday, August 10, 2017 12:55 PM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Lofstedt, Marta <marta.lofstedt@xxxxxxxxx>; Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> ---
>  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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux