[Intel-gfx] [PATCH v3 1/2] drm/i915: Move SAGV block time to dev_priv

[Ville Syrjälä]

On Fri, Oct 04, 2019 at 03:14:48PM -0700, James Ausmus wrote:
> In prep for newer platforms having more complicated ways to determine
> the SAGV block time, move the variable to dev_priv, and extract the
> setting to an initial setup function. While we're at it, update the if
> ladder to follow the new gen -> old gen order preference, and warn on
> any non-specified gen.
> 
> v2: Shorten the function name (Ville), return directly (Ville), move
> sagv_block_time_us value to dev_priv (Ville)
> 
> v3: Change sagv_block_time_us to u32 (Lucas), Change fallback value to
> -1 (Lucas), use intel_has_sagv for setup check rather than hand-rolling
> (Lucas)
> 
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Signed-off-by: James Ausmus <james.ausmus at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++---------
>  2 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cde4c7fb5570..1d9a9e827261 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1560,6 +1560,8 @@ struct drm_i915_private {
>  		I915_SAGV_NOT_CONTROLLED
>  	} sagv_status;
>  
> +	u32 sagv_block_time_us;
> +

u32 seems a bit excessive. Although the pcode command doesn't document
any upper bound so maybe u32 is really the correct choice here. And we'd
need to put it somewhere else to avoid the hole anyway.

Series is
Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

>  	struct {
>  		/*
>  		 * Raw watermark latency values:
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bfcf03ab5245..0ffcafe97216 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3642,6 +3642,26 @@ intel_has_sagv(struct drm_i915_private *dev_priv)
>  		dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
>  }
>  
> +static void
> +skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
> +{
> +	if (IS_GEN(dev_priv, 11)) {
> +		dev_priv->sagv_block_time_us = 10;
> +		return;
> +	} else if (IS_GEN(dev_priv, 10)) {
> +		dev_priv->sagv_block_time_us = 20;
> +		return;
> +	} else if (IS_GEN(dev_priv, 9)) {
> +		dev_priv->sagv_block_time_us = 30;
> +		return;
> +	} else {
> +		MISSING_CASE(INTEL_GEN(dev_priv));
> +	}
> +
> +	/* Default to an unusable block time */
> +	dev_priv->sagv_block_time_us = -1;
> +}
> +
>  /*
>   * SAGV dynamically adjusts the system agent voltage and clock frequencies
>   * depending on power and performance requirements. The display engine access
> @@ -3730,18 +3750,10 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state)
>  	struct intel_crtc_state *crtc_state;
>  	enum pipe pipe;
>  	int level, latency;
> -	int sagv_block_time_us;
>  
>  	if (!intel_has_sagv(dev_priv))
>  		return false;
>  
> -	if (IS_GEN(dev_priv, 9))
> -		sagv_block_time_us = 30;
> -	else if (IS_GEN(dev_priv, 10))
> -		sagv_block_time_us = 20;
> -	else
> -		sagv_block_time_us = 10;
> -
>  	/*
>  	 * If there are no active CRTCs, no additional checks need be performed
>  	 */
> @@ -3788,7 +3800,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state)
>  		 * incur memory latencies higher than sagv_block_time_us we
>  		 * can't enable SAGV.
>  		 */
> -		if (latency < sagv_block_time_us)
> +		if (latency < dev_priv->sagv_block_time_us)
>  			return false;
>  	}
>  
> @@ -9013,6 +9025,9 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
>  	else if (IS_GEN(dev_priv, 5))
>  		i915_ironlake_get_mem_freq(dev_priv);
>  
> +	if (intel_has_sagv(dev_priv))
> +		skl_setup_sagv_block_time(dev_priv);
> +
>  	/* For FIFO watermark updates */
>  	if (INTEL_GEN(dev_priv) >= 9) {
>  		skl_setup_wm_latency(dev_priv);
> -- 
> 2.22.1

-- 
Ville Syrjälä
Intel