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

[Lucas De Marchi]

On Fri, Sep 27, 2019 at 03:24:26PM -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)
>
>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>
>---
>
>Ville - with the amount of v1..v2 change in this first patch, I wasn't
>comfortable applying your R-b, could you take another look? Patch 2 just
>has the trivial changes you suggested, so I kept that one.
>
> 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 337d8306416a..87a835a0210b 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -1579,6 +1579,8 @@ struct drm_i915_private {
> 		I915_SAGV_NOT_CONTROLLED
> 	} sagv_status;
>
>+	int sagv_block_time_us;
>+
> 	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..b413a7f3bc5d 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 = 1000;

I would actually make sagv_block_time_us unsigned and assign -1 here.
But making it unsigned would mean cascade that down to the wm
calculations. Humn... Maybe there's a reason for that to be signed that
I'm not seeing.


>+}
>+
> /*
>  * 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 (IS_GEN9_BC(dev_priv) || INTEL_GEN(dev_priv) >= 10)
>+		skl_setup_sagv_block_time(dev_priv);

Do we want to use intel_has_sagv() here?

>+
> 	/* For FIFO watermark updates */
> 	if (INTEL_GEN(dev_priv) >= 9) {
> 		skl_setup_wm_latency(dev_priv);
>-- 
>2.22.1
>