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

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

[James Ausmus]

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@linux.intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: James Ausmus <james.ausmus@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;
+}
+
 /*
  * 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);
+
 	/* For FIFO watermark updates */
 	if (INTEL_GEN(dev_priv) >= 9) {
 		skl_setup_wm_latency(dev_priv);
-- 
2.22.1

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

[PATCH v2 2/2] drm/i915/tgl: Read SAGV block time from PCODE

[James Ausmus]

Starting from TGL, we now need to read the SAGV block time via a PCODE
mailbox, rather than having a static value.

BSpec: 49326

v2: Fix up pcode val data type (Ville), tighten variable scope (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: James Ausmus <james.ausmus@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 058aa5ca8b73..6a45df9dad9c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8869,6 +8869,7 @@ enum {
 #define     GEN9_SAGV_DISABLE			0x0
 #define     GEN9_SAGV_IS_DISABLED		0x1
 #define     GEN9_SAGV_ENABLE			0x3
+#define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
 #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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b413a7f3bc5d..13721ba44013 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3645,7 +3645,20 @@ intel_has_sagv(struct drm_i915_private *dev_priv)
 static void
 skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
 {
-	if (IS_GEN(dev_priv, 11)) {
+	if (INTEL_GEN(dev_priv) >= 12) {
+		u32 val = 0;
+		int ret;
+
+		ret = sandybridge_pcode_read(dev_priv,
+					     GEN12_PCODE_READ_SAGV_BLOCK_TIME_US,
+					     &val, NULL);
+		if (!ret) {
+			dev_priv->sagv_block_time_us = val;
+			return;
+		}
+
+		DRM_DEBUG_DRIVER("Couldn't read SAGV block time!\n");
+	} else if (IS_GEN(dev_priv, 11)) {
 		dev_priv->sagv_block_time_us = 10;
 		return;
 	} else if (IS_GEN(dev_priv, 10)) {
-- 
2.22.1

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

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

[James Ausmus]

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@linux.intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: James Ausmus <james.ausmus@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.

Review ping. :)

Thanks!

-James

> 
>  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;
> +}
> +
>  /*
>   * 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);
> +
>  	/* For FIFO watermark updates */
>  	if (INTEL_GEN(dev_priv) >= 9) {
>  		skl_setup_wm_latency(dev_priv);
> -- 
> 2.22.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@linux.intel.com>
>Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>Signed-off-by: James Ausmus <james.ausmus@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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915/tgl: Read SAGV block time from PCODE

[Lucas De Marchi]

On Fri, Sep 27, 2019 at 03:24:27PM -0700, James Ausmus wrote:
>Starting from TGL, we now need to read the SAGV block time via a PCODE
>mailbox, rather than having a static value.
>
>BSpec: 49326
>
>v2: Fix up pcode val data type (Ville), tighten variable scope (Ville)
>
>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>Signed-off-by: James Ausmus <james.ausmus@intel.com>
>Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/i915_reg.h |  1 +
> drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++++++-
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>index 058aa5ca8b73..6a45df9dad9c 100644
>--- a/drivers/gpu/drm/i915/i915_reg.h
>+++ b/drivers/gpu/drm/i915/i915_reg.h
>@@ -8869,6 +8869,7 @@ enum {
> #define     GEN9_SAGV_DISABLE			0x0
> #define     GEN9_SAGV_IS_DISABLED		0x1
> #define     GEN9_SAGV_ENABLE			0x3
>+#define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
> #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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>index b413a7f3bc5d..13721ba44013 100644
>--- a/drivers/gpu/drm/i915/intel_pm.c
>+++ b/drivers/gpu/drm/i915/intel_pm.c
>@@ -3645,7 +3645,20 @@ intel_has_sagv(struct drm_i915_private *dev_priv)
> static void
> skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
> {
>-	if (IS_GEN(dev_priv, 11)) {
>+	if (INTEL_GEN(dev_priv) >= 12) {

sagv will still never be enabled for TGL. Are you going to revert 
8ffa4392a32e ("drm/i915/tgl: disable SAGV temporarily")
in a separete patch?

Lucas De Marchi

>+		u32 val = 0;
>+		int ret;
>+
>+		ret = sandybridge_pcode_read(dev_priv,
>+					     GEN12_PCODE_READ_SAGV_BLOCK_TIME_US,
>+					     &val, NULL);
>+		if (!ret) {
>+			dev_priv->sagv_block_time_us = val;
>+			return;
>+		}
>+
>+		DRM_DEBUG_DRIVER("Couldn't read SAGV block time!\n");
>+	} else if (IS_GEN(dev_priv, 11)) {
> 		dev_priv->sagv_block_time_us = 10;
> 		return;
> 	} else if (IS_GEN(dev_priv, 10)) {
>-- 
>2.22.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915/tgl: Read SAGV block time from PCODE

[James Ausmus]

On Fri, Oct 04, 2019 at 01:55:46PM -0700, Lucas De Marchi wrote:
> On Fri, Sep 27, 2019 at 03:24:27PM -0700, James Ausmus wrote:
> >Starting from TGL, we now need to read the SAGV block time via a PCODE
> >mailbox, rather than having a static value.
> >
> >BSpec: 49326
> >
> >v2: Fix up pcode val data type (Ville), tighten variable scope (Ville)
> >
> >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> >Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >Signed-off-by: James Ausmus <james.ausmus@intel.com>
> >Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/i915_reg.h |  1 +
> > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++++++-
> > 2 files changed, 15 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >index 058aa5ca8b73..6a45df9dad9c 100644
> >--- a/drivers/gpu/drm/i915/i915_reg.h
> >+++ b/drivers/gpu/drm/i915/i915_reg.h
> >@@ -8869,6 +8869,7 @@ enum {
> > #define     GEN9_SAGV_DISABLE			0x0
> > #define     GEN9_SAGV_IS_DISABLED		0x1
> > #define     GEN9_SAGV_ENABLE			0x3
> >+#define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
> > #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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >index b413a7f3bc5d..13721ba44013 100644
> >--- a/drivers/gpu/drm/i915/intel_pm.c
> >+++ b/drivers/gpu/drm/i915/intel_pm.c
> >@@ -3645,7 +3645,20 @@ intel_has_sagv(struct drm_i915_private *dev_priv)
> > static void
> > skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
> > {
> >-	if (IS_GEN(dev_priv, 11)) {
> >+	if (INTEL_GEN(dev_priv) >= 12) {
> 
> sagv will still never be enabled for TGL. Are you going to revert 
> 8ffa4392a32e ("drm/i915/tgl: disable SAGV temporarily")
> in a separete patch?

Yes, that's the idea - we land these two patches, then once HSD
1409542895 gets resolved, we revert 8ffa4392a32e and everything Just
Works. ;)

-James

> 
> Lucas De Marchi
> 
> >+		u32 val = 0;
> >+		int ret;
> >+
> >+		ret = sandybridge_pcode_read(dev_priv,
> >+					     GEN12_PCODE_READ_SAGV_BLOCK_TIME_US,
> >+					     &val, NULL);
> >+		if (!ret) {
> >+			dev_priv->sagv_block_time_us = val;
> >+			return;
> >+		}
> >+
> >+		DRM_DEBUG_DRIVER("Couldn't read SAGV block time!\n");
> >+	} else if (IS_GEN(dev_priv, 11)) {
> > 		dev_priv->sagv_block_time_us = 10;
> > 		return;
> > 	} else if (IS_GEN(dev_priv, 10)) {
> >-- 
> >2.22.1
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[James Ausmus]

On Fri, Oct 04, 2019 at 01:53:57PM -0700, Lucas De Marchi wrote:
> 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@linux.intel.com>
> >Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> >Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >Signed-off-by: James Ausmus <james.ausmus@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.

Hmm - yeah, I don't see a reason why it needs to be signed either.
Hadn't looked in to it previously, was just following the existing code.
:)

I'll switch that up and send a v3.

> 
> 
> >+}
> >+
> > /*
> >  * 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?

Yeah, that would make sense, will fix up.


Thanks!

-James

> 
> >+
> > 	/* For FIFO watermark updates */
> > 	if (INTEL_GEN(dev_priv) >= 9) {
> > 		skl_setup_wm_latency(dev_priv);
> >-- 
> >2.22.1
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[James Ausmus]

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@linux.intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: James Ausmus <james.ausmus@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;
+
 	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

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

[PATCH v3 2/2] drm/i915/tgl: Read SAGV block time from PCODE

[James Ausmus]

Starting from TGL, we now need to read the SAGV block time via a PCODE
mailbox, rather than having a static value.

BSpec: 49326

v2: Fix up pcode val data type (Ville), tighten variable scope (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: James Ausmus <james.ausmus@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6d67bd238cfe..030f0c37f64b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8870,6 +8870,7 @@ enum {
 #define     GEN9_SAGV_DISABLE			0x0
 #define     GEN9_SAGV_IS_DISABLED		0x1
 #define     GEN9_SAGV_ENABLE			0x3
+#define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
 #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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0ffcafe97216..e2aca3e81d28 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3645,7 +3645,20 @@ intel_has_sagv(struct drm_i915_private *dev_priv)
 static void
 skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
 {
-	if (IS_GEN(dev_priv, 11)) {
+	if (INTEL_GEN(dev_priv) >= 12) {
+		u32 val = 0;
+		int ret;
+
+		ret = sandybridge_pcode_read(dev_priv,
+					     GEN12_PCODE_READ_SAGV_BLOCK_TIME_US,
+					     &val, NULL);
+		if (!ret) {
+			dev_priv->sagv_block_time_us = val;
+			return;
+		}
+
+		DRM_DEBUG_DRIVER("Couldn't read SAGV block time!\n");
+	} else if (IS_GEN(dev_priv, 11)) {
 		dev_priv->sagv_block_time_us = 10;
 		return;
 	} else if (IS_GEN(dev_priv, 10)) {
-- 
2.22.1

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

Re: [PATCH v2 2/2] drm/i915/tgl: Read SAGV block time from PCODE

[Ville Syrjälä]

On Fri, Oct 04, 2019 at 02:51:34PM -0700, James Ausmus wrote:
> On Fri, Oct 04, 2019 at 01:55:46PM -0700, Lucas De Marchi wrote:
> > On Fri, Sep 27, 2019 at 03:24:27PM -0700, James Ausmus wrote:
> > >Starting from TGL, we now need to read the SAGV block time via a PCODE
> > >mailbox, rather than having a static value.
> > >
> > >BSpec: 49326
> > >
> > >v2: Fix up pcode val data type (Ville), tighten variable scope (Ville)
> > >
> > >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > >Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > >Signed-off-by: James Ausmus <james.ausmus@intel.com>
> > >Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >---
> > > drivers/gpu/drm/i915/i915_reg.h |  1 +
> > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++++++-
> > > 2 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > >diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > >index 058aa5ca8b73..6a45df9dad9c 100644
> > >--- a/drivers/gpu/drm/i915/i915_reg.h
> > >+++ b/drivers/gpu/drm/i915/i915_reg.h
> > >@@ -8869,6 +8869,7 @@ enum {
> > > #define     GEN9_SAGV_DISABLE			0x0
> > > #define     GEN9_SAGV_IS_DISABLED		0x1
> > > #define     GEN9_SAGV_ENABLE			0x3
> > >+#define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
> > > #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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > >index b413a7f3bc5d..13721ba44013 100644
> > >--- a/drivers/gpu/drm/i915/intel_pm.c
> > >+++ b/drivers/gpu/drm/i915/intel_pm.c
> > >@@ -3645,7 +3645,20 @@ intel_has_sagv(struct drm_i915_private *dev_priv)
> > > static void
> > > skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
> > > {
> > >-	if (IS_GEN(dev_priv, 11)) {
> > >+	if (INTEL_GEN(dev_priv) >= 12) {
> > 
> > sagv will still never be enabled for TGL. Are you going to revert 
> > 8ffa4392a32e ("drm/i915/tgl: disable SAGV temporarily")
> > in a separete patch?
> 
> Yes, that's the idea - we land these two patches, then once HSD
> 1409542895 gets resolved, we revert 8ffa4392a32e and everything Just
> Works. ;)

The whole sagv stuff is wrong for icl+. Stan is attempting to remedy
that.

> 
> -James
> 
> > 
> > Lucas De Marchi
> > 
> > >+		u32 val = 0;
> > >+		int ret;
> > >+
> > >+		ret = sandybridge_pcode_read(dev_priv,
> > >+					     GEN12_PCODE_READ_SAGV_BLOCK_TIME_US,
> > >+					     &val, NULL);
> > >+		if (!ret) {
> > >+			dev_priv->sagv_block_time_us = val;
> > >+			return;
> > >+		}
> > >+
> > >+		DRM_DEBUG_DRIVER("Couldn't read SAGV block time!\n");
> > >+	} else if (IS_GEN(dev_priv, 11)) {
> > > 		dev_priv->sagv_block_time_us = 10;
> > > 		return;
> > > 	} else if (IS_GEN(dev_priv, 10)) {
> > >-- 
> > >2.22.1
> > >

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915/tgl: Read SAGV block time from PCODE

[James Ausmus]

On Mon, Oct 07, 2019 at 01:15:24PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 04, 2019 at 02:51:34PM -0700, James Ausmus wrote:
> > On Fri, Oct 04, 2019 at 01:55:46PM -0700, Lucas De Marchi wrote:
> > > On Fri, Sep 27, 2019 at 03:24:27PM -0700, James Ausmus wrote:
> > > >Starting from TGL, we now need to read the SAGV block time via a PCODE
> > > >mailbox, rather than having a static value.
> > > >
> > > >BSpec: 49326
> > > >
> > > >v2: Fix up pcode val data type (Ville), tighten variable scope (Ville)
> > > >
> > > >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > >Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > >Signed-off-by: James Ausmus <james.ausmus@intel.com>
> > > >Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >---
> > > > drivers/gpu/drm/i915/i915_reg.h |  1 +
> > > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++++++-
> > > > 2 files changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > >diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > >index 058aa5ca8b73..6a45df9dad9c 100644
> > > >--- a/drivers/gpu/drm/i915/i915_reg.h
> > > >+++ b/drivers/gpu/drm/i915/i915_reg.h
> > > >@@ -8869,6 +8869,7 @@ enum {
> > > > #define     GEN9_SAGV_DISABLE			0x0
> > > > #define     GEN9_SAGV_IS_DISABLED		0x1
> > > > #define     GEN9_SAGV_ENABLE			0x3
> > > >+#define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
> > > > #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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > >index b413a7f3bc5d..13721ba44013 100644
> > > >--- a/drivers/gpu/drm/i915/intel_pm.c
> > > >+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > >@@ -3645,7 +3645,20 @@ intel_has_sagv(struct drm_i915_private *dev_priv)
> > > > static void
> > > > skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
> > > > {
> > > >-	if (IS_GEN(dev_priv, 11)) {
> > > >+	if (INTEL_GEN(dev_priv) >= 12) {
> > > 
> > > sagv will still never be enabled for TGL. Are you going to revert 
> > > 8ffa4392a32e ("drm/i915/tgl: disable SAGV temporarily")
> > > in a separete patch?
> > 
> > Yes, that's the idea - we land these two patches, then once HSD
> > 1409542895 gets resolved, we revert 8ffa4392a32e and everything Just
> > Works. ;)
> 
> The whole sagv stuff is wrong for icl+. Stan is attempting to remedy
> that.

Well, we'll at least need to do this read of the block time - do you
think these two patches can land in the meantime, to help prep the TGL
path for actually working when Stan's work lands?

Thanks!

-James

> 
> > 
> > -James
> > 
> > > 
> > > Lucas De Marchi
> > > 
> > > >+		u32 val = 0;
> > > >+		int ret;
> > > >+
> > > >+		ret = sandybridge_pcode_read(dev_priv,
> > > >+					     GEN12_PCODE_READ_SAGV_BLOCK_TIME_US,
> > > >+					     &val, NULL);
> > > >+		if (!ret) {
> > > >+			dev_priv->sagv_block_time_us = val;
> > > >+			return;
> > > >+		}
> > > >+
> > > >+		DRM_DEBUG_DRIVER("Couldn't read SAGV block time!\n");
> > > >+	} else if (IS_GEN(dev_priv, 11)) {
> > > > 		dev_priv->sagv_block_time_us = 10;
> > > > 		return;
> > > > 	} else if (IS_GEN(dev_priv, 10)) {
> > > >-- 
> > > >2.22.1
> > > >
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@linux.intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: James Ausmus <james.ausmus@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/2] drm/i915/tgl: Read SAGV block time from PCODE

[James Ausmus]

Starting from TGL, we now need to read the SAGV block time via a PCODE
mailbox, rather than having a static value.

BSpec: 49326

v2: Fix up pcode val data type (Ville), tighten variable scope (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: James Ausmus <james.ausmus@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 058aa5ca8b73..6a45df9dad9c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8869,6 +8869,7 @@ enum {
 #define     GEN9_SAGV_DISABLE			0x0
 #define     GEN9_SAGV_IS_DISABLED		0x1
 #define     GEN9_SAGV_ENABLE			0x3
+#define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
 #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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b413a7f3bc5d..13721ba44013 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3645,7 +3645,20 @@ intel_has_sagv(struct drm_i915_private *dev_priv)
 static void
 skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
 {
-	if (IS_GEN(dev_priv, 11)) {
+	if (INTEL_GEN(dev_priv) >= 12) {
+		u32 val = 0;
+		int ret;
+
+		ret = sandybridge_pcode_read(dev_priv,
+					     GEN12_PCODE_READ_SAGV_BLOCK_TIME_US,
+					     &val, NULL);
+		if (!ret) {
+			dev_priv->sagv_block_time_us = val;
+			return;
+		}
+
+		DRM_DEBUG_DRIVER("Couldn't read SAGV block time!\n");
+	} else if (IS_GEN(dev_priv, 11)) {
 		dev_priv->sagv_block_time_us = 10;
 		return;
 	} else if (IS_GEN(dev_priv, 10)) {
-- 
2.22.1

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