[PATCH v3 1/3] drm/i915/gen6+: Clear upper data byte during PCODE write

[PATCH v3 1/3] drm/i915/gen6+: Clear upper data byte during PCODE write

[Imre Deak]

The spec calls for the upper data byte to be cleared before most of the
PCODE write commands, for others like IPS control it doesn't say
anything about this byte. Let's clear it in case it's clobbered somehow,
especially that there are places where we only do a PCODE write without
a preceeding PCODE read.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 29b6653..66c62f3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7838,6 +7838,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
 	}
 
 	I915_WRITE_FW(GEN6_PCODE_DATA, val);
+	I915_WRITE_FW(GEN6_PCODE_DATA1, 0);
 	I915_WRITE_FW(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
 
 	if (intel_wait_for_register_fw(dev_priv,
-- 
2.5.0

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

[PATCH v3 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Imre Deak]

commit 848496e5902833600f7992f4faa82dc1546051ba
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Wed Jul 13 16:32:03 2016 +0300

    drm/i915: Wait up to 3ms for the pcu to ack the cdclk change request on SKL

increased the timeout to match the spec, but we still see a timeout on
at least one SKL. A CDCLK change request following the failed one will
succeed nevertheless.

I could reproduce this problem easily by running kms_pipe_crc_basic in a
loop. In all failure cases _wait_for() was pre-empted for >3ms and so in
the worst case - when the pre-emption happened right after calculating
timeout__ in _wait_for() - we called skl_cdclk_wait_for_pcu_ready() only
once which failed and so _wait_for() timed out. As opposed to this the
spec says to keep retrying the request for at most a 3ms period.

To fix this disable pre-emption to maximize the number of times we retry
the request. Also increase the timeout to 10ms to account for interrupts
that could reduce the number of these attempts. With this change I
couldn't trigger the problem.

v2:
- Use 1ms poll period instead of 10us. (Chris)
v3:
- Poll with pre-emption disabled to increase the number of request
  attempts. (Ville, Chris)
- Factor out a helper to poll, it's also needed by the next patch.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reference: https://bugs.freedesktop.org/show_bug.cgi?id=97929
Testcase: igt/kms_pipe_crc_basic/suspend-read-crc-pipe-B
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/i915_reg.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 29 +++++++-----------------
 drivers/gpu/drm/i915/intel_pm.c      | 43 ++++++++++++++++++++++++++++++++++++
 4 files changed, 53 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 01f5067..f618807 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3593,6 +3593,7 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
 
 int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
 int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
+int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request);
 
 /* intel_sideband.c */
 u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6747d68..f542cbc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7424,7 +7424,6 @@ enum {
 #define     GEN9_MEM_LATENCY_LEVEL_3_7_SHIFT	24
 #define   SKL_PCODE_CDCLK_CONTROL		0x7
 #define     SKL_CDCLK_PREPARE_FOR_CHANGE	0x3
-#define     SKL_CDCLK_READY_FOR_CHANGE		0x1
 #define   GEN6_PCODE_WRITE_MIN_FREQ_TABLE	0x8
 #define   GEN6_PCODE_READ_MIN_FREQ_TABLE	0x9
 #define   GEN6_READ_OC_PARAMS			0xc
@@ -7437,6 +7436,7 @@ enum {
 #define     GEN9_SAGV_DISABLE			0x0
 #define     GEN9_SAGV_IS_DISABLED		0x1
 #define     GEN9_SAGV_ENABLE			0x3
+#define   GEN9_PCODE_REQUEST_DONE		0x1
 #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 5d11002..46c0e42 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6245,35 +6245,22 @@ skl_dpll0_disable(struct drm_i915_private *dev_priv)
 	dev_priv->cdclk_pll.vco = 0;
 }
 
-static bool skl_cdclk_pcu_ready(struct drm_i915_private *dev_priv)
-{
-	int ret;
-	u32 val;
-
-	/* inform PCU we want to change CDCLK */
-	val = SKL_CDCLK_PREPARE_FOR_CHANGE;
-	mutex_lock(&dev_priv->rps.hw_lock);
-	ret = sandybridge_pcode_read(dev_priv, SKL_PCODE_CDCLK_CONTROL, &val);
-	mutex_unlock(&dev_priv->rps.hw_lock);
-
-	return ret == 0 && (val & SKL_CDCLK_READY_FOR_CHANGE);
-}
-
-static bool skl_cdclk_wait_for_pcu_ready(struct drm_i915_private *dev_priv)
-{
-	return _wait_for(skl_cdclk_pcu_ready(dev_priv), 3000, 10) == 0;
-}
-
 static void skl_set_cdclk(struct drm_i915_private *dev_priv, int cdclk, int vco)
 {
 	u32 freq_select, pcu_ack;
+	int ret;
 
 	WARN_ON((cdclk == 24000) != (vco == 0));
 
 	DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz (VCO %d kHz)\n", cdclk, vco);
 
-	if (!skl_cdclk_wait_for_pcu_ready(dev_priv)) {
-		DRM_ERROR("failed to inform PCU about cdclk change\n");
+	mutex_lock(&dev_priv->rps.hw_lock);
+	ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
+				SKL_CDCLK_PREPARE_FOR_CHANGE);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+	if (ret) {
+		DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
+			  ret);
 		return;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 66c62f3..edd68f3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7864,6 +7864,49 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
 	return 0;
 }
 
+static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox,
+				  u32 request, u32 *status)
+{
+	u32 val = request;
+
+	*status = sandybridge_pcode_read(dev_priv, mbox, &val);
+
+	return *status || (val & GEN9_PCODE_REQUEST_DONE);
+}
+
+/**
+ * skl_pcode_request - send PCODE request until acknowledgment
+ * @dev_priv: device private
+ * @mbox: PCODE mailbox ID the request is targeted for
+ * @request: request ID
+ *
+ * Keep resending the @request to @mbox until PCODE acknowledges it, or a 10ms
+ * timeout expires.
+ *
+ * Returns 0 on success, %-ETIMEDOUT in case of a timeout, <0 in case of some
+ * other error as reported by PCODE.
+ */
+int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request)
+{
+	u32 status;
+	int ret;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+	/*
+	 * The spec says to keep retrying the request for at most 3ms until
+	 * acknowledgement, so disable pre-emption to maximize the number of
+	 * attempts within this duration. Use a 10ms overall timeout to
+	 * account for interrupts that could reduce the number of attempts.
+	 */
+	preempt_disable();
+	ret = wait_for_atomic(skl_pcode_try_request(dev_priv, mbox, request,
+						    &status), 10);
+	preempt_enable();
+
+	return ret ? ret : status;
+}
+
 static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
 	/*
-- 
2.5.0

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

[PATCH v3 3/3] drm/i915/gen9: Fix PCODE polling during SAGV disabling

[Imre Deak]

According to the previous patch, it's possible atm that we call
intel_do_sagv_disable() only once during the 1ms period and time out if
that call fails. As opposed to this the spec says that we need to keep
retrying this request for a 1ms duration, so let's do this similarly to
the CDCLK change notification request.

Cc: Lyude <cpaul@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  1 -
 drivers/gpu/drm/i915/intel_pm.c | 33 +++++++--------------------------
 2 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f542cbc..c7458b7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7434,7 +7434,6 @@ enum {
 #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
 #define   GEN9_PCODE_SAGV_CONTROL		0x21
 #define     GEN9_SAGV_DISABLE			0x0
-#define     GEN9_SAGV_IS_DISABLED		0x1
 #define     GEN9_SAGV_ENABLE			0x3
 #define   GEN9_PCODE_REQUEST_DONE		0x1
 #define GEN6_PCODE_DATA				_MMIO(0x138128)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index edd68f3..43b0b40 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2954,24 +2954,10 @@ intel_enable_sagv(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
-static int
-intel_do_sagv_disable(struct drm_i915_private *dev_priv)
-{
-	int ret;
-	uint32_t temp = GEN9_SAGV_DISABLE;
-
-	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
-				     &temp);
-	if (ret)
-		return ret;
-	else
-		return temp & GEN9_SAGV_IS_DISABLED;
-}
-
 int
 intel_disable_sagv(struct drm_i915_private *dev_priv)
 {
-	int ret, result;
+	int ret;
 
 	if (!intel_has_sagv(dev_priv))
 		return 0;
@@ -2981,27 +2967,22 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
 
 	DRM_DEBUG_KMS("Disabling the SAGV\n");
 	mutex_lock(&dev_priv->rps.hw_lock);
-
 	/* bspec says to keep retrying for at least 1 ms */
-	ret = wait_for(result = intel_do_sagv_disable(dev_priv), 1);
+	ret = skl_pcode_request(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+				GEN9_SAGV_DISABLE);
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
-	if (ret == -ETIMEDOUT) {
-		DRM_ERROR("Request to disable SAGV timed out\n");
-		return -ETIMEDOUT;
-	}
-
 	/*
 	 * Some skl systems, pre-release machines in particular,
 	 * don't actually have an SAGV.
 	 */
-	if (IS_SKYLAKE(dev_priv) && result == -ENXIO) {
+	if (IS_SKYLAKE(dev_priv) && ret == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
 		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
 		return 0;
-	} else if (result < 0) {
-		DRM_ERROR("Failed to disable the SAGV\n");
-		return result;
+	} else if (ret < 0) {
+		DRM_ERROR("Failed to disable the SAGV (%d)\n", ret);
+		return ret;
 	}
 
 	dev_priv->sagv_status = I915_SAGV_DISABLED;
-- 
2.5.0

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

✗ Fi.CI.BAT: failure for series starting with [v3,1/3] drm/i915/gen6+: Clear upper data byte during PCODE write

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/3] drm/i915/gen6+: Clear upper data byte during PCODE write
URL   : https://patchwork.freedesktop.org/series/16028/
State : failure

== Summary ==

Series 16028v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16028/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a:
                dmesg-warn -> PASS       (fi-ilk-650)
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-skl-6700hq)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                pass       -> INCOMPLETE (fi-skl-6700k)

fi-bdw-5557u     total:245  pass:230  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:245  pass:205  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:245  pass:192  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7500u     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:245  pass:223  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:210  pass:188  dwarn:1   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:245  pass:212  dwarn:0   dfail:0   fail:0   skip:33 

4d904cb07bb992d4b5f3c8b8d00ed5297b774c0c drm-tip: 2016y-11m-28d-08h-24m-29s UTC integration manifest
5202a43 drm/i915/gen9: Fix PCODE polling during SAGV disabling
7ce4e0f drm/i915/gen9: Fix PCODE polling during CDCLK change notification
73463f9 drm/i915/gen6+: Clear upper data byte during PCODE write

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3122/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Ville Syrjälä]

On Mon, Nov 28, 2016 at 01:12:57PM +0200, Imre Deak wrote:
> commit 848496e5902833600f7992f4faa82dc1546051ba
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Wed Jul 13 16:32:03 2016 +0300
> 
>     drm/i915: Wait up to 3ms for the pcu to ack the cdclk change request on SKL
> 
> increased the timeout to match the spec, but we still see a timeout on
> at least one SKL. A CDCLK change request following the failed one will
> succeed nevertheless.
> 
> I could reproduce this problem easily by running kms_pipe_crc_basic in a
> loop. In all failure cases _wait_for() was pre-empted for >3ms and so in
> the worst case - when the pre-emption happened right after calculating
> timeout__ in _wait_for() - we called skl_cdclk_wait_for_pcu_ready() only
> once which failed and so _wait_for() timed out. As opposed to this the
> spec says to keep retrying the request for at most a 3ms period.
> 
> To fix this disable pre-emption to maximize the number of times we retry
> the request. Also increase the timeout to 10ms to account for interrupts
> that could reduce the number of these attempts. With this change I
> couldn't trigger the problem.
> 
> v2:
> - Use 1ms poll period instead of 10us. (Chris)
> v3:
> - Poll with pre-emption disabled to increase the number of request
>   attempts. (Ville, Chris)
> - Factor out a helper to poll, it's also needed by the next patch.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=97929
> Testcase: igt/kms_pipe_crc_basic/suspend-read-crc-pipe-B
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/i915_reg.h      |  2 +-
>  drivers/gpu/drm/i915/intel_display.c | 29 +++++++-----------------
>  drivers/gpu/drm/i915/intel_pm.c      | 43 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 53 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 01f5067..f618807 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3593,6 +3593,7 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
>  
>  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
>  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
> +int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request);
>  
>  /* intel_sideband.c */
>  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6747d68..f542cbc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7424,7 +7424,6 @@ enum {
>  #define     GEN9_MEM_LATENCY_LEVEL_3_7_SHIFT	24
>  #define   SKL_PCODE_CDCLK_CONTROL		0x7
>  #define     SKL_CDCLK_PREPARE_FOR_CHANGE	0x3
> -#define     SKL_CDCLK_READY_FOR_CHANGE		0x1
>  #define   GEN6_PCODE_WRITE_MIN_FREQ_TABLE	0x8
>  #define   GEN6_PCODE_READ_MIN_FREQ_TABLE	0x9
>  #define   GEN6_READ_OC_PARAMS			0xc
> @@ -7437,6 +7436,7 @@ enum {
>  #define     GEN9_SAGV_DISABLE			0x0
>  #define     GEN9_SAGV_IS_DISABLED		0x1
>  #define     GEN9_SAGV_ENABLE			0x3
> +#define   GEN9_PCODE_REQUEST_DONE		0x1

Is that really a generic thing?

>  #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 5d11002..46c0e42 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6245,35 +6245,22 @@ skl_dpll0_disable(struct drm_i915_private *dev_priv)
>  	dev_priv->cdclk_pll.vco = 0;
>  }
>  
> -static bool skl_cdclk_pcu_ready(struct drm_i915_private *dev_priv)
> -{
> -	int ret;
> -	u32 val;
> -
> -	/* inform PCU we want to change CDCLK */
> -	val = SKL_CDCLK_PREPARE_FOR_CHANGE;
> -	mutex_lock(&dev_priv->rps.hw_lock);
> -	ret = sandybridge_pcode_read(dev_priv, SKL_PCODE_CDCLK_CONTROL, &val);
> -	mutex_unlock(&dev_priv->rps.hw_lock);
> -
> -	return ret == 0 && (val & SKL_CDCLK_READY_FOR_CHANGE);
> -}
> -
> -static bool skl_cdclk_wait_for_pcu_ready(struct drm_i915_private *dev_priv)
> -{
> -	return _wait_for(skl_cdclk_pcu_ready(dev_priv), 3000, 10) == 0;
> -}
> -
>  static void skl_set_cdclk(struct drm_i915_private *dev_priv, int cdclk, int vco)
>  {
>  	u32 freq_select, pcu_ack;
> +	int ret;
>  
>  	WARN_ON((cdclk == 24000) != (vco == 0));
>  
>  	DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz (VCO %d kHz)\n", cdclk, vco);
>  
> -	if (!skl_cdclk_wait_for_pcu_ready(dev_priv)) {
> -		DRM_ERROR("failed to inform PCU about cdclk change\n");
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> +				SKL_CDCLK_PREPARE_FOR_CHANGE);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +	if (ret) {
> +		DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
> +			  ret);
>  		return;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 66c62f3..edd68f3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7864,6 +7864,49 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
>  	return 0;
>  }
>  
> +static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox,
> +				  u32 request, u32 *status)
> +{
> +	u32 val = request;
> +
> +	*status = sandybridge_pcode_read(dev_priv, mbox, &val);
> +
> +	return *status || (val & GEN9_PCODE_REQUEST_DONE);
> +}
> +
> +/**
> + * skl_pcode_request - send PCODE request until acknowledgment
> + * @dev_priv: device private
> + * @mbox: PCODE mailbox ID the request is targeted for
> + * @request: request ID
> + *
> + * Keep resending the @request to @mbox until PCODE acknowledges it, or a 10ms
> + * timeout expires.
> + *
> + * Returns 0 on success, %-ETIMEDOUT in case of a timeout, <0 in case of some
> + * other error as reported by PCODE.
> + */
> +int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request)
> +{
> +	u32 status;
> +	int ret;
> +
> +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> +
> +	/*
> +	 * The spec says to keep retrying the request for at most 3ms until
> +	 * acknowledgement, so disable pre-emption to maximize the number of
> +	 * attempts within this duration. Use a 10ms overall timeout to
> +	 * account for interrupts that could reduce the number of attempts.
> +	 */
> +	preempt_disable();
> +	ret = wait_for_atomic(skl_pcode_try_request(dev_priv, mbox, request,
> +						    &status), 10);
> +	preempt_enable();
> +
> +	return ret ? ret : status;
> +}
> +
>  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  {
>  	/*
> -- 
> 2.5.0

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

Re: [PATCH v3 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Imre Deak]

On ma, 2016-11-28 at 15:51 +0200, Ville Syrjälä wrote:
> On Mon, Nov 28, 2016 at 01:12:57PM +0200, Imre Deak wrote:
> > commit 848496e5902833600f7992f4faa82dc1546051ba
> > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Date:   Wed Jul 13 16:32:03 2016 +0300
> > 
> >     drm/i915: Wait up to 3ms for the pcu to ack the cdclk change request on SKL
> > 
> > increased the timeout to match the spec, but we still see a timeout on
> > at least one SKL. A CDCLK change request following the failed one will
> > succeed nevertheless.
> > 
> > I could reproduce this problem easily by running kms_pipe_crc_basic in a
> > loop. In all failure cases _wait_for() was pre-empted for >3ms and so in
> > the worst case - when the pre-emption happened right after calculating
> > timeout__ in _wait_for() - we called skl_cdclk_wait_for_pcu_ready() only
> > once which failed and so _wait_for() timed out. As opposed to this the
> > spec says to keep retrying the request for at most a 3ms period.
> > 
> > To fix this disable pre-emption to maximize the number of times we retry
> > the request. Also increase the timeout to 10ms to account for interrupts
> > that could reduce the number of these attempts. With this change I
> > couldn't trigger the problem.
> > 
> > v2:
> > - Use 1ms poll period instead of 10us. (Chris)
> > v3:
> > - Poll with pre-emption disabled to increase the number of request
> >   attempts. (Ville, Chris)
> > - Factor out a helper to poll, it's also needed by the next patch.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=97929
> > Testcase: igt/kms_pipe_crc_basic/suspend-read-crc-pipe-B
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >  drivers/gpu/drm/i915/i915_reg.h      |  2 +-
> >  drivers/gpu/drm/i915/intel_display.c | 29 +++++++-----------------
> >  drivers/gpu/drm/i915/intel_pm.c      | 43 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 53 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 01f5067..f618807 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3593,6 +3593,7 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
> >  
> >  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
> >  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
> > +int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request);
> >  
> >  /* intel_sideband.c */
> >  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 6747d68..f542cbc 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7424,7 +7424,6 @@ enum {
> >  #define     GEN9_MEM_LATENCY_LEVEL_3_7_SHIFT	24
> >  #define   SKL_PCODE_CDCLK_CONTROL		0x7
> >  #define     SKL_CDCLK_PREPARE_FOR_CHANGE	0x3
> > -#define     SKL_CDCLK_READY_FOR_CHANGE		0x1
> >  #define   GEN6_PCODE_WRITE_MIN_FREQ_TABLE	0x8
> >  #define   GEN6_PCODE_READ_MIN_FREQ_TABLE	0x9
> >  #define   GEN6_READ_OC_PARAMS			0xc
> > @@ -7437,6 +7436,7 @@ enum {
> >  #define     GEN9_SAGV_DISABLE			0x0
> >  #define     GEN9_SAGV_IS_DISABLED		0x1
> >  #define     GEN9_SAGV_ENABLE			0x3
> > +#define   GEN9_PCODE_REQUEST_DONE		0x1
> 
> Is that really a generic thing?

At least SAGV uses the same and there is no other request I know of
that would need this polling request. It will be used in the next patch
for SAGV too.

> 
> >  #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 5d11002..46c0e42 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6245,35 +6245,22 @@ skl_dpll0_disable(struct drm_i915_private *dev_priv)
> >  	dev_priv->cdclk_pll.vco = 0;
> >  }
> >  
> > -static bool skl_cdclk_pcu_ready(struct drm_i915_private *dev_priv)
> > -{
> > -	int ret;
> > -	u32 val;
> > -
> > -	/* inform PCU we want to change CDCLK */
> > -	val = SKL_CDCLK_PREPARE_FOR_CHANGE;
> > -	mutex_lock(&dev_priv->rps.hw_lock);
> > -	ret = sandybridge_pcode_read(dev_priv, SKL_PCODE_CDCLK_CONTROL, &val);
> > -	mutex_unlock(&dev_priv->rps.hw_lock);
> > -
> > -	return ret == 0 && (val & SKL_CDCLK_READY_FOR_CHANGE);
> > -}
> > -
> > -static bool skl_cdclk_wait_for_pcu_ready(struct drm_i915_private *dev_priv)
> > -{
> > -	return _wait_for(skl_cdclk_pcu_ready(dev_priv), 3000, 10) == 0;
> > -}
> > -
> >  static void skl_set_cdclk(struct drm_i915_private *dev_priv, int cdclk, int vco)
> >  {
> >  	u32 freq_select, pcu_ack;
> > +	int ret;
> >  
> >  	WARN_ON((cdclk == 24000) != (vco == 0));
> >  
> >  	DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz (VCO %d kHz)\n", cdclk, vco);
> >  
> > -	if (!skl_cdclk_wait_for_pcu_ready(dev_priv)) {
> > -		DRM_ERROR("failed to inform PCU about cdclk change\n");
> > +	mutex_lock(&dev_priv->rps.hw_lock);
> > +	ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> > +				SKL_CDCLK_PREPARE_FOR_CHANGE);
> > +	mutex_unlock(&dev_priv->rps.hw_lock);
> > +	if (ret) {
> > +		DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
> > +			  ret);
> >  		return;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 66c62f3..edd68f3 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -7864,6 +7864,49 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> >  	return 0;
> >  }
> >  
> > +static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox,
> > +				  u32 request, u32 *status)
> > +{
> > +	u32 val = request;
> > +
> > +	*status = sandybridge_pcode_read(dev_priv, mbox, &val);
> > +
> > +	return *status || (val & GEN9_PCODE_REQUEST_DONE);
> > +}
> > +
> > +/**
> > + * skl_pcode_request - send PCODE request until acknowledgment
> > + * @dev_priv: device private
> > + * @mbox: PCODE mailbox ID the request is targeted for
> > + * @request: request ID
> > + *
> > + * Keep resending the @request to @mbox until PCODE acknowledges it, or a 10ms
> > + * timeout expires.
> > + *
> > + * Returns 0 on success, %-ETIMEDOUT in case of a timeout, <0 in case of some
> > + * other error as reported by PCODE.
> > + */
> > +int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request)
> > +{
> > +	u32 status;
> > +	int ret;
> > +
> > +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> > +
> > +	/*
> > +	 * The spec says to keep retrying the request for at most 3ms until
> > +	 * acknowledgement, so disable pre-emption to maximize the number of
> > +	 * attempts within this duration. Use a 10ms overall timeout to
> > +	 * account for interrupts that could reduce the number of attempts.
> > +	 */
> > +	preempt_disable();
> > +	ret = wait_for_atomic(skl_pcode_try_request(dev_priv, mbox, request,
> > +						    &status), 10);
> > +	preempt_enable();
> > +
> > +	return ret ? ret : status;
> > +}
> > +
> >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >  {
> >  	/*
> > -- 
> > 2.5.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Ville Syrjälä]

On Mon, Nov 28, 2016 at 03:54:08PM +0200, Imre Deak wrote:
> On ma, 2016-11-28 at 15:51 +0200, Ville Syrjälä wrote:
> > On Mon, Nov 28, 2016 at 01:12:57PM +0200, Imre Deak wrote:
> > > commit 848496e5902833600f7992f4faa82dc1546051ba
> > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Date:   Wed Jul 13 16:32:03 2016 +0300
> > > 
> > >     drm/i915: Wait up to 3ms for the pcu to ack the cdclk change request on SKL
> > > 
> > > increased the timeout to match the spec, but we still see a timeout on
> > > at least one SKL. A CDCLK change request following the failed one will
> > > succeed nevertheless.
> > > 
> > > I could reproduce this problem easily by running kms_pipe_crc_basic in a
> > > loop. In all failure cases _wait_for() was pre-empted for >3ms and so in
> > > the worst case - when the pre-emption happened right after calculating
> > > timeout__ in _wait_for() - we called skl_cdclk_wait_for_pcu_ready() only
> > > once which failed and so _wait_for() timed out. As opposed to this the
> > > spec says to keep retrying the request for at most a 3ms period.
> > > 
> > > To fix this disable pre-emption to maximize the number of times we retry
> > > the request. Also increase the timeout to 10ms to account for interrupts
> > > that could reduce the number of these attempts. With this change I
> > > couldn't trigger the problem.
> > > 
> > > v2:
> > > - Use 1ms poll period instead of 10us. (Chris)
> > > v3:
> > > - Poll with pre-emption disabled to increase the number of request
> > >   attempts. (Ville, Chris)
> > > - Factor out a helper to poll, it's also needed by the next patch.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=97929
> > > Testcase: igt/kms_pipe_crc_basic/suspend-read-crc-pipe-B
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> > >  drivers/gpu/drm/i915/i915_reg.h      |  2 +-
> > >  drivers/gpu/drm/i915/intel_display.c | 29 +++++++-----------------
> > >  drivers/gpu/drm/i915/intel_pm.c      | 43 ++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 53 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 01f5067..f618807 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -3593,6 +3593,7 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
> > >  
> > >  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
> > >  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
> > > +int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request);
> > >  
> > >  /* intel_sideband.c */
> > >  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 6747d68..f542cbc 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7424,7 +7424,6 @@ enum {
> > >  #define     GEN9_MEM_LATENCY_LEVEL_3_7_SHIFT	24
> > >  #define   SKL_PCODE_CDCLK_CONTROL		0x7
> > >  #define     SKL_CDCLK_PREPARE_FOR_CHANGE	0x3
> > > -#define     SKL_CDCLK_READY_FOR_CHANGE		0x1
> > >  #define   GEN6_PCODE_WRITE_MIN_FREQ_TABLE	0x8
> > >  #define   GEN6_PCODE_READ_MIN_FREQ_TABLE	0x9
> > >  #define   GEN6_READ_OC_PARAMS			0xc
> > > @@ -7437,6 +7436,7 @@ enum {
> > >  #define     GEN9_SAGV_DISABLE			0x0
> > >  #define     GEN9_SAGV_IS_DISABLED		0x1
> > >  #define     GEN9_SAGV_ENABLE			0x3
> > > +#define   GEN9_PCODE_REQUEST_DONE		0x1
> > 
> > Is that really a generic thing?
> 
> At least SAGV uses the same and there is no other request I know of
> that would need this polling request. It will be used in the next patch
> for SAGV too.

At least it's a bit mispaced since now it looks like it would be
a pcode command, but command 0x1 == CMD_CONFIG according to the spec.

> 
> > 
> > >  #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 5d11002..46c0e42 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6245,35 +6245,22 @@ skl_dpll0_disable(struct drm_i915_private *dev_priv)
> > >  	dev_priv->cdclk_pll.vco = 0;
> > >  }
> > >  
> > > -static bool skl_cdclk_pcu_ready(struct drm_i915_private *dev_priv)
> > > -{
> > > -	int ret;
> > > -	u32 val;
> > > -
> > > -	/* inform PCU we want to change CDCLK */
> > > -	val = SKL_CDCLK_PREPARE_FOR_CHANGE;
> > > -	mutex_lock(&dev_priv->rps.hw_lock);
> > > -	ret = sandybridge_pcode_read(dev_priv, SKL_PCODE_CDCLK_CONTROL, &val);
> > > -	mutex_unlock(&dev_priv->rps.hw_lock);
> > > -
> > > -	return ret == 0 && (val & SKL_CDCLK_READY_FOR_CHANGE);
> > > -}
> > > -
> > > -static bool skl_cdclk_wait_for_pcu_ready(struct drm_i915_private *dev_priv)
> > > -{
> > > -	return _wait_for(skl_cdclk_pcu_ready(dev_priv), 3000, 10) == 0;
> > > -}
> > > -
> > >  static void skl_set_cdclk(struct drm_i915_private *dev_priv, int cdclk, int vco)
> > >  {
> > >  	u32 freq_select, pcu_ack;
> > > +	int ret;
> > >  
> > >  	WARN_ON((cdclk == 24000) != (vco == 0));
> > >  
> > >  	DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz (VCO %d kHz)\n", cdclk, vco);
> > >  
> > > -	if (!skl_cdclk_wait_for_pcu_ready(dev_priv)) {
> > > -		DRM_ERROR("failed to inform PCU about cdclk change\n");
> > > +	mutex_lock(&dev_priv->rps.hw_lock);
> > > +	ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> > > +				SKL_CDCLK_PREPARE_FOR_CHANGE);
> > > +	mutex_unlock(&dev_priv->rps.hw_lock);
> > > +	if (ret) {
> > > +		DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
> > > +			  ret);
> > >  		return;
> > >  	}
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 66c62f3..edd68f3 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -7864,6 +7864,49 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> > >  	return 0;
> > >  }
> > >  
> > > +static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox,
> > > +				  u32 request, u32 *status)
> > > +{
> > > +	u32 val = request;
> > > +
> > > +	*status = sandybridge_pcode_read(dev_priv, mbox, &val);
> > > +
> > > +	return *status || (val & GEN9_PCODE_REQUEST_DONE);
> > > +}
> > > +
> > > +/**
> > > + * skl_pcode_request - send PCODE request until acknowledgment
> > > + * @dev_priv: device private
> > > + * @mbox: PCODE mailbox ID the request is targeted for
> > > + * @request: request ID
> > > + *
> > > + * Keep resending the @request to @mbox until PCODE acknowledges it, or a 10ms
> > > + * timeout expires.
> > > + *
> > > + * Returns 0 on success, %-ETIMEDOUT in case of a timeout, <0 in case of some
> > > + * other error as reported by PCODE.
> > > + */
> > > +int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request)
> > > +{
> > > +	u32 status;
> > > +	int ret;
> > > +
> > > +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> > > +
> > > +	/*
> > > +	 * The spec says to keep retrying the request for at most 3ms until
> > > +	 * acknowledgement, so disable pre-emption to maximize the number of
> > > +	 * attempts within this duration. Use a 10ms overall timeout to
> > > +	 * account for interrupts that could reduce the number of attempts.
> > > +	 */
> > > +	preempt_disable();
> > > +	ret = wait_for_atomic(skl_pcode_try_request(dev_priv, mbox, request,
> > > +						    &status), 10);
> > > +	preempt_enable();
> > > +
> > > +	return ret ? ret : status;
> > > +}
> > > +
> > >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> > >  {
> > >  	/*
> > > -- 
> > > 2.5.0
> > 

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

Re: [PATCH v3 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Ville Syrjälä]

On Mon, Nov 28, 2016 at 04:06:05PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 28, 2016 at 03:54:08PM +0200, Imre Deak wrote:
> > On ma, 2016-11-28 at 15:51 +0200, Ville Syrjälä wrote:
> > > On Mon, Nov 28, 2016 at 01:12:57PM +0200, Imre Deak wrote:
> > > > commit 848496e5902833600f7992f4faa82dc1546051ba
> > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Date:   Wed Jul 13 16:32:03 2016 +0300
> > > > 
> > > >     drm/i915: Wait up to 3ms for the pcu to ack the cdclk change request on SKL
> > > > 
> > > > increased the timeout to match the spec, but we still see a timeout on
> > > > at least one SKL. A CDCLK change request following the failed one will
> > > > succeed nevertheless.
> > > > 
> > > > I could reproduce this problem easily by running kms_pipe_crc_basic in a
> > > > loop. In all failure cases _wait_for() was pre-empted for >3ms and so in
> > > > the worst case - when the pre-emption happened right after calculating
> > > > timeout__ in _wait_for() - we called skl_cdclk_wait_for_pcu_ready() only
> > > > once which failed and so _wait_for() timed out. As opposed to this the
> > > > spec says to keep retrying the request for at most a 3ms period.
> > > > 
> > > > To fix this disable pre-emption to maximize the number of times we retry
> > > > the request. Also increase the timeout to 10ms to account for interrupts
> > > > that could reduce the number of these attempts. With this change I
> > > > couldn't trigger the problem.
> > > > 
> > > > v2:
> > > > - Use 1ms poll period instead of 10us. (Chris)
> > > > v3:
> > > > - Poll with pre-emption disabled to increase the number of request
> > > >   attempts. (Ville, Chris)
> > > > - Factor out a helper to poll, it's also needed by the next patch.
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=97929
> > > > Testcase: igt/kms_pipe_crc_basic/suspend-read-crc-pipe-B
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> > > >  drivers/gpu/drm/i915/i915_reg.h      |  2 +-
> > > >  drivers/gpu/drm/i915/intel_display.c | 29 +++++++-----------------
> > > >  drivers/gpu/drm/i915/intel_pm.c      | 43 ++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 53 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 01f5067..f618807 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -3593,6 +3593,7 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
> > > >  
> > > >  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
> > > >  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
> > > > +int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request);
> > > >  
> > > >  /* intel_sideband.c */
> > > >  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 6747d68..f542cbc 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -7424,7 +7424,6 @@ enum {
> > > >  #define     GEN9_MEM_LATENCY_LEVEL_3_7_SHIFT	24
> > > >  #define   SKL_PCODE_CDCLK_CONTROL		0x7
> > > >  #define     SKL_CDCLK_PREPARE_FOR_CHANGE	0x3
> > > > -#define     SKL_CDCLK_READY_FOR_CHANGE		0x1
> > > >  #define   GEN6_PCODE_WRITE_MIN_FREQ_TABLE	0x8
> > > >  #define   GEN6_PCODE_READ_MIN_FREQ_TABLE	0x9
> > > >  #define   GEN6_READ_OC_PARAMS			0xc
> > > > @@ -7437,6 +7436,7 @@ enum {
> > > >  #define     GEN9_SAGV_DISABLE			0x0
> > > >  #define     GEN9_SAGV_IS_DISABLED		0x1
> > > >  #define     GEN9_SAGV_ENABLE			0x3
> > > > +#define   GEN9_PCODE_REQUEST_DONE		0x1
> > > 
> > > Is that really a generic thing?
> > 
> > At least SAGV uses the same and there is no other request I know of
> > that would need this polling request. It will be used in the next patch
> > for SAGV too.
> 
> At least it's a bit mispaced since now it looks like it would be
> a pcode command, but command 0x1 == CMD_CONFIG according to the spec.

If we don't want to assume it's all that generic, I guess we could
always have the caller pass in the mask+value to wait for?

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

Re: [PATCH v3 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Imre Deak]

On ma, 2016-11-28 at 16:11 +0200, Ville Syrjälä wrote:
> On Mon, Nov 28, 2016 at 04:06:05PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 28, 2016 at 03:54:08PM +0200, Imre Deak wrote:
> > > On ma, 2016-11-28 at 15:51 +0200, Ville Syrjälä wrote:
> > > > On Mon, Nov 28, 2016 at 01:12:57PM +0200, Imre Deak wrote:
> > > > > commit 848496e5902833600f7992f4faa82dc1546051ba
> > > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Date:   Wed Jul 13 16:32:03 2016 +0300
> > > > > 
> > > > >     drm/i915: Wait up to 3ms for the pcu to ack the cdclk change request on SKL
> > > > > 
> > > > > increased the timeout to match the spec, but we still see a timeout on
> > > > > at least one SKL. A CDCLK change request following the failed one will
> > > > > succeed nevertheless.
> > > > > 
> > > > > I could reproduce this problem easily by running kms_pipe_crc_basic in a
> > > > > loop. In all failure cases _wait_for() was pre-empted for >3ms and so in
> > > > > the worst case - when the pre-emption happened right after calculating
> > > > > timeout__ in _wait_for() - we called skl_cdclk_wait_for_pcu_ready() only
> > > > > once which failed and so _wait_for() timed out. As opposed to this the
> > > > > spec says to keep retrying the request for at most a 3ms period.
> > > > > 
> > > > > To fix this disable pre-emption to maximize the number of times we retry
> > > > > the request. Also increase the timeout to 10ms to account for interrupts
> > > > > that could reduce the number of these attempts. With this change I
> > > > > couldn't trigger the problem.
> > > > > 
> > > > > v2:
> > > > > - Use 1ms poll period instead of 10us. (Chris)
> > > > > v3:
> > > > > - Poll with pre-emption disabled to increase the number of request
> > > > >   attempts. (Ville, Chris)
> > > > > - Factor out a helper to poll, it's also needed by the next patch.
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=97929
> > > > > Testcase: igt/kms_pipe_crc_basic/suspend-read-crc-pipe-B
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> > > > >  drivers/gpu/drm/i915/i915_reg.h      |  2 +-
> > > > >  drivers/gpu/drm/i915/intel_display.c | 29 +++++++-----------------
> > > > >  drivers/gpu/drm/i915/intel_pm.c      | 43 ++++++++++++++++++++++++++++++++++++
> > > > >  4 files changed, 53 insertions(+), 22 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 01f5067..f618807 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -3593,6 +3593,7 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
> > > > >  
> > > > >  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
> > > > >  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
> > > > > +int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request);
> > > > >  
> > > > >  /* intel_sideband.c */
> > > > >  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index 6747d68..f542cbc 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -7424,7 +7424,6 @@ enum {
> > > > >  #define     GEN9_MEM_LATENCY_LEVEL_3_7_SHIFT	24
> > > > >  #define   SKL_PCODE_CDCLK_CONTROL		0x7
> > > > >  #define     SKL_CDCLK_PREPARE_FOR_CHANGE	0x3
> > > > > -#define     SKL_CDCLK_READY_FOR_CHANGE		0x1
> > > > >  #define   GEN6_PCODE_WRITE_MIN_FREQ_TABLE	0x8
> > > > >  #define   GEN6_PCODE_READ_MIN_FREQ_TABLE	0x9
> > > > >  #define   GEN6_READ_OC_PARAMS			0xc
> > > > > @@ -7437,6 +7436,7 @@ enum {
> > > > >  #define     GEN9_SAGV_DISABLE			0x0
> > > > >  #define     GEN9_SAGV_IS_DISABLED		0x1
> > > > >  #define     GEN9_SAGV_ENABLE			0x3
> > > > > +#define   GEN9_PCODE_REQUEST_DONE		0x1
> > > > 
> > > > Is that really a generic thing?
> > > 
> > > At least SAGV uses the same and there is no other request I know of
> > > that would need this polling request. It will be used in the next patch
> > > for SAGV too.
> > 
> > At least it's a bit mispaced since now it looks like it would be
> > a pcode command, but command 0x1 == CMD_CONFIG according to the spec.
> 
> If we don't want to assume it's all that generic, I guess we could
> always have the caller pass in the mask+value to wait for?

Ok, will change that.

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

Re: [PATCH v3 3/3] drm/i915/gen9: Fix PCODE polling during SAGV disabling

[Lyude Paul]

One tiny nitpick below

On Mon, 2016-11-28 at 13:12 +0200, Imre Deak wrote:
> According to the previous patch, it's possible atm that we call
> intel_do_sagv_disable() only once during the 1ms period and time out
> if
> that call fails. As opposed to this the spec says that we need to
> keep
> retrying this request for a 1ms duration, so let's do this similarly
> to
> the CDCLK change notification request.
> 
> Cc: Lyude <cpaul@redhat.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  1 -
>  drivers/gpu/drm/i915/intel_pm.c | 33 +++++++----------------------
> ----
>  2 files changed, 7 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index f542cbc..c7458b7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7434,7 +7434,6 @@ enum {
>  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
>  #define   GEN9_PCODE_SAGV_CONTROL		0x21
>  #define     GEN9_SAGV_DISABLE			0x0
> -#define     GEN9_SAGV_IS_DISABLED		0x1
>  #define     GEN9_SAGV_ENABLE			0x3
>  #define   GEN9_PCODE_REQUEST_DONE		0x1
>  #define GEN6_PCODE_DATA				_MMIO(0x13812
> 8)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index edd68f3..43b0b40 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2954,24 +2954,10 @@ intel_enable_sagv(struct drm_i915_private
> *dev_priv)
>  	return 0;
>  }
>  
> -static int
> -intel_do_sagv_disable(struct drm_i915_private *dev_priv)
> -{
> -	int ret;
> -	uint32_t temp = GEN9_SAGV_DISABLE;
> -
> -	ret = sandybridge_pcode_read(dev_priv,
> GEN9_PCODE_SAGV_CONTROL,
> -				     &temp);
> -	if (ret)
> -		return ret;
> -	else
> -		return temp & GEN9_SAGV_IS_DISABLED;
> -}
> -
>  int
>  intel_disable_sagv(struct drm_i915_private *dev_priv)
>  {
> -	int ret, result;
> +	int ret;
>  
>  	if (!intel_has_sagv(dev_priv))
>  		return 0;
> @@ -2981,27 +2967,22 @@ intel_disable_sagv(struct drm_i915_private
> *dev_priv)
>  
>  	DRM_DEBUG_KMS("Disabling the SAGV\n");
>  	mutex_lock(&dev_priv->rps.hw_lock);
> -
I'd leave the whitespace here.

Other then that:

	Reviewed-by: Lyude <lyude@redhat.com>
>  	/* bspec says to keep retrying for at least 1 ms */
> -	ret = wait_for(result = intel_do_sagv_disable(dev_priv), 1);
> +	ret = skl_pcode_request(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				GEN9_SAGV_DISABLE);
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
> -	if (ret == -ETIMEDOUT) {
> -		DRM_ERROR("Request to disable SAGV timed out\n");
> -		return -ETIMEDOUT;
> -	}
> -
>  	/*
>  	 * Some skl systems, pre-release machines in particular,
>  	 * don't actually have an SAGV.
>  	 */
> -	if (IS_SKYLAKE(dev_priv) && result == -ENXIO) {
> +	if (IS_SKYLAKE(dev_priv) && ret == -ENXIO) {
>  		DRM_DEBUG_DRIVER("No SAGV found on system,
> ignoring\n");
>  		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
>  		return 0;
> -	} else if (result < 0) {
> -		DRM_ERROR("Failed to disable the SAGV\n");
> -		return result;
> +	} else if (ret < 0) {
> +		DRM_ERROR("Failed to disable the SAGV (%d)\n", ret);
> +		return ret;
>  	}
>  
>  	dev_priv->sagv_status = I915_SAGV_DISABLED;
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx