Re: [PATCH v6 1/6] drm/i915/skl: Add support for the SAGV, fix underrun hangs

[Maarten Lankhorst]

Op 03-08-16 om 00:37 schreef Lyude:
> Since the watermark calculations for Skylake are still broken, we're apt
> to hitting underruns very easily under multi-monitor configurations.
> While it would be lovely if this was fixed, it's not. Another problem
> that's been coming from this however, is the mysterious issue of
> underruns causing full system hangs. An easy way to reproduce this with
> a skylake system:
>
> - Get a laptop with a skylake GPU, and hook up two external monitors to
>   it
> - Move the cursor from the built-in LCD to one of the external displays
>   as quickly as you can
> - You'll get a few pipe underruns, and eventually the entire system will
>   just freeze.
>
> After doing a lot of investigation and reading through the bspec, I
> found the existence of the SAGV, which is responsible for adjusting the
> system agent voltage and clock frequencies depending on how much power
> we need. According to the bspec:
>
> "The display engine access to system memory is blocked during the
>  adjustment time. SAGV defaults to enabled. Software must use the
>  GT-driver pcode mailbox to disable SAGV when the display engine is not
>  able to tolerate the blocking time."
>
> The rest of the bspec goes on to explain that software can simply leave
> the SAGV enabled, and disable it when we use interlaced pipes/have more
> then one pipe active.
>
> Sure enough, with this patchset the system hangs resulting from pipe
> underruns on Skylake have completely vanished on my T460s. Additionally,
> the bspec mentions turning off the SAGV	with more then one pipe enabled
> as a workaround for display underruns. While this patch doesn't entirely
> fix that, it looks like it does improve the situation a little bit so
> it's likely this is going to be required to make watermarks on Skylake
> fully functional.
>
> Changes since v5:
>  - Don't use is_power_of_2. Makes things confusing
>  - Don't use the old state to figure out whether or not to
>    enable/disable the sagv, use the new one
>  - Split the loop in skl_disable_sagv into it's own function
>  - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail()
> Changes since v4:
>  - Use is_power_of_2 against active_crtcs to check whether we have > 1
>    pipe enabled
>  - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0
>    enabled
>  - Call skl_sagv_enable/disable() from pre/post-plane updates
> Changes since v3:
>  - Use time_before() to compare timeout to jiffies
> Changes since v2:
>  - Really apply minor style nitpicks to patch this time
> Changes since v1:
>  - Added comments about this probably being one of the requirements to
>    fixing Skylake's watermark issues
>  - Minor style nitpicks from Matt Roper
>  - Disable these functions on Broxton, since it doesn't have an SAGV
>
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Signed-off-by: Lyude <cpaul@xxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Ville SyrjÃlà <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   2 +
>  drivers/gpu/drm/i915/i915_reg.h      |   5 ++
>  drivers/gpu/drm/i915/intel_display.c |  11 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_pm.c      | 112 +++++++++++++++++++++++++++++++++++
>  5 files changed, 132 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 65ada5d..87018d3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1962,6 +1962,8 @@ struct drm_i915_private {
>  	struct i915_suspend_saved_registers regfile;
>  	struct vlv_s0ix_state vlv_s0ix_state;
>  
> +	bool skl_sagv_enabled;
> +
>  	struct {
>  		/*
>  		 * Raw watermark latency values:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2f93d4a..5fb1c63 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7170,6 +7170,11 @@ enum {
>  #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
>  #define   DISPLAY_IPS_CONTROL			0x19
>  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> +#define   GEN9_PCODE_SAGV_CONTROL		0x21
> +#define     GEN9_SAGV_DISABLE			0x0
> +#define     GEN9_SAGV_LOW_FREQ			0x1
> +#define     GEN9_SAGV_HIGH_FREQ			0x2
> +#define     GEN9_SAGV_DYNAMIC_FREQ              0x3
>  #define GEN6_PCODE_DATA				_MMIO(0x138128)
>  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
>  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a8e8cc8..001c885 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13692,6 +13692,14 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		     intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco))
>  			dev_priv->display.modeset_commit_cdclk(state);
>  
> +		/*
> +		 * SKL workaround: bspec recommends we disable the SAGV when we
> +		 * have more then one pipe enabled
> +		 */
> +		if (IS_SKYLAKE(dev_priv) &&
> +		    hweight32(intel_state->active_crtcs) > 1)
> +			skl_disable_sagv(dev_priv);
>  		intel_modeset_verify_disabled(dev);
>  	}
>  
> @@ -13765,6 +13773,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
>  	}
>  
> +	if (hweight32(intel_state->active_crtcs) <= 1)
> +		skl_enable_sagv(dev_priv);
Should be guarded with a if (intel_state->modeset &&

Looks ok otherwise. :-)

With that fixed:

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>